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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

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

QTYPE_QDICT, but I get what you mean.

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

Hmm.  The schema currently doesn't let you control enumeration values.
qapi-code-gen.txt specifies:

    The enumeration values [...] are encoded as C enum integral values
    in generated code.  While the C code starts numbering at 0, it is
    better to use explicit comparisons to enum values than implicit
    comparisons to 0; the C code will also include a generated enum
    member ending in _MAX for tracking the size of the enum, useful when
    using common functions for converting between strings and enum
    values.

Strictly speaking, this needn't apply to implicit enums like
BlockdevRefKind.  But is it a good idea to make them different?

Hmm, your new BlockdevRefKind is basically a subset of qtype_code with
the members renamed.  Could we simply use qtype_code directly?

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