Eric Blake <ebl...@redhat.com> writes:
> Previous commits demonstrated that the generator overlooked various
> bad naming situations:
> - types, commands, and events need a valid name
> - union and alternate branches cannot be marked optional
>
> The set of valid names includes [a-zA-Z0-9._-] (where '.' is
> useful only in downstream extensions).
>
> Signed-off-by: Eric Blake <ebl...@redhat.com>
> ---
> scripts/qapi.py | 57
> ++++++++++++++++------
> tests/qapi-schema/bad-ident.err | 1 +
> tests/qapi-schema/bad-ident.exit | 2 +-
> tests/qapi-schema/bad-ident.json | 2 +-
> tests/qapi-schema/bad-ident.out | 3 --
> tests/qapi-schema/flat-union-bad-discriminator.err | 2 +-
> .../flat-union-optional-discriminator.err | 1 +
> .../flat-union-optional-discriminator.exit | 2 +-
> .../flat-union-optional-discriminator.json | 2 +-
> .../flat-union-optional-discriminator.out | 5 --
> tests/qapi-schema/union-optional-branch.err | 1 +
> tests/qapi-schema/union-optional-branch.exit | 2 +-
> tests/qapi-schema/union-optional-branch.json | 2 +-
> tests/qapi-schema/union-optional-branch.out | 3 --
> 14 files changed, 53 insertions(+), 32 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index c42683b..ed5385a 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -15,6 +15,7 @@ import re
> from ordereddict import OrderedDict
> import os
> import sys
> +import string
>
> builtin_types = {
> 'str': 'QTYPE_QSTRING',
> @@ -276,8 +277,27 @@ def discriminator_find_enum_define(expr):
>
> return find_enum(discriminator_type)
>
> +valid_characters = set(string.ascii_letters + string.digits + '.' + '-' +
> '_')
strings.ascii_letters depends on the locale...
> +def check_name(expr_info, source, name, allow_optional = False):
> + membername = name
> +
> + if not isinstance(name, str):
> + raise QAPIExprError(expr_info,
> + "%s requires a string name" % source)
> + if name == '**':
> + return
Doesn't this permit '**' anywhere, not just as pseudo-type in command
arguments and results?
> + if name.startswith('*'):
> + membername = name[1:]
> + if not allow_optional:
> + raise QAPIExprError(expr_info,
> + "%s does not allow optional name '%s'"
> + % (source, name))
> + if not set(membername) <= valid_characters:
... so this check would break if we called locale.setlocale() in this
program. While I don't think we need to worry about it, I think you
could just as well use something like
valid_name = re.compile(r"^[A-Za-z0-9-._]+$")
if not valid_name.match(membername):
> + raise QAPIExprError(expr_info,
> + "%s uses invalid name '%s'" % (source, name))
> +
> def check_type(expr_info, source, data, allow_array = False,
> - allowed_metas = [], allow_dict = True):
> + allowed_metas = [], allow_dict = True, allow_optional =
> False):
> global all_names
>
> if data is None:
> @@ -317,21 +337,23 @@ def check_type(expr_info, source, data, allow_array =
> False,
> raise QAPIExprError(expr_info,
> "%s should be a type name" % source)
> for (key, value) in data.items():
> + check_name(expr_info, "Member of %s" % source, key,
> + allow_optional=allow_optional)
> check_type(expr_info, "Member '%s' of %s" % (key, source), value,
> allow_array=True,
> allowed_metas=['built-in', 'union', 'alternate', 'struct',
> 'enum'],
> - allow_dict=True)
> + allow_dict=True, allow_optional=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'])
> + allowed_metas=['union', 'struct'], allow_optional=True)
> check_type(expr_info, "'returns' for command '%s'" % name,
> expr.get('returns'), allow_array=True,
> allowed_metas=['built-in', 'union', 'alternate', 'struct',
> - 'enum'])
> + 'enum'], allow_optional=True)
>
> def check_event(expr, expr_info):
> global events
> @@ -345,7 +367,8 @@ def check_event(expr, expr_info):
> % name)
> events.append(name)
> check_type(expr_info, "'data' for event '%s'" % name,
> - expr.get('data'), allowed_metas=['union', 'struct'])
> + expr.get('data'), allowed_metas=['union', 'struct'],
> + allow_optional=True)
> if params:
> for argname, argentry, optional, structured in parse_args(params):
> if structured:
> @@ -385,12 +408,10 @@ def check_union(expr, expr_info):
> "Base '%s' is not a valid base type"
> % base)
>
> - # The value of member 'discriminator' must name a member of the
> - # base type.
> - if not isinstance(discriminator, str):
> - raise QAPIExprError(expr_info,
> - "Flat union '%s' must have a string "
> - "discriminator field" % name)
> + # The value of member 'discriminator' must name a non-optional
> + # member of the base type.
> + check_name(expr_info, "Discriminator of flat union '%s'" % name,
> + discriminator)
> discriminator_type = base_fields.get(discriminator)
> if not discriminator_type:
> raise QAPIExprError(expr_info,
What happens when I try 'discriminator': '**'?
> @@ -406,6 +427,8 @@ def check_union(expr, expr_info):
>
> # Check every branch
> for (key, value) in members.items():
> + check_name(expr_info, "Member of union '%s'" % name, key)
> +
> # Each value must name a known type
> check_type(expr_info, "Member '%s' of union '%s'" % (key, name),
> value, allow_array=True,
> @@ -439,6 +462,8 @@ def check_alternate(expr, expr_info):
>
> # Check every branch
> for (key, value) in members.items():
> + check_name(expr_info, "Member of alternate '%s'" % name, key)
> +
> # Check for conflicts in the generated enum
> c_key = _generate_enum_string(key)
> if c_key in values:
> @@ -485,7 +510,8 @@ def check_struct(expr, expr_info):
> name = expr['type']
> members = expr['data']
>
> - check_type(expr_info, "'data' for type '%s'" % name, members)
> + check_type(expr_info, "'data' for type '%s'" % name, members,
> + allow_optional=True)
> check_type(expr_info, "'base' for type '%s'" % name, expr.get('base'),
> allowed_metas=['struct'], allow_dict=False)
>
> @@ -676,8 +702,11 @@ def type_name(name):
> return c_list_type(name[0])
> return name
>
> -def add_name(name, info, meta, implicit = False):
> +def add_name(name, info, meta, implicit = False, source = None):
> global all_names
> + if not source:
> + source = "'%s'" % meta
> + check_name(info, source, name)
> if name in all_names:
> raise QAPIExprError(info,
> "%s '%s' is already defined"
> @@ -691,7 +720,7 @@ def add_name(name, info, meta, implicit = False):
> def add_struct(definition, info):
> global struct_types
> name = definition['type']
> - add_name(name, info, 'struct')
> + add_name(name, info, 'struct', source="'type'")
> struct_types.append(definition)
>
> def find_struct(name):
[...]
