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 <ebl...@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
signature.asc
Description: OpenPGP digital signature
