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