[pve-devel] [PATCH manager 1/3] Add audio device support

Thomas Lamprecht t.lamprecht at proxmox.com
Mon Jul 15 16:58:03 CEST 2019


On 7/15/19 3:34 PM, Aaron Lauterer wrote:
> Used `audio0` with the extra `0` to be able to add support for multiple
> audio devices in the future if it will ever be necessary.
> 

why a format string and not a simple direct enum for the "audioX"?
Are there (potential) additional options we can have here?

Also some general info, like short description what the different devices
are, if there are more which we do not allow as they may not make sense,
why one needs to have "hda-XYZ" devs for some but not others, ...
(just as example)

Does not have to be much, but a bit more could be great..

> Cleaned some old commented out code regarding audio device support.
> 
> Signed-off-by: Aaron Lauterer <a.lauterer at proxmox.com>
> ---
>  PVE/QemuServer.pm | 36 ++++++++++++++++++++++++++++++------
>  1 file changed, 30 insertions(+), 6 deletions(-)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 9f29927..779ae7c 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -234,6 +234,14 @@ my $agent_fmt = {
>      },
>  };
>  
> +my $audio_fmt = {
> +    type => {
> +	description => "Select the audio device.",
> +	type => 'string',
> +	enum => [qw(ich9-intel-hda intel-hda AC97)]
> +    },
> +};
> +
>  my $vga_fmt = {
>      type => {
>  	description => "Select the VGA type.",
> @@ -637,7 +645,13 @@ EODESCR
>  	format => $ivshmem_fmt,
>  	description => "Inter-VM shared memory. Useful for direct communication between VMs, or to the host.",
>  	optional => 1,
> -    }
> +    },
> +    audio0 => {
> +	type => 'string',
> +	format => $audio_fmt,
> +	description => "Configure a audio device.",
> +	optional => 1
> +    },
>  };
>  
>  my $cicustom_fmt = {
> @@ -3780,6 +3794,21 @@ sub config_to_command {
>  	}
>      }
>  
> +    # audio devices

use-less comment ("audio0" say the same, also rather "why"
then "what" comments)

> +    if ($conf->{"audio0"}) {
> +	my $audioproperties = PVE::JSONSchema::parse_property_string($audio_fmt, $conf->{audio0});
> +	my $audiodevice = $audioproperties->{type};
> +	my $audiopciaddr = print_pci_addr("audio0", $bridges, $arch, $machine_type);
> +
> +	if ($audiodevice eq 'AC97') {
> +	    push @$devices, '-device', "AC97,id=sound0$audiopciaddr";
> +	}
> +	else {

nak for the style, we always use:

if (expr) {
    codeA();
} elsif {
    codeB();
} else {
    codeC();
}


> +	    push @$devices, '-device', "$audiodevice,id=sound5$audiopciaddr";

maybe use the curly braces "string${interpolation}" syntax, to clarify what is
a variable and what is a static string.

> +	    push @$devices, '-device', "hda-micro,id=sound5-codec0,bus=sound5.0,cad=0";
> +	    push @$devices, '-device', "hda-duplex,id=sound5-codec1,bus=sound5.0,cad=1";
> +	}
> +    }
>  
>      my $sockets = 1;
>      $sockets = $conf->{smp} if $conf->{smp}; # old style - no longer iused
> @@ -3877,11 +3906,6 @@ sub config_to_command {
>  
>      push @$cmd, '-k', $conf->{keyboard} if defined($conf->{keyboard});
>  
> -    # enable sound
> -    #my $soundhw = $conf->{soundhw} || $defaults->{soundhw};
> -    #push @$cmd, '-soundhw', 'es1370';
> -    #push @$cmd, '-soundhw', $soundhw if $soundhw;
> -
>      if (parse_guest_agent($conf)->{enabled}) {
>  	my $qgasocket = qmp_socket($vmid, 1);
>  	my $pciaddr = print_pci_addr("qga0", $bridges, $arch, $machine_type);
> 





More information about the pve-devel mailing list