Re: [Qemu-devel] [PATCH v5 20/28] 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 uses of a type (the 'data':
> of command, type, union, alternate, and event; the 'returns':
> of command; the 'base': of type and union) must resolve to an
> appropriate subset of metatypes  declared by the current qapi
> parse; this includes recursing into each member of a data
> dictionary.  Dealing with '**' and nested anonymous structs
> will be done in later patches.
>
> Update the testsuite to match improved output.
>
> Signed-off-by: Eric Blake <address@hidden>
> ---
[...]
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 6ed6a34..c42683b 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -276,6 +276,63 @@ def discriminator_find_enum_define(expr):
>
>      return find_enum(discriminator_type)
>
> +def check_type(expr_info, source, data, allow_array = False,
> +               allowed_metas = [], allow_dict = True):

I'd allow_dict = False here, because I like defaulting to false.  Matter
of taste.

I'd name the third parameter def rather than data.  Matter of taste
again.

> +    global all_names
> +
> +    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], str):
> +            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, str):
> +        if not data in all_names:
> +            raise QAPIExprError(expr_info,
> +                                "%s uses unknown type '%s'"
> +                                % (source, data))
> +        if not all_names[data] in allowed_metas:
> +            raise QAPIExprError(expr_info,
> +                                "%s cannot use %s type '%s'"
> +                                % (source, all_names[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,

This can produce messages like

    Member 'inner' of Member 'outer' of ...

I figure the problem will go away when you ditch nested structs.  Not
worth worrying about it then.

> +                   allow_array=True,
> +                   allowed_metas=['built-in', 'union', 'alternate', 'struct',
> +                                  'enum'],
> +                   allow_dict=True)
> +
> +def check_command(expr, expr_info):
> +    name = expr['command']
> +    check_type(expr_info, "'data' for command '%s'" % name,
> +               expr.get('data'),
> +               allowed_metas=['union', 'struct'])
> +    check_type(expr_info, "'returns' for command '%s'" % name,
> +               expr.get('returns'), allow_array=True,
> +               allowed_metas=['built-in', 'union', 'alternate', 'struct',
> +                              'enum'])
> +
>  def check_event(expr, expr_info):
>      global events
>      name = expr['event']
> @@ -287,7 +344,8 @@ def check_event(expr, expr_info):
>          raise QAPIExprError(expr_info, "Event name '%s' should be upper case"
>                              % name)
>      events.append(name)
> -
> +    check_type(expr_info, "'data' for event '%s'" % name,
> +               expr.get('data'), allowed_metas=['union', 'struct'])
>      if params:
>          for argname, argentry, optional, structured in parse_args(params):
>              if structured:
> @@ -348,6 +406,13 @@ def check_union(expr, expr_info):
>
>      # Check every branch
>      for (key, value) in members.items():
> +        # Each value must name a known type
> +        check_type(expr_info, "Member '%s' of union '%s'" % (key, name),
> +                   value, allow_array=True,
> +                   allowed_metas=['built-in', 'union', 'alternate', 'struct',
> +                                  'enum'],
> +                   allow_dict=False)
> +
>          # If the discriminator names an enum type, then all members
>          # of 'data' must also be members of the enum type.
>          if enum_define:
> @@ -383,15 +448,12 @@ def check_alternate(expr, expr_info):
>          values[c_key] = key
>
>          # Ensure alternates have no type conflicts.
> -        if isinstance(value, list):
> -            raise QAPIExprError(expr_info,
> -                                "Alternate '%s' member '%s' must "
> -                                "not be array type" % (name, key))
> +        check_type(expr_info, "Member '%s' of alternate '%s'" % (key, name),
> +                   value,
> +                   allowed_metas=['built-in', 'union', 'struct', 'enum'],
> +                   allow_dict=False)
>          qtype = find_alternate_member_qtype(value)
> -        if not qtype:
> -            raise QAPIExprError(expr_info,
> -                                "Alternate '%s' member '%s' has "
> -                                "invalid type '%s'" % (name, key, value))
> +        assert qtype
>          if qtype in types_seen:
>              raise QAPIExprError(expr_info,
>                                  "Alternate '%s' member '%s' has "
> @@ -419,6 +481,14 @@ 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)

This one gave me pause, until I realized that allowed_metas=[],
allow_dict=True accepts exactly dictionary members, which is what we
want.

> +    check_type(expr_info, "'base' for type '%s'" % name, expr.get('base'),
> +               allowed_metas=['struct'], allow_dict=False)
> +
>  def check_exprs(schema):
>      for expr_elem in schema.exprs:
>          expr = expr_elem['expr']
> @@ -430,8 +500,14 @@ def check_exprs(schema):
>              check_union(expr, info)
>          elif expr.has_key('alternate'):
>              check_alternate(expr, info)
> +        elif expr.has_key('type'):
> +            check_struct(expr, info)
> +        elif expr.has_key('command'):
> +            check_command(expr, info)
>          elif expr.has_key('event'):
>              check_event(expr, info)
> +        else:
> +            assert False, 'unexpected meta type'
>
>  def check_keys(expr_elem, meta, required, optional=[]):
>      expr = expr_elem['expr']
[...]

Reviewed-by: Markus Armbruster <address@hidden>