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

[Eric Blake]

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.


>>> +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.

>> [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.


>>> +
>>> +"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.


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


>>> +
>>> +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.

>>> +++ 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).


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


>>> +{ '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().


>>> +
>>> +# 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.

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

signature.asc
Description: OpenPGP digital signature