[libvirt] [PATCH 2/6] qemu: move QEMU_AUDIO_DRIVER out of graphic into sound

[John Ferlan]

On 11/14/2017 08:45 AM, Pavel Hrdina wrote:
> Setting the default audio output depends on specific graphic device
> but requires having sound device configured as well and it's the sound
> device that handles the audio.
> 
> Signed-off-by: Pavel Hrdina <phrdina at redhat.com>
> ---
>  src/qemu/qemu_command.c                            | 84 +++++++++++++++-------
>  .../qemuxml2argv-clock-france.args                 |  2 +-
>  2 files changed, 58 insertions(+), 28 deletions(-)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index eb72db33ba..e1ef1b05fa 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -4442,10 +4442,62 @@ qemuBuildSoundCodecStr(virDomainSoundDefPtr sound,
>  }
>  >
> +static void
> +qemuBuildSoundAudioEnv(virCommandPtr cmd,
> +                       const virDomainDef *def,
> +                       virQEMUDriverConfigPtr cfg)
> +{
> +    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) {

So it's the "last" graphics device that then defines "how" this all
works?  Makes sense for QEMU_AUDIO_DRV since whichever is last would be
the winner and get the chicken dinner, but...

> +        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);
> +            virCommandAddEnvPassBlockSUID(cmd, "SDL_AUDIODRIVER", NULL);

... if there was more than one graphics device defined, where one was
SDL and it wasn't the last one, then theoretically at least this would
not be defined.

I think it's easily fixable...

> +
> +            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;
> +        }
> +    }
> +}
> +
> +
>  static int
>  qemuBuildSoundCommandLine(virCommandPtr cmd,
>                            const virDomainDef *def,
> -                          virQEMUCapsPtr qemuCaps)
> +                          virQEMUCapsPtr qemuCaps,
> +                          virQEMUDriverConfigPtr cfg)
>  {
>      size_t i, j;
>  
> @@ -4498,6 +4550,9 @@ qemuBuildSoundCommandLine(virCommandPtr cmd,
>              }
>          }
>      }
> +
> +    qemuBuildSoundAudioEnv(cmd, def, cfg);
> +
>      return 0;
>  }
>  
> @@ -7951,15 +8006,6 @@ qemuBuildGraphicsVNCCommandLine(virQEMUDriverConfigPtr cfg,
>      if (graphics->data.vnc.keymap)
>          virCommandAddArgList(cmd, "-k", graphics->data.vnc.keymap, NULL);
>  
> -    /* 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");
> -
>      return 0;
>  
>   error:
> @@ -8201,10 +8247,6 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg,
>      if (graphics->data.spice.keymap)
>          virCommandAddArgList(cmd, "-k",
>                               graphics->data.spice.keymap, NULL);
> -    /* 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");
>  
>      return 0;
>  
> @@ -8235,13 +8277,6 @@ qemuBuildGraphicsCommandLine(virQEMUDriverConfigPtr cfg,
>          if (graphics->data.sdl.fullscreen)
>              virCommandAddArg(cmd, "-full-screen");
>  
> -        /* 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);
> -        virCommandAddEnvPassBlockSUID(cmd, "SDL_AUDIODRIVER", NULL);

... perhaps the fix to above is to just keep SDL_AUDIODRIVER here, just
in case there's more than one graphics device and SDL isn't last.

Reviewed-by: John Ferlan <jferlan at redhat.com>

John

> -
>          /* New QEMU has this flag to let us explicitly ask for
>           * SDL graphics. This is better than relying on the
>           * default, since the default changes :-( */
> @@ -9995,11 +10030,6 @@ qemuBuildCommandLine(virQEMUDriverPtr driver,
>          } else {
>              virCommandAddArg(cmd, "-nographic");
>          }
> -
> -        if (cfg->nogfxAllowHostAudio)
> -            virCommandAddEnvPassBlockSUID(cmd, "QEMU_AUDIO_DRV", NULL);
> -        else
> -            virCommandAddEnvString(cmd, "QEMU_AUDIO_DRV=none");
>      }
>  
>      /* Disable global config files and default devices */
> @@ -10083,7 +10113,7 @@ qemuBuildCommandLine(virQEMUDriverPtr driver,
>      if (qemuBuildVideoCommandLine(cmd, def, qemuCaps) < 0)
>          goto error;
>  
> -    if (qemuBuildSoundCommandLine(cmd, def, qemuCaps) < 0)
> +    if (qemuBuildSoundCommandLine(cmd, def, qemuCaps, cfg) < 0)
>          goto error;
>  
>      if (qemuBuildWatchdogCommandLine(cmd, def, qemuCaps) < 0)
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-clock-france.args b/tests/qemuxml2argvdata/qemuxml2argv-clock-france.args
> index 9bde6d967b..2701179273 100644
> --- a/tests/qemuxml2argvdata/qemuxml2argv-clock-france.args
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-clock-france.args
> @@ -3,8 +3,8 @@ PATH=/bin \
>  HOME=/home/test \
>  USER=test \
>  LOGNAME=test \
> -QEMU_AUDIO_DRV=none \
>  TZ=Europe/Paris \
> +QEMU_AUDIO_DRV=none \
>  /usr/bin/qemu-system-i686 \
>  -name QEMUGuest1 \
>  -S \
>