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

[John Ferlan]

On 11/21/2017 03:59 AM, Pavel Hrdina wrote:
> 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 at redhat.com>
>>> ---
>>>  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
> 

I figured you had gone through the history - I certain hadn't ;-)...  I
would say by this point in the review I was still "missing" the final
detail regarding the bug you're working on O:) - that the audio was
being 'tied to' that last device only. Guess I was thinking it could
somehow be magically applied to "any" device.

Anyway, long way of saying - it's fine here. Adding a bit more detail to
the commit message will help though.

John