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

[John Ferlan]

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

Understood, but it also changes each configured sound device that didn't
have <output> defined to have the <output> value from the one that did
have it.

That would then be saved - something that would have been shown in the
qemuxml2xmltest output, e.g the multi output from patch 6 would be:


    <sound model='ich6'>
      <address type='pci' domain='0x0000' bus='0x00' slot='0x03'
function='0x0'/>
      <output type='pa'/>
    </sound>
    <sound model='ich6'>
      <address type='pci' domain='0x0000' bus='0x00' slot='0x04'
function='0x0'/>
      <output type='pa'/>
    </sound>

I also note that <output> comes after <address>... not that it matters,
but my brain recollects that <address> is generally last for most
elements, although not required to be last - it just is generally.

In any case, I'm not sure it's "right" to change/add that output. It
should be simple enough to ignore those that don't define some output.
That was my point about making an optional element required.

Being able to provide/determine some default when no sound device has an
output defined would thus become the "job" of the Prepare API I think.

> 
> 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.
> 

Right - these are the live entries not the config/saved defs - so to
that degree altering things does make some sense. However, your choice
to modify each live entry, but really only use the [0]th one in the
command line building makes me want to consider whether it's better to
create some sort of qemuDomainObjPrivate field instead. Since there can
only be "one" (at this time) it would seem to be mechanism used
elsewhere to keep track of QEMU private and specific shtuff.

>>
>> 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
> 

I always have to go back through to context switch in the ordering of
processing and the "rules" between PostParse and Validate...

The reason I remarked here was purely based on order. That is, it's not
clear until later that the [0]th entry could/would be filled in
automagically...

John