Re: [Qemu-devel] [PATCH v5 20/28] qapi: More rigourous checking of types

[Eric Blake]

On 03/27/2015 02:23 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 uses of a type (the 'data':
>> of command, type, union, alternate, and event; the 'returns':
>> of command; the 'base': of type and union) must resolve to an
>> appropriate subset of metatypes  declared by the current qapi
>> parse; this includes recursing into each member of a data
>> dictionary.  Dealing with '**' and nested anonymous structs
>> will be done in later patches.
>>
>> Update the testsuite to match improved output.
>>
>> Signed-off-by: Eric Blake <address@hidden>
>> ---
> [...]
>> diff --git a/scripts/qapi.py b/scripts/qapi.py
>> index 6ed6a34..c42683b 100644
>> --- a/scripts/qapi.py
>> +++ b/scripts/qapi.py
>> @@ -276,6 +276,63 @@ def discriminator_find_enum_define(expr):
>>
>>      return find_enum(discriminator_type)
>>
>> +def check_type(expr_info, source, data, allow_array = False,
>> +               allowed_metas = [], allow_dict = True):
> 
> I'd allow_dict = False here, because I like defaulting to false.  Matter
> of taste.

Not too hard.

> 
> I'd name the third parameter def rather than data.  Matter of taste
> again.
> 

Oh, definitely a better name. When I first prototyped the function, I
was only crawling the 'data' member, but since I now also use it to
crawl 'returns' and 'base' it does make a difference.


>> +    for (key, value) in data.items():
>> +        check_type(expr_info, "Member '%s' of %s" % (key, source), value,
> 
> This can produce messages like
> 
>     Member 'inner' of Member 'outer' of ...
> 
> I figure the problem will go away when you ditch nested structs.  Not
> worth worrying about it then.

Yep, it disappears quite nicely.  (I was torn on whether to make the
message start with lower-case, to avoid the mid-sentence Cap, but the
prevailing practice in this file before my patches was to start all
QAPIExprError with a capital)


>>
>> +def check_struct(expr, expr_info):
>> +    name = expr['type']
>> +    members = expr['data']
>> +
>> +    check_type(expr_info, "'data' for type '%s'" % name, members)
> 
> This one gave me pause, until I realized that allowed_metas=[],
> allow_dict=True accepts exactly dictionary members, which is what we
> want.

And your idea of having allow_dict default to False in the prototype to
make it explicit here might make it easier to understand (where the
default allows nothing, and then callers add the pieces that they want).
 Indeed, just typing this email, that sounds nicer than the current case
of defaults cater to the majority, but force some callers to have to
explicitly opt out.

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

signature.asc
Description: OpenPGP digital signature