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

[Eric Blake <eblake@redhat.com>]

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

On 09/03/2015 08:03 PM, Eric Blake wrote:
> On 09/03/2015 08:30 AM, Markus Armbruster wrote:
>> qapi/introspect.json defines the introspection schema.  It's designed
>> for QMP introspection, but should do for similar uses, such as QGA.
> 
> [review in this sub-thread; for comments on 'int' munging or other
> followups, see other subthread]
> 
> There is at least one definite bug (see multiple references below to
> [1]), and several ideas for cleanups, but in general, I think that the
> remaining changes are going to be small enough that I'd probably be okay
> if v5 started life with:
> Reviewed-by: Eric Blake <eblake@redhat.com>
> (Of course, you have every right to not add the R-b, especially if you
> want me to do another close review :)
> 

> 
>> +
>> +    { "name": "BlockdevOptions", "meta-type": "object",
>> +      "members": [
>> +          { "name": "kind", "type": "BlockdevOptionsKind" } ],
> 
> [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.
> 

>> +    def _gen_variants(self, tag_name, variants):
>> +        return {'tag': tag_name or 'type',
> 
> [1] Ouch. Why are we still printing 'kind' as the name for simple
> unions?  Oh, it's because tag_name is ALWAYS provided by the visitor, so
> the "or 'type'" clause never fires.

Not quite the right comment.  'tag' is being output correctly (tag_name
was indeed None), what was wrong was the 'members':[] array (which was
assuming the member was named 'kind').

>   I have a pending patch to change
> the C code to generate 'type' as the C code name to match the wire ABI;
> but until that patch is in, I think a hack solution might be to fix the
> visitor to supply None instead of a tag_name for simple unions.  I'll
> respond to the appropriate earlier patch.

Rather, 3 earlier patches.  See my comments on 2, 10, and 11.

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