[pve-devel] applied: [PATCH qemu-server 1/3] Add audio device support

[Thomas Lamprecht]

On 7/17/19 3:58 PM, Aaron Lauterer wrote:
> Until now audio devices had to be added manually with the `args` option
> in the VMs config file. This adds a new config option to define an audio
> device. It is called `audio0` with the extra `0` to be prepared should
> we ever implement support for more than one audio device.
> 
> Supported devices are:
> 
> * ich9-intel-hda: Intel HD Audio Controller, emulates ICH9
> * intel-hda: Intel HD Audio Controller, emulates ICH6
> * AC97: useful for older OSs like Win XP
> 
> If any of the `intel-hda`s is selected two additional devices will be
> created: hda-micro and hda-duplex. The `*intel-hda` controllers need at
> least one of them as they emulate microphones and speakers (hda-micro)
> or line in and out devices (hda-duplex).
> 
> Having both is deemed best practice to avoid problems if some software
> needs one of the other kind of output/input ports.
> 
> I also cleaned up some old, commented out, code regarding audio devices.
> 

much better description, applied. what still missed is the biggest (and
currently almost only practical) use case for this: useful in combination
with SPICE

Added that to the description as followup.

Also, a another comment and followup, see below.

> Signed-off-by: Aaron Lauterer <a.lauterer at proxmox.com>
> ---
>  PVE/QemuServer.pm | 25 +++++++++++++++++++------
>  1 file changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 9f29927..7264116 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -637,7 +637,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',
> +	enum => [qw(ich9-intel-hda intel-hda AC97)],
> +	description => "Configure a audio device.",
> +	optional => 1
> +    },
>  };
>  
>  my $cicustom_fmt = {
> @@ -3780,6 +3786,18 @@ sub config_to_command {
>  	}
>      }
>  
> +    if ($conf->{"audio0"}) {
> +	my $audiodevice = $conf->{audio0};
> +	my $audiopciaddr = print_pci_addr("audio0", $bridges, $arch, $machine_type);

Above is problematic, you only add the address in the next patch so this
will error here, please try to order your patches by there dependence,
no single patch should depend on a later patch.

I'd have been probably best to just to the audio PCI addition in this
patch, no need to have a separate one (they both do not make much sense
without the other one anyway..)

> +
> +	if ($audiodevice eq 'AC97') {
> +	    push @$devices, '-device', "AC97,id=sound0${audiopciaddr}";
> +	} else {

even if our API can filter and cry on wrong params it's often still no
good idea to just unconditionally hope for an "intel-hda" type here..

>From the followup commit message:
> die if we get some unknown value, which is either a bug (new dev type
> added to enum but not here) or a user manual edit error, good to
> catch both