[libvirt] [PATCH 5/6] qemu: implement <output> element for <sound> devices
John Ferlan
jferlan at redhat.com
Tue Nov 21 00:44:25 UTC 2017
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 at 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);
>
More information about the libvir-list
mailing list