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

[Markus Armbruster <armbru@redhat.com>]

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')])]
> -[]
> -[]