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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> 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 :)

Documentation for 'gen': 'no', '**', and 'visitor' is most welcome.

I wonder why 'no', and not simply false.  Hmm, looks like the code
generator ignores the value.  'gen': 'sure, why not, if you feel like
it'.

Should 'visitor' be in builtin_types[]?

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

Yeah.

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

You're right.  I misread the sequence of if statements as one big
if... elif... elif... else statement.

A blank line before the if... else would probably do.

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

Thanks!