[libvirt] [PATCH 4/6] conf: introduce <output> element for <sound> devices

Pavel Hrdina posted 6 patches 7 years, 6 months ago
[libvirt] [PATCH 4/6] conf: introduce <output> element for <sound> devices
Posted by Pavel Hrdina 7 years, 6 months ago
So far it was not possible to specify how the audio output from guest
should be presented to host/users.  Now it will be possible to do so
via <output> element for <sound> device where you specify the output
"type".

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 docs/formatdomain.html.in     |  9 +++++++
 docs/schemas/domaincommon.rng | 14 ++++++++++
 src/conf/domain_conf.c        | 61 +++++++++++++++++++++++++++++++++++++++++++
 src/conf/domain_conf.h        | 14 ++++++++++
 src/libvirt_private.syms      |  2 ++
 5 files changed, 100 insertions(+)

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
index 47c43d0666..3b7c367fc7 100644
--- a/docs/formatdomain.html.in
+++ b/docs/formatdomain.html.in
@@ -7058,6 +7058,15 @@ qemu-kvm -net nic,model=? /dev/null
       slot, <a href="#elementsAddress">documented above</a>.
     </p>
 
+    <p>
+      <span class="since">Since 3.10.0</span> sound device can have
+      an optional <code>output</code> element which configures where
+      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.
+    </p>
+
     <h4><a id="elementsWatchdog">Watchdog device</a></h4>
 
     <p>
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
index 9cec1a0637..c499229c43 100644
--- a/docs/schemas/domaincommon.rng
+++ b/docs/schemas/domaincommon.rng
@@ -3803,6 +3803,20 @@
         <zeroOrMore>
           <ref name="codec"/>
         </zeroOrMore>
+        <optional>
+          <element name="output">
+            <attribute name="type">
+              <choice>
+                <value>none</value>
+                <value>spice</value>
+                <value>pa</value>
+                <value>sdl</value>
+                <value>alsa</value>
+                <value>oss</value>
+              </choice>
+            </attribute>
+          </element>
+        </optional>
       </interleave>
     </element>
   </define>
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index fffcc8e9da..33e59c7667 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -517,6 +517,15 @@ VIR_ENUM_IMPL(virDomainSoundModel, VIR_DOMAIN_SOUND_MODEL_LAST,
               "ich9",
               "usb")
 
+VIR_ENUM_IMPL(virDomainSoundOutput, VIR_DOMAIN_SOUND_OUTPUT_TYPE_LAST,
+              "default",
+              "none",
+              "spice",
+              "pa",
+              "sdl",
+              "alsa",
+              "oss")
+
 VIR_ENUM_IMPL(virDomainKeyWrapCipherName,
               VIR_DOMAIN_KEY_WRAP_CIPHER_NAME_LAST,
               "aes",
@@ -13687,6 +13696,50 @@ virDomainSoundCodecDefParseXML(xmlNodePtr node)
 }
 
 
+static int
+virDomainSoundOutputParseXML(xmlXPathContextPtr ctxt,
+                             virDomainSoundDefPtr sound)
+{
+    int ret = -1;
+    char *type = NULL;
+    int typeVal;
+    xmlNodePtr *outputNodes = NULL;
+    int noutputs;
+
+    noutputs = virXPathNodeSet("./output", ctxt, &outputNodes);
+    if (noutputs < 0)
+        return -1;
+
+    if (noutputs > 1) {
+        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                       _("sound device can have only one output configured"));
+        goto cleanup;
+    }
+
+    if (noutputs > 0) {
+        if (!(type = virXMLPropString(outputNodes[0], "type"))) {
+            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
+                           _("sound output type must be specified"));
+            goto cleanup;
+        }
+
+        if ((typeVal = virDomainSoundOutputTypeFromString(type)) < 0) {
+            virReportError(VIR_ERR_INVALID_ARG,
+                           _("invalid sound output type '%s'"), type);
+            goto cleanup;
+        }
+
+        sound->output = typeVal;
+    }
+
+    ret = 0;
+ cleanup:
+    VIR_FREE(outputNodes);
+    VIR_FREE(type);
+    return ret;
+}
+
+
 static virDomainSoundDefPtr
 virDomainSoundDefParseXML(virDomainXMLOptionPtr xmlopt,
                           xmlNodePtr node,
@@ -13741,6 +13794,9 @@ virDomainSoundDefParseXML(virDomainXMLOptionPtr xmlopt,
         }
     }
 
+    if (virDomainSoundOutputParseXML(ctxt, def) < 0)
+        goto error;
+
     if (virDomainDeviceInfoParseXML(xmlopt, node, NULL, &def->info, flags) < 0)
         goto error;
 
@@ -24111,6 +24167,11 @@ virDomainSoundDefFormat(virBufferPtr buf,
 
     virDomainDeviceInfoFormat(&childBuf, &def->info, flags);
 
+    if (def->output != VIR_DOMAIN_SOUND_OUTPUT_TYPE_DEFAULT) {
+        virBufferAsprintf(&childBuf, "<output type='%s'/>\n",
+                          virDomainSoundOutputTypeToString(def->output));
+    }
+
     if (virBufferCheckError(&childBuf) < 0)
         return -1;
 
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index e3f060b122..55a984c781 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1332,12 +1332,25 @@ struct _virDomainSoundCodecDef {
     int cad;
 };
 
+typedef enum {
+    VIR_DOMAIN_SOUND_OUTPUT_TYPE_DEFAULT,
+    VIR_DOMAIN_SOUND_OUTPUT_TYPE_NONE,
+    VIR_DOMAIN_SOUND_OUTPUT_TYPE_SPICE,
+    VIR_DOMAIN_SOUND_OUTPUT_TYPE_PA,
+    VIR_DOMAIN_SOUND_OUTPUT_TYPE_SDL,
+    VIR_DOMAIN_SOUND_OUTPUT_TYPE_ALSA,
+    VIR_DOMAIN_SOUND_OUTPUT_TYPE_OSS,
+
+    VIR_DOMAIN_SOUND_OUTPUT_TYPE_LAST
+} virDomainSoundOutputType;
+
 struct _virDomainSoundDef {
     int model;
     virDomainDeviceInfo info;
 
     size_t ncodecs;
     virDomainSoundCodecDefPtr *codecs;
+    virDomainSoundOutputType output;
 };
 
 typedef enum {
@@ -3246,6 +3259,7 @@ VIR_ENUM_DECL(virDomainChrTcpProtocol)
 VIR_ENUM_DECL(virDomainChrSpicevmc)
 VIR_ENUM_DECL(virDomainSoundCodec)
 VIR_ENUM_DECL(virDomainSoundModel)
+VIR_ENUM_DECL(virDomainSoundOutput)
 VIR_ENUM_DECL(virDomainKeyWrapCipherName)
 VIR_ENUM_DECL(virDomainMemballoonModel)
 VIR_ENUM_DECL(virDomainSmbiosMode)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 5a4d50471d..0ef7e896d8 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -516,6 +516,8 @@ virDomainSmbiosModeTypeToString;
 virDomainSoundDefFree;
 virDomainSoundModelTypeFromString;
 virDomainSoundModelTypeToString;
+virDomainSoundOutputTypeFromString;
+virDomainSoundOutputTypeToString;
 virDomainStartupPolicyTypeFromString;
 virDomainStartupPolicyTypeToString;
 virDomainStateReasonFromString;
-- 
2.13.6

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/6] conf: introduce <output> element for <sound> devices
Posted by John Ferlan 7 years, 5 months ago

On 11/14/2017 08:45 AM, Pavel Hrdina wrote:
> So far it was not possible to specify how the audio output from guest

s/So far it was/It is/

> should be presented to host/users.  Now it will be possible to do so
> via <output> element for <sound> device where you specify the output
> "type".
> 
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>  docs/formatdomain.html.in     |  9 +++++++
>  docs/schemas/domaincommon.rng | 14 ++++++++++
>  src/conf/domain_conf.c        | 61 +++++++++++++++++++++++++++++++++++++++++++
>  src/conf/domain_conf.h        | 14 ++++++++++
>  src/libvirt_private.syms      |  2 ++
>  5 files changed, 100 insertions(+)
> 
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 47c43d0666..3b7c367fc7 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -7058,6 +7058,15 @@ qemu-kvm -net nic,model=? /dev/null
>        slot, <a href="#elementsAddress">documented above</a>.
>      </p>
>  
> +    <p>
> +      <span class="since">Since 3.10.0</span> sound device can have

s/sound/, a sound/

> +      an optional <code>output</code> element which configures where
> +      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'.

I've become preferential to seeing these in a list. I have no idea based
on the text here what 'spice', ... 'oss' really means.  IOW: Why would I
want to set to something else - what does it do/provide?

> +      This might not be supported by all hypervisors.

True, but perhaps also true of many other things, too.

> +    </p>
> +
>      <h4><a id="elementsWatchdog">Watchdog device</a></h4>
>  
>      <p>


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

John

[...]

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/6] conf: introduce <output> element for <sound> devices
Posted by Pavel Hrdina 7 years, 5 months ago
On Mon, Nov 20, 2017 at 06:04:27PM -0500, John Ferlan wrote:
> 
> 
> On 11/14/2017 08:45 AM, Pavel Hrdina wrote:
> > So far it was not possible to specify how the audio output from guest
> 
> s/So far it was/It is/
> 
> > should be presented to host/users.  Now it will be possible to do so
> > via <output> element for <sound> device where you specify the output
> > "type".
> > 
> > Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> > ---
> >  docs/formatdomain.html.in     |  9 +++++++
> >  docs/schemas/domaincommon.rng | 14 ++++++++++
> >  src/conf/domain_conf.c        | 61 +++++++++++++++++++++++++++++++++++++++++++
> >  src/conf/domain_conf.h        | 14 ++++++++++
> >  src/libvirt_private.syms      |  2 ++
> >  5 files changed, 100 insertions(+)
> > 
> > diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> > index 47c43d0666..3b7c367fc7 100644
> > --- a/docs/formatdomain.html.in
> > +++ b/docs/formatdomain.html.in
> > @@ -7058,6 +7058,15 @@ qemu-kvm -net nic,model=? /dev/null
> >        slot, <a href="#elementsAddress">documented above</a>.
> >      </p>
> >  
> > +    <p>
> > +      <span class="since">Since 3.10.0</span> sound device can have
> 
> s/sound/, a sound/
> 
> > +      an optional <code>output</code> element which configures where
> > +      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'.
> 
> I've become preferential to seeing these in a list. I have no idea based
> on the text here what 'spice', ... 'oss' really means.  IOW: Why would I
> want to set to something else - what does it do/provide?

I'm not sure how to describe it more than providing links to all the
projects.  SPICE is the only unique output since the audio stream is
sent over SPICE protocol, but the remaining outputs are IMHO
self-explanatory because the are standard linux/unix audio
layers/libraries.

Describing all the differences is out of scope of our documentation.

> 
> > +      This might not be supported by all hypervisors.
> 
> True, but perhaps also true of many other things, too.

I can change it to "Currently supported only by QEMU driver."

> > +    </p>
> > +
> >      <h4><a id="elementsWatchdog">Watchdog device</a></h4>
> >  
> >      <p>
> 
> 
> Reviewed-by: John Ferlan <jferlan@redhat.com>
> 
> John
> 
> [...]
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/6] conf: introduce <output> element for <sound> devices
Posted by John Ferlan 7 years, 5 months ago

On 11/21/2017 08:12 AM, Pavel Hrdina wrote:
> On Mon, Nov 20, 2017 at 06:04:27PM -0500, John Ferlan wrote:
>>
>>
>> On 11/14/2017 08:45 AM, Pavel Hrdina wrote:
>>> So far it was not possible to specify how the audio output from guest
>>
>> s/So far it was/It is/
>>
>>> should be presented to host/users.  Now it will be possible to do so
>>> via <output> element for <sound> device where you specify the output
>>> "type".
>>>
>>> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
>>> ---
>>>  docs/formatdomain.html.in     |  9 +++++++
>>>  docs/schemas/domaincommon.rng | 14 ++++++++++
>>>  src/conf/domain_conf.c        | 61 +++++++++++++++++++++++++++++++++++++++++++
>>>  src/conf/domain_conf.h        | 14 ++++++++++
>>>  src/libvirt_private.syms      |  2 ++
>>>  5 files changed, 100 insertions(+)
>>>
>>> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
>>> index 47c43d0666..3b7c367fc7 100644
>>> --- a/docs/formatdomain.html.in
>>> +++ b/docs/formatdomain.html.in
>>> @@ -7058,6 +7058,15 @@ qemu-kvm -net nic,model=? /dev/null
>>>        slot, <a href="#elementsAddress">documented above</a>.
>>>      </p>
>>>  
>>> +    <p>
>>> +      <span class="since">Since 3.10.0</span> sound device can have
>>
>> s/sound/, a sound/
>>
>>> +      an optional <code>output</code> element which configures where
>>> +      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'.
>>
>> I've become preferential to seeing these in a list. I have no idea based
>> on the text here what 'spice', ... 'oss' really means.  IOW: Why would I
>> want to set to something else - what does it do/provide?
> 
> I'm not sure how to describe it more than providing links to all the
> projects.  SPICE is the only unique output since the audio stream is
> sent over SPICE protocol, but the remaining outputs are IMHO
> self-explanatory because the are standard linux/unix audio
> layers/libraries.

I was reading literally vis-a-vis a list of supported values and there
was something missing. I was not thinking in terms that the list is/was
from the standard linux/unix audio layers/libraries. As I've learned
since originally looking at this - the overall design of graphics/sound
is a bit, well, odd/unusual...  Anwyay restated:

There is mandatory <code>type</code> attribute where valid values to
enable the audio output are 'spice', 'pa', 'sdl', 'alsa', or 'oss'. Use
'none' in order to disable the audio output.

With a bit more thought on this now - why would someone add a sound
card, but select an output of 'none'?  Why even add the sound card then?
You have a sound card that doesn't emit sound anywhere? Why have it?!

And in summary, it seems to me now that what the 'output' is designed to
do is supply the driver for the QEMU_AUDIO_DRV environment variable,
true? To override what may be globally set?


> 
> Describing all the differences is out of scope of our documentation.
> 

Hmmm.. maybe I wasn't clear enough - it wasn't the differences that I
cared about it was the what are those shorthand acronyms. Someone
reading the list literally may not know what they are. I certainly don't
think open sound system (I had to look it up)... So pa is perhaps a
known synonym for pulseaudio...

>>
>>> +      This might not be supported by all hypervisors.
>>
>> True, but perhaps also true of many other things, too.
> 
> I can change it to "Currently supported only by QEMU driver."
> 

OK, so this makes sense in light of what appears to be the goal - to
provide something for the env variable.


BTW: Probably should add the xml2xml parsing tests to this patch -
although they are present later - it is something important to ask of
any XML changes.


John

>>> +    </p>
>>> +
>>>      <h4><a id="elementsWatchdog">Watchdog device</a></h4>
>>>  
>>>      <p>
>>
>>
>> Reviewed-by: John Ferlan <jferlan@redhat.com>
>>
>> John
>>
>> [...]
>>
>> --
>> libvir-list mailing list
>> libvir-list@redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 4/6] conf: introduce <output> element for <sound> devices
Posted by Daniel P. Berrange 7 years, 5 months ago
On Tue, Nov 14, 2017 at 02:45:09PM +0100, Pavel Hrdina wrote:
> So far it was not possible to specify how the audio output from guest
> should be presented to host/users.  Now it will be possible to do so
> via <output> element for <sound> device where you specify the output
> "type".
> 
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>  docs/formatdomain.html.in     |  9 +++++++
>  docs/schemas/domaincommon.rng | 14 ++++++++++
>  src/conf/domain_conf.c        | 61 +++++++++++++++++++++++++++++++++++++++++++
>  src/conf/domain_conf.h        | 14 ++++++++++
>  src/libvirt_private.syms      |  2 ++
>  5 files changed, 100 insertions(+)
> 

> +static int
> +virDomainSoundOutputParseXML(xmlXPathContextPtr ctxt,
> +                             virDomainSoundDefPtr sound)
> +{
> +    int ret = -1;
> +    char *type = NULL;
> +    int typeVal;
> +    xmlNodePtr *outputNodes = NULL;
> +    int noutputs;
> +
> +    noutputs = virXPathNodeSet("./output", ctxt, &outputNodes);
> +    if (noutputs < 0)
> +        return -1;
> +
> +    if (noutputs > 1) {
> +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                       _("sound device can have only one output configured"));
> +        goto cleanup;
> +    }

The implication of this is that if you have multiple display backends enabled
only one of them will ever be selectable as the audio backend. It is not
unreasonable to consider that audio should go to all backends, or depending
on which is in use.  eg if QEMU has SDL + SPICE enabled, audio could
conceptually go to SDL by default, and get dynamically switched to SPICE
whenver there is a SPICE client actively connected. This is a risk of
trying to design a configuration approach for this, without us asking
QEMU to consider their corresponding design possibilities

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
Re: [libvirt] [PATCH 4/6] conf: introduce <output> element for <sound> devices
Posted by Pavel Hrdina 7 years, 5 months ago
On Tue, Nov 21, 2017 at 01:25:33PM +0000, Daniel P. Berrange wrote:
> On Tue, Nov 14, 2017 at 02:45:09PM +0100, Pavel Hrdina wrote:
> > So far it was not possible to specify how the audio output from guest
> > should be presented to host/users.  Now it will be possible to do so
> > via <output> element for <sound> device where you specify the output
> > "type".
> > 
> > Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> > ---
> >  docs/formatdomain.html.in     |  9 +++++++
> >  docs/schemas/domaincommon.rng | 14 ++++++++++
> >  src/conf/domain_conf.c        | 61 +++++++++++++++++++++++++++++++++++++++++++
> >  src/conf/domain_conf.h        | 14 ++++++++++
> >  src/libvirt_private.syms      |  2 ++
> >  5 files changed, 100 insertions(+)
> > 
> 
> > +static int
> > +virDomainSoundOutputParseXML(xmlXPathContextPtr ctxt,
> > +                             virDomainSoundDefPtr sound)
> > +{
> > +    int ret = -1;
> > +    char *type = NULL;
> > +    int typeVal;
> > +    xmlNodePtr *outputNodes = NULL;
> > +    int noutputs;
> > +
> > +    noutputs = virXPathNodeSet("./output", ctxt, &outputNodes);
> > +    if (noutputs < 0)
> > +        return -1;
> > +
> > +    if (noutputs > 1) {
> > +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> > +                       _("sound device can have only one output configured"));
> > +        goto cleanup;
> > +    }
> 
> The implication of this is that if you have multiple display backends enabled
> only one of them will ever be selectable as the audio backend. It is not
> unreasonable to consider that audio should go to all backends, or depending
> on which is in use.  eg if QEMU has SDL + SPICE enabled, audio could
> conceptually go to SDL by default, and get dynamically switched to SPICE
> whenver there is a SPICE client actively connected. This is a risk of
> trying to design a configuration approach for this, without us asking
> QEMU to consider their corresponding design possibilities

Having this limitation now doesn't mean that we cannot remove it in the
future if QEMU implements a way how to configure multiple audio
backends.  I can move it into qemuValidateCallback to make it QEMU
specific and if we need to remove it we can do so.

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