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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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