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