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@xxxxxxxxxx> >>>>> --- >>>>> 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. John >> 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 > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
