Re: [libvirt] [PATCH 5/6] qemu: implement <output> element for <sound> devices

On 11/14/2017 08:45 AM, Pavel Hrdina wrote: > So far we were configuring the sound output based on what graphic device > was configured in domain XML. This had a several disadvantages, it's > not transparent, in case of multiple graphic devices it was overwritten > by the last one and there was no simple way how to configure this per > domain. > > The new <output> element for <sound> devices allows you to configure > which output will be used for each domain, however QEMU has a limitation > that all sound devices will always use the same output because it is > configured by environment variable QEMU_AUDIO_DRV per domain. > > For backward compatibility we need to preserve the defaults if no output > is specified: > > - for vnc graphic it's by default NONE unless "vnc_allow_host_audio" > was enabled, in that case we use DEFAULT which means it will pass > the environment variable visible by libvirtd > > - for sdl graphic by default we pass the environment variable > > - for spice graphic we configure the SPICE output > > - if no graphic is configured we use by default NONE unless > "nogfx_allow_host_audio" was enabled, in that case we pass > the environment variable > > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1375433 > > Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; > --- > docs/formatdomain.html.in | 4 ++- > src/qemu/qemu_command.c | 84 +++++++++++++++++++++-------------------------- > src/qemu/qemu_domain.c | 54 ++++++++++++++++++++++++++++++ > src/qemu/qemu_process.c | 41 +++++++++++++++++++++++ > 4 files changed, 135 insertions(+), 48 deletions(-) > Is there any way to make less things happen in one patch? Maybe even the PostParse, Prepare, and Validate together?

I need to look at this one with fresh eyes in the morning - it's confusing at best especially since I don't find the functions self documenting. I'm having trouble with the settings between PostParse and Prepare as well as setting a default output which essentially changes an optional parameter into a required one, doesn't it? I'm sure there's a harder and even more confusing way to do things ;-).

John > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in > index 3b7c367fc7..ae0d8b86be 100644 > --- a/docs/formatdomain.html.in > +++ b/docs/formatdomain.html.in > @@ -7064,7 +7064,9 @@ qemu-kvm -net nic,model=? /dev/null > the audio output is connected within host. There is mandatory > <code>type</code> attribute where valid values are 'none' to > disable the audio output, 'spice', 'pa', 'sdl', 'alsa', 'oss'. > - This might not be supported by all hypervisors. > + This might not be supported by all hypervisors. QEMU driver > + has a limitation that all sound devices have to use the same > + output. > </p> > > <h4><a id="elementsWatchdog">Watchdog device</a></h4> > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > index c5c7bd7e54..5cbd1d0d46 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c > @@ -4442,67 +4442,57 @@ qemuBuildSoundCodecStr(virDomainSoundDefPtr sound, > } > > > -static void > +static int > qemuBuildSoundAudioEnv(virCommandPtr cmd, > - const virDomainDef *def, > - virQEMUDriverConfigPtr cfg) > + const virDomainDef *def) > { > + char *envStr = NULL; > + > if (def->nsounds == 0) { > virCommandAddEnvString(cmd, "QEMU_AUDIO_DRV=none"); > - return; > + return 0; > } > > - if (def->ngraphics == 0) { > - if (cfg->nogfxAllowHostAudio) > - virCommandAddEnvPassBlockSUID(cmd, "QEMU_AUDIO_DRV", NULL); > - else > - virCommandAddEnvString(cmd, "QEMU_AUDIO_DRV=none"); > - } else { > - switch (def->graphics[def->ngraphics - 1]->type) { > - case VIR_DOMAIN_GRAPHICS_TYPE_SDL: > - /* If using SDL for video, then we should just let it > - * use QEMU's host audio drivers, possibly SDL too > - * User can set these two before starting libvirtd > - */ > - virCommandAddEnvPassBlockSUID(cmd, "QEMU_AUDIO_DRV", NULL); > + /* QEMU doesn't allow setting different audio output per sound device > + * so it will always be the same for all devices. */ This is a case where the SoundPostParse should either be in its own patch or in the previous patch... Of course reading this "out of order" made me wonder how the the [0]th element was filled in...