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

On Mon, Nov 20, 2017 at 07:44:25PM -0500, John Ferlan wrote: > > > 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? It would be probably possible to split this patch into two separate changes: 1. move the code out of command line generation into qemuProcessPrepareSound() 2. the rest of this patch > 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 ;-). The PostParse function tries to find the first sound device with output configured (the first for loop) and sets this output to all other sound devices without any output configured (the second for loop). This is executed while parsing domain XML and implements the new feature.

The Prepare function is executed only while starting domain and basically supplements the code removed from building command line. It prepares only live definition that is about to be started so the qemu command line code can only take the definition and convert it into command line without any logic or without modifying the live definition.

> > 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... Actually no, this is tied together, the PostParse and Validate callbacks make sure that the definition is correct so that qemu build command line doesn't have to check anything and simply takes the definition as it is and converts it into command line. Pavel