[libvirt] [PATCH 5/6] qemu: implement <output> element for <sound> devices
John Ferlan
jferlan at redhat.com
Tue Nov 21 17:44:04 UTC 2017
On 11/21/2017 12:12 PM, Pavel Hrdina wrote:
> On Tue, Nov 21, 2017 at 10:52:48AM -0500, John Ferlan wrote:
>>
>>
>> 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>
>
> Which part of the PostParse code does that? If you configure the domain
> XML like this:
>
> <sound model='ich6'/>
> <sound model='ich6'/>
>
> The resulting config XML saved to disk would be:
>
> <sound model='ich6'>
> <<address type='pci' domain='0x0000' bus='0x00' slot='0x03' unction='0x0'/>
> </sound>
> <sound model='ich6'>
> <<address type='pci' domain='0x0000' bus='0x00' slot='0x04' unction='0x0'/>
> </sound>
>From patch 6 I used:
tests/qemuxml2argvdata/qemuxml2argv-sound-multi-pa-output-spice.xml
which has
<sound model='ich6'>
<output type='pa'/>
</sound>
<sound model='ich6'/>
>
> One of the reasons why I didn't add qemuxml2xml tests is that only the
> offline part is somehow working, but the active and status part of that
> test is broken and doesn't reflect how libvirt changes the active and
> status XML.
>
I didn't pay close attention to which was which, just that some output
xml was generated by the regenerate.
John
>> 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.
>
> Good point, I'll fix that.
>
>> 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.
>
> Well, that's how it works with this patches.
>
>>>
>>> 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.
>
> If there wouldn't be the output attribute introduced by this patch
> series I would agree with you to using qemuDomainObjPrivate field, but
> since this patch series introduces the output attribute and required
> field in structure for sound devices we should use that one and not
> introduce yet another place where to store this information.
>
> Pavel
>
More information about the libvir-list
mailing list