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

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. */
+    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
+     * 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);
-- 
2.13.6

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

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

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

On Mon, Nov 20, 2017 at 07:44:25PM -0500, John Ferlan wrote:
> 
> 
> 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?

It would be probably possible to split this patch into two separate
changes:

    1. move the code out of command line generation into
    qemuProcessPrepareSound()

    2. the rest of this patch

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

The PostParse function tries to find the first sound device with output
configured (the first for loop) and sets this output to all other sound
devices without any output configured (the second for loop).  This is
executed while parsing domain XML and implements the new feature.

The Prepare function is executed only while starting domain and
basically supplements the code removed from building command line.
It prepares only live definition that is about to be started so
the qemu command line code can only take the definition and convert
it into command line without any logic or without modifying the
live definition.

> 
> 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...

Actually no, this is tied together, the PostParse and Validate callbacks
make sure that the definition is correct so that qemu build command line
doesn't have to check anything and simply takes the definition as it is
and converts it into command line.

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

On 11/21/2017 08:42 AM, Pavel Hrdina wrote:
> On Mon, Nov 20, 2017 at 07:44:25PM -0500, John Ferlan wrote:
>>
>>
>> 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?
> 
> It would be probably possible to split this patch into two separate
> changes:
> 
>     1. move the code out of command line generation into
>     qemuProcessPrepareSound()
> 
>     2. the rest of this patch
> 
>> 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 ;-).
> 
> The PostParse function tries to find the first sound device with output
> configured (the first for loop) and sets this output to all other sound
> devices without any output configured (the second for loop).  This is
> executed while parsing domain XML and implements the new feature.

Understood, but it also changes each configured sound device that didn't
have <output> defined to have the <output> value from the one that did
have it.

That would then be saved - something that would have been shown in the
qemuxml2xmltest output, e.g the multi output from patch 6 would be:


    <sound model='ich6'>
      <address type='pci' domain='0x0000' bus='0x00' slot='0x03'
function='0x0'/>
      <output type='pa'/>
    </sound>
    <sound model='ich6'>
      <address type='pci' domain='0x0000' bus='0x00' slot='0x04'
function='0x0'/>
      <output type='pa'/>
    </sound>

I also note that <output> comes after <address>... not that it matters,
but my brain recollects that <address> is generally last for most
elements, although not required to be last - it just is generally.

In any case, I'm not sure it's "right" to change/add that output. It
should be simple enough to ignore those that don't define some output.
That was my point about making an optional element required.

Being able to provide/determine some default when no sound device has an
output defined would thus become the "job" of the Prepare API I think.

> 
> The Prepare function is executed only while starting domain and
> basically supplements the code removed from building command line.
> It prepares only live definition that is about to be started so
> the qemu command line code can only take the definition and convert
> it into command line without any logic or without modifying the
> live definition.
> 

Right - these are the live entries not the config/saved defs - so to
that degree altering things does make some sense. However, your choice
to modify each live entry, but really only use the [0]th one in the
command line building makes me want to consider whether it's better to
create some sort of qemuDomainObjPrivate field instead. Since there can
only be "one" (at this time) it would seem to be mechanism used
elsewhere to keep track of QEMU private and specific shtuff.

>>
>> 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...
> 
> Actually no, this is tied together, the PostParse and Validate callbacks
> make sure that the definition is correct so that qemu build command line
> doesn't have to check anything and simply takes the definition as it is
> and converts it into command line.
> 
> Pavel
> 

I always have to go back through to context switch in the ordering of
processing and the "rules" between PostParse and Validate...

The reason I remarked here was purely based on order. That is, it's not
clear until later that the [0]th entry could/would be filled in
automagically...

John

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

On Tue, Nov 21, 2017 at 10:52:48AM -0500, John Ferlan wrote:
> 
> 
> On 11/21/2017 08:42 AM, Pavel Hrdina wrote:
> > On Mon, Nov 20, 2017 at 07:44:25PM -0500, John Ferlan wrote:
> >>
> >>
> >> 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?
> > 
> > It would be probably possible to split this patch into two separate
> > changes:
> > 
> >     1. move the code out of command line generation into
> >     qemuProcessPrepareSound()
> > 
> >     2. the rest of this patch
> > 
> >> 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 ;-).
> > 
> > The PostParse function tries to find the first sound device with output
> > configured (the first for loop) and sets this output to all other sound
> > devices without any output configured (the second for loop).  This is
> > executed while parsing domain XML and implements the new feature.
> 
> Understood, but it also changes each configured sound device that didn't
> have <output> defined to have the <output> value from the one that did
> have it.
> 
> That would then be saved - something that would have been shown in the
> qemuxml2xmltest output, e.g the multi output from patch 6 would be:
> 
> 
>     <sound model='ich6'>
>       <address type='pci' domain='0x0000' bus='0x00' slot='0x03'
> function='0x0'/>
>       <output type='pa'/>
>     </sound>
>     <sound model='ich6'>
>       <address type='pci' domain='0x0000' bus='0x00' slot='0x04'
> function='0x0'/>
>       <output type='pa'/>
>     </sound>
> 
> I also note that <output> comes after <address>... not that it matters,
> but my brain recollects that <address> is generally last for most
> elements, although not required to be last - it just is generally.

Yeah, semantically its fine, but it'd be better to stuck with our
convention that address & alias are last.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

On Tue, Nov 21, 2017 at 10:52:48AM -0500, John Ferlan wrote:
> 
> 
> On 11/21/2017 08:42 AM, Pavel Hrdina wrote:
> > On Mon, Nov 20, 2017 at 07:44:25PM -0500, John Ferlan wrote:
> >>
> >>
> >> 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?
> > 
> > It would be probably possible to split this patch into two separate
> > changes:
> > 
> >     1. move the code out of command line generation into
> >     qemuProcessPrepareSound()
> > 
> >     2. the rest of this patch
> > 
> >> 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 ;-).
> > 
> > The PostParse function tries to find the first sound device with output
> > configured (the first for loop) and sets this output to all other sound
> > devices without any output configured (the second for loop).  This is
> > executed while parsing domain XML and implements the new feature.
> 
> Understood, but it also changes each configured sound device that didn't
> have <output> defined to have the <output> value from the one that did
> have it.
> 
> That would then be saved - something that would have been shown in the
> qemuxml2xmltest output, e.g the multi output from patch 6 would be:
> 
> 
>     <sound model='ich6'>
>       <address type='pci' domain='0x0000' bus='0x00' slot='0x03'
> function='0x0'/>
>       <output type='pa'/>
>     </sound>
>     <sound model='ich6'>
>       <address type='pci' domain='0x0000' bus='0x00' slot='0x04'
> function='0x0'/>
>       <output type='pa'/>
>     </sound>

Which part of the PostParse code does that?  If you configure the domain
XML like this:

    <sound model='ich6'/>
    <sound model='ich6'/>

The resulting config XML saved to disk would be:

    <sound model='ich6'>
      <<address type='pci' domain='0x0000' bus='0x00' slot='0x03' unction='0x0'/>
    </sound>
    <sound model='ich6'>
      <<address type='pci' domain='0x0000' bus='0x00' slot='0x04' unction='0x0'/>
    </sound>

One of the reasons why I didn't add qemuxml2xml tests is that only the
offline part is somehow working, but the active and status part of that
test is broken and doesn't reflect how libvirt changes the active and
status XML.

> I also note that <output> comes after <address>... not that it matters,
> but my brain recollects that <address> is generally last for most
> elements, although not required to be last - it just is generally.

Good point, I'll fix that.

> In any case, I'm not sure it's "right" to change/add that output. It
> should be simple enough to ignore those that don't define some output.
> That was my point about making an optional element required.
> 
> Being able to provide/determine some default when no sound device has an
> output defined would thus become the "job" of the Prepare API I think.

Well, that's how it works with this patches.

> > 
> > The Prepare function is executed only while starting domain and
> > basically supplements the code removed from building command line.
> > It prepares only live definition that is about to be started so
> > the qemu command line code can only take the definition and convert
> > it into command line without any logic or without modifying the
> > live definition.
> > 
> 
> Right - these are the live entries not the config/saved defs - so to
> that degree altering things does make some sense. However, your choice
> to modify each live entry, but really only use the [0]th one in the
> command line building makes me want to consider whether it's better to
> create some sort of qemuDomainObjPrivate field instead. Since there can
> only be "one" (at this time) it would seem to be mechanism used
> elsewhere to keep track of QEMU private and specific shtuff.

If there wouldn't be the output attribute introduced by this patch
series I would agree with you to using qemuDomainObjPrivate field, but
since this patch series introduces the output attribute and required
field in structure for sound devices we should use that one and not
introduce yet another place where to store this information.

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

On 11/21/2017 12:12 PM, Pavel Hrdina wrote:
> On Tue, Nov 21, 2017 at 10:52:48AM -0500, John Ferlan wrote:
>>
>>
>> On 11/21/2017 08:42 AM, Pavel Hrdina wrote:
>>> On Mon, Nov 20, 2017 at 07:44:25PM -0500, John Ferlan wrote:
>>>>
>>>>
>>>> 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?
>>>
>>> It would be probably possible to split this patch into two separate
>>> changes:
>>>
>>>     1. move the code out of command line generation into
>>>     qemuProcessPrepareSound()
>>>
>>>     2. the rest of this patch
>>>
>>>> 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 ;-).
>>>
>>> The PostParse function tries to find the first sound device with output
>>> configured (the first for loop) and sets this output to all other sound
>>> devices without any output configured (the second for loop).  This is
>>> executed while parsing domain XML and implements the new feature.
>>
>> Understood, but it also changes each configured sound device that didn't
>> have <output> defined to have the <output> value from the one that did
>> have it.
>>
>> That would then be saved - something that would have been shown in the
>> qemuxml2xmltest output, e.g the multi output from patch 6 would be:
>>
>>
>>     <sound model='ich6'>
>>       <address type='pci' domain='0x0000' bus='0x00' slot='0x03'
>> function='0x0'/>
>>       <output type='pa'/>
>>     </sound>
>>     <sound model='ich6'>
>>       <address type='pci' domain='0x0000' bus='0x00' slot='0x04'
>> function='0x0'/>
>>       <output type='pa'/>
>>     </sound>
> 
> Which part of the PostParse code does that?  If you configure the domain
> XML like this:
> 
>     <sound model='ich6'/>
>     <sound model='ich6'/>
> 
> The resulting config XML saved to disk would be:
> 
>     <sound model='ich6'>
>       <<address type='pci' domain='0x0000' bus='0x00' slot='0x03' unction='0x0'/>
>     </sound>
>     <sound model='ich6'>
>       <<address type='pci' domain='0x0000' bus='0x00' slot='0x04' unction='0x0'/>
>     </sound>


>From patch 6 I used:

tests/qemuxml2argvdata/qemuxml2argv-sound-multi-pa-output-spice.xml

which has

    <sound model='ich6'>
      <output type='pa'/>
    </sound>
    <sound model='ich6'/>


> 
> One of the reasons why I didn't add qemuxml2xml tests is that only the
> offline part is somehow working, but the active and status part of that
> test is broken and doesn't reflect how libvirt changes the active and
> status XML.
> 

I didn't pay close attention to which was which, just that some output
xml was generated by the regenerate.

John

>> I also note that <output> comes after <address>... not that it matters,
>> but my brain recollects that <address> is generally last for most
>> elements, although not required to be last - it just is generally.
> 
> Good point, I'll fix that.
> 
>> In any case, I'm not sure it's "right" to change/add that output. It
>> should be simple enough to ignore those that don't define some output.
>> That was my point about making an optional element required.
>>
>> Being able to provide/determine some default when no sound device has an
>> output defined would thus become the "job" of the Prepare API I think.
> 
> Well, that's how it works with this patches.
> 
>>>
>>> The Prepare function is executed only while starting domain and
>>> basically supplements the code removed from building command line.
>>> It prepares only live definition that is about to be started so
>>> the qemu command line code can only take the definition and convert
>>> it into command line without any logic or without modifying the
>>> live definition.
>>>
>>
>> Right - these are the live entries not the config/saved defs - so to
>> that degree altering things does make some sense. However, your choice
>> to modify each live entry, but really only use the [0]th one in the
>> command line building makes me want to consider whether it's better to
>> create some sort of qemuDomainObjPrivate field instead. Since there can
>> only be "one" (at this time) it would seem to be mechanism used
>> elsewhere to keep track of QEMU private and specific shtuff.
> 
> If there wouldn't be the output attribute introduced by this patch
> series I would agree with you to using qemuDomainObjPrivate field, but
> since this patch series introduces the output attribute and required
> field in structure for sound devices we should use that one and not
> introduce yet another place where to store this information.
> 
> Pavel
> 

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

On Tue, Nov 21, 2017 at 12:44:04PM -0500, John Ferlan wrote:
> 
> 
> On 11/21/2017 12:12 PM, Pavel Hrdina wrote:
> > On Tue, Nov 21, 2017 at 10:52:48AM -0500, John Ferlan wrote:
> >>
> >>
> >> On 11/21/2017 08:42 AM, Pavel Hrdina wrote:
> >>> On Mon, Nov 20, 2017 at 07:44:25PM -0500, John Ferlan wrote:
> >>>>
> >>>>
> >>>> 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?
> >>>
> >>> It would be probably possible to split this patch into two separate
> >>> changes:
> >>>
> >>>     1. move the code out of command line generation into
> >>>     qemuProcessPrepareSound()
> >>>
> >>>     2. the rest of this patch
> >>>
> >>>> 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 ;-).
> >>>
> >>> The PostParse function tries to find the first sound device with output
> >>> configured (the first for loop) and sets this output to all other sound
> >>> devices without any output configured (the second for loop).  This is
> >>> executed while parsing domain XML and implements the new feature.
> >>
> >> Understood, but it also changes each configured sound device that didn't
> >> have <output> defined to have the <output> value from the one that did
> >> have it.
> >>
> >> That would then be saved - something that would have been shown in the
> >> qemuxml2xmltest output, e.g the multi output from patch 6 would be:
> >>
> >>
> >>     <sound model='ich6'>
> >>       <address type='pci' domain='0x0000' bus='0x00' slot='0x03'
> >> function='0x0'/>
> >>       <output type='pa'/>
> >>     </sound>
> >>     <sound model='ich6'>
> >>       <address type='pci' domain='0x0000' bus='0x00' slot='0x04'
> >> function='0x0'/>
> >>       <output type='pa'/>
> >>     </sound>
> > 
> > Which part of the PostParse code does that?  If you configure the domain
> > XML like this:
> > 
> >     <sound model='ich6'/>
> >     <sound model='ich6'/>
> > 
> > The resulting config XML saved to disk would be:
> > 
> >     <sound model='ich6'>
> >       <<address type='pci' domain='0x0000' bus='0x00' slot='0x03' unction='0x0'/>
> >     </sound>
> >     <sound model='ich6'>
> >       <<address type='pci' domain='0x0000' bus='0x00' slot='0x04' unction='0x0'/>
> >     </sound>
> 
> 
> >From patch 6 I used:
> 
> tests/qemuxml2argvdata/qemuxml2argv-sound-multi-pa-output-spice.xml
> 
> which has
> 
>     <sound model='ich6'>
>       <output type='pa'/>
>     </sound>
>     <sound model='ich6'/>

Right, it was not clear what you meant, next time please include this
input XML right away, it will save both of us some time to
understand each other :).

I see your point that this update could have been done in Prepare
function.  I chose this implementation to make it clear that there
is the limitation with QEMU, but it would also work to put it into
Prepare function.  That way if QEMU introduces support to configure
different output for each sound device users would benefit from it
automatically for existing domains once we add it also into libvirt.

> > 
> > One of the reasons why I didn't add qemuxml2xml tests is that only the
> > offline part is somehow working, but the active and status part of that
> > test is broken and doesn't reflect how libvirt changes the active and
> > status XML.
> > 
> 
> I didn't pay close attention to which was which, just that some output
> xml was generated by the regenerate.

I'll send a v2 to make sure that all the points and notes were addressed
correctly.

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