Re: [Qemu-devel] [PATCH v6 25/36] qapi: Require valid names

[Markus Armbruster]

Eric Blake <address@hidden> 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 <address@hidden>
>
> ---
>
> 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 <address@hidden>