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

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:

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