Re: [Qemu-devel] [PATCH v3 09/14] qapi: add check_type helper function

[Eric Blake]

On 08/14/2014 04:10 AM, Markus Armbruster wrote:
> Subject suggests you're merely adding a helper function.  You're
> actually fixing bugs.  Something like
> 
>     qapi: More rigorous checking of type expressions
> 
> would be clearer.

Along with squashing in 8/14, you are correct about this fixing bugs
(serves me right for refactoring, then realizing that we needed more
tests, then fixing the tests without checking that the subject line
stayed appropriate).

>> +++ b/scripts/qapi.py
>> @@ -329,6 +329,32 @@ def check_union(expr, expr_info):
>>          # Todo: add checking for values. Key is checked as above, value can 
>> be
>>          # also checked here, but we need more functions to handle array 
>> case.
>>
>> +def check_type(expr_info, name, data, allow_native = False):
>> +    if not data:
>> +        return
>> +    if isinstance(data, list):
>> +        if len(data) != 1 or not isinstance(data[0], basestring):
>> +            raise QAPIExprError(expr_info,
>> +                                "Use of array type in '%s' must contain "
>> +                                "single element type name" % name)
>> +        data = data[0]
> 
> Peeling off the array here means error messages below won't mention it.
> Visible in data-array-unknown.err below.  But good enough is good
> enough.
> 
>> +
>> +    if isinstance(data, basestring):
>> +        if find_struct(data) or find_enum(data) or find_union(data):
>> +            return
>> +        if data == 'str' or data == 'int' or data == 'visitor':
> 
> Is this complete?  Should we be checking against builtin_types[]?

It was complete insofar that it passes with the current
qapi-schema.json.  Checking builtin_types[] would indeed be nicer (I
didn't know that existed).

> 
> Pardon my ignorance: where does 'visitor' come from?

qom-set, qom-get.  Nasty commands that aren't typesafe, and which
explicitly use 'gen':'no' to abuse the qapi system by bypassing the code
generator while still exposing a backdoor command that can break the rules.

netdev_add ('*props':'**') and object-add ('*props':'dict') are the only
other 'gen':'no' clients; I'm not sure why they didn't cause the parser
to barf the way 'visitor' did.  Maybe my approach should instead be to
unify all four 'gen':'no' expressions to use the same syntax of "**" to
represent the fact that the QMP code is not type-safe, as well as
actually documenting this (ab)use of ** (the fact that none of the
documentation mentions 'gen' is telling).  Looks like I've just added a
pre-req patch into my v4 :)

> 
>> +            if allow_native:
>> +                return
>> +            raise QAPIExprError(expr_info,
>> +                                "Primitive type '%s' not allowed as data "
>> +                                "of '%s'" % (data, name))
>> +        raise QAPIExprError(expr_info,
>> +                            "Unknown type '%s' as data of '%s'"
>> +                            % (data, name))
> 
> "as data of" suggests the problem is in member 'data', even when it's
> actually in member 'returns'.  Visible in returns-unknown.err below.

The function is declared as:

>> +def check_type(expr_info, name, data, allow_native = False):

but allow_native is True only for the 'returns' case.  Maybe I should
just repurpose that parameter for what it really is - 'data' vs.
'returns', and use it for improving the error message in addition to
deciding whether native types are allowed.

> 
>> +    elif not isinstance(data, OrderedDict):
>> +        raise QAPIExprError(expr_info,
>> +                            "Expecting dictionary for data of '%s'" % name)
>> +
>>  def check_exprs(schema):
>>      for expr_elem in schema.exprs:
>>          expr = expr_elem['expr']
>> @@ -356,6 +382,10 @@ def check_exprs(schema):
>>                  raise QAPIExprError(info,
>>                                      "enum '%s' requires an array for 'data'"
>>                                      % expr.get('enum'))
>> +        else:
> 
> This is for 'type' and 'command', right?

'type', 'union', 'event', and 'command' ('data' is a dict for those
four, and an array for the fifth meta-type of 'enum').  I'll add a comment.

> 
>> +            check_type(info, expr_name(expr), members)
>> +            if expr.has_key('command') and expr.has_key('returns'):
>> +                check_type(info, expr['command'], expr['returns'], True)
>>
>>  def parse_schema(input_file):
>>      try:
> 
> Looks pretty good, but I didn't check systematically against
> qapi-code-gen.txt for correctness and completeness.  We can always
> improve on top.

I'll double-check things as well, when posting v4.

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

signature.asc
Description: OpenPGP digital signature