Re: [Qemu-devel] [PATCH v5 21/28] 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
> - 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 <address@hidden>
> ---
>  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 + '.' + '-' + 
> '_')
> +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

I'm afraid this conditional is superfluous or wrong.  Our schemata don't
trigger it.

As far as I know, '**' may occur as pseudo-type in a command's 'data' or
'returns', and nowhere else.

Example 1 (qom-get):
  'returns': '**'

Example 2 (qom-set):
  'data': { 'path': 'str', 'property': 'str', 'value': '**' },

Example 3 (hypothetical)
  'returns': { 'frob': '**' }

Both 'data' and 'returns' are checked by check_type().

Example 1 takes the early exit on data = '**' there.

Example 2 goes through this loop

    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_optional=True)

The '**' is fed to check_type(), *not* to check_name().  check_type()
takes the same early exit.

[...]