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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 08/04/2015 09:58 AM, Markus Armbruster wrote:
>> * All type references are by name.
>> 
>
>>   Dictionary argument/result or list result is an implicit type
>>   definition.
>> 
>
>> 
>> * Clients should *not* look up types by name, because type names are
>>   not ABI.  Look up the command or event you're interested in, then
>>   follow the references.
>> 
>>   TODO Should we hide the type names to eliminate the temptation?
>
> [not sure this conversation belongs better on 32 than here]
>
> I was just experimenting with things, and noticed that our choice of
> array names might not be ideal for clients.  We already discussed
> earlier in the series that the name 'intList' might collide (the parser
> currently rejects collisions with '*Kind', but not '*List').  But in
> addition to that, look at what happens here in this patch:
>
>     "{ \"element-type\": \"str\", \"meta-type\": \"array\", \"name\":
> \"strList\" }, "
>
>     "{ \"element-type\": \"BlockStats\", \"meta-type\": \"array\",
> \"name\": \"BlockStatsList\" }, "
>
>     "{ \"members\": [{ \"name\": \"values\", \"type\": \"strList\" } ],
> \"meta-type\": \"object\", \"name\": \"SchemaInfoEnum\" }, "
>
>     "{ \"arg-type\": \":obj-query-blockstats-arg\", \"meta-type\":
> \"command\", \"name\": \"query-blockstats\", \"ret-type\":
> \"BlockStatsList\" }, "
>
>
> and particularly what happens to it after patch 32 is applied:
>
>     "{ \"element-type\": \"str\", \"meta-type\": \"array\", \"name\":
> \"270\" }, "
>
>     "{ \"element-type\": \"179\", \"meta-type\": \"array\", \"name\":
> \"96\" }, "
>
>     "{ \"members\": [{ \"name\": \"values\", \"type\": \"270\" } ],
> \"meta-type\": \"object\", \"name\": \"273\" }, "
>
>     "{ \"arg-type\": \"95\", \"meta-type\": \"command\", \"name\":
> \"query-blockstats\", \"ret-type\": \"96\" }, "
>
> In order to learn the return type of query-blockstats (to see if a new
> member was added to the struct), I now have to query what type 96 is,
> see that it is an array, and then query what members type 179 has.  And
> when dealing with type 273, I don't know whether to expect an array or
> an object for the "type" member until I look up type 270 (but I _do_
> know not to expect an int, number, boolean, or null).

To check for keys returned by query-blockstats:

1. Look up command query-blockstats.

2. Look up its ret-type, it must be array (or else we screwed up).

3. Look up the array's element-type, it must be object (or else we
   screwed up).

4. Examine the element-type's member's names.

> What if we instead munged the name of array types in introspection
> output to be "[int]" or "[BlockStats]"; or, with type masking in place,
> "[int]" or "[179]"?  As in the following for the above examples:
>
>  "{ \"element-type\": \"str\", \"meta-type\": \"array\", \"name\":
> \"[str]\" }, "
>
> "{ \"element-type\": \"179\", \"meta-type\": \"array\", \"name\":
> \"[179]\" }, "
>
> "{ \"members\": [{ \"name\": \"values\", \"type\": \"[str]\" } ],
> \"meta-type\": \"object\", \"name\": \"273\" }, "
>
> "{ \"arg-type\": \"95\", \"meta-type\": \"command\", \"name\":
> \"query-blockstats\", \"ret-type\": \"[179]\" }, "
>
> This way, libvirt or other clients could take shortcuts: I know that the
> return of query-blockstats will be an array, and I only have to look up
> the single type 179 to learn what members will be in the struct of that
> array; likewise, when inspecting struct 273, I know that "type" is an
> array of builtin strings and not an array of dicts, without needing to
> do another type lookup.

To check for keys returned by query-blockstats:

1. Look up command query-blockstats.

2'. Its ret-type must be of the form '[element-type]' (or else we screwed
   up).  Extract element-type.

3. Look up the array's element-type, it must be object (or else we
   screwed up).

4. Examine the element-type's member's names.

Trades a type lookup (code you already have) for a special case
(additional code).  I wouldn't do it.

> I've always mentioned that I'm not a fan of packing multiple pieces of
> information into a single JSON member (often a sign that the schema is
> not specific enough), but in this particular case, the names would still
> resolve without paying attention to the contents of the name.  That is,
> we'd be providing extra information (whether a name starts with leading
> '[') that clients can optionally use to be more efficient, but which
> will not penalize clients that stick with the tried-and-true approach of
> finding the array member with the matching name (that is, if I chase
> "[179]", I will still learn that I need to look up "179").

Aha, you're *not* proposing to omit the array type definitions.  Good,
because I'd object to that :)  I intentionally made all types explicit
to keep things as simple as possible.

You're proposing a more innocent change: a special naming convention for
array types.

>                                                             And it gives
> us the nice property that the first character of a type specifies its
> JSON properties (letter=>builtin, [=>array, digit=>object), other than
> the magic "any".

Yes, but do we want to make that first letter convention ABI?

If not, then clients can't rely on it.

If we use it, we risk clients relying on it anyway, making it de facto
ABI.

> I still think we need to munge to '*List' internally (as in our
> qapi_free_*() functions, for example), but what I'm trying to propose is
> that we don't expose the '*List' naming convention through
> introspection.  We'd also have to consider what this means for the
> technical possibility of any 2D array types.

After this patch, the only names visible in query-schema's result are:

* Built-in type names

* Command and event names

* Object member names

* Enumeration constants