[PATCH 1/3] qapi, audio: add query-audiodev command

Daniel P. Berrangé posted 3 patches 4 years, 11 months ago
[PATCH 1/3] qapi, audio: add query-audiodev command
Posted by Daniel P. Berrangé 4 years, 11 months ago
Way back in QEMU 4.0, the -audiodev command line option was introduced
for configuring audio backends. This CLI option does not use QemuOpts
so it is not visible for introspection in 'query-command-line-options',
instead using the QAPI Audiodev type.  Unfortunately there is also no
QMP command that uses the Audiodev type, so it is not introspectable
with 'query-qmp-schema' either.

This introduces a 'query-audiodev' command that simply reflects back
the list of configured -audiodev command line options. This in turn
makes Audiodev introspectable via 'query-qmp-schema'.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 audio/audio.c   | 19 +++++++++++++++++++
 qapi/audio.json | 13 +++++++++++++
 2 files changed, 32 insertions(+)

diff --git a/audio/audio.c b/audio/audio.c
index 6734c8af70..40a4bbd7ce 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -28,8 +28,10 @@
 #include "monitor/monitor.h"
 #include "qemu/timer.h"
 #include "qapi/error.h"
+#include "qapi/clone-visitor.h"
 #include "qapi/qobject-input-visitor.h"
 #include "qapi/qapi-visit-audio.h"
+#include "qapi/qapi-commands-audio.h"
 #include "qemu/cutils.h"
 #include "qemu/module.h"
 #include "sysemu/replay.h"
@@ -2201,3 +2203,20 @@ size_t audio_rate_get_bytes(struct audio_pcm_info *info, RateCtl *rate,
     rate->bytes_sent += ret;
     return ret;
 }
+
+AudiodevList *qmp_query_audiodevs(Error **errp)
+{
+    AudiodevList *ret = NULL, *prev = NULL, *curr;
+    AudiodevListEntry *e;
+    QSIMPLEQ_FOREACH(e, &audiodevs, next) {
+        curr = g_new0(AudiodevList, 1);
+        curr->value = QAPI_CLONE(Audiodev, e->dev);
+        if (prev) {
+            prev->next = curr;
+            prev = curr;
+        } else {
+            ret = prev = curr;
+        }
+    }
+    return ret;
+}
diff --git a/qapi/audio.json b/qapi/audio.json
index 9cba0df8a4..d7b91230d7 100644
--- a/qapi/audio.json
+++ b/qapi/audio.json
@@ -419,3 +419,16 @@
     'sdl':       'AudiodevSdlOptions',
     'spice':     'AudiodevGenericOptions',
     'wav':       'AudiodevWavOptions' } }
+
+##
+# @query-audiodevs:
+#
+# Returns information about audiodev configuration
+#
+# Returns: array of @Audiodev
+#
+# Since: 6.0
+#
+##
+{ 'command': 'query-audiodevs',
+  'returns': ['Audiodev'] }
-- 
2.29.2


Re: [PATCH 1/3] qapi, audio: add query-audiodev command
Posted by Eric Blake 4 years, 11 months ago
On 3/2/21 11:55 AM, Daniel P. Berrangé wrote:
> Way back in QEMU 4.0, the -audiodev command line option was introduced
> for configuring audio backends. This CLI option does not use QemuOpts
> so it is not visible for introspection in 'query-command-line-options',
> instead using the QAPI Audiodev type.  Unfortunately there is also no
> QMP command that uses the Audiodev type, so it is not introspectable
> with 'query-qmp-schema' either.
> 
> This introduces a 'query-audiodev' command that simply reflects back
> the list of configured -audiodev command line options. This in turn
> makes Audiodev introspectable via 'query-qmp-schema'.

Even if we never call the query-audiodev command, its mere existence is
useful ;)

> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  audio/audio.c   | 19 +++++++++++++++++++
>  qapi/audio.json | 13 +++++++++++++
>  2 files changed, 32 insertions(+)


> +AudiodevList *qmp_query_audiodevs(Error **errp)
> +{
> +    AudiodevList *ret = NULL, *prev = NULL, *curr;
> +    AudiodevListEntry *e;
> +    QSIMPLEQ_FOREACH(e, &audiodevs, next) {
> +        curr = g_new0(AudiodevList, 1);
> +        curr->value = QAPI_CLONE(Audiodev, e->dev);
> +        if (prev) {
> +            prev->next = curr;
> +            prev = curr;
> +        } else {
> +            ret = prev = curr;
> +        }

Please use QAPI_LIST_PREPEND here instead of open-coding it.

> +
> +##
> +# @query-audiodevs:
> +#
> +# Returns information about audiodev configuration
> +#
> +# Returns: array of @Audiodev
> +#
> +# Since: 6.0
> +#
> +##
> +{ 'command': 'query-audiodevs',
> +  'returns': ['Audiodev'] }
> 

Otherwise looks nice.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


Re: [PATCH 1/3] qapi, audio: add query-audiodev command
Posted by Philippe Mathieu-Daudé 4 years, 11 months ago
On 3/2/21 6:55 PM, Daniel P. Berrangé wrote:
> Way back in QEMU 4.0, the -audiodev command line option was introduced
> for configuring audio backends. This CLI option does not use QemuOpts
> so it is not visible for introspection in 'query-command-line-options',
> instead using the QAPI Audiodev type.  Unfortunately there is also no
> QMP command that uses the Audiodev type, so it is not introspectable
> with 'query-qmp-schema' either.
> 
> This introduces a 'query-audiodev' command that simply reflects back
> the list of configured -audiodev command line options. This in turn
> makes Audiodev introspectable via 'query-qmp-schema'.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  audio/audio.c   | 19 +++++++++++++++++++
>  qapi/audio.json | 13 +++++++++++++
>  2 files changed, 32 insertions(+)

> +
> +##
> +# @query-audiodevs:
> +#
> +# Returns information about audiodev configuration
> +#
> +# Returns: array of @Audiodev
> +#
> +# Since: 6.0
> +#
> +##
> +{ 'command': 'query-audiodevs',
> +  'returns': ['Audiodev'] }
> 

Can we use 'query-audiodev-backends' similarly to
'query-chardev-backends'?


Re: [PATCH 1/3] qapi, audio: add query-audiodev command
Posted by Philippe Mathieu-Daudé 4 years, 11 months ago
On 3/2/21 10:10 PM, Philippe Mathieu-Daudé wrote:
> On 3/2/21 6:55 PM, Daniel P. Berrangé wrote:
>> Way back in QEMU 4.0, the -audiodev command line option was introduced
>> for configuring audio backends. This CLI option does not use QemuOpts
>> so it is not visible for introspection in 'query-command-line-options',
>> instead using the QAPI Audiodev type.  Unfortunately there is also no
>> QMP command that uses the Audiodev type, so it is not introspectable
>> with 'query-qmp-schema' either.
>>
>> This introduces a 'query-audiodev' command that simply reflects back
>> the list of configured -audiodev command line options. This in turn
>> makes Audiodev introspectable via 'query-qmp-schema'.
>>
>> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>> ---
>>  audio/audio.c   | 19 +++++++++++++++++++
>>  qapi/audio.json | 13 +++++++++++++
>>  2 files changed, 32 insertions(+)
> 
>> +
>> +##
>> +# @query-audiodevs:
>> +#
>> +# Returns information about audiodev configuration
>> +#
>> +# Returns: array of @Audiodev

Also with chardev we return ChardevBackendInfo. I'm not sure
if this is because I'm custom to read it, but it seems clearer.
Can we return a AudiodevBackendInfo type?

>> +#
>> +# Since: 6.0
>> +#
>> +##
>> +{ 'command': 'query-audiodevs',
>> +  'returns': ['Audiodev'] }
>>
> 
> Can we use 'query-audiodev-backends' similarly to
> 'query-chardev-backends'?
> 


Re: [PATCH 1/3] qapi, audio: add query-audiodev command
Posted by Markus Armbruster 4 years, 11 months ago
Daniel P. Berrangé <berrange@redhat.com> writes:

> Way back in QEMU 4.0, the -audiodev command line option was introduced
> for configuring audio backends. This CLI option does not use QemuOpts
> so it is not visible for introspection in 'query-command-line-options',
> instead using the QAPI Audiodev type.  Unfortunately there is also no
> QMP command that uses the Audiodev type, so it is not introspectable
> with 'query-qmp-schema' either.

This is a gap that will only widen.

By design, query-qmp-schema covers just QMP.  It doesn't cover the
QAPIfied parts of the CLI.  They have been growing slowly, and this
trend will continue.  We need schema introspection to cover the CLI,
too.  Observation, not demand.

Work-arounds:

1. When a QMP command equivalent to a QAPIfied CLI option exists,
introspect that.  Involves hardcoding the connection between the two.
Example: -blockdev and blockdev-add.

2. When a QMP query exists that returns the CLI option argument,
introspect that.  Involves hardcoding the connection between the two.
Example: -display and query-display-options.

3. When neither exists, create a suitable query just to enable
introspection.  Example: query-display-options.  Commit e1ca8f7e19
"qapi: add query-display-options command" explains this clearly:

    Add query-display-options command, which allows querying the qemu
    display configuration.  This isn't particularly useful, except it
    exposes QAPI type DisplayOptions in query-qmp-schema, so that libvirt
    can discover recently added -display parameter rendernode (commit
    d4dc4ab133b).  Works around lack of sufficiently powerful command line
    introspection.

> This introduces a 'query-audiodev' command that simply reflects back
> the list of configured -audiodev command line options. This in turn
> makes Audiodev introspectable via 'query-qmp-schema'.

Is the query just for enabling introspection, or does it have other
uses?

If the former, please steal from Gerd's explanation for your commit
message.

>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  audio/audio.c   | 19 +++++++++++++++++++
>  qapi/audio.json | 13 +++++++++++++
>  2 files changed, 32 insertions(+)
>
> diff --git a/audio/audio.c b/audio/audio.c
> index 6734c8af70..40a4bbd7ce 100644
> --- a/audio/audio.c
> +++ b/audio/audio.c
> @@ -28,8 +28,10 @@
>  #include "monitor/monitor.h"
>  #include "qemu/timer.h"
>  #include "qapi/error.h"
> +#include "qapi/clone-visitor.h"
>  #include "qapi/qobject-input-visitor.h"
>  #include "qapi/qapi-visit-audio.h"
> +#include "qapi/qapi-commands-audio.h"
>  #include "qemu/cutils.h"
>  #include "qemu/module.h"
>  #include "sysemu/replay.h"
> @@ -2201,3 +2203,20 @@ size_t audio_rate_get_bytes(struct audio_pcm_info *info, RateCtl *rate,
>      rate->bytes_sent += ret;
>      return ret;
>  }
> +
> +AudiodevList *qmp_query_audiodevs(Error **errp)
> +{
> +    AudiodevList *ret = NULL, *prev = NULL, *curr;
> +    AudiodevListEntry *e;
> +    QSIMPLEQ_FOREACH(e, &audiodevs, next) {
> +        curr = g_new0(AudiodevList, 1);
> +        curr->value = QAPI_CLONE(Audiodev, e->dev);
> +        if (prev) {
> +            prev->next = curr;
> +            prev = curr;
> +        } else {
> +            ret = prev = curr;
> +        }

Please use QAPI_LIST_APPEND().

> +    }
> +    return ret;
> +}
> diff --git a/qapi/audio.json b/qapi/audio.json
> index 9cba0df8a4..d7b91230d7 100644
> --- a/qapi/audio.json
> +++ b/qapi/audio.json
> @@ -419,3 +419,16 @@
>      'sdl':       'AudiodevSdlOptions',
>      'spice':     'AudiodevGenericOptions',
>      'wav':       'AudiodevWavOptions' } }
> +
> +##
> +# @query-audiodevs:
> +#
> +# Returns information about audiodev configuration
> +#
> +# Returns: array of @Audiodev
> +#
> +# Since: 6.0
> +#
> +##
> +{ 'command': 'query-audiodevs',
> +  'returns': ['Audiodev'] }


Re: [PATCH 1/3] qapi, audio: add query-audiodev command
Posted by Daniel P. Berrangé 4 years, 11 months ago
On Fri, Mar 05, 2021 at 02:01:14PM +0100, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > Way back in QEMU 4.0, the -audiodev command line option was introduced
> > for configuring audio backends. This CLI option does not use QemuOpts
> > so it is not visible for introspection in 'query-command-line-options',
> > instead using the QAPI Audiodev type.  Unfortunately there is also no
> > QMP command that uses the Audiodev type, so it is not introspectable
> > with 'query-qmp-schema' either.
> 
> This is a gap that will only widen.
> 
> By design, query-qmp-schema covers just QMP.  It doesn't cover the
> QAPIfied parts of the CLI.  They have been growing slowly, and this
> trend will continue.  We need schema introspection to cover the CLI,
> too.  Observation, not demand.
> 
> Work-arounds:
> 
> 1. When a QMP command equivalent to a QAPIfied CLI option exists,
> introspect that.  Involves hardcoding the connection between the two.
> Example: -blockdev and blockdev-add.

I'd say this is the scenario that wil lapply long term. As we aim
for a world where you can run "qemu -qmp unix:/tmp/sock,server"
and then configure everything over QMP, we should cease to have
any scenarios where stuff is missing from query-qmp-schemas,
as we'll have a QMP command that covers everything.

IOW, in this case we really ought to have a 'audiodev-add'
command as a counterpart to -audiodev CLI. Not sure if there

are any dragons wrt to creating audio backends on the fly.

> 2. When a QMP query exists that returns the CLI option argument,
> introspect that.  Involves hardcoding the connection between the two.
> Example: -display and query-display-options.
>
> 3. When neither exists, create a suitable query just to enable
> introspection.  Example: query-display-options.  Commit e1ca8f7e19
> "qapi: add query-display-options command" explains this clearly:
> 
>     Add query-display-options command, which allows querying the qemu
>     display configuration.  This isn't particularly useful, except it
>     exposes QAPI type DisplayOptions in query-qmp-schema, so that libvirt
>     can discover recently added -display parameter rendernode (commit
>     d4dc4ab133b).  Works around lack of sufficiently powerful command line
>     introspection.

I figure having a 'query-foo' QMP corresponding to any 'foo-add' QMP
is useful as a general rule to allow mgmt (or human support engineers)
to inspect an running instance and understand its configuration,
especially since it won't be visible in CLI args if we move to 100%
QMP based config.

> > This introduces a 'query-audiodev' command that simply reflects back
> > the list of configured -audiodev command line options. This in turn
> > makes Audiodev introspectable via 'query-qmp-schema'.
> 
> Is the query just for enabling introspection, or does it have other
> uses?

On the libvirt side we only need it for enabling introspection.

As a general rule, being able to inspect any aspect of configuration
via QMP is useful for support purposes if nothing else.

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