Re: [Qemu-devel] [PATCH RFC v4 30/32] qapi: New QMP command query-schema for QMP schema introspection

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 09/04/2015 03:55 AM, Markus Armbruster wrote:
>
>>>> +
>>>> +query-schema returns a JSON array of SchemaInfo objects.  These
>>>> +objects together describe the wire ABI, as defined in the QAPI schema.
>>>
>>> Maybe an additional sentence here, perhaps along the lines of:
>>>
>>> Note that the wire format cannot express everything; it is not designed
>>> to show semantic constraints (such as when exactly one of a pair of
>>> mutually-exclusive optional members must be present, or when an integral
>>> value has specific limitations on valid values).  Introspection allows
>>> an application to know if a feature is present, but the application must
>>> obey the qapi documentation to properly interact with that feature.
>> 
>> Michael raised the same point, and I drafted a paragraph in my reply to
>> him:
>> 
>>     The SchemaInfo can't reflect all the rules and restrictions that
>>     apply to QMP.  It's interface introspection (figuring out what's
>>     there), not interface specification.  The specification is in the
>>     QAPI schema, and users should peruse it to understand how QMP is to
>>     be used.
>> 
>> Then continue with an example showing a restriction that's spelled out
>> in the QAPI schema, but can't be expressed in SchemaInfo.
>> 
>> Use mine as is?  Merge our two proposals somehow?
>
> Your wording works okay for me, but if you want to merge any elements of
> my wording in, go for it.

I'll probably use mine, but I'll look at it once more when I respin this
patch.

>>>> +The SchemaInfo for a command has meta-type "command", and variant
>>>> +members "arg-type" and "ret-type".  The arguments member clients pass
>>>
>>> I found it easier to read with:
>>> s/arguments/arguments that/
>> 
>> Easier to read, but no longer emphasizes that the specific "arguments"
>> member of the { "execute" ... } form needs to conform.  I'll use yours
>> anyway, unless you have a better idea.
>> 
>>>> +with a command on the wire must conform to the type named by
>>>> +"arg-type", which is always an object type.  The return value the
>>>
>>> Likewise s/value/value that/
>> 
>> Likewise.
>> 
>>>> +server passes in a success response conforms to the type named by
>>>> +"ret-type".
>
> Maybe:
>
> On the wire, the "arguments" key of a client's "execute" command must
> conform to the type named by "arg-type", which is always an object type.
> The "return" key that the server passes in a success response will
> always conform to the type named by "ret-type".
>
> Up to you; we'll see how it looks in v5, and at that point, any further
> word-smithing can be done in followup patches rather than delaying this
> series further.

Sounds nice, I'll probably adopt it.

>>> [not for this patch. Thinking aloud: I wonder if QGA's success-response
>>> could be coded in by having 'ret-type':null as a way of saying the
>>> command will have no response if it is successful; perhaps a bit nicer
>>> than exposing an additional boolean flag member]
>> 
>> The QAPI schema could also do it this way.  What do you think?
>
> Sounds like I have another trivial followup patch added to my work queue :)
>
>
>>>> +The SchemaInfo for an event has meta-type "event", and variant member
>>>> +"arg-type".  The data member the server passes with an event conforms
>>>
>>> again, reads a little better with s/member/member that/
>> 
>> My remark on command arguments applies.
>> 
>>>> +to the type named by "arg-type".  It is always an object type.
>
> Maybe:
>
> On the wire, the "data" member included in any event passed by the
> server will conform to the type named by "arg-type".
>
> I'm not sure if we need to document whether it is always an object type;
> command arguments are user-supplied, while event data is
> server-supplied.  And there's the question of any possible duplication
> with the QMP spec file.  I guess I'm fine as long as we aren't stating
> conflicting things between the two places.

I feel duplication is okay when it makes qapi-code-gen.txt easier to
understand.

>>>> +
>>>> +"members" is a JSON array describing the object's common members.
>> 
>> Could say "the object's common members, if any."  What do you think?
>
> Yes, adding the 'if any' helps.

Sold.

>>>
>>> Hmm, the "Union types" section has two mentions of BlockdevOptions; and
>>> this example matches the second. It may help readability if we rename
>>> one of the two examples to be distinct (preferably the first, since it
>>> doesn't match actual QMP).  I guess the phrase "flat union
>>> BlockdevOptions" is sufficient to make it obvious that we are referring
>>> to the second usage, but it is subtle.
>> 
>> Follow-up patch?
>
> Yes.
>
>>>
>>> [1] Ouch.  That should be "name": "type".  I pointed out the bug in v3,
>>> but it looks like it still hasn't been fixed.  Or rather, that our
>>> attempt to fix it wasn't correct.
>> 
>> I'll double-check.
>
> As you noted later, I've already done that legwork.

Thanks a lot for that!

>>>> +
>>>> +As explained above, type names are not part of the wire ABI.  Not even
>>>> +the names of built-in types.  Clients should examine member
>>>> +"json-type" instead of hard-coding names of built-in types.
>>>
>>> Good point (although we may still end up with clients that disobey this,
>>> I will certainly make sure to do this in libvirt's interactions).
>> 
>> We can't fully prevent misuse of our interfaces.  Where we can't, we can
>> still try to make correct use as easy and obvious as we can.
>> 
>> PATCH 32 prevents misuse of most type names by hiding them.  RFC v2 hid
>> all type names.  In review, we decided not to hide the names of built-in
>> types.  Tradeoff: makes the introspection value easier to read for
>> humans (good), but enables misuse by machines (bad).
>> 
>> Either way works for me, hide non-built-in type names (and have docs
>> tell users not to rely on them), or hide them all.  Got a preference?
>> 
>
> I'm still leaning towards exposing built-in type names directly, and
> furthermore exposing arrays via '[123]' naming. As you say, correct
> clients shouldn't be abusing this, but it IS easier to see, and I'm
> having a hard time seeing how it could paint us into corners in the
> future for having exposed too much information up front (while we DO
> rename object types and don't want that to affect ABI, I don't see us
> trying to rename builtin types - at most, we might add subtypes with
> range or other restrictions, but those would be new types and still
> leave the basic 'int' and 'str' unchanged).  So leaving it as just the
> documentation on what proper clients should do is sufficient; don't go
> back to the v2 munging of builtin names.

Okay, no change then.  We can explore arrays in follow-up patches.

>>>> +++ b/qapi/introspect.json
>>>> @@ -0,0 +1,271 @@
>>>
>>>> +
>>>> +##
>>>> +# @query-schema
>>>> +#
>>>> +# Command query-schema exposes the QMP wire ABI as an array of
>>>> +# SchemaInfo.  This lets QMP clients figure out what commands and
>>>> +# events are available in this QEMU, and their parameters and results.
>>>> +#
>>>> +# Returns: array of @SchemaInfo, where each element describes an
>>>> +# entity in the ABI: command, event, type, ...
>>>> +#
>>>> +# Note: the QAPI schema is also used to help define *internal*
>>>> +# interfaces, by defining QAPI types.  These are not part of the QMP
>>>> +# wire ABI, and therefore not returned by this command.
>>>
>>> Same as for the docs above: may be worth a paragraph explicitly
>>> mentioning that the wire ABI cannot express everything, such as integer
>>> ranges, or such as semantics where exactly one of two mutually-exclusive
>>> optional members must be present.
>> 
>> Here's where Michael commented.
>
> Actually, he mentioned the .hx file, which I didn't. I don't know how
> many duplicate places need to refer to the same information, or just
> abbreviate by pointing to other places. But I'll be happy as long as the
> information is present at least somewhere, and that somewhere is easy to
> find (perhaps by good references rather than duplications in the other
> places).

Judging from how I look for information, the QAPI schema should have the
complete reference information, while pointers suffice for
qmp-commands.hx.  docs/qapi-code-gen.txt is introduction / big picture,
so it should *explain* things, but it doesn't have to be exhaustively
complete.

>>>
>>> As required by RFC 7159, the order of individual elements with in the
>>> array sent over the wire is assumed to be significant unless the
>>> documented semantics in qapi state otherwise.
>> 
>> Got an example where insignificance of order actually matters?
>
> migrate-set-capabilities does not care what order the 'capabilities'
> array is in, for example.

Correct, but I'm not sure it matters enough to justify repeating basic
JSON semantics here once more, so let's leave things as they are.  Could
be just me getting worn out by this series (I really, really want it
finished).  Perhaps I'll be more amenable to a follow-up patch than to
changing it now.

>>>> +{ 'struct': 'SchemaInfoObjectVariant',
>>>> +  'data': { 'case': 'str', 'type': 'str' } }
>>>
>>> Do we want to document whether all case values of the tag are guaranteed
>>> to be covered (happens to be true for all current qapi, but is not yet
>>> enforced; although that's one of my planned followup patches is to start
>>> enforcing it - so I guess I can document it at that time if we don't do
>>> it here)
>> 
>> Requiring all cases to be covered explicitly may have value in the QAPI
>> schema (more verbose, but you get a reminder to update the union after
>> you extended the tag enum), but it's pretty useless in introspection,
>> isn't it?
>
> At first glance, I wouldn't call it useless.  But thinking about it
> more: Right now, if not all members of the enum are listed, the
> generated code abort()s when passing the uncovered enum value.  But
> ideally we want semantics where an omitted case in the qapi (or maybe
> explicit 'case':null or 'case':{} notation) results in adding no further
> variant members to the overall object.  In that case, knowing that the
> tag enum value is valid and maps to ':empty' on the wire is worth
> advertising, even if we choose to allow omitting a case in the qapi
> .json as the syntax for that action.
>
> So we can touch up the wording at the same time we fix code to not abort().

SchemaInfoObject needs to show the complete set of accepted type tag
values, and for each value, the variant members.

Currently, SchemaInfoObject.tag always names a member of enumeration
type (I guess I should add that to the docs).  That type is the set of
accepted type tag values.

SchemaInfoObject.variants neither adds nor removes from that set.  All
it does is show variant members.

If the object has none for a certain type tag value, then a natural and
compact way to encode that is not to have it in .variants.

Another way is to have a rather pointless .variants member that
explicitly states "no members", basically {"case": "FOO", "type":
":empty"}.

Only if we permitted the type tags type to have more values than object
actually accepts as type tags, the presence of a type tag value in
.variants becomes meaningful even when it adds no variant members.

Why would we ever want that?

>>>> +
>>>> +# Caveman's json.dumps() replacement (we're stuck at 2.4)
>>>
>>> s/2.4/python 2.4/
>> 
>> Okay.
>
> Of course, if Dan's query proves we can bump our minimum to python 2.6
> anyways, this may go away soon.

I'll start a thread.