Re: [libvirt] [PATCH 2/6] qemu: move QEMU_AUDIO_DRIVER out of graphic into sound

On Mon, Nov 20, 2017 at 05:39:38PM -0500, John Ferlan wrote: > > > On 11/14/2017 08:45 AM, Pavel Hrdina wrote: >> Setting the default audio output depends on specific graphic device >> but requires having sound device configured as well and it's the sound >> device that handles the audio. >> >> Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; >> --- >> src/qemu/qemu_command.c | 84 +++++++++++++++------- >> .../qemuxml2argv-clock-france.args | 2 +- >> 2 files changed, 58 insertions(+), 28 deletions(-) >> >> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c >> index eb72db33ba..e1ef1b05fa 100644 >> --- a/src/qemu/qemu_command.c >> +++ b/src/qemu/qemu_command.c >> @@ -4442,10 +4442,62 @@ qemuBuildSoundCodecStr(virDomainSoundDefPtr sound, >> } >> > >> +static void >> +qemuBuildSoundAudioEnv(virCommandPtr cmd, >> + const virDomainDef *def, >> + virQEMUDriverConfigPtr cfg) >> +{ >> + 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) { > > So it's the "last" graphics device that then defines "how" this all > works? Makes sense for QEMU_AUDIO_DRV since whichever is last would be > the winner and get the chicken dinner, but... > >> + 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); >> + virCommandAddEnvPassBlockSUID(cmd, "SDL_AUDIODRIVER", NULL); > > ... if there was more than one graphics device defined, where one was > SDL and it wasn't the last one, then theoretically at least this would > not be defined. This is intentional, I should have mentioned it in the commit message. The original design was just wrong, nothing else. Setting "SDL_AUDIODRIVER" if the SDL audio output is not used is pointless and we shouldn't do it. I can move this change to separate patch. Pavel