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/01/2015 02:22 PM, Markus Armbruster wrote:
>> Fixes flat unions to get the base's base members.  Test case is from
>> commit 2fc0043, in qapi-schema-test.json:
>> 
>
>> -def generate_alternate_qtypes(expr):
>> +def gen_alternate_qtypes_decl(name):
>> +    return mcgen('''
>>  
>> -    name = expr['alternate']
>> -    members = expr['data']
>> +extern const int %(c_name)s_qtypes[];
>> +''',
>> +                 c_name=c_name(name))
>>  
>> +def gen_alternate_qtypes(name, variants):
>>      ret = mcgen('''
>>  
>>  const int %(name)s_qtypes[QTYPE_MAX] = {
>>  ''',
>>                  name=c_name(name))
>>  
>> -    for key in members:
>> -        qtype = find_alternate_member_qtype(members[key])
>> -        assert qtype, "Invalid alternate member"
>> +    for var in variants.variants:
>> +        qtype = var.type.alternate_qtype()
>> +        assert qtype
>
> I think I found a couple more corner case bugs here. We are using C99
> initialization of the array; for example:
>
> const int BlockdevRef_qtypes[QTYPE_MAX] = {
>     [QTYPE_QDICT] = BLOCKDEV_REF_KIND_DEFINITION,
>     [QTYPE_QSTRING] = BLOCKDEV_REF_KIND_REFERENCE,
> };
>
> but paired with an enum that starts at 0:
>
> typedef enum BlockdevRefKind {
>     BLOCKDEV_REF_KIND_DEFINITION = 0,
>     BLOCKDEV_REF_KIND_REFERENCE = 1,
>     BLOCKDEV_REF_KIND_MAX = 2,
> } BlockdevRefKind;
>
>
> and that means that every QTYPE_ constant that we don't specify in
> _qtypes[] is also assigned the value 0 (aka BLOCKDEV_REF_KIND_DEFINITION

I see where this is going...

> in this example).  In operation, calling something like:
>
> {"execute":"blockdev-add","arguments":{"options":
>  {"driver":"raw","id":"a","file":true}}}
>
> which is invalid per the .json description ("file" must be string or
> object, not boolean), still manages to get past visit_get_next_type()
> with success, and fall through to the 0th branch of the switch.  If that
> 0th branch happens to be a struct (as it is for BlockdevRef), then we
> fortunately catch the error on the very next parse call, where
> qmp_input_start_struct() complains:
>
> {"error": {"class": "GenericError", "desc": "Invalid parameter type for
> 'file', expected: QDict"}}

A lucky case.

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

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

> So, the bugs are: visit_get_next_type() can't tell the difference
> between a *_qtypes[] lookup that was explicitly initialized to 0 from
> one that was accidentally left that way, and therefore can't report
> failure for an unexpected type (but mostly mitigated by the fact that
> always returning 0 means the parser will attempt to parse the first
> branch of the alternate and gracefully fail);

Yes.

>                                               and that we don't properly
> handle QInt for an alternate that accepts 'number' but not 'int'.

Yes.

> I don't think either bug has to be fixed in your series, although you
> may want to add tests.

Agree.

> The first bug could be resolved by guaranteeing that the _qtypes[] array
> has non-zero values for the explicitly initialized lookups, and teaching
> visit_get_next_type() that a lookup that produces 0 meant that an
> unexpected type was encountered.  Perhaps by changing the creation of
> _qtypes[] in qapi-types.c to list:
>
> const int BlockdevRef_qtypes[QTYPE_MAX] = {
>   [QTYPE_QDICT] = BLOCKDEV_REF_KIND_DEFINITION + 1,
>   [QTYPE_QSTRING] = BLOCKDEV_REF_KIND_REFERENCE + 1,
> };
>
> and then having visit_get_next_type() subtract one after verifying a
> non-zero value was looked up.

+ 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:

    const int BlockdevRef_qtypes[QTYPE_MAX] = {
        [QTYPE_NONE] = -1,
        [QTYPE_QNULL] = -1,
        [QTYPE_QINT] = -1,
        [QTYPE_QSTRING] = BLOCKDEV_REF_KIND_REFERENCE + 1,
        [QTYPE_QDICT] = BLOCKDEV_REF_KIND_DEFINITION + 1,
        [QTYPE_QLIST] = -1,
        [QTYPE_QFLOAT] = -1,
        [QTYPE_QBOOL] = -1,
    };

I wouldn't recommend that in hand-written code, but generating it should
be fine.

>                                Or perhaps leave _qtypes alone, and
> instead change the alternate enum to have a placeholder at 0:
>
> typedef enum BlockdevRefKind {
>     BLOCKDEV_REF_KIND_INVALID = 0,
>     BLOCKDEV_REF_KIND_DEFINITION = 1,
>     BLOCKDEV_REF_KIND_REFERENCE = 2,
>     BLOCKDEV_REF_KIND_MAX = 3,
> } BlockdevRefKind;
>
> and then teaching the generator for visit_type_BlockdevRef() to emit an
> error if branch 0 is hit.

BLOCKDEV_REF_KIND_INVALID could get in the way elsewhere, e.g. with
-Wswitch.

> Fixing the second bug probably entails teaching the generator that if an
> alternate contains 'number' but not 'int', then we need [QTYPE_QINT] to
> map to the same lookup value as [QTYPE_QNUMBER].

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