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

[Eric Blake]

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
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"}}

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.

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?

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); and that we don't properly
handle QInt for an alternate that accepts 'number' but not 'int'.

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

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

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature