Re: [Qemu-devel] [PATCH 0/1] Get the list of arguments from a QMP command

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4633 bytes --]

On 02/24/2015 06:51 AM, Alberto Garcia wrote:
> Hello,
> 
> this is a follow-up to the comments from Eric Blake about my patches
> to extend the block streaming API:
> 
>    https://lists.gnu.org/archive/html/qemu-devel/2015-02/msg04231.html
> 
> Since QEMU doesn't have an easy way to tell the arguments that a
> particular QMP command supports, and it seems that it would be useful
> for libvirt and possibly others, I decided to write a simple patch
> that adds that feature.

This is potentially a step in the right direction for full
introspection, but I'm a bit worried that it's half-baked.  That is, it
only states whether an argument is present or not, but doesn't say what
type those arguments are, and most glaringly, doesn't tell me anything
about type changes, such as adding new enum values or new struct
members.  In other words, while this interface might help libvirt, it
won't solve all the qapi questions that libvirt has, and I'm worried
that committing this and then adding full introspection will burden us
with multiple implementations to keep running.

> 
> This is not an attempt to implement full introspection, but rather a
> subset for this use case. I anyway tried to design it so it's easy to
> extend in the future with the rest of the information.
> 
> I added a new type, CommandArgumentInfo, which includes the name of an
> argument and a boolean expressing whether it's optional or not.
> 
>      { 'type': 'CommandArgumentInfo',
>        'data': {'name': 'str', 'optional': 'bool'} }

At least this representation IS extensible - you could add another dict
member giving the argument's type at a later date, as needed.

> 
> I did not include the type of the argument since it would complicate
> the code and would require me to parse the json files in order to
> include all the details. My understanding is that for this use case
> what's important for libvirt is knowing whether the argument is
> supported, not its type (which libvirt should already know). But
> please correct me if I'm wrong.

Indeed, knowing that an argument exists is often more important that
knowing its type (since we try to keep the type backward-compatible,
even when changing from 'str' to an enum means that libvirt could see
two different type answers according to which version of qemu it is
talking to, but libvirt's behavior in managing that parameter doesn't
care which type was reported, only that the parameter exists).

> 
> A new command 'query-command-args' returns the list of all supported
> arguments for a particular command:
> 
>      { 'command': 'query-command-args',
>        'data': {'command': 'str' },
>        'returns': ['CommandArgumentInfo'] }

The 'command' argument be optional, where omitting it gives an array of
ALL commands in one QMP call.  Otherwise, returning an array doesn't
make as much sense if it will always be a one-element array because the
filtering was mandatory.

But do we need a new command?

> 
> Alternatively, the existing 'query-commands' can be extended to accept
> an optional parameter that specifies whether to return the arguments
> for each command. That list of arguments would be added to the
> 'CommandInfo' type:
> 
>      { 'command': 'query-commands',
>        'data': {'*query-args': 'bool' },
>        'returns': ['CommandInfo'] }
> 
>      { 'type': 'CommandInfo',
>        'data': {'name': 'str', 'args': ['CommandArgumentInfo'] } }

Indeed, you already anticipated my question - I think that extending the
existing 'query-commands' API is just as easy to do.

> 
> I added both alternatives in this patch since I don't know what's the
> most convenient one for libvirt, but of course either of them can be
> removed. The amount of C code needed to have both compared to just one
> is negligible, though.
> 
> Feedback is welcome.

I'm still thinking about the actual patch, and whether we want to commit
to this or just bite the bullet and go for full introspection.  At any
rate, it's a bit late for 2.3, so we have the full 2.4 cycle to get it
right.

> 
> Thanks,
> 
> Berto
> 
> Alberto Garcia (1):
>   qmp: Add support for requesting the list of arguments from a command
> 
>  monitor.c        | 53 +++++++++++++++++++++++++++++++-
>  qapi/common.json | 41 +++++++++++++++++++++++--
>  qmp-commands.hx  | 93 ++++++++++++++++++++++++++++++++++++++++++++++++++++----
>  3 files changed, 178 insertions(+), 9 deletions(-)
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]