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

[Daniel P. Berrange]

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

Yeah, semantically its fine, but it'd be better to stuck with our
convention that address & alias are last.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|