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

[Daniel P . Berrangé]

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