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? 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 ;-). John > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in > index 3b7c367fc7..ae0d8b86be 100644 > --- a/docs/formatdomain.html.in > +++ b/docs/formatdomain.html.in > @@ -7064,7 +7064,9 @@ qemu-kvm -net nic,model=? /dev/null > the audio output is connected within host. There is mandatory > <code>type</code> attribute where valid values are 'none' to > disable the audio output, 'spice', 'pa', 'sdl', 'alsa', 'oss'. > - This might not be supported by all hypervisors. > + This might not be supported by all hypervisors. QEMU driver > + has a limitation that all sound devices have to use the same > + output. > </p> > > <h4><a id="elementsWatchdog">Watchdog device</a></h4> > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > index c5c7bd7e54..5cbd1d0d46 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c > @@ -4442,67 +4442,57 @@ qemuBuildSoundCodecStr(virDomainSoundDefPtr sound, > } > > > -static void > +static int > qemuBuildSoundAudioEnv(virCommandPtr cmd, > - const virDomainDef *def, > - virQEMUDriverConfigPtr cfg) > + const virDomainDef *def) > { > + char *envStr = NULL; > + > if (def->nsounds == 0) { > virCommandAddEnvString(cmd, "QEMU_AUDIO_DRV=none"); > - return; > + return 0; > } > > - if (def->ngraphics == 0) { > - if (cfg->nogfxAllowHostAudio) > - virCommandAddEnvPassBlockSUID(cmd, "QEMU_AUDIO_DRV", NULL); > - else > - virCommandAddEnvString(cmd, "QEMU_AUDIO_DRV=none"); > - } else { > - switch (def->graphics[def->ngraphics - 1]->type) { > - case VIR_DOMAIN_GRAPHICS_TYPE_SDL: > - /* If using SDL for video, then we should just let it > - * use QEMU's host audio drivers, possibly SDL too > - * User can set these two before starting libvirtd > - */ > - virCommandAddEnvPassBlockSUID(cmd, "QEMU_AUDIO_DRV", NULL); > + /* QEMU doesn't allow setting different audio output per sound device > + * so it will always be the same for all devices. */ This is a case where the SoundPostParse should either be in its own patch or in the previous patch... Of course reading this "out of order" made me wonder how the the [0]th element was filled in... > + switch (def->sounds[0]->output) { > + case VIR_DOMAIN_SOUND_OUTPUT_TYPE_DEFAULT: > + /* The default output is used only as backward compatible way to > + * pass-through environment variables configured before starting > + * libvirtd. */ > + virCommandAddEnvPassBlockSUID(cmd, "QEMU_AUDIO_DRV", NULL); > + if (def->ngraphics > 0 && > + def->graphics[def->ngraphics - 1]->type == VIR_DOMAIN_GRAPHICS_TYPE_SDL) { > virCommandAddEnvPassBlockSUID(cmd, "SDL_AUDIODRIVER", NULL); > + } > + break; > > - break; > - > - case VIR_DOMAIN_GRAPHICS_TYPE_VNC: > - /* Unless user requested it, set the audio backend to none, to > - * prevent it opening the host OS audio devices, since that causes > - * security issues and might not work when using VNC. > - */ > - if (cfg->vncAllowHostAudio) > - virCommandAddEnvPassBlockSUID(cmd, "QEMU_AUDIO_DRV", NULL); > - else > - virCommandAddEnvString(cmd, "QEMU_AUDIO_DRV=none"); > - > - break; > - > - case VIR_DOMAIN_GRAPHICS_TYPE_SPICE: > - /* SPICE includes native support for tunnelling audio, so we > - * set the audio backend to point at SPICE's own driver > - */ > - virCommandAddEnvString(cmd, "QEMU_AUDIO_DRV=spice"); > - > - break; > - > - case VIR_DOMAIN_GRAPHICS_TYPE_RDP: > - case VIR_DOMAIN_GRAPHICS_TYPE_DESKTOP: > - case VIR_DOMAIN_GRAPHICS_TYPE_LAST: > - break; > + case VIR_DOMAIN_SOUND_OUTPUT_TYPE_NONE: > + case VIR_DOMAIN_SOUND_OUTPUT_TYPE_SPICE: > + case VIR_DOMAIN_SOUND_OUTPUT_TYPE_PA: > + case VIR_DOMAIN_SOUND_OUTPUT_TYPE_SDL: > + case VIR_DOMAIN_SOUND_OUTPUT_TYPE_ALSA: > + case VIR_DOMAIN_SOUND_OUTPUT_TYPE_OSS: > + if (virAsprintf(&envStr, "QEMU_AUDIO_DRV=%s", > + virDomainSoundOutputTypeToString(def->sounds[0]->output)) < 0) { > + return -1; > } > + virCommandAddEnvString(cmd, envStr); > + VIR_FREE(envStr); > + break; > + > + case VIR_DOMAIN_SOUND_OUTPUT_TYPE_LAST: > + break; > } > + > + return 0; > } > > > static int > qemuBuildSoundCommandLine(virCommandPtr cmd, > const virDomainDef *def, > - virQEMUCapsPtr qemuCaps, > - virQEMUDriverConfigPtr cfg) > + virQEMUCapsPtr qemuCaps) > { > size_t i, j; > > @@ -4556,7 +4546,7 @@ qemuBuildSoundCommandLine(virCommandPtr cmd, > } > } > > - qemuBuildSoundAudioEnv(cmd, def, cfg); > + qemuBuildSoundAudioEnv(cmd, def); > > return 0; > } > @@ -10118,7 +10108,7 @@ qemuBuildCommandLine(virQEMUDriverPtr driver, > if (qemuBuildVideoCommandLine(cmd, def, qemuCaps) < 0) > goto error; > > - if (qemuBuildSoundCommandLine(cmd, def, qemuCaps, cfg) < 0) > + if (qemuBuildSoundCommandLine(cmd, def, qemuCaps) < 0) > goto error; > > if (qemuBuildWatchdogCommandLine(cmd, def, qemuCaps) < 0) > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c > index a36e157529..3b8fa2d79c 100644 > --- a/src/qemu/qemu_domain.c > +++ b/src/qemu/qemu_domain.c > @@ -3148,6 +3148,31 @@ qemuDomainDefVerifyFeatures(const virDomainDef *def) > } > > > +static void > +qemuDomainDefSoundPostParse(virDomainDefPtr def) > +{ > + size_t i; > + virDomainSoundOutputType output = VIR_DOMAIN_SOUND_OUTPUT_TYPE_DEFAULT; > + > + for (i = 0; i < def->nsounds; i++) { > + if (output != def->sounds[i]->output) { > + output = def->sounds[i]->output; > + break; > + } > + } > + > + /* For convenience we will copy the first configured sound output to all > + * sound devices that doesn't have any output configured because QEMU s/doesn't/don't/ > + * will use only one output for all sound devices. */ > + if (output != VIR_DOMAIN_SOUND_OUTPUT_TYPE_DEFAULT) { > + for (i = 0; i < def->nsounds; i++) { > + if (def->sounds[i]->output == VIR_DOMAIN_SOUND_OUTPUT_TYPE_DEFAULT) > + def->sounds[i]->output = output; > + } > + } > +} > + > + > static int > qemuDomainDefPostParseBasic(virDomainDefPtr def, > virCapsPtr caps, > @@ -3221,6 +3246,8 @@ qemuDomainDefPostParse(virDomainDefPtr def, > if (qemuDomainDefCPUPostParse(def) < 0) > goto cleanup; > > + qemuDomainDefSoundPostParse(def); > + > ret = 0; > cleanup: > virObjectUnref(cfg); > @@ -3301,6 +3328,30 @@ qemuDomainDefValidateVideo(const virDomainDef *def) > } > > > +static int > +qemuDomainDefValidateSound(const virDomainDef *def) > +{ > + size_t i; > + virDomainSoundOutputType output = VIR_DOMAIN_SOUND_OUTPUT_TYPE_DEFAULT; > + > + for (i = 0; i < def->nsounds; i++) { > + if (output == VIR_DOMAIN_SOUND_OUTPUT_TYPE_DEFAULT) { > + output = def->sounds[i]->output; > + continue; > + } > + > + if (output != def->sounds[i]->output) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("all sound devices must be configured to use " > + "the same output")); > + return -1; > + } > + } > + > + return 0; > +} > + > + > #define QEMU_MAX_VCPUS_WITHOUT_EIM 255 > > > @@ -3403,6 +3454,9 @@ qemuDomainDefValidate(const virDomainDef *def, > if (qemuDomainDefValidateVideo(def) < 0) > goto cleanup; > > + if (qemuDomainDefValidateSound(def) < 0) > + goto cleanup; > + > ret = 0; > > cleanup: > diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c > index 6d242b1b51..2957c4a074 100644 > --- a/src/qemu/qemu_process.c > +++ b/src/qemu/qemu_process.c > @@ -5393,6 +5393,45 @@ qemuProcessPrepareAllowReboot(virDomainObjPtr vm) > } > > > +static void > +qemuProcessPrepareSound(virDomainDefPtr def, > + virQEMUDriverConfigPtr cfg) > +{ > + virDomainSoundOutputType output = VIR_DOMAIN_SOUND_OUTPUT_TYPE_DEFAULT; > + size_t i; > + > + if (def->nsounds == 0) > + return; > + > + if (def->ngraphics == 0) { > + if (!cfg->nogfxAllowHostAudio) > + output = VIR_DOMAIN_SOUND_OUTPUT_TYPE_NONE; > + } else { > + switch (def->graphics[def->ngraphics - 1]->type) { > + case VIR_DOMAIN_GRAPHICS_TYPE_SPICE: > + output = VIR_DOMAIN_SOUND_OUTPUT_TYPE_SPICE; > + break; > + > + case VIR_DOMAIN_GRAPHICS_TYPE_VNC: > + if (!cfg->vncAllowHostAudio) > + output = VIR_DOMAIN_SOUND_OUTPUT_TYPE_NONE; > + break; > + > + case VIR_DOMAIN_GRAPHICS_TYPE_SDL: > + case VIR_DOMAIN_GRAPHICS_TYPE_RDP: > + case VIR_DOMAIN_GRAPHICS_TYPE_DESKTOP: > + case VIR_DOMAIN_GRAPHICS_TYPE_LAST: > + break; > + } > + } > + > + for (i = 0; i < def->nsounds; i++) { > + if (def->sounds[i]->output == VIR_DOMAIN_SOUND_OUTPUT_TYPE_DEFAULT) > + def->sounds[i]->output = output; > + } > +} > + > + > /** > * qemuProcessPrepareDomain: > * @conn: connection object (for looking up storage volumes) > @@ -5513,6 +5552,8 @@ qemuProcessPrepareDomain(virConnectPtr conn, > goto cleanup; > } > > + qemuProcessPrepareSound(vm->def, cfg); > + > ret = 0; > cleanup: > virObjectUnref(caps); > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
