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
> - enum members must be valid names, when combined with prefix
> - union and alternate branches cannot be marked optional
>
> The set of valid names includes [a-zA-Z_][a-zA-Z0-9._-]* (where
> '.' is useful only in downstream extensions). Since we have
> existing enum values beginning with a digit (see QKeyCode), a
Meh. Didn't see the patch, or else I would've shot down these names.
Too late now.
> small hack is used to pass the same regex.
A bit vague.
>
> Signed-off-by: Eric Blake <ebl...@redhat.com>
>
> ---
>
> v6: rebase onto earlier changes; use regex instead of sets, and
> ensure leading letter or _; don't force event case; drop dead
> code for exempting '**'; extend coverage to enum members
> ---
> scripts/qapi.py | 63
> ++++++++++++++++------
> 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/enum-bad-name.err | 1 +
> tests/qapi-schema/enum-bad-name.exit | 2 +-
> tests/qapi-schema/enum-bad-name.json | 2 +-
> tests/qapi-schema/enum-bad-name.out | 3 --
> tests/qapi-schema/enum-dict-member.err | 2 +-
> 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 | 7 ---
> 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 --
> 19 files changed, 60 insertions(+), 43 deletions(-)
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 9f64a0d..9b97683 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -276,8 +276,31 @@ def discriminator_find_enum_define(expr):
>
> return find_enum(discriminator_type)
>
> +valid_name = re.compile('^[a-zA-Z_][a-zA-Z0-9_.-]*$')
> +def check_name(expr_info, source, name, allow_optional = False,
> + enum_member = False):
> + global valid_name
> + membername = name
> +
> + if not isinstance(name, str):
> + raise QAPIExprError(expr_info,
> + "%s requires a string name" % source)
> + if name.startswith('*'):
> + membername = name[1:]
> + if not allow_optional:
> + raise QAPIExprError(expr_info,
> + "%s does not allow optional name '%s'"
> + % (source, name))
> + # Enum members can start with a digit, because the generated C
> + # code always prefixes it with the enum name
> + if enum_member:
> + membername = "_%s" %membername
This is the hack.
Less vague commit message:
Valid names match [a-zA-Z_][a-zA-Z0-9._-]*. Except for enumeration
names, which match [a-zA-Z0-9._-]+. By convention, '.' is used only
in downstream extensions.
> + if not valid_name.match(membername):
> + raise QAPIExprError(expr_info,
> + "%s uses invalid name '%s'" % (source, name))
> +
> def check_type(expr_info, source, value, allow_array = False,
> - allow_dict = False, allow_metas = []):
> + allow_dict = False, allow_optional = False, allow_metas = []):
> global all_names
> orig_value = value
>
We could reject new enumeration names beginning with a digit with a
whitelist, similar to how we reject new commands returning
non-dictionaries in the next patch. Probably not worth the bother.
Reviewed-by: Markus Armbruster <arm...@redhat.com>
