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