[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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




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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]