Re: [Qemu-devel] [PATCH v4 13/19] qapi: More rigourous checking of types

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> Now that we know every expression is valid with regards to
> its keys, we can add further tests that those keys refer to
> valid types.  With this patch, all references to a type (the
> 'data': of command, type, union, and event, and the 'returns':
> of command) must resolve to a builtin or another type declared
> by the current qapi parse; this includes recursing into each
> member of a data dictionary.  Dealing with '**' and nested
> sub-structs will be done in later patches.
>
> Update the testsuite to match improved output.
>
> Signed-off-by: Eric Blake <address@hidden>
> ---
>  scripts/qapi.py                              | 73 
> ++++++++++++++++++++++++++--
>  tests/qapi-schema/data-array-empty.err       |  1 +
>  tests/qapi-schema/data-array-empty.exit      |  2 +-
>  tests/qapi-schema/data-array-empty.json      |  2 +-
>  tests/qapi-schema/data-array-empty.out       |  3 --
>  tests/qapi-schema/data-array-unknown.err     |  1 +
>  tests/qapi-schema/data-array-unknown.exit    |  2 +-
>  tests/qapi-schema/data-array-unknown.json    |  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.json              |  2 +-
>  tests/qapi-schema/data-int.out               |  3 --
>  tests/qapi-schema/data-member-array-bad.err  |  1 +
>  tests/qapi-schema/data-member-array-bad.exit |  2 +-
>  tests/qapi-schema/data-member-array-bad.json |  2 +-
>  tests/qapi-schema/data-member-array-bad.out  |  3 --
>  tests/qapi-schema/data-member-unknown.err    |  1 +
>  tests/qapi-schema/data-member-unknown.exit   |  2 +-
>  tests/qapi-schema/data-member-unknown.json   |  2 +-
>  tests/qapi-schema/data-member-unknown.out    |  3 --
>  tests/qapi-schema/data-unknown.err           |  1 +
>  tests/qapi-schema/data-unknown.exit          |  2 +-
>  tests/qapi-schema/data-unknown.json          |  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.json     |  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.json       |  2 +-
>  tests/qapi-schema/returns-unknown.out        |  3 --
>  33 files changed, 93 insertions(+), 44 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index bf243fa..20c0ce9 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -255,6 +255,52 @@ def discriminator_find_enum_define(expr):
>
>      return find_enum(discriminator_type)
>
> +def check_type(expr_info, source, data, allow_array = False,
> +               allowed_names = [], allow_dict = True):
> +    global all_types
> +
> +    if data is None:
> +        return
> +
> +    if data == '**':
> +        return
> +
> +    # Check if array type for data is okay
> +    if isinstance(data, list):
> +        if not allow_array:
> +            raise QAPIExprError(expr_info,
> +                                "%s cannot be an array" % source)
> +        if len(data) != 1 or not isinstance(data[0], basestring):
> +            raise QAPIExprError(expr_info,
> +                                "%s: array type must contain single type 
> name"
> +                                % source)
> +        data = data[0]
> +
> +    # Check if type name for data is okay
> +    if isinstance(data, basestring):
> +        if not data in all_types:
> +            raise QAPIExprError(expr_info,
> +                                "%s references unknown type '%s'"
> +                                % (source, data))
> +        if not all_types[data] in allowed_names:
> +            raise QAPIExprError(expr_info,
> +                                "%s cannot reference %s type '%s'"
> +                                % (source, all_types[data], data))
> +        return
> +
> +    # data is a dictionary, check that each member is okay
> +    if not isinstance(data, OrderedDict):
> +        raise QAPIExprError(expr_info,
> +                            "%s should be a dictionary" % source)
> +    if not allow_dict:
> +        raise QAPIExprError(expr_info,
> +                            "%s should be a type name" % source)
> +    for (key, value) in data.items():
> +        check_type(expr_info, "member '%s' of %s" % (key, source), value,
> +                   allow_array=True,
> +                   allowed_names=['built-in', 'union', 'struct', 'enum'],
> +                   allow_dict=True)

Hmm, allowed_names isn't about allowed names, it's about allowed
meta-types.  Rename?

> +
>  def check_command(expr, expr_info):
>      global commands
>      name = expr['command']
> @@ -262,6 +308,15 @@ def check_command(expr, expr_info):
>          raise QAPIExprError(expr_info,
>                              "command '%s' is already defined" % name)
>      commands.append(name)
> +    check_type(expr_info, "'data' for command '%s'" % name,
> +               expr.get('data'), allow_array=True,
> +               allowed_names=['union', 'struct'])
> +    check_type(expr_info, "'base' for command '%s'" % name,
> +               expr.get('base'), allowed_names=['struct'],
> +               allow_dict=False)
> +    check_type(expr_info, "'returns' for command '%s'" % name,
> +               expr.get('returns'), allow_array=True,
> +               allowed_names=['built-in', 'union', 'struct', 'enum'])
>

Nicely done.

>  def check_event(expr, expr_info):
>      global events
> @@ -271,6 +326,8 @@ def check_event(expr, expr_info):
>          raise QAPIExprError(expr_info,
>                              "event '%s' is already defined" % name)
>      events.append(name)
> +    check_type(expr_info, "'data' for event '%s'" % name,
> +               expr.get('data'), allowed_names=['union', 'struct'])
>      if params:
>          for argname, argentry, optional, structured in parse_args(params):
>              if structured:
> @@ -357,19 +414,27 @@ def check_enum(expr, expr_info):
>                                  % (name, member, values[key]))
>          values[key] = member
>
> +def check_struct(expr, expr_info):
> +    name = expr['type']
> +    members = expr['data']
> +
> +    check_type(expr_info, "'data' for type '%s'" % name, members)
> +
>  def check_exprs(schema):
>      for expr_elem in schema.exprs:
>          expr = expr_elem['expr']
>          info = expr_elem['info']
>
> -        if expr.has_key('union'):
> -            check_union(expr, info)
> -        if expr.has_key('event'):
> -            check_event(expr, info)
>          if expr.has_key('enum'):
>              check_enum(expr, info)
> +        if expr.has_key('union'):
> +            check_union(expr, info)
> +        if expr.has_key('type'):
> +            check_struct(expr, info)
>          if expr.has_key('command'):
>              check_command(expr, info)
> +        if expr.has_key('event'):
> +            check_event(expr, info)

Not related to this patch: this chain of ifs bothers me.  The conditions
should be exclusive, because each expression must have a well-defined
meta-type.  If I remember correctly, you actually enforce this elsewhere
in your series.  Do we want to make it obvious in the code here?  elif
instead of if, perhaps?

>
>  def check_keys(expr_elem, meta, required, optional=[]):
>      expr = expr_elem['expr']
> diff --git a/tests/qapi-schema/data-array-empty.err 
> b/tests/qapi-schema/data-array-empty.err
> index e69de29..94e046b 100644
> --- a/tests/qapi-schema/data-array-empty.err
> +++ b/tests/qapi-schema/data-array-empty.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/data-array-empty.json:2: 'data' for command 'oops': array 
> type must contain single type name
> diff --git a/tests/qapi-schema/data-array-empty.exit 
> b/tests/qapi-schema/data-array-empty.exit
> index 573541a..d00491f 100644
> --- a/tests/qapi-schema/data-array-empty.exit
> +++ b/tests/qapi-schema/data-array-empty.exit
> @@ -1 +1 @@
> -0
> +1
> diff --git a/tests/qapi-schema/data-array-empty.json 
> b/tests/qapi-schema/data-array-empty.json
> index 41b6c1e..fdd5612 100644
> --- a/tests/qapi-schema/data-array-empty.json
> +++ b/tests/qapi-schema/data-array-empty.json
> @@ -1,2 +1,2 @@
> -# FIXME: we should reject an array for data if it does not contain a known 
> type
> +# we reject an array for data if it does not contain a known type
>  { 'command': 'oops', 'data': [ ] }
> diff --git a/tests/qapi-schema/data-array-empty.out 
> b/tests/qapi-schema/data-array-empty.out
> index 67802be..e69de29 100644
> --- a/tests/qapi-schema/data-array-empty.out
> +++ b/tests/qapi-schema/data-array-empty.out
> @@ -1,3 +0,0 @@
> -[OrderedDict([('command', 'oops'), ('data', [])])]
> -[]
> -[]
> diff --git a/tests/qapi-schema/data-array-unknown.err 
> b/tests/qapi-schema/data-array-unknown.err
> index e69de29..a66c2d6 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:2: 'data' for command 'oops' 
> references unknown type 'NoSuchType'

The wording "references type" somehow unduly excites my "reference type"
synapses :)

Perhaps something like "Type 'NoSuchType' is unknown" would suffice.
Entirely up to you; feel free to keep the wording as is.

> 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.json 
> b/tests/qapi-schema/data-array-unknown.json
> index 434fb5f..9c75b96 100644
> --- a/tests/qapi-schema/data-array-unknown.json
> +++ b/tests/qapi-schema/data-array-unknown.json
> @@ -1,2 +1,2 @@
> -# FIXME: we should reject an array for data if it does not contain a known 
> type
> +# we reject an array for data if it does not contain a known type
>  { 'command': 'oops', 'data': [ 'NoSuchType' ] }
> 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..e1d3ed5 100644
> --- a/tests/qapi-schema/data-int.err
> +++ b/tests/qapi-schema/data-int.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/data-int.json:2: 'data' for command 'oops' cannot 
> reference built-in type 'int'

Perhaps something like "'data' for command 'oops' can't be of built-in
type 'int'", or maybe positive instead of negative: "a command's 'data'
must be of complex type".

Again, entirely up to you.

[...]