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

Tue Nov 21 18:01:57 UTC 2017

On Tue, Nov 21, 2017 at 12:44:04PM -0500, John Ferlan wrote:
> 
> 
> 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'/>

Right, it was not clear what you meant, next time please include this
input XML right away, it will save both of us some time to
understand each other :).

I see your point that this update could have been done in Prepare
function.  I chose this implementation to make it clear that there
is the limitation with QEMU, but it would also work to put it into
Prepare function.  That way if QEMU introduces support to configure
different output for each sound device users would benefit from it
automatically for existing domains once we add it also into libvirt.

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

I'll send a v2 to make sure that all the points and notes were addressed
correctly.

Pavel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20171121/935015ca/attachment-0001.sig>