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

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(a)redhat.com&gt; >>> --- >>> 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.