[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 5/6] qemu: implement <output> element for <sound> devices




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);
> 


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]