Re: [Qemu-devel] [PATCH RFC v2 26/47] qapi-types: Convert to QAPISchemaVisitor, fixing flat unions

[Eric Blake <eblake@redhat.com>]

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

On 07/30/2015 12:42 AM, Markus Armbruster wrote:

>> But what happens if the 0th branch is mapped to a different parser, as
>> would be the case if one of the alternate's branches is 'number'?  In
>> particular, qmp_input_type_number() accepts BOTH QFloat and QInt types.
>>  So, if we have this qapi:
>>  { 'alternate': 'Foo', 'data': { 'a': 'str', 'b': 'number' } }
>> but pass in an integer, visit_get_next_type() will see a qtype of QInt,
>> but Foo_qtypes[QTYPE_QINT] will be 0 (due to default initialization) and
>> we will wrongly try to visit the 0th branch (FOO_KIND_A) and fail (the
>> string parser doesn't like ints) even though the parse should succeed by
>> using the FOO_KIND_B branch.
> 
> Yup, bug.

And it's an order-dependent bug - merely declaring 'b' first makes it
appear to work correctly.

> 
>> Interestingly, this means that if we ever write an alternate type that
>> accepts both 'int' and 'number' (we have not attempted that so far),
>> then the number branch will only be taken for inputs that don't also
>> look like ints (normally, 'number' accepts anything numeric). Maybe that
>> means we should document and enforce that 'number' and 'int' cannot be
>> mixed in the same alternate?
> 
> Even if we outlaw mixing the two, I'm afraid we still have this bug: an
> alternate with a 'number' member rejects input that gets parsed as
> QTYPE_QINT.
> 
> Let's simply make alternates behave sanely:
> 
>     alternate has      case selected for
>     'int'  'number'    QTYPE_QINT  QTYPE_QFLOAT
>       no        no     error       error
>       no       yes     'number'    'number'
>      yes        no     'int'       error
>      yes       yes     'int'       'number'

Works for me.


> 
> + 1 works, because the element type is int, not BlockdevRefKind.  It's
> int so it can serve as argument for visit_get_next_type()'s parameter
> const int *qtypes.
> 
> The + 1, - 1 business could be mildly confusing.  We could set all
> unused elements to -1 instead:

Or, we could ditch the qtypes lookup altogether, and merely create the
alternate enum as a non-consecutive QTYPE mapping, for one less level of
indirection, as in:

typedef enum BlockdevRefKind {
    BLOCKDEV_REF_DEFINITION = QTYPE_QOBJECT,
    BLOCKDEV_REF_REFERENCE = QTYPE_QSTRING,
};

then rewrite visit_get_next_type() to directly return the qtype, as well
as rewrite the generated switch statement in visit_type_BlockdevRef() to
directly inspect the qtypes it cares about.  In fact, that's the
approach I'm currently playing with.

> Add test eight test cases from my table above, then fix the generator to
> make them pass.

I hope to post an RFC followup patch along those lines later today.

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