[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v4 13/19] qapi: More rigourous checking of types
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v4 13/19] qapi: More rigourous checking of types |
Date: |
Mon, 29 Sep 2014 08:35:29 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.1.1 |
On 09/26/2014 03:26 AM, Markus Armbruster wrote:
> Eric Blake <address@hidden> writes:
>
>> Now that we know every expression is valid with regards to
>> its keys, we can add further tests that those keys refer to
>> valid types. With this patch, all references to a type (the
>> 'data': of command, type, union, and event, and the 'returns':
>> of command) must resolve to a builtin or another type declared
>> by the current qapi parse; this includes recursing into each
>> member of a data dictionary. Dealing with '**' and nested
>> sub-structs will be done in later patches.
>>
>> + for (key, value) in data.items():
>> + check_type(expr_info, "member '%s' of %s" % (key, source), value,
>> + allow_array=True,
>> + allowed_names=['built-in', 'union', 'struct', 'enum'],
>> + allow_dict=True)
>
> Hmm, allowed_names isn't about allowed names, it's about allowed
> meta-types. Rename?
Will do.
>> def check_exprs(schema):
>> for expr_elem in schema.exprs:
>> expr = expr_elem['expr']
>> info = expr_elem['info']
>>
>> - if expr.has_key('union'):
>> - check_union(expr, info)
>> - if expr.has_key('event'):
>> - check_event(expr, info)
>> if expr.has_key('enum'):
>> check_enum(expr, info)
>> + if expr.has_key('union'):
>> + check_union(expr, info)
>> + if expr.has_key('type'):
>> + check_struct(expr, info)
>> if expr.has_key('command'):
>> check_command(expr, info)
>> + if expr.has_key('event'):
>> + check_event(expr, info)
>
> Not related to this patch: this chain of ifs bothers me. The conditions
> should be exclusive, because each expression must have a well-defined
> meta-type. If I remember correctly, you actually enforce this elsewhere
> in your series. Do we want to make it obvious in the code here? elif
> instead of if, perhaps?
Yes, earlier in the series guarantees that by this point, the conditions
are now exclusive. It also bothers me that different points in the file
are iterating over the 5 key types in different order, I'll clean that
up in v5.
>> +++ b/tests/qapi-schema/data-array-unknown.err
>> @@ -0,0 +1 @@
>> +tests/qapi-schema/data-array-unknown.json:2: 'data' for command 'oops'
>> references unknown type 'NoSuchType'
>
> The wording "references type" somehow unduly excites my "reference type"
> synapses :)
>
> Perhaps something like "Type 'NoSuchType' is unknown" would suffice.
> Entirely up to you; feel free to keep the wording as is.
I'll play with it.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
- Re: [Qemu-devel] [PATCH v4 08/19] qapi: Better error messages for bad expressions, (continued)
[Qemu-devel] [PATCH v4 09/19] qapi: Add tests of redefined expressions, Eric Blake, 2014/09/19
[Qemu-devel] [PATCH v4 11/19] qapi: Add tests of type bypass, Eric Blake, 2014/09/19
[Qemu-devel] [PATCH v4 13/19] qapi: More rigourous checking of types, Eric Blake, 2014/09/19
[Qemu-devel] [PATCH v4 07/19] qapi: Add some expr tests, Eric Blake, 2014/09/19
[Qemu-devel] [PATCH v4 12/19] qapi: Add some type check tests, Eric Blake, 2014/09/19
Re: [Qemu-devel] [PATCH v4 12/19] qapi: Add some type check tests, Eric Blake, 2014/09/25
Re: [Qemu-devel] [PATCH v4 12/19] qapi: Add some type check tests, Markus Armbruster, 2014/09/25
Re: [Qemu-devel] [PATCH v4 12/19] qapi: Add some type check tests, Eric Blake, 2014/09/25