1. Patchew
  2. Libvirt
  3. [libvirt] [PATCH 0/6] implement support to configure sound output
  4. View patch

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

Pavel Hrdina posted 6 patches 7 years, 11 months ago
[libvirt] [PATCH 2/6] qemu: move QEMU_AUDIO_DRIVER out of graphic into sound
Posted by Pavel Hrdina 7 years, 11 months ago 
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@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) {
+        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);
+
+            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);
-
         /* 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 \
-- 
2.13.6

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/6] qemu: move QEMU_AUDIO_DRIVER out of graphic into sound
Posted by John Ferlan 7 years, 11 months ago 

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@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@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 \
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/6] qemu: move QEMU_AUDIO_DRIVER out of graphic into sound
Posted by Pavel Hrdina 7 years, 11 months ago 
On Mon, Nov 20, 2017 at 05:39:38PM -0500, John Ferlan wrote:
> 
> 
> 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@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.

This is intentional, I should have mentioned it in the commit message.
The original design was just wrong, nothing else.  Setting
"SDL_AUDIODRIVER" if the SDL audio output is not used is pointless
and we shouldn't do it.  I can move this change to separate patch.

Pavel
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 2/6] qemu: move QEMU_AUDIO_DRIVER out of graphic into sound
Posted by John Ferlan 7 years, 11 months ago 

On 11/21/2017 03:59 AM, Pavel Hrdina wrote:
> On Mon, Nov 20, 2017 at 05:39:38PM -0500, John Ferlan wrote:
>>
>>
>> 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@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.
> 
> This is intentional, I should have mentioned it in the commit message.
> The original design was just wrong, nothing else.  Setting
> "SDL_AUDIODRIVER" if the SDL audio output is not used is pointless
> and we shouldn't do it.  I can move this change to separate patch.
> 
> Pavel
> 

I figured you had gone through the history - I certain hadn't ;-)...  I
would say by this point in the review I was still "missing" the final
detail regarding the bug you're working on O:) - that the audio was
being 'tied to' that last device only. Guess I was thinking it could
somehow be magically applied to "any" device.

Anyway, long way of saying - it's fine here. Adding a bit more detail to
the commit message will help though.

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-8c64711

© 2016 - 2025 Red Hat, Inc.