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

Thomas Huth posted 2 patches 2 years, 5 months ago
[PATCH v2 1/2] qapi, audio: add query-audiodev command
Posted by Thomas Huth 2 years, 5 months ago
From: Daniel P. Berrangé <berrange@redhat.com>

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 alone is
maybe not very useful by itself, but it makes Audiodev introspectable
via 'query-qmp-schema', so that libvirt (and other upper layer tools)
can discover the available audiodevs.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
[thuth: Update for upcoming QEMU v8.0, and use QAPI_LIST_PREPEND]
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 qapi/audio.json | 13 +++++++++++++
 audio/audio.c   | 12 ++++++++++++
 2 files changed, 25 insertions(+)

diff --git a/qapi/audio.json b/qapi/audio.json
index 1e0a24bdfc..c7aafa2763 100644
--- a/qapi/audio.json
+++ b/qapi/audio.json
@@ -443,3 +443,16 @@
     'sndio':     'AudiodevSndioOptions',
     'spice':     'AudiodevGenericOptions',
     'wav':       'AudiodevWavOptions' } }
+
+##
+# @query-audiodevs:
+#
+# Returns information about audiodev configuration
+#
+# Returns: array of @Audiodev
+#
+# Since: 8.0
+#
+##
+{ 'command': 'query-audiodevs',
+  'returns': ['Audiodev'] }
diff --git a/audio/audio.c b/audio/audio.c
index d849a94a81..6f270c07b7 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 "qemu/help_option.h"
@@ -2311,3 +2313,13 @@ size_t audio_rate_get_bytes(RateCtl *rate, struct audio_pcm_info *info,
 
     return bytes;
 }
+
+AudiodevList *qmp_query_audiodevs(Error **errp)
+{
+    AudiodevList *ret = NULL;
+    AudiodevListEntry *e;
+    QSIMPLEQ_FOREACH(e, &audiodevs, next) {
+        QAPI_LIST_PREPEND(ret, QAPI_CLONE(Audiodev, e->dev));
+    }
+    return ret;
+}
-- 
2.31.1


Re: [PATCH v2 1/2] qapi, audio: add query-audiodev command
Posted by Philippe Mathieu-Daudé 2 years, 5 months ago
On 23/1/23 09:39, Thomas Huth wrote:
> From: Daniel P. Berrangé <berrange@redhat.com>
> 
> 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 alone is
> maybe not very useful by itself, but it makes Audiodev introspectable
> via 'query-qmp-schema', so that libvirt (and other upper layer tools)
> can discover the available audiodevs.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> [thuth: Update for upcoming QEMU v8.0, and use QAPI_LIST_PREPEND]
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   qapi/audio.json | 13 +++++++++++++
>   audio/audio.c   | 12 ++++++++++++
>   2 files changed, 25 insertions(+)
> 
> diff --git a/qapi/audio.json b/qapi/audio.json
> index 1e0a24bdfc..c7aafa2763 100644
> --- a/qapi/audio.json
> +++ b/qapi/audio.json
> @@ -443,3 +443,16 @@
>       'sndio':     'AudiodevSndioOptions',
>       'spice':     'AudiodevGenericOptions',
>       'wav':       'AudiodevWavOptions' } }
> +
> +##
> +# @query-audiodevs:
> +#
> +# Returns information about audiodev configuration

Maybe clearer as 'audio backends'?

So similarly, wouldn't be clearer to name this command
'query-audio-backends'? Otherwise we need to go read QEMU
source to understand what is 'audiodevs'.

> +#
> +# Returns: array of @Audiodev
> +#
> +# Since: 8.0
> +#
> +##
> +{ 'command': 'query-audiodevs',
> +  'returns': ['Audiodev'] }
> diff --git a/audio/audio.c b/audio/audio.c
> index d849a94a81..6f270c07b7 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 "qemu/help_option.h"
> @@ -2311,3 +2313,13 @@ size_t audio_rate_get_bytes(RateCtl *rate, struct audio_pcm_info *info,
>   
>       return bytes;
>   }
> +
> +AudiodevList *qmp_query_audiodevs(Error **errp)
> +{
> +    AudiodevList *ret = NULL;
> +    AudiodevListEntry *e;
> +    QSIMPLEQ_FOREACH(e, &audiodevs, next) {

I am a bit confused here, isn't &audiodevs containing what the user 
provided from CLI? How is that useful to libvirt? Maybe the corner case
of a user hand-modifying the QEMU launch arguments from a XML config?

Wouldn't a list of linked in AudiodevDriver be more useful to libvirt
so it could pick the best available backend to start a VM?

> +        QAPI_LIST_PREPEND(ret, QAPI_CLONE(Audiodev, e->dev));
> +    }
> +    return ret;
> +}


Re: [PATCH v2 1/2] qapi, audio: add query-audiodev command
Posted by Daniel P. Berrangé 2 years, 5 months ago
On Mon, Jan 23, 2023 at 10:20:29AM +0100, Philippe Mathieu-Daudé wrote:
> On 23/1/23 09:39, Thomas Huth wrote:
> > From: Daniel P. Berrangé <berrange@redhat.com>
> > 
> > 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 alone is
> > maybe not very useful by itself, but it makes Audiodev introspectable
> > via 'query-qmp-schema', so that libvirt (and other upper layer tools)
> > can discover the available audiodevs.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > [thuth: Update for upcoming QEMU v8.0, and use QAPI_LIST_PREPEND]
> > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > ---
> >   qapi/audio.json | 13 +++++++++++++
> >   audio/audio.c   | 12 ++++++++++++
> >   2 files changed, 25 insertions(+)
> > 
> > diff --git a/qapi/audio.json b/qapi/audio.json
> > index 1e0a24bdfc..c7aafa2763 100644
> > --- a/qapi/audio.json
> > +++ b/qapi/audio.json
> > @@ -443,3 +443,16 @@
> >       'sndio':     'AudiodevSndioOptions',
> >       'spice':     'AudiodevGenericOptions',
> >       'wav':       'AudiodevWavOptions' } }
> > +
> > +##
> > +# @query-audiodevs:
> > +#
> > +# Returns information about audiodev configuration
> 
> Maybe clearer as 'audio backends'?
> 
> So similarly, wouldn't be clearer to name this command
> 'query-audio-backends'? Otherwise we need to go read QEMU
> source to understand what is 'audiodevs'.

The command line parameter is called '-audiodev' and this
query-audiodevs command reports the same data, so that
looks easy enough to understand IMHO.

> > +#
> > +# Returns: array of @Audiodev
> > +#
> > +# Since: 8.0
> > +#
> > +##
> > +{ 'command': 'query-audiodevs',
> > +  'returns': ['Audiodev'] }
> > diff --git a/audio/audio.c b/audio/audio.c
> > index d849a94a81..6f270c07b7 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 "qemu/help_option.h"
> > @@ -2311,3 +2313,13 @@ size_t audio_rate_get_bytes(RateCtl *rate, struct audio_pcm_info *info,
> >       return bytes;
> >   }
> > +
> > +AudiodevList *qmp_query_audiodevs(Error **errp)
> > +{
> > +    AudiodevList *ret = NULL;
> > +    AudiodevListEntry *e;
> > +    QSIMPLEQ_FOREACH(e, &audiodevs, next) {
> 
> I am a bit confused here, isn't &audiodevs containing what the user provided
> from CLI? How is that useful to libvirt? Maybe the corner case
> of a user hand-modifying the QEMU launch arguments from a XML config?
> 
> Wouldn't a list of linked in AudiodevDriver be more useful to libvirt
> so it could pick the best available backend to start a VM?

On the libvirt side we're never going to need to actually call the
query-audiodevs commands. The mere existance of the command, means
that the QMP schema now exposes information about what audio backends
have been compiled into the binary. This is the same trick we've used
for other aspects of QMP. IOW we don't need a separate command just
for the purpose of listing AudiodevDrivers.

The idea of a query-audiodevs command is useful in general. When we
later gain support for hotplug/unplug of audio, the set of audiodevs
will no longer be guaranteed to match the original CLI args.

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


Re: [PATCH v2 1/2] qapi, audio: add query-audiodev command
Posted by Philippe Mathieu-Daudé 2 years, 5 months ago
On 23/1/23 12:11, Daniel P. Berrangé wrote:
> On Mon, Jan 23, 2023 at 10:20:29AM +0100, Philippe Mathieu-Daudé wrote:
>> On 23/1/23 09:39, Thomas Huth wrote:
>>> From: Daniel P. Berrangé <berrange@redhat.com>
>>>
>>> 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 alone is
>>> maybe not very useful by itself, but it makes Audiodev introspectable
>>> via 'query-qmp-schema', so that libvirt (and other upper layer tools)
>>> can discover the available audiodevs.
>>>
>>> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>>> [thuth: Update for upcoming QEMU v8.0, and use QAPI_LIST_PREPEND]
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>>    qapi/audio.json | 13 +++++++++++++
>>>    audio/audio.c   | 12 ++++++++++++
>>>    2 files changed, 25 insertions(+)
>>>
>>> diff --git a/qapi/audio.json b/qapi/audio.json
>>> index 1e0a24bdfc..c7aafa2763 100644
>>> --- a/qapi/audio.json
>>> +++ b/qapi/audio.json
>>> @@ -443,3 +443,16 @@
>>>        'sndio':     'AudiodevSndioOptions',
>>>        'spice':     'AudiodevGenericOptions',
>>>        'wav':       'AudiodevWavOptions' } }
>>> +
>>> +##
>>> +# @query-audiodevs:
>>> +#
>>> +# Returns information about audiodev configuration
>>
>> Maybe clearer as 'audio backends'?
>>
>> So similarly, wouldn't be clearer to name this command
>> 'query-audio-backends'? Otherwise we need to go read QEMU
>> source to understand what is 'audiodevs'.
> 
> The command line parameter is called '-audiodev' and this
> query-audiodevs command reports the same data, so that
> looks easy enough to understand IMHO.
> 
>>> +#
>>> +# Returns: array of @Audiodev
>>> +#
>>> +# Since: 8.0
>>> +#
>>> +##
>>> +{ 'command': 'query-audiodevs',
>>> +  'returns': ['Audiodev'] }
>>> diff --git a/audio/audio.c b/audio/audio.c
>>> index d849a94a81..6f270c07b7 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 "qemu/help_option.h"
>>> @@ -2311,3 +2313,13 @@ size_t audio_rate_get_bytes(RateCtl *rate, struct audio_pcm_info *info,
>>>        return bytes;
>>>    }
>>> +
>>> +AudiodevList *qmp_query_audiodevs(Error **errp)
>>> +{
>>> +    AudiodevList *ret = NULL;
>>> +    AudiodevListEntry *e;
>>> +    QSIMPLEQ_FOREACH(e, &audiodevs, next) {
>>
>> I am a bit confused here, isn't &audiodevs containing what the user provided
>> from CLI? How is that useful to libvirt? Maybe the corner case
>> of a user hand-modifying the QEMU launch arguments from a XML config?
>>
>> Wouldn't a list of linked in AudiodevDriver be more useful to libvirt
>> so it could pick the best available backend to start a VM?
> 
> On the libvirt side we're never going to need to actually call the
> query-audiodevs commands. The mere existance of the command, means
> that the QMP schema now exposes information about what audio backends
> have been compiled into the binary. This is the same trick we've used
> for other aspects of QMP. IOW we don't need a separate command just
> for the purpose of listing AudiodevDrivers.

I understand having "what audio backends have been compiled into the
binary" is useful, but I am missing how you get that from &audiodevs.

AFAICT &audiodevs is for the CLI parsed backends, not all the backends
linked within a binary. I probably need sugar / coffee and will revisit
after lunch.

> The idea of a query-audiodevs command is useful in general. When we
> later gain support for hotplug/unplug of audio, the set of audiodevs
> will no longer be guaranteed to match the original CLI args.
> 
> With regards,
> Daniel


Re: [PATCH v2 1/2] qapi, audio: add query-audiodev command
Posted by Daniel P. Berrangé 2 years, 5 months ago
On Mon, Jan 23, 2023 at 01:05:45PM +0100, Philippe Mathieu-Daudé wrote:
> On 23/1/23 12:11, Daniel P. Berrangé wrote:
> > On Mon, Jan 23, 2023 at 10:20:29AM +0100, Philippe Mathieu-Daudé wrote:
> > > On 23/1/23 09:39, Thomas Huth wrote:
> > > > From: Daniel P. Berrangé <berrange@redhat.com>
> > > > 
> > > > 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 alone is
> > > > maybe not very useful by itself, but it makes Audiodev introspectable
> > > > via 'query-qmp-schema', so that libvirt (and other upper layer tools)
> > > > can discover the available audiodevs.
> > > > 
> > > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > > > [thuth: Update for upcoming QEMU v8.0, and use QAPI_LIST_PREPEND]
> > > > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > > > ---
> > > >    qapi/audio.json | 13 +++++++++++++
> > > >    audio/audio.c   | 12 ++++++++++++
> > > >    2 files changed, 25 insertions(+)
> > > > 
> > > > diff --git a/qapi/audio.json b/qapi/audio.json
> > > > index 1e0a24bdfc..c7aafa2763 100644
> > > > --- a/qapi/audio.json
> > > > +++ b/qapi/audio.json
> > > > @@ -443,3 +443,16 @@
> > > >        'sndio':     'AudiodevSndioOptions',
> > > >        'spice':     'AudiodevGenericOptions',
> > > >        'wav':       'AudiodevWavOptions' } }
> > > > +
> > > > +##
> > > > +# @query-audiodevs:
> > > > +#
> > > > +# Returns information about audiodev configuration
> > > 
> > > Maybe clearer as 'audio backends'?
> > > 
> > > So similarly, wouldn't be clearer to name this command
> > > 'query-audio-backends'? Otherwise we need to go read QEMU
> > > source to understand what is 'audiodevs'.
> > 
> > The command line parameter is called '-audiodev' and this
> > query-audiodevs command reports the same data, so that
> > looks easy enough to understand IMHO.
> > 
> > > > +#
> > > > +# Returns: array of @Audiodev
> > > > +#
> > > > +# Since: 8.0
> > > > +#
> > > > +##
> > > > +{ 'command': 'query-audiodevs',
> > > > +  'returns': ['Audiodev'] }
> > > > diff --git a/audio/audio.c b/audio/audio.c
> > > > index d849a94a81..6f270c07b7 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 "qemu/help_option.h"
> > > > @@ -2311,3 +2313,13 @@ size_t audio_rate_get_bytes(RateCtl *rate, struct audio_pcm_info *info,
> > > >        return bytes;
> > > >    }
> > > > +
> > > > +AudiodevList *qmp_query_audiodevs(Error **errp)
> > > > +{
> > > > +    AudiodevList *ret = NULL;
> > > > +    AudiodevListEntry *e;
> > > > +    QSIMPLEQ_FOREACH(e, &audiodevs, next) {
> > > 
> > > I am a bit confused here, isn't &audiodevs containing what the user provided
> > > from CLI? How is that useful to libvirt? Maybe the corner case
> > > of a user hand-modifying the QEMU launch arguments from a XML config?
> > > 
> > > Wouldn't a list of linked in AudiodevDriver be more useful to libvirt
> > > so it could pick the best available backend to start a VM?
> > 
> > On the libvirt side we're never going to need to actually call the
> > query-audiodevs commands. The mere existance of the command, means
> > that the QMP schema now exposes information about what audio backends
> > have been compiled into the binary. This is the same trick we've used
> > for other aspects of QMP. IOW we don't need a separate command just
> > for the purpose of listing AudiodevDrivers.
> 
> I understand having "what audio backends have been compiled into the
> binary" is useful, but I am missing how you get that from &audiodevs.
> 
> AFAICT &audiodevs is for the CLI parsed backends, not all the backends
> linked within a binary. I probably need sugar / coffee and will revisit
> after lunch.

It ties into the 'query-qmp-schema' command, along with patch #2 in
this series.

The query-audiodevs command will cause the 'AudiodevDriver' enum to
be reported in the 'query-qmp-schema' response. Patch #2, makes all
the AudiodevDriver enum entries conditional on CONFIG_XXXX.

IOW now query-qmp-schema data will tell you what AudiodevDriver
backends are compiled into the binary you're talking to.

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


Re: [PATCH v2 1/2] qapi, audio: add query-audiodev command
Posted by Philippe Mathieu-Daudé 2 years, 5 months ago
On 23/1/23 13:09, Daniel P. Berrangé wrote:
> On Mon, Jan 23, 2023 at 01:05:45PM +0100, Philippe Mathieu-Daudé wrote:
>> On 23/1/23 12:11, Daniel P. Berrangé wrote:
>>> On Mon, Jan 23, 2023 at 10:20:29AM +0100, Philippe Mathieu-Daudé wrote:
>>>> On 23/1/23 09:39, Thomas Huth wrote:
>>>>> From: Daniel P. Berrangé <berrange@redhat.com>

>>>>> +AudiodevList *qmp_query_audiodevs(Error **errp)
>>>>> +{
>>>>> +    AudiodevList *ret = NULL;
>>>>> +    AudiodevListEntry *e;
>>>>> +    QSIMPLEQ_FOREACH(e, &audiodevs, next) {
>>>>
>>>> I am a bit confused here, isn't &audiodevs containing what the user provided
>>>> from CLI? How is that useful to libvirt? Maybe the corner case
>>>> of a user hand-modifying the QEMU launch arguments from a XML config?
>>>>
>>>> Wouldn't a list of linked in AudiodevDriver be more useful to libvirt
>>>> so it could pick the best available backend to start a VM?
>>>
>>> On the libvirt side we're never going to need to actually call the
>>> query-audiodevs commands. The mere existance of the command, means
>>> that the QMP schema now exposes information about what audio backends
>>> have been compiled into the binary. This is the same trick we've used
>>> for other aspects of QMP. IOW we don't need a separate command just
>>> for the purpose of listing AudiodevDrivers.
>>
>> I understand having "what audio backends have been compiled into the
>> binary" is useful, but I am missing how you get that from &audiodevs.
>>
>> AFAICT &audiodevs is for the CLI parsed backends, not all the backends
>> linked within a binary. I probably need sugar / coffee and will revisit
>> after lunch.
> 
> It ties into the 'query-qmp-schema' command, along with patch #2 in
> this series.
> 
> The query-audiodevs command will cause the 'AudiodevDriver' enum to
> be reported in the 'query-qmp-schema' response. Patch #2, makes all
> the AudiodevDriver enum entries conditional on CONFIG_XXXX.
> 
> IOW now query-qmp-schema data will tell you what AudiodevDriver
> backends are compiled into the binary you're talking to.

Thanks for the explanation Daniel.

Just FTR, while I'm not confident enough to add a R-b tag, I am
not opposed to it.

Regards,

Phil.

Re: [PATCH v2 1/2] qapi, audio: add query-audiodev command
Posted by Thomas Huth 2 years, 5 months ago
On 23/01/2023 13.09, Daniel P. Berrangé wrote:
> On Mon, Jan 23, 2023 at 01:05:45PM +0100, Philippe Mathieu-Daudé wrote:
>> On 23/1/23 12:11, Daniel P. Berrangé wrote:
>>> On Mon, Jan 23, 2023 at 10:20:29AM +0100, Philippe Mathieu-Daudé wrote:
>>>> On 23/1/23 09:39, Thomas Huth wrote:
>>>>> From: Daniel P. Berrangé <berrange@redhat.com>
>>>>>
>>>>> 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 alone is
>>>>> maybe not very useful by itself, but it makes Audiodev introspectable
>>>>> via 'query-qmp-schema', so that libvirt (and other upper layer tools)
>>>>> can discover the available audiodevs.
>>>>>
>>>>> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>>>>> [thuth: Update for upcoming QEMU v8.0, and use QAPI_LIST_PREPEND]
>>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>>> ---
>>>>>     qapi/audio.json | 13 +++++++++++++
>>>>>     audio/audio.c   | 12 ++++++++++++
>>>>>     2 files changed, 25 insertions(+)
>>>>>
>>>>> diff --git a/qapi/audio.json b/qapi/audio.json
>>>>> index 1e0a24bdfc..c7aafa2763 100644
>>>>> --- a/qapi/audio.json
>>>>> +++ b/qapi/audio.json
>>>>> @@ -443,3 +443,16 @@
>>>>>         'sndio':     'AudiodevSndioOptions',
>>>>>         'spice':     'AudiodevGenericOptions',
>>>>>         'wav':       'AudiodevWavOptions' } }
>>>>> +
>>>>> +##
>>>>> +# @query-audiodevs:
>>>>> +#
>>>>> +# Returns information about audiodev configuration
>>>>
>>>> Maybe clearer as 'audio backends'?
>>>>
>>>> So similarly, wouldn't be clearer to name this command
>>>> 'query-audio-backends'? Otherwise we need to go read QEMU
>>>> source to understand what is 'audiodevs'.
>>>
>>> The command line parameter is called '-audiodev' and this
>>> query-audiodevs command reports the same data, so that
>>> looks easy enough to understand IMHO.

Should we maybe use a "x-" prefix here if we feel not certain enough about 
this command yet? ... that would still enable the Audiodev part in the qapi 
schema, but give us the flexibility to rename or drop it later in case there 
is some better way to enable the Audiodevs in the schema?

  Thomas


Re: [PATCH v2 1/2] qapi, audio: add query-audiodev command
Posted by Daniel P. Berrangé 2 years, 5 months ago
On Wed, Jan 25, 2023 at 12:06:40PM +0100, Thomas Huth wrote:
> On 23/01/2023 13.09, Daniel P. Berrangé wrote:
> > On Mon, Jan 23, 2023 at 01:05:45PM +0100, Philippe Mathieu-Daudé wrote:
> > > On 23/1/23 12:11, Daniel P. Berrangé wrote:
> > > > On Mon, Jan 23, 2023 at 10:20:29AM +0100, Philippe Mathieu-Daudé wrote:
> > > > > On 23/1/23 09:39, Thomas Huth wrote:
> > > > > > From: Daniel P. Berrangé <berrange@redhat.com>
> > > > > > 
> > > > > > 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 alone is
> > > > > > maybe not very useful by itself, but it makes Audiodev introspectable
> > > > > > via 'query-qmp-schema', so that libvirt (and other upper layer tools)
> > > > > > can discover the available audiodevs.
> > > > > > 
> > > > > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > > > > > [thuth: Update for upcoming QEMU v8.0, and use QAPI_LIST_PREPEND]
> > > > > > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > > > > > ---
> > > > > >     qapi/audio.json | 13 +++++++++++++
> > > > > >     audio/audio.c   | 12 ++++++++++++
> > > > > >     2 files changed, 25 insertions(+)
> > > > > > 
> > > > > > diff --git a/qapi/audio.json b/qapi/audio.json
> > > > > > index 1e0a24bdfc..c7aafa2763 100644
> > > > > > --- a/qapi/audio.json
> > > > > > +++ b/qapi/audio.json
> > > > > > @@ -443,3 +443,16 @@
> > > > > >         'sndio':     'AudiodevSndioOptions',
> > > > > >         'spice':     'AudiodevGenericOptions',
> > > > > >         'wav':       'AudiodevWavOptions' } }
> > > > > > +
> > > > > > +##
> > > > > > +# @query-audiodevs:
> > > > > > +#
> > > > > > +# Returns information about audiodev configuration
> > > > > 
> > > > > Maybe clearer as 'audio backends'?
> > > > > 
> > > > > So similarly, wouldn't be clearer to name this command
> > > > > 'query-audio-backends'? Otherwise we need to go read QEMU
> > > > > source to understand what is 'audiodevs'.
> > > > 
> > > > The command line parameter is called '-audiodev' and this
> > > > query-audiodevs command reports the same data, so that
> > > > looks easy enough to understand IMHO.
> 
> Should we maybe use a "x-" prefix here if we feel not certain enough about
> this command yet? ... that would still enable the Audiodev part in the qapi
> schema, but give us the flexibility to rename or drop it later in case there
> is some better way to enable the Audiodevs in the schema?

IMHO there's no reason to add an 'x-' prefix. Even if we found a better
way to detect existance of Audiodev backend types, I think query-audiodev
is still conceptually a useful command for interogating QEMU's config.
To get to our goal of a 100% QMP based replacement for the CLI, we want
to have QMP commands for adding, removing and querying every backend
config (audiodev, device, object, chardev, netdev, etc). This command
addresses one of those gaps for audiodevs

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