[PATCH 0/3] audio: make audiodev introspectable by mgmt apps

[PATCH 0/3] audio: make audiodev introspectable by mgmt apps

[Daniel P. Berrangé]

The Audiodev QAPI type is not introspectable via query-qmp-schema as
nothing in QEMU uses it. -audiodev is not introspectable via
query-command-line-options because it avoided legacy QemuOpts

Even once that is fixed, the introspection lies about what is
actually possible because nearly all the audio backends are compile
time conditional, but the QAPI schema is fixed.

The last patch is a trivial addition that aided my debugging while
investigating the problems and not directly related/dependant.

Daniel P. Berrangé (3):
  qapi, audio: add query-audiodev command
  qapi, audio: respect build time conditions in audio schema
  qapi: provide a friendly string representation of QAPI classes

 audio/audio.c          | 35 ++++++++++++++++++++++++++
 audio/audio_legacy.c   | 41 +++++++++++++++++++++++++++++-
 audio/audio_template.h | 16 ++++++++++++
 qapi/audio.json        | 57 ++++++++++++++++++++++++++++++++++--------
 scripts/qapi/schema.py |  3 +++
 5 files changed, 141 insertions(+), 11 deletions(-)

-- 
2.29.2

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

[Daniel P. Berrangé]

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

[PATCH 2/3] qapi, audio: respect build time conditions in audio schema

[Daniel P. Berrangé]

Currently the -audiodev accepts any audiodev type regardless of what is
built in to QEMU. An error only occurs later at runtime when a sound
device tries to use the audio backend.

With this change QEMU will immediately reject -audiodev args that are
not compiled into the binary. The QMP schema will also be introspectable
to identify what is compiled in.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 audio/audio.c          | 16 +++++++++++++++
 audio/audio_legacy.c   | 41 ++++++++++++++++++++++++++++++++++++++-
 audio/audio_template.h | 16 +++++++++++++++
 qapi/audio.json        | 44 ++++++++++++++++++++++++++++++++----------
 4 files changed, 106 insertions(+), 11 deletions(-)

diff --git a/audio/audio.c b/audio/audio.c
index 40a4bbd7ce..53434fc674 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -1989,14 +1989,30 @@ void audio_create_pdos(Audiodev *dev)
         break
 
         CASE(NONE, none, );
+#ifdef CONFIG_AUDIO_ALSA
         CASE(ALSA, alsa, Alsa);
+#endif
+#ifdef CONFIG_AUDIO_COREAUDIO
         CASE(COREAUDIO, coreaudio, Coreaudio);
+#endif
+#ifdef CONFIG_AUDIO_DSOUND
         CASE(DSOUND, dsound, );
+#endif
+#ifdef CONFIG_AUDIO_JACK
         CASE(JACK, jack, Jack);
+#endif
+#ifdef CONFIG_AUDIO_OSS
         CASE(OSS, oss, Oss);
+#endif
+#ifdef CONFIG_AUDIO_PA
         CASE(PA, pa, Pa);
+#endif
+#ifdef CONFIG_AUDIO_SDL
         CASE(SDL, sdl, Sdl);
+#endif
+#ifdef CONFIG_SPICE
         CASE(SPICE, spice, );
+#endif
         CASE(WAV, wav, );
 
     case AUDIODEV_DRIVER__MAX:
diff --git a/audio/audio_legacy.c b/audio/audio_legacy.c
index 0fe827b057..bb2268f2b2 100644
--- a/audio/audio_legacy.c
+++ b/audio/audio_legacy.c
@@ -93,6 +93,7 @@ static void get_fmt(const char *env, AudioFormat *dst, bool *has_dst)
 }
 
 
+#if defined(CONFIG_AUDIO_ALSA) || defined(CONFIG_AUDIO_DSOUND)
 static void get_millis_to_usecs(const char *env, uint32_t *dst, bool *has_dst)
 {
     const char *val = getenv(env);
@@ -101,15 +102,20 @@ static void get_millis_to_usecs(const char *env, uint32_t *dst, bool *has_dst)
         *has_dst = true;
     }
 }
+#endif
 
+#if defined(CONFIG_AUDIO_ALSA) || defined(CONFIG_AUDIO_COREAUDIO) || \
+    defined(CONFIG_AUDIO_PA) || defined(CONFIG_AUDIO_SDL) || \
+    defined(CONFIG_AUDIO_DSOUND) || defined(CONFIG_AUDIO_OSS)
 static uint32_t frames_to_usecs(uint32_t frames,
                                 AudiodevPerDirectionOptions *pdo)
 {
     uint32_t freq = pdo->has_frequency ? pdo->frequency : 44100;
     return (frames * 1000000 + freq / 2) / freq;
 }
+#endif
 
-
+#ifdef CONFIG_AUDIO_COREAUDIO
 static void get_frames_to_usecs(const char *env, uint32_t *dst, bool *has_dst,
                                 AudiodevPerDirectionOptions *pdo)
 {
@@ -119,14 +125,19 @@ static void get_frames_to_usecs(const char *env, uint32_t *dst, bool *has_dst,
         *has_dst = true;
     }
 }
+#endif
 
+#if defined(CONFIG_AUDIO_PA) || defined(CONFIG_AUDIO_SDL) || \
+    defined(CONFIG_AUDIO_DSOUND) || defined(CONFIG_AUDIO_OSS)
 static uint32_t samples_to_usecs(uint32_t samples,
                                  AudiodevPerDirectionOptions *pdo)
 {
     uint32_t channels = pdo->has_channels ? pdo->channels : 2;
     return frames_to_usecs(samples / channels, pdo);
 }
+#endif
 
+#if defined(CONFIG_AUDIO_PA) || defined(CONFIG_AUDIO_SDL)
 static void get_samples_to_usecs(const char *env, uint32_t *dst, bool *has_dst,
                                  AudiodevPerDirectionOptions *pdo)
 {
@@ -136,7 +147,9 @@ static void get_samples_to_usecs(const char *env, uint32_t *dst, bool *has_dst,
         *has_dst = true;
     }
 }
+#endif
 
+#if defined(CONFIG_AUDIO_DSOUND) || defined(CONFIG_AUDIO_OSS)
 static uint32_t bytes_to_usecs(uint32_t bytes, AudiodevPerDirectionOptions *pdo)
 {
     AudioFormat fmt = pdo->has_format ? pdo->format : AUDIO_FORMAT_S16;
@@ -153,8 +166,11 @@ static void get_bytes_to_usecs(const char *env, uint32_t *dst, bool *has_dst,
         *has_dst = true;
     }
 }
+#endif
 
 /* backend specific functions */
+
+#ifdef CONFIG_AUDIO_ALSA
 /* ALSA */
 static void handle_alsa_per_direction(
     AudiodevAlsaPerDirectionOptions *apdo, const char *prefix)
@@ -200,7 +216,9 @@ static void handle_alsa(Audiodev *dev)
     get_millis_to_usecs("QEMU_ALSA_THRESHOLD",
                         &aopt->threshold, &aopt->has_threshold);
 }
+#endif
 
+#ifdef CONFIG_AUDIO_COREAUDIO
 /* coreaudio */
 static void handle_coreaudio(Audiodev *dev)
 {
@@ -213,7 +231,9 @@ static void handle_coreaudio(Audiodev *dev)
             &dev->u.coreaudio.out->buffer_count,
             &dev->u.coreaudio.out->has_buffer_count);
 }
+#endif
 
+#ifdef CONFIG_AUDIO_DSOUND
 /* dsound */
 static void handle_dsound(Audiodev *dev)
 {
@@ -228,7 +248,9 @@ static void handle_dsound(Audiodev *dev)
                        &dev->u.dsound.in->has_buffer_length,
                        dev->u.dsound.in);
 }
+#endif
 
+#ifdef CONFIG_AUDIO_OSS
 /* OSS */
 static void handle_oss_per_direction(
     AudiodevOssPerDirectionOptions *opdo, const char *try_poll_env,
@@ -256,7 +278,9 @@ static void handle_oss(Audiodev *dev)
     get_bool("QEMU_OSS_EXCLUSIVE", &oopt->exclusive, &oopt->has_exclusive);
     get_int("QEMU_OSS_POLICY", &oopt->dsp_policy, &oopt->has_dsp_policy);
 }
+#endif
 
+#ifdef CONFIG_AUDIO_PA
 /* pulseaudio */
 static void handle_pa_per_direction(
     AudiodevPaPerDirectionOptions *ppdo, const char *env)
@@ -280,7 +304,9 @@ static void handle_pa(Audiodev *dev)
 
     get_str("QEMU_PA_SERVER", &dev->u.pa.server, &dev->u.pa.has_server);
 }
+#endif
 
+#ifdef CONFIG_AUDIO_SDL
 /* SDL */
 static void handle_sdl(Audiodev *dev)
 {
@@ -289,6 +315,7 @@ static void handle_sdl(Audiodev *dev)
         &dev->u.sdl.out->has_buffer_length,
         qapi_AudiodevSdlPerDirectionOptions_base(dev->u.sdl.out));
 }
+#endif
 
 /* wav */
 static void handle_wav(Audiodev *dev)
@@ -348,29 +375,41 @@ static AudiodevListEntry *legacy_opt(const char *drvname)
     }
 
     switch (e->dev->driver) {
+#ifdef CONFIG_AUDIO_ALSA
     case AUDIODEV_DRIVER_ALSA:
         handle_alsa(e->dev);
         break;
+#endif
 
+#ifdef CONFIG_AUDIO_COREAUDIO
     case AUDIODEV_DRIVER_COREAUDIO:
         handle_coreaudio(e->dev);
         break;
+#endif
 
+#ifdef CONFIG_AUDIO_DSOUND
     case AUDIODEV_DRIVER_DSOUND:
         handle_dsound(e->dev);
         break;
+#endif
 
+#ifdef CONFIG_AUDIO_OSS
     case AUDIODEV_DRIVER_OSS:
         handle_oss(e->dev);
         break;
+#endif
 
+#ifdef CONFIG_AUDIO_PA
     case AUDIODEV_DRIVER_PA:
         handle_pa(e->dev);
         break;
+#endif
 
+#ifdef CONFIG_AUDIO_SDL
     case AUDIODEV_DRIVER_SDL:
         handle_sdl(e->dev);
         break;
+#endif
 
     case AUDIODEV_DRIVER_WAV:
         handle_wav(e->dev);
diff --git a/audio/audio_template.h b/audio/audio_template.h
index c6714946aa..0847b643be 100644
--- a/audio/audio_template.h
+++ b/audio/audio_template.h
@@ -322,23 +322,39 @@ AudiodevPerDirectionOptions *glue(audio_get_pdo_, TYPE)(Audiodev *dev)
     switch (dev->driver) {
     case AUDIODEV_DRIVER_NONE:
         return dev->u.none.TYPE;
+#ifdef CONFIG_AUDIO_ALSA
     case AUDIODEV_DRIVER_ALSA:
         return qapi_AudiodevAlsaPerDirectionOptions_base(dev->u.alsa.TYPE);
+#endif
+#ifdef CONFIG_AUDIO_COREAUDIO
     case AUDIODEV_DRIVER_COREAUDIO:
         return qapi_AudiodevCoreaudioPerDirectionOptions_base(
             dev->u.coreaudio.TYPE);
+#endif
+#ifdef CONFIG_AUDIO_DSOUND
     case AUDIODEV_DRIVER_DSOUND:
         return dev->u.dsound.TYPE;
+#endif
+#ifdef CONFIG_AUDIO_JACK
     case AUDIODEV_DRIVER_JACK:
         return qapi_AudiodevJackPerDirectionOptions_base(dev->u.jack.TYPE);
+#endif
+#ifdef CONFIG_AUDIO_OSS
     case AUDIODEV_DRIVER_OSS:
         return qapi_AudiodevOssPerDirectionOptions_base(dev->u.oss.TYPE);
+#endif
+#ifdef CONFIG_AUDIO_PA
     case AUDIODEV_DRIVER_PA:
         return qapi_AudiodevPaPerDirectionOptions_base(dev->u.pa.TYPE);
+#endif
+#ifdef CONFIG_AUDIO_SDL
     case AUDIODEV_DRIVER_SDL:
         return qapi_AudiodevSdlPerDirectionOptions_base(dev->u.sdl.TYPE);
+#endif
+#ifdef CONFIG_SPICE
     case AUDIODEV_DRIVER_SPICE:
         return dev->u.spice.TYPE;
+#endif
     case AUDIODEV_DRIVER_WAV:
         return dev->u.wav.TYPE;
 
diff --git a/qapi/audio.json b/qapi/audio.json
index d7b91230d7..9af1b8140c 100644
--- a/qapi/audio.json
+++ b/qapi/audio.json
@@ -386,8 +386,24 @@
 # Since: 4.0
 ##
 { 'enum': 'AudiodevDriver',
-  'data': [ 'none', 'alsa', 'coreaudio', 'dsound', 'jack', 'oss', 'pa',
-            'sdl', 'spice', 'wav' ] }
+  'data': [ 'none',
+            { 'name': 'alsa',
+              'if': 'defined(CONFIG_AUDIO_ALSA)' },
+            { 'name': 'coreaudio',
+              'if': 'defined(CONFIG_AUDIO_COREAUDIO)' },
+            { 'name': 'dsound',
+              'if': 'defined(CONFIG_AUDIO_DSOUND)' },
+            { 'name': 'jack',
+              'if': 'defined(CONFIG_AUDIO_JACK)' },
+            { 'name': 'oss',
+              'if': 'defined(CONFIG_AUDIO_OSS)' },
+            { 'name': 'pa',
+              'if': 'defined(CONFIG_AUDIO_PA)' },
+            { 'name': 'sdl',
+              'if': 'defined(CONFIG_AUDIO_SDL)' },
+            { 'name': 'spice',
+              'if': 'defined(CONFIG_SPICE)' },
+            'wav' ] }
 
 ##
 # @Audiodev:
@@ -410,14 +426,22 @@
   'discriminator': 'driver',
   'data': {
     'none':      'AudiodevGenericOptions',
-    'alsa':      'AudiodevAlsaOptions',
-    'coreaudio': 'AudiodevCoreaudioOptions',
-    'dsound':    'AudiodevDsoundOptions',
-    'jack':      'AudiodevJackOptions',
-    'oss':       'AudiodevOssOptions',
-    'pa':        'AudiodevPaOptions',
-    'sdl':       'AudiodevSdlOptions',
-    'spice':     'AudiodevGenericOptions',
+    'alsa':      { 'type': 'AudiodevAlsaOptions',
+                   'if': 'defined(CONFIG_AUDIO_ALSA)' },
+    'coreaudio': { 'type': 'AudiodevCoreaudioOptions',
+                   'if': 'defined(CONFIG_AUDIO_COREAUDIO)' },
+    'dsound':    { 'type': 'AudiodevDsoundOptions',
+                   'if': 'defined(CONFIG_AUDIO_DSOUND)' },
+    'jack':      { 'type': 'AudiodevJackOptions',
+                   'if': 'defined(CONFIG_AUDIO_JACK)' },
+    'oss':       { 'type': 'AudiodevOssOptions',
+                   'if': 'defined(CONFIG_AUDIO_OSS)' },
+    'pa':        { 'type': 'AudiodevPaOptions',
+                   'if': 'defined(CONFIG_AUDIO_PA)' },
+    'sdl':       { 'type': 'AudiodevSdlOptions',
+                   'if': 'defined(CONFIG_AUDIO_SDL)' },
+    'spice':     { 'type': 'AudiodevGenericOptions',
+                   'if': 'defined(CONFIG_SPICE)' },
     'wav':       'AudiodevWavOptions' } }
 
 ##
-- 
2.29.2

[PATCH 3/3] qapi: provide a friendly string representation of QAPI classes

[Daniel P. Berrangé]

If printing a QAPI schema object for debugging we get the classname and
a hex value for the instance. With this change we instead get the
classname and the human friendly name of the QAPI type instance.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 scripts/qapi/schema.py | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index ff16578f6d..800bc5994b 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -46,6 +46,9 @@ def __init__(self, name: str, info, doc, ifcond=None, features=None):
         self.features = features or []
         self._checked = False
 
+    def __repr__(self):
+        return "%s<%s>" % (type(self).__name__, self.name)
+
     def c_name(self):
         return c_name(self.name)
 
-- 
2.29.2

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

[Eric Blake]

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 2/3] qapi, audio: respect build time conditions in audio schema

[Eric Blake]

On 3/2/21 11:55 AM, Daniel P. Berrangé wrote:
> Currently the -audiodev accepts any audiodev type regardless of what is
> built in to QEMU. An error only occurs later at runtime when a sound
> device tries to use the audio backend.
> 
> With this change QEMU will immediately reject -audiodev args that are
> not compiled into the binary. The QMP schema will also be introspectable
> to identify what is compiled in.

Nice!

> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  audio/audio.c          | 16 +++++++++++++++
>  audio/audio_legacy.c   | 41 ++++++++++++++++++++++++++++++++++++++-
>  audio/audio_template.h | 16 +++++++++++++++
>  qapi/audio.json        | 44 ++++++++++++++++++++++++++++++++----------
>  4 files changed, 106 insertions(+), 11 deletions(-)
> 

> +++ b/qapi/audio.json
> @@ -386,8 +386,24 @@
>  # Since: 4.0
>  ##
>  { 'enum': 'AudiodevDriver',
> -  'data': [ 'none', 'alsa', 'coreaudio', 'dsound', 'jack', 'oss', 'pa',
> -            'sdl', 'spice', 'wav' ] }
> +  'data': [ 'none',
> +            { 'name': 'alsa',
> +              'if': 'defined(CONFIG_AUDIO_ALSA)' },
> +            { 'name': 'coreaudio',
> +              'if': 'defined(CONFIG_AUDIO_COREAUDIO)' },
> +            { 'name': 'dsound',
> +              'if': 'defined(CONFIG_AUDIO_DSOUND)' },
> +            { 'name': 'jack',
> +              'if': 'defined(CONFIG_AUDIO_JACK)' },
> +            { 'name': 'oss',
> +              'if': 'defined(CONFIG_AUDIO_OSS)' },
> +            { 'name': 'pa',
> +              'if': 'defined(CONFIG_AUDIO_PA)' },
> +            { 'name': 'sdl',
> +              'if': 'defined(CONFIG_AUDIO_SDL)' },
> +            { 'name': 'spice',
> +              'if': 'defined(CONFIG_SPICE)' },
> +            'wav' ] }

I'll trust that you compiled multiple times to test the various
interplays between options.

Reviewed-by: Eric Blake <eblake@redhat.com>

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

Re: [PATCH 3/3] qapi: provide a friendly string representation of QAPI classes

[Eric Blake]

On 3/2/21 11:55 AM, Daniel P. Berrangé wrote:
> If printing a QAPI schema object for debugging we get the classname and
> a hex value for the instance. With this change we instead get the
> classname and the human friendly name of the QAPI type instance.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  scripts/qapi/schema.py | 3 +++
>  1 file changed, 3 insertions(+)

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index ff16578f6d..800bc5994b 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -46,6 +46,9 @@ def __init__(self, name: str, info, doc, ifcond=None, features=None):
>          self.features = features or []
>          self._checked = False
>  
> +    def __repr__(self):
> +        return "%s<%s>" % (type(self).__name__, self.name)
> +
>      def c_name(self):
>          return c_name(self.name)
>  
> 

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

Re: [PATCH 3/3] qapi: provide a friendly string representation of QAPI classes

[Philippe Mathieu-Daudé]

On 3/2/21 6:55 PM, Daniel P. Berrangé wrote:
> If printing a QAPI schema object for debugging we get the classname and
> a hex value for the instance. With this change we instead get the
> classname and the human friendly name of the QAPI type instance.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  scripts/qapi/schema.py | 3 +++
>  1 file changed, 3 insertions(+)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

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

[Philippe Mathieu-Daudé]

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

[Philippe Mathieu-Daudé]

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 2/3] qapi, audio: respect build time conditions in audio schema

[Gerd Hoffmann]

On Tue, Mar 02, 2021 at 05:55:23PM +0000, Daniel P. Berrangé wrote:
> Currently the -audiodev accepts any audiodev type regardless of what is
> built in to QEMU. An error only occurs later at runtime when a sound
> device tries to use the audio backend.
> 
> With this change QEMU will immediately reject -audiodev args that are
> not compiled into the binary. The QMP schema will also be introspectable
> to identify what is compiled in.

Note that audio backends are modularized, so "compiled with
CONFIG_AUDIO_ALSA" doesn't imply "alsa support is available".

take care,
  Gerd

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

[Daniel P. Berrangé]

On Tue, Mar 02, 2021 at 10:12:43PM +0100, Philippe Mathieu-Daudé wrote:
> 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?

Chardevs are not a good guide as they have this wierd split betweeen
manually written Chardev type and QAPI driven ChardeBackendInfo type.

With audio backends, we only have the Audiodev QAPI type. There is
no AudiodevBackendInfo.

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

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

[Daniel P. Berrangé]

On Tue, Mar 02, 2021 at 10:10:56PM +0100, 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
> > +#
> > +# Since: 6.0
> > +#
> > +##
> > +{ 'command': 'query-audiodevs',
> > +  'returns': ['Audiodev'] }
> > 
> 
> Can we use 'query-audiodev-backends' similarly to
> 'query-chardev-backends'?

IMHO adding "-backends" is redundant, because audiodev and chardev
objects are always backends.

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

Re: [PATCH 2/3] qapi, audio: respect build time conditions in audio schema

[Daniel P. Berrangé]

On Tue, Mar 02, 2021 at 01:05:45PM -0600, Eric Blake wrote:
> On 3/2/21 11:55 AM, Daniel P. Berrangé wrote:
> > Currently the -audiodev accepts any audiodev type regardless of what is
> > built in to QEMU. An error only occurs later at runtime when a sound
> > device tries to use the audio backend.
> > 
> > With this change QEMU will immediately reject -audiodev args that are
> > not compiled into the binary. The QMP schema will also be introspectable
> > to identify what is compiled in.
> 
> Nice!
> 
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  audio/audio.c          | 16 +++++++++++++++
> >  audio/audio_legacy.c   | 41 ++++++++++++++++++++++++++++++++++++++-
> >  audio/audio_template.h | 16 +++++++++++++++
> >  qapi/audio.json        | 44 ++++++++++++++++++++++++++++++++----------
> >  4 files changed, 106 insertions(+), 11 deletions(-)
> > 
> 
> > +++ b/qapi/audio.json
> > @@ -386,8 +386,24 @@
> >  # Since: 4.0
> >  ##
> >  { 'enum': 'AudiodevDriver',
> > -  'data': [ 'none', 'alsa', 'coreaudio', 'dsound', 'jack', 'oss', 'pa',
> > -            'sdl', 'spice', 'wav' ] }
> > +  'data': [ 'none',
> > +            { 'name': 'alsa',
> > +              'if': 'defined(CONFIG_AUDIO_ALSA)' },
> > +            { 'name': 'coreaudio',
> > +              'if': 'defined(CONFIG_AUDIO_COREAUDIO)' },
> > +            { 'name': 'dsound',
> > +              'if': 'defined(CONFIG_AUDIO_DSOUND)' },
> > +            { 'name': 'jack',
> > +              'if': 'defined(CONFIG_AUDIO_JACK)' },
> > +            { 'name': 'oss',
> > +              'if': 'defined(CONFIG_AUDIO_OSS)' },
> > +            { 'name': 'pa',
> > +              'if': 'defined(CONFIG_AUDIO_PA)' },
> > +            { 'name': 'sdl',
> > +              'if': 'defined(CONFIG_AUDIO_SDL)' },
> > +            { 'name': 'spice',
> > +              'if': 'defined(CONFIG_SPICE)' },
> > +            'wav' ] }
> 
> I'll trust that you compiled multiple times to test the various
> interplays between options.

No, just sent it through gitlab CI which I assumed would cover it

> 
> Reviewed-by: Eric Blake <eblake@redhat.com>

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

Re: [PATCH 2/3] qapi, audio: respect build time conditions in audio schema

[Daniel P. Berrangé]

On Wed, Mar 03, 2021 at 08:00:59AM +0100, Gerd Hoffmann wrote:
> On Tue, Mar 02, 2021 at 05:55:23PM +0000, Daniel P. Berrangé wrote:
> > Currently the -audiodev accepts any audiodev type regardless of what is
> > built in to QEMU. An error only occurs later at runtime when a sound
> > device tries to use the audio backend.
> > 
> > With this change QEMU will immediately reject -audiodev args that are
> > not compiled into the binary. The QMP schema will also be introspectable
> > to identify what is compiled in.
> 
> Note that audio backends are modularized, so "compiled with
> CONFIG_AUDIO_ALSA" doesn't imply "alsa support is available".

AFAIK, there's no way to handle this with QAPI schema reporting. We
can only conditionalize based on what's available at compile time,
not what's installed at runtime.

To get runtime info, we would have to introduce an explicit
"query-audiodev-types" command where just report the backends
that have been installed.

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

Re: [PATCH 2/3] qapi, audio: respect build time conditions in audio schema

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Wed, Mar 03, 2021 at 08:00:59AM +0100, Gerd Hoffmann wrote:
>> On Tue, Mar 02, 2021 at 05:55:23PM +0000, Daniel P. Berrangé wrote:
>> > Currently the -audiodev accepts any audiodev type regardless of what is
>> > built in to QEMU. An error only occurs later at runtime when a sound
>> > device tries to use the audio backend.
>> > 
>> > With this change QEMU will immediately reject -audiodev args that are
>> > not compiled into the binary. The QMP schema will also be introspectable
>> > to identify what is compiled in.
>> 
>> Note that audio backends are modularized, so "compiled with
>> CONFIG_AUDIO_ALSA" doesn't imply "alsa support is available".
>
> AFAIK, there's no way to handle this with QAPI schema reporting. We
> can only conditionalize based on what's available at compile time,
> not what's installed at runtime.

Correct.  The schema is fixed at compile-time.  query-qmp-schema
reflects what we compiled into the binary or modules we built along with
the binary.  It cannot tell which of the modules the binary can load.

I'd like the commit message to discuss how the patch interacts with
modules, because my own memory of the details is rather uncertain :)

I guess we can configure which audio backends to build, and whether to
build them as modules.

When not building them as modules, the patch compiles out some useless
code.  Correct?

When building them as modules, the patch compiles out code for modules
we don't build.  Correct?

Such code is useless, unless you somehow manage to supply the resulting
binary with working modules from another build.  Which is probably a bad
idea.  Compiling out the code stops this (questionable) usage cold.  No
objection, but the commit message should at least hint at it.

> To get runtime info, we would have to introduce an explicit
> "query-audiodev-types" command where just report the backends
> that have been installed.

TTOCTOU.  Harmless one.  Still, the robust check for "can module M be
loaded?" is to try loading it.

Re: [PATCH 2/3] qapi, audio: respect build time conditions in audio schema

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> Currently the -audiodev accepts any audiodev type regardless of what is
> built in to QEMU. An error only occurs later at runtime when a sound
> device tries to use the audio backend.
>
> With this change QEMU will immediately reject -audiodev args that are
> not compiled into the binary. The QMP schema will also be introspectable
> to identify what is compiled in.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

Subject "qapi, audio: respect build time conditions in audio schema"
feels too narrow.  The patch goes beyond the schema, because it has to:
guarding QAPI schema parts with 'if' requires guarding use of C code
generated for it with #if.

An easy way out is perhaps stating just the aim:

    audio: Make introspection reflect build configuration

This assumes the patch does a complete job.  If there's more audio build
configuration to reflect, add a suitable qualifier like "more closely".

Fun: before the patch, the CONFIG_AUDIO_ conditionals are effectively
applied just to output of -help.

[...]
> diff --git a/qapi/audio.json b/qapi/audio.json
> index d7b91230d7..9af1b8140c 100644
> --- a/qapi/audio.json
> +++ b/qapi/audio.json
> @@ -386,8 +386,24 @@
>  # Since: 4.0
>  ##
>  { 'enum': 'AudiodevDriver',
> -  'data': [ 'none', 'alsa', 'coreaudio', 'dsound', 'jack', 'oss', 'pa',
> -            'sdl', 'spice', 'wav' ] }
> +  'data': [ 'none',
> +            { 'name': 'alsa',
> +              'if': 'defined(CONFIG_AUDIO_ALSA)' },
> +            { 'name': 'coreaudio',
> +              'if': 'defined(CONFIG_AUDIO_COREAUDIO)' },
> +            { 'name': 'dsound',
> +              'if': 'defined(CONFIG_AUDIO_DSOUND)' },
> +            { 'name': 'jack',
> +              'if': 'defined(CONFIG_AUDIO_JACK)' },
> +            { 'name': 'oss',
> +              'if': 'defined(CONFIG_AUDIO_OSS)' },
> +            { 'name': 'pa',
> +              'if': 'defined(CONFIG_AUDIO_PA)' },
> +            { 'name': 'sdl',
> +              'if': 'defined(CONFIG_AUDIO_SDL)' },
> +            { 'name': 'spice',
> +              'if': 'defined(CONFIG_SPICE)' },
> +            'wav' ] }
>  
>  ##
>  # @Audiodev:
> @@ -410,14 +426,22 @@
>    'discriminator': 'driver',
>    'data': {
>      'none':      'AudiodevGenericOptions',
> -    'alsa':      'AudiodevAlsaOptions',
> -    'coreaudio': 'AudiodevCoreaudioOptions',
> -    'dsound':    'AudiodevDsoundOptions',
> -    'jack':      'AudiodevJackOptions',
> -    'oss':       'AudiodevOssOptions',
> -    'pa':        'AudiodevPaOptions',
> -    'sdl':       'AudiodevSdlOptions',
> -    'spice':     'AudiodevGenericOptions',
> +    'alsa':      { 'type': 'AudiodevAlsaOptions',
> +                   'if': 'defined(CONFIG_AUDIO_ALSA)' },
> +    'coreaudio': { 'type': 'AudiodevCoreaudioOptions',
> +                   'if': 'defined(CONFIG_AUDIO_COREAUDIO)' },
> +    'dsound':    { 'type': 'AudiodevDsoundOptions',
> +                   'if': 'defined(CONFIG_AUDIO_DSOUND)' },
> +    'jack':      { 'type': 'AudiodevJackOptions',
> +                   'if': 'defined(CONFIG_AUDIO_JACK)' },
> +    'oss':       { 'type': 'AudiodevOssOptions',
> +                   'if': 'defined(CONFIG_AUDIO_OSS)' },
> +    'pa':        { 'type': 'AudiodevPaOptions',
> +                   'if': 'defined(CONFIG_AUDIO_PA)' },
> +    'sdl':       { 'type': 'AudiodevSdlOptions',
> +                   'if': 'defined(CONFIG_AUDIO_SDL)' },
> +    'spice':     { 'type': 'AudiodevGenericOptions',
> +                   'if': 'defined(CONFIG_SPICE)' },
>      'wav':       'AudiodevWavOptions' } }
>  
>  ##

For the QAPI schema part:
Acked-by: Markus Armbruster <armbru@redhat.com>

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

[Markus Armbruster]

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 3/3] qapi: provide a friendly string representation of QAPI classes

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> If printing a QAPI schema object for debugging we get the classname and
> a hex value for the instance. With this change we instead get the
> classname and the human friendly name of the QAPI type instance.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  scripts/qapi/schema.py | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index ff16578f6d..800bc5994b 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -46,6 +46,9 @@ def __init__(self, name: str, info, doc, ifcond=None, features=None):
>          self.features = features or []
>          self._checked = False
>  
> +    def __repr__(self):
> +        return "%s<%s>" % (type(self).__name__, self.name)
> +
>      def c_name(self):
>          return c_name(self.name)

https://docs.python.org/3.6/reference/datamodel.html#object.__repr__

    Called by the repr() built-in function to compute the “official”
    string representation of an object.  If at all possible, this should
    look like a valid Python expression that could be used to recreate
    an object with the same value (given an appropriate environment).

Making QAPISchemaEntity.__repr__() return "a valid Python expression
that could be used to recreate an object with the same value" is
probably more trouble than it's worth.

    If this is not possible, a string of the form <...some useful
    description...> should be returned.

I'm afraid your .__repr__() has the < in the wrong place.

                                         The return value must be a
    __repr__() is also used when an “informal” string representation of
    instances of that class is required.

    This is typically used for debugging, so it is important that the
    representation is information-rich and unambiguous.


I guess your .__repr__() is unambiguous enough for practical purposes,
as entity names are typically unique within a schema.  *Except* for
QAPISchemaInclude, where self.name is always None.

What about self.name or id(self)?

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

[Daniel P. Berrangé]

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

Re: [PATCH 2/3] qapi, audio: respect build time conditions in audio schema

[Daniel P. Berrangé]

On Fri, Mar 05, 2021 at 11:56:13AM +0100, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Wed, Mar 03, 2021 at 08:00:59AM +0100, Gerd Hoffmann wrote:
> >> On Tue, Mar 02, 2021 at 05:55:23PM +0000, Daniel P. Berrangé wrote:
> >> > Currently the -audiodev accepts any audiodev type regardless of what is
> >> > built in to QEMU. An error only occurs later at runtime when a sound
> >> > device tries to use the audio backend.
> >> > 
> >> > With this change QEMU will immediately reject -audiodev args that are
> >> > not compiled into the binary. The QMP schema will also be introspectable
> >> > to identify what is compiled in.
> >> 
> >> Note that audio backends are modularized, so "compiled with
> >> CONFIG_AUDIO_ALSA" doesn't imply "alsa support is available".
> >
> > AFAIK, there's no way to handle this with QAPI schema reporting. We
> > can only conditionalize based on what's available at compile time,
> > not what's installed at runtime.
> 
> Correct.  The schema is fixed at compile-time.  query-qmp-schema
> reflects what we compiled into the binary or modules we built along with
> the binary.  It cannot tell which of the modules the binary can load.
> 
> I'd like the commit message to discuss how the patch interacts with
> modules, because my own memory of the details is rather uncertain :)
> 
> I guess we can configure which audio backends to build, and whether to
> build them as modules.
> 
> When not building them as modules, the patch compiles out some useless
> code.  Correct?

Yes.

> When building them as modules, the patch compiles out code for modules
> we don't build.  Correct?

Yes.

> Such code is useless, unless you somehow manage to supply the resulting
> binary with working modules from another build.  Which is probably a bad
> idea.  Compiling out the code stops this (questionable) usage cold.  No
> objection, but the commit message should at least hint at it.
> 
> > To get runtime info, we would have to introduce an explicit
> > "query-audiodev-types" command where just report the backends
> > that have been installed.
> 
> TTOCTOU.  Harmless one.  Still, the robust check for "can module M be
> loaded?" is to try loading it.

Ultimately from libvirt's POV, the introspection is merely used to
let libvirt report errors about unsupported configurations earlier.
So if we don't deal with compiled-but-not-installed modules, then
the effect is that libvirt won't report errors if user requests
'alsa', but QEMU will eventually report such an error when it
starts. So I'm not too worried about optimizing to cope with
modules.

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

Re: [PATCH 2/3] qapi, audio: respect build time conditions in audio schema

[Thomas Huth]

On 02/03/2021 18.55, Daniel P. Berrangé wrote:
> Currently the -audiodev accepts any audiodev type regardless of what is
> built in to QEMU. An error only occurs later at runtime when a sound
> device tries to use the audio backend.
> 
> With this change QEMU will immediately reject -audiodev args that are
> not compiled into the binary. The QMP schema will also be introspectable
> to identify what is compiled in.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   audio/audio.c          | 16 +++++++++++++++
>   audio/audio_legacy.c   | 41 ++++++++++++++++++++++++++++++++++++++-
>   audio/audio_template.h | 16 +++++++++++++++
>   qapi/audio.json        | 44 ++++++++++++++++++++++++++++++++----------
>   4 files changed, 106 insertions(+), 11 deletions(-)

  Hi Daniel!

Would you have time to respin this patch for QEMU 8.0 ?

  Thomas


> diff --git a/audio/audio.c b/audio/audio.c
> index 40a4bbd7ce..53434fc674 100644
> --- a/audio/audio.c
> +++ b/audio/audio.c
> @@ -1989,14 +1989,30 @@ void audio_create_pdos(Audiodev *dev)
>           break
>   
>           CASE(NONE, none, );
> +#ifdef CONFIG_AUDIO_ALSA
>           CASE(ALSA, alsa, Alsa);
> +#endif
> +#ifdef CONFIG_AUDIO_COREAUDIO
>           CASE(COREAUDIO, coreaudio, Coreaudio);
> +#endif
> +#ifdef CONFIG_AUDIO_DSOUND
>           CASE(DSOUND, dsound, );
> +#endif
> +#ifdef CONFIG_AUDIO_JACK
>           CASE(JACK, jack, Jack);
> +#endif
> +#ifdef CONFIG_AUDIO_OSS
>           CASE(OSS, oss, Oss);
> +#endif
> +#ifdef CONFIG_AUDIO_PA
>           CASE(PA, pa, Pa);
> +#endif
> +#ifdef CONFIG_AUDIO_SDL
>           CASE(SDL, sdl, Sdl);
> +#endif
> +#ifdef CONFIG_SPICE
>           CASE(SPICE, spice, );
> +#endif
>           CASE(WAV, wav, );
>   
>       case AUDIODEV_DRIVER__MAX:
> diff --git a/audio/audio_legacy.c b/audio/audio_legacy.c
> index 0fe827b057..bb2268f2b2 100644
> --- a/audio/audio_legacy.c
> +++ b/audio/audio_legacy.c
> @@ -93,6 +93,7 @@ static void get_fmt(const char *env, AudioFormat *dst, bool *has_dst)
>   }
>   
>   
> +#if defined(CONFIG_AUDIO_ALSA) || defined(CONFIG_AUDIO_DSOUND)
>   static void get_millis_to_usecs(const char *env, uint32_t *dst, bool *has_dst)
>   {
>       const char *val = getenv(env);
> @@ -101,15 +102,20 @@ static void get_millis_to_usecs(const char *env, uint32_t *dst, bool *has_dst)
>           *has_dst = true;
>       }
>   }
> +#endif
>   
> +#if defined(CONFIG_AUDIO_ALSA) || defined(CONFIG_AUDIO_COREAUDIO) || \
> +    defined(CONFIG_AUDIO_PA) || defined(CONFIG_AUDIO_SDL) || \
> +    defined(CONFIG_AUDIO_DSOUND) || defined(CONFIG_AUDIO_OSS)
>   static uint32_t frames_to_usecs(uint32_t frames,
>                                   AudiodevPerDirectionOptions *pdo)
>   {
>       uint32_t freq = pdo->has_frequency ? pdo->frequency : 44100;
>       return (frames * 1000000 + freq / 2) / freq;
>   }
> +#endif
>   
> -
> +#ifdef CONFIG_AUDIO_COREAUDIO
>   static void get_frames_to_usecs(const char *env, uint32_t *dst, bool *has_dst,
>                                   AudiodevPerDirectionOptions *pdo)
>   {
> @@ -119,14 +125,19 @@ static void get_frames_to_usecs(const char *env, uint32_t *dst, bool *has_dst,
>           *has_dst = true;
>       }
>   }
> +#endif
>   
> +#if defined(CONFIG_AUDIO_PA) || defined(CONFIG_AUDIO_SDL) || \
> +    defined(CONFIG_AUDIO_DSOUND) || defined(CONFIG_AUDIO_OSS)
>   static uint32_t samples_to_usecs(uint32_t samples,
>                                    AudiodevPerDirectionOptions *pdo)
>   {
>       uint32_t channels = pdo->has_channels ? pdo->channels : 2;
>       return frames_to_usecs(samples / channels, pdo);
>   }
> +#endif
>   
> +#if defined(CONFIG_AUDIO_PA) || defined(CONFIG_AUDIO_SDL)
>   static void get_samples_to_usecs(const char *env, uint32_t *dst, bool *has_dst,
>                                    AudiodevPerDirectionOptions *pdo)
>   {
> @@ -136,7 +147,9 @@ static void get_samples_to_usecs(const char *env, uint32_t *dst, bool *has_dst,
>           *has_dst = true;
>       }
>   }
> +#endif
>   
> +#if defined(CONFIG_AUDIO_DSOUND) || defined(CONFIG_AUDIO_OSS)
>   static uint32_t bytes_to_usecs(uint32_t bytes, AudiodevPerDirectionOptions *pdo)
>   {
>       AudioFormat fmt = pdo->has_format ? pdo->format : AUDIO_FORMAT_S16;
> @@ -153,8 +166,11 @@ static void get_bytes_to_usecs(const char *env, uint32_t *dst, bool *has_dst,
>           *has_dst = true;
>       }
>   }
> +#endif
>   
>   /* backend specific functions */
> +
> +#ifdef CONFIG_AUDIO_ALSA
>   /* ALSA */
>   static void handle_alsa_per_direction(
>       AudiodevAlsaPerDirectionOptions *apdo, const char *prefix)
> @@ -200,7 +216,9 @@ static void handle_alsa(Audiodev *dev)
>       get_millis_to_usecs("QEMU_ALSA_THRESHOLD",
>                           &aopt->threshold, &aopt->has_threshold);
>   }
> +#endif
>   
> +#ifdef CONFIG_AUDIO_COREAUDIO
>   /* coreaudio */
>   static void handle_coreaudio(Audiodev *dev)
>   {
> @@ -213,7 +231,9 @@ static void handle_coreaudio(Audiodev *dev)
>               &dev->u.coreaudio.out->buffer_count,
>               &dev->u.coreaudio.out->has_buffer_count);
>   }
> +#endif
>   
> +#ifdef CONFIG_AUDIO_DSOUND
>   /* dsound */
>   static void handle_dsound(Audiodev *dev)
>   {
> @@ -228,7 +248,9 @@ static void handle_dsound(Audiodev *dev)
>                          &dev->u.dsound.in->has_buffer_length,
>                          dev->u.dsound.in);
>   }
> +#endif
>   
> +#ifdef CONFIG_AUDIO_OSS
>   /* OSS */
>   static void handle_oss_per_direction(
>       AudiodevOssPerDirectionOptions *opdo, const char *try_poll_env,
> @@ -256,7 +278,9 @@ static void handle_oss(Audiodev *dev)
>       get_bool("QEMU_OSS_EXCLUSIVE", &oopt->exclusive, &oopt->has_exclusive);
>       get_int("QEMU_OSS_POLICY", &oopt->dsp_policy, &oopt->has_dsp_policy);
>   }
> +#endif
>   
> +#ifdef CONFIG_AUDIO_PA
>   /* pulseaudio */
>   static void handle_pa_per_direction(
>       AudiodevPaPerDirectionOptions *ppdo, const char *env)
> @@ -280,7 +304,9 @@ static void handle_pa(Audiodev *dev)
>   
>       get_str("QEMU_PA_SERVER", &dev->u.pa.server, &dev->u.pa.has_server);
>   }
> +#endif
>   
> +#ifdef CONFIG_AUDIO_SDL
>   /* SDL */
>   static void handle_sdl(Audiodev *dev)
>   {
> @@ -289,6 +315,7 @@ static void handle_sdl(Audiodev *dev)
>           &dev->u.sdl.out->has_buffer_length,
>           qapi_AudiodevSdlPerDirectionOptions_base(dev->u.sdl.out));
>   }
> +#endif
>   
>   /* wav */
>   static void handle_wav(Audiodev *dev)
> @@ -348,29 +375,41 @@ static AudiodevListEntry *legacy_opt(const char *drvname)
>       }
>   
>       switch (e->dev->driver) {
> +#ifdef CONFIG_AUDIO_ALSA
>       case AUDIODEV_DRIVER_ALSA:
>           handle_alsa(e->dev);
>           break;
> +#endif
>   
> +#ifdef CONFIG_AUDIO_COREAUDIO
>       case AUDIODEV_DRIVER_COREAUDIO:
>           handle_coreaudio(e->dev);
>           break;
> +#endif
>   
> +#ifdef CONFIG_AUDIO_DSOUND
>       case AUDIODEV_DRIVER_DSOUND:
>           handle_dsound(e->dev);
>           break;
> +#endif
>   
> +#ifdef CONFIG_AUDIO_OSS
>       case AUDIODEV_DRIVER_OSS:
>           handle_oss(e->dev);
>           break;
> +#endif
>   
> +#ifdef CONFIG_AUDIO_PA
>       case AUDIODEV_DRIVER_PA:
>           handle_pa(e->dev);
>           break;
> +#endif
>   
> +#ifdef CONFIG_AUDIO_SDL
>       case AUDIODEV_DRIVER_SDL:
>           handle_sdl(e->dev);
>           break;
> +#endif
>   
>       case AUDIODEV_DRIVER_WAV:
>           handle_wav(e->dev);
> diff --git a/audio/audio_template.h b/audio/audio_template.h
> index c6714946aa..0847b643be 100644
> --- a/audio/audio_template.h
> +++ b/audio/audio_template.h
> @@ -322,23 +322,39 @@ AudiodevPerDirectionOptions *glue(audio_get_pdo_, TYPE)(Audiodev *dev)
>       switch (dev->driver) {
>       case AUDIODEV_DRIVER_NONE:
>           return dev->u.none.TYPE;
> +#ifdef CONFIG_AUDIO_ALSA
>       case AUDIODEV_DRIVER_ALSA:
>           return qapi_AudiodevAlsaPerDirectionOptions_base(dev->u.alsa.TYPE);
> +#endif
> +#ifdef CONFIG_AUDIO_COREAUDIO
>       case AUDIODEV_DRIVER_COREAUDIO:
>           return qapi_AudiodevCoreaudioPerDirectionOptions_base(
>               dev->u.coreaudio.TYPE);
> +#endif
> +#ifdef CONFIG_AUDIO_DSOUND
>       case AUDIODEV_DRIVER_DSOUND:
>           return dev->u.dsound.TYPE;
> +#endif
> +#ifdef CONFIG_AUDIO_JACK
>       case AUDIODEV_DRIVER_JACK:
>           return qapi_AudiodevJackPerDirectionOptions_base(dev->u.jack.TYPE);
> +#endif
> +#ifdef CONFIG_AUDIO_OSS
>       case AUDIODEV_DRIVER_OSS:
>           return qapi_AudiodevOssPerDirectionOptions_base(dev->u.oss.TYPE);
> +#endif
> +#ifdef CONFIG_AUDIO_PA
>       case AUDIODEV_DRIVER_PA:
>           return qapi_AudiodevPaPerDirectionOptions_base(dev->u.pa.TYPE);
> +#endif
> +#ifdef CONFIG_AUDIO_SDL
>       case AUDIODEV_DRIVER_SDL:
>           return qapi_AudiodevSdlPerDirectionOptions_base(dev->u.sdl.TYPE);
> +#endif
> +#ifdef CONFIG_SPICE
>       case AUDIODEV_DRIVER_SPICE:
>           return dev->u.spice.TYPE;
> +#endif
>       case AUDIODEV_DRIVER_WAV:
>           return dev->u.wav.TYPE;
>   
> diff --git a/qapi/audio.json b/qapi/audio.json
> index d7b91230d7..9af1b8140c 100644
> --- a/qapi/audio.json
> +++ b/qapi/audio.json
> @@ -386,8 +386,24 @@
>   # Since: 4.0
>   ##
>   { 'enum': 'AudiodevDriver',
> -  'data': [ 'none', 'alsa', 'coreaudio', 'dsound', 'jack', 'oss', 'pa',
> -            'sdl', 'spice', 'wav' ] }
> +  'data': [ 'none',
> +            { 'name': 'alsa',
> +              'if': 'defined(CONFIG_AUDIO_ALSA)' },
> +            { 'name': 'coreaudio',
> +              'if': 'defined(CONFIG_AUDIO_COREAUDIO)' },
> +            { 'name': 'dsound',
> +              'if': 'defined(CONFIG_AUDIO_DSOUND)' },
> +            { 'name': 'jack',
> +              'if': 'defined(CONFIG_AUDIO_JACK)' },
> +            { 'name': 'oss',
> +              'if': 'defined(CONFIG_AUDIO_OSS)' },
> +            { 'name': 'pa',
> +              'if': 'defined(CONFIG_AUDIO_PA)' },
> +            { 'name': 'sdl',
> +              'if': 'defined(CONFIG_AUDIO_SDL)' },
> +            { 'name': 'spice',
> +              'if': 'defined(CONFIG_SPICE)' },
> +            'wav' ] }
>   
>   ##
>   # @Audiodev:
> @@ -410,14 +426,22 @@
>     'discriminator': 'driver',
>     'data': {
>       'none':      'AudiodevGenericOptions',
> -    'alsa':      'AudiodevAlsaOptions',
> -    'coreaudio': 'AudiodevCoreaudioOptions',
> -    'dsound':    'AudiodevDsoundOptions',
> -    'jack':      'AudiodevJackOptions',
> -    'oss':       'AudiodevOssOptions',
> -    'pa':        'AudiodevPaOptions',
> -    'sdl':       'AudiodevSdlOptions',
> -    'spice':     'AudiodevGenericOptions',
> +    'alsa':      { 'type': 'AudiodevAlsaOptions',
> +                   'if': 'defined(CONFIG_AUDIO_ALSA)' },
> +    'coreaudio': { 'type': 'AudiodevCoreaudioOptions',
> +                   'if': 'defined(CONFIG_AUDIO_COREAUDIO)' },
> +    'dsound':    { 'type': 'AudiodevDsoundOptions',
> +                   'if': 'defined(CONFIG_AUDIO_DSOUND)' },
> +    'jack':      { 'type': 'AudiodevJackOptions',
> +                   'if': 'defined(CONFIG_AUDIO_JACK)' },
> +    'oss':       { 'type': 'AudiodevOssOptions',
> +                   'if': 'defined(CONFIG_AUDIO_OSS)' },
> +    'pa':        { 'type': 'AudiodevPaOptions',
> +                   'if': 'defined(CONFIG_AUDIO_PA)' },
> +    'sdl':       { 'type': 'AudiodevSdlOptions',
> +                   'if': 'defined(CONFIG_AUDIO_SDL)' },
> +    'spice':     { 'type': 'AudiodevGenericOptions',
> +                   'if': 'defined(CONFIG_SPICE)' },
>       'wav':       'AudiodevWavOptions' } }
>   
>   ##

Re: [PATCH 2/3] qapi, audio: respect build time conditions in audio schema

[Daniel P. Berrangé]

On Mon, Dec 12, 2022 at 05:53:21PM +0100, Thomas Huth wrote:
> On 02/03/2021 18.55, Daniel P. Berrangé wrote:
> > Currently the -audiodev accepts any audiodev type regardless of what is
> > built in to QEMU. An error only occurs later at runtime when a sound
> > device tries to use the audio backend.
> > 
> > With this change QEMU will immediately reject -audiodev args that are
> > not compiled into the binary. The QMP schema will also be introspectable
> > to identify what is compiled in.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >   audio/audio.c          | 16 +++++++++++++++
> >   audio/audio_legacy.c   | 41 ++++++++++++++++++++++++++++++++++++++-
> >   audio/audio_template.h | 16 +++++++++++++++
> >   qapi/audio.json        | 44 ++++++++++++++++++++++++++++++++----------
> >   4 files changed, 106 insertions(+), 11 deletions(-)
> 
>  Hi Daniel!
> 
> Would you have time to respin this patch for QEMU 8.0 ?

Realistically I'm not going to get it soon. If you want to take over
feel free.

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