Eric Blake <ebl...@redhat.com> 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 <ebl...@redhat.com>
> ---
[...]
> 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 <arm...@redhat.com>
