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

["Daniel P. Berrangé" <berrange@redhat.com>]

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