[v3,09/14] qapi: add check_type helper function

Add a helper function for use in a later patch that will
validate that a 'data':... or 'returns':... member of an
expression evaluates to a valid type.

* scripts/qapi.py (check_type): New function.
(check_exprs): Use it.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 scripts/qapi.py                           | 30 ++++++++++++++++++++++++++++++
 tests/qapi-schema/data-array-unknown.err  |  1 +
 tests/qapi-schema/data-array-unknown.exit |  2 +-
 tests/qapi-schema/data-array-unknown.out  |  3 ---
 tests/qapi-schema/data-int.err            |  1 +
 tests/qapi-schema/data-int.exit           |  2 +-
 tests/qapi-schema/data-int.out            |  3 ---
 tests/qapi-schema/data-unknown.err        |  1 +
 tests/qapi-schema/data-unknown.exit       |  2 +-
 tests/qapi-schema/data-unknown.out        |  3 ---
 tests/qapi-schema/returns-array-bad.err   |  1 +
 tests/qapi-schema/returns-array-bad.exit  |  2 +-
 tests/qapi-schema/returns-array-bad.out   |  3 ---
 tests/qapi-schema/returns-unknown.err     |  1 +
 tests/qapi-schema/returns-unknown.exit    |  2 +-
 tests/qapi-schema/returns-unknown.out     |  3 ---
 16 files changed, 40 insertions(+), 20 deletions(-)

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.

While I'm nit-picking your subjects: start with a capital letter after
the subsystem: prefix.

Eric Blake <eblake@redhat.com> writes:

> Add a helper function for use in a later patch that will
> validate that a 'data':... or 'returns':... member of an
> expression evaluates to a valid type.
>
> * scripts/qapi.py (check_type): New function.
> (check_exprs): Use it.
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  scripts/qapi.py                           | 30 ++++++++++++++++++++++++++++++
>  tests/qapi-schema/data-array-unknown.err  |  1 +
>  tests/qapi-schema/data-array-unknown.exit |  2 +-
>  tests/qapi-schema/data-array-unknown.out  |  3 ---
>  tests/qapi-schema/data-int.err            |  1 +
>  tests/qapi-schema/data-int.exit           |  2 +-
>  tests/qapi-schema/data-int.out            |  3 ---
>  tests/qapi-schema/data-unknown.err        |  1 +
>  tests/qapi-schema/data-unknown.exit       |  2 +-
>  tests/qapi-schema/data-unknown.out        |  3 ---
>  tests/qapi-schema/returns-array-bad.err   |  1 +
>  tests/qapi-schema/returns-array-bad.exit  |  2 +-
>  tests/qapi-schema/returns-array-bad.out   |  3 ---
>  tests/qapi-schema/returns-unknown.err     |  1 +
>  tests/qapi-schema/returns-unknown.exit    |  2 +-
>  tests/qapi-schema/returns-unknown.out     |  3 ---
>  16 files changed, 40 insertions(+), 20 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index e02fa0b..e4d27d6 100644
> --- a/scripts/qapi.py
> +++ 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[]?

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

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

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

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

> diff --git a/tests/qapi-schema/data-array-unknown.err b/tests/qapi-schema/data-array-unknown.err
> index e69de29..e0433b8 100644
> --- a/tests/qapi-schema/data-array-unknown.err
> +++ b/tests/qapi-schema/data-array-unknown.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/data-array-unknown.json:1: Unknown type 'NoSuchType' as data of 'oops'
> diff --git a/tests/qapi-schema/data-array-unknown.exit b/tests/qapi-schema/data-array-unknown.exit
> index 573541a..d00491f 100644
> --- a/tests/qapi-schema/data-array-unknown.exit
> +++ b/tests/qapi-schema/data-array-unknown.exit
> @@ -1 +1 @@
> -0
> +1
> diff --git a/tests/qapi-schema/data-array-unknown.out b/tests/qapi-schema/data-array-unknown.out
> index c3bc05e..e69de29 100644
> --- a/tests/qapi-schema/data-array-unknown.out
> +++ b/tests/qapi-schema/data-array-unknown.out
> @@ -1,3 +0,0 @@
> -[OrderedDict([('command', 'oops'), ('data', ['NoSuchType'])])]
> -[]
> -[]
> diff --git a/tests/qapi-schema/data-int.err b/tests/qapi-schema/data-int.err
> index e69de29..30bbffc 100644
> --- a/tests/qapi-schema/data-int.err
> +++ b/tests/qapi-schema/data-int.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/data-int.json:1: Primitive type 'int' not allowed as data of 'oops'
> diff --git a/tests/qapi-schema/data-int.exit b/tests/qapi-schema/data-int.exit
> index 573541a..d00491f 100644
> --- a/tests/qapi-schema/data-int.exit
> +++ b/tests/qapi-schema/data-int.exit
> @@ -1 +1 @@
> -0
> +1
> diff --git a/tests/qapi-schema/data-int.out b/tests/qapi-schema/data-int.out
> index e589a4f..e69de29 100644
> --- a/tests/qapi-schema/data-int.out
> +++ b/tests/qapi-schema/data-int.out
> @@ -1,3 +0,0 @@
> -[OrderedDict([('command', 'oops'), ('data', 'int')])]
> -[]
> -[]
> diff --git a/tests/qapi-schema/data-unknown.err b/tests/qapi-schema/data-unknown.err
> index e69de29..f7fd635 100644
> --- a/tests/qapi-schema/data-unknown.err
> +++ b/tests/qapi-schema/data-unknown.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/data-unknown.json:1: Unknown type 'NoSuchType' as data of 'oops'
> diff --git a/tests/qapi-schema/data-unknown.exit b/tests/qapi-schema/data-unknown.exit
> index 573541a..d00491f 100644
> --- a/tests/qapi-schema/data-unknown.exit
> +++ b/tests/qapi-schema/data-unknown.exit
> @@ -1 +1 @@
> -0
> +1
> diff --git a/tests/qapi-schema/data-unknown.out b/tests/qapi-schema/data-unknown.out
> index 2c60726..e69de29 100644
> --- a/tests/qapi-schema/data-unknown.out
> +++ b/tests/qapi-schema/data-unknown.out
> @@ -1,3 +0,0 @@
> -[OrderedDict([('command', 'oops'), ('data', 'NoSuchType')])]
> -[]
> -[]
> diff --git a/tests/qapi-schema/returns-array-bad.err b/tests/qapi-schema/returns-array-bad.err
> index e69de29..d2a216f 100644
> --- a/tests/qapi-schema/returns-array-bad.err
> +++ b/tests/qapi-schema/returns-array-bad.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/returns-array-bad.json:1: Use of array type in 'oops' must contain single element type name
> diff --git a/tests/qapi-schema/returns-array-bad.exit b/tests/qapi-schema/returns-array-bad.exit
> index 573541a..d00491f 100644
> --- a/tests/qapi-schema/returns-array-bad.exit
> +++ b/tests/qapi-schema/returns-array-bad.exit
> @@ -1 +1 @@
> -0
> +1
> diff --git a/tests/qapi-schema/returns-array-bad.out b/tests/qapi-schema/returns-array-bad.out
> index cfc474e..e69de29 100644
> --- a/tests/qapi-schema/returns-array-bad.out
> +++ b/tests/qapi-schema/returns-array-bad.out
> @@ -1,3 +0,0 @@
> -[OrderedDict([('command', 'oops'), ('data', ['str', 'str'])])]
> -[]
> -[]
> diff --git a/tests/qapi-schema/returns-unknown.err b/tests/qapi-schema/returns-unknown.err
> index e69de29..d09fb92 100644
> --- a/tests/qapi-schema/returns-unknown.err
> +++ b/tests/qapi-schema/returns-unknown.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/returns-unknown.json:1: Unknown type 'NoSuchType' as data of 'oops'
> diff --git a/tests/qapi-schema/returns-unknown.exit b/tests/qapi-schema/returns-unknown.exit
> index 573541a..d00491f 100644
> --- a/tests/qapi-schema/returns-unknown.exit
> +++ b/tests/qapi-schema/returns-unknown.exit
> @@ -1 +1 @@
> -0
> +1
> diff --git a/tests/qapi-schema/returns-unknown.out b/tests/qapi-schema/returns-unknown.out
> index a482c83..e69de29 100644
> --- a/tests/qapi-schema/returns-unknown.out
> +++ b/tests/qapi-schema/returns-unknown.out
> @@ -1,3 +0,0 @@
> -[OrderedDict([('command', 'oops'), ('returns', 'NoSuchType')])]
> -[]
> -[]

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> 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!