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@redhat.com> --- 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);