Re: [Qemu-devel] [PATCH v4 13/19] qapi: More rigourous checking of types

[Eric Blake]

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