Eric Blake <ebl...@redhat.com> 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!
