Re: [Qemu-devel] [PATCH v9 03/17] qapi: Reserve 'u' and 'has[-_]*' member names

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> To make collision detection between member names easier, we
> might as well reject all attempts to use anything that would
> collide with our use of 'has_' as a flag for optional members.
> Also, a later patch will introduce a named union 'u' for
> holding the branch names of a qapi union in a separate
> namespace from non-variant QMP members, at which point it
> would collide with a 'u' member.  Fortunately, none of our
> qapi files were using these names, so we can reserve them now.
>
> Note that we cannot forbid 'u' everywhere (by adding the
> rejection code to check_name()), because the existing QKeyCode
> enum already uses it, and because qapi-schema-test.json
> intentionally uses it as a branch name (we could feasibly have
> a union with one branch per QKeyCode).  Also, forbidding 'has_'
> in branch names will eventually disappear, once branch names
> cannot collide with other names.  So instead, this code does
> separate checks for check_type() (when iterating over a dict,
> common to struct, command args, and event data) and for
> unions.  We do NOT need to reserve the names for alternates,
> because alternates do not have any QMP members alongside its
> branch names.

I'm not quite comfortable with reserving 'u' now, becaue I feel we
haven't fully explored the design space for avoiding branch - member
clashes.

I still like the basic idea to give the unnamed union a name.  It needs
to be a short one, to keep the C code legible.  'u' is an obvious
option, but it requires reserving 'u' at least as member name.  '_u'
wouldn't.  Alternatively, call the union 'u', but avoid the clash by
mapping QAPI member name 'u' to C identifier '_u'.

I feel the decision should be made over the patch that give the union a
name.

> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> v9: new patch
> ---
>  docs/qapi-code-gen.txt                         |  5 ++++-
>  scripts/qapi.py                                | 12 ++++++++++++
>  tests/qapi-schema/args-name-has.err            |  1 +
>  tests/qapi-schema/args-name-has.exit           |  2 +-
>  tests/qapi-schema/args-name-has.json           |  7 +++----
>  tests/qapi-schema/args-name-has.out            |  6 ------
>  tests/qapi-schema/flat-union-clash-branch.err  |  1 +
>  tests/qapi-schema/flat-union-clash-branch.exit |  2 +-
>  tests/qapi-schema/flat-union-clash-branch.json |  7 ++++---
>  tests/qapi-schema/flat-union-clash-branch.out  | 11 -----------
>  tests/qapi-schema/qapi-schema-test.json        |  9 ++++-----
>  tests/qapi-schema/struct-member-u.err          |  1 +
>  tests/qapi-schema/struct-member-u.exit         |  2 +-
>  tests/qapi-schema/struct-member-u.json         |  8 ++++----
>  tests/qapi-schema/struct-member-u.out          |  3 ---
>  15 files changed, 37 insertions(+), 40 deletions(-)
>
> diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
> index c4264a8..df2d198 100644
> --- a/docs/qapi-code-gen.txt
> +++ b/docs/qapi-code-gen.txt
> @@ -112,7 +112,10 @@ and field names within a type, should be all lower case 
> with words
>  separated by a hyphen.  However, some existing older commands and
>  complex types use underscore; when extending such expressions,
>  consistency is preferred over blindly avoiding underscore.  Event
> -names should be ALL_CAPS with words separated by underscore.
> +names should be ALL_CAPS with words separated by underscore.  Field
> +names cannot be 'u', as this is reserved for generated code for
> +unions, nor can they start with 'has-' or 'has_', as this is
> +reserved for tracking optional fields.
>
>  Any name (command, event, type, field, or enum value) beginning with
>  "x-" is marked experimental, and may be withdrawn or changed
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 09ba44b..1e7e08b 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -488,6 +488,10 @@ def check_type(expr_info, source, value, 
> allow_array=False,
>      for (key, arg) in value.items():
>          check_name(expr_info, "Member of %s" % source, key,
>                     allow_optional=allow_optional)
> +        if key == 'u' or key.startswith('has-') or key.startswith('has_'):

Something like c_name(key).startswith('has_') would avoid hardcoding the
mapping of '-' to '_' here.  Dunno.

> +            raise QAPIExprError(expr_info,
> +                                "Member of %s uses reserved name '%s'"
> +                                % (source, key))
>          # Todo: allow dictionaries to represent default values of
>          # an optional argument.
>          check_type(expr_info, "Member '%s' of %s" % (key, source), arg,
> @@ -588,6 +592,14 @@ 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)
> +        # TODO: As long as branch names can collide with QMP names, we
> +        # must prevent branches starting with 'has_'. However, we do not
> +        # need to reject 'u', because that is reserved for when we start
> +        # sticking branch names in a C union named 'u'.
> +        if key.startswith('has-') or key.startswith('has_'):
> +            raise QAPIExprError(expr_info,
> +                                "Branch of union '%s' uses reserved name 
> '%s'"
> +                                % (name, key))

This will go away again when we give the unnamed union a name.

I feel we should punt all further clash detection until late in the
cleanup work.  It's merely nice to have (sane error message from
generator instead of possibly confusing one from the C compiler,
basically), and adding it now causes churn later on.

>
>          # Each value must name a known type; furthermore, in flat unions,
>          # branches must be a struct with no overlapping member names
> diff --git a/tests/qapi-schema/args-name-has.err 
> b/tests/qapi-schema/args-name-has.err
> index e69de29..cb57552 100644
> --- a/tests/qapi-schema/args-name-has.err
> +++ b/tests/qapi-schema/args-name-has.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/args-name-has.json:5: Member of 'data' for command 'oops' 
> uses reserved name 'has-a'
> diff --git a/tests/qapi-schema/args-name-has.exit 
> b/tests/qapi-schema/args-name-has.exit
> index 573541a..d00491f 100644
> --- a/tests/qapi-schema/args-name-has.exit
> +++ b/tests/qapi-schema/args-name-has.exit
> @@ -1 +1 @@
> -0
> +1
> diff --git a/tests/qapi-schema/args-name-has.json 
> b/tests/qapi-schema/args-name-has.json
> index a2197de..4297549 100644
> --- a/tests/qapi-schema/args-name-has.json
> +++ b/tests/qapi-schema/args-name-has.json
> @@ -1,6 +1,5 @@
>  # C member name collision
> -# FIXME - This parses, but fails to compile, because the C struct is given
> -# two 'has_a' members, one from the flag for optional 'a', and the other
> -# from member 'has-a'.  Either reject this at parse time, or munge the C
> -# names to avoid the collision.
> +# This would attempt to create two 'has_a' members of the C struct, one
> +# from the flag for optional 'a', and the other from member 'has-a'.
> +# TODO we could munge the optional flag name to avoid the collision.

You mean call them _has_FOO instead of has_FOO?  The generated code
would be rather confusing...

If we don't want to reserve all names starting with 'has_', then I'd
narrowly outlaw having both an optional member FOO and a member has_FOO.
I think I'd like that a bit better than outlawing 'has_'.  But not
enough to accept much implementation complexity.

>  { 'command': 'oops', 'data': { '*a': 'str', 'has-a': 'str' } }
> diff --git a/tests/qapi-schema/args-name-has.out 
> b/tests/qapi-schema/args-name-has.out
> index 5a18b6b..e69de29 100644
> --- a/tests/qapi-schema/args-name-has.out
> +++ b/tests/qapi-schema/args-name-has.out
> @@ -1,6 +0,0 @@
> -object :empty
> -object :obj-oops-arg
> -    member a: str optional=True
> -    member has-a: str optional=False
> -command oops :obj-oops-arg -> None
> -   gen=True success_response=True
> diff --git a/tests/qapi-schema/flat-union-clash-branch.err 
> b/tests/qapi-schema/flat-union-clash-branch.err
> index e69de29..e6b6294 100644
> --- a/tests/qapi-schema/flat-union-clash-branch.err
> +++ b/tests/qapi-schema/flat-union-clash-branch.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/flat-union-clash-branch.json:13: Branch of union 
> 'TestUnion' uses reserved name 'has-a'
> diff --git a/tests/qapi-schema/flat-union-clash-branch.exit 
> b/tests/qapi-schema/flat-union-clash-branch.exit
> index 573541a..d00491f 100644
> --- a/tests/qapi-schema/flat-union-clash-branch.exit
> +++ b/tests/qapi-schema/flat-union-clash-branch.exit
> @@ -1 +1 @@
> -0
> +1
> diff --git a/tests/qapi-schema/flat-union-clash-branch.json 
> b/tests/qapi-schema/flat-union-clash-branch.json
> index c9f08e3..8efbcfd 100644
> --- a/tests/qapi-schema/flat-union-clash-branch.json
> +++ b/tests/qapi-schema/flat-union-clash-branch.json
> @@ -1,8 +1,9 @@
>  # Flat union branch name collision
> -# FIXME: this parses, but then fails to compile due to a duplicate 'has_a'
> +# This is rejected because the C struct would have duplicate 'has_a'
>  # (one as the implicit flag for the optional base member, the other from
> -# the C member for the branch name).  We should either reject the collision
> -# at parse time, or munge the generated branch name to allow this to compile.
> +# the C member for the branch name).
> +# TODO: we should munge generated branch names to not collide with the
> +# non-variant struct members.

Okay if I read "munge branch names" to include giving the unnamed union
a name.

>  { 'enum': 'TestEnum',
>    'data': [ 'has-a' ] }
>  { 'struct': 'Base',
> diff --git a/tests/qapi-schema/flat-union-clash-branch.out 
> b/tests/qapi-schema/flat-union-clash-branch.out
> index 1491081..e69de29 100644
> --- a/tests/qapi-schema/flat-union-clash-branch.out
> +++ b/tests/qapi-schema/flat-union-clash-branch.out
> @@ -1,11 +0,0 @@
> -object :empty
> -object Base
> -    member enum1: TestEnum optional=False
> -    member a: str optional=True
> -object Branch1
> -    member string: str optional=False
> -enum TestEnum ['has-a']
> -object TestUnion
> -    base Base
> -    tag enum1
> -    case has-a: Branch1
> diff --git a/tests/qapi-schema/qapi-schema-test.json 
> b/tests/qapi-schema/qapi-schema-test.json
> index 81d19d0..1ca7e4f 100644
> --- a/tests/qapi-schema/qapi-schema-test.json
> +++ b/tests/qapi-schema/qapi-schema-test.json
> @@ -105,11 +105,10 @@
>              'sizes': ['size'],
>              'any': ['any'] } }
>
> -# Even if 'u' is forbidden as a struct member name, it should still be
> -# valid as a type or union branch name. And although '*Kind' and '*List'
> -# are forbidden as type names, they should not be forbidden as a member
> -# or branch name.
> -# TODO - 'has_*' should also be forbidden as a member name.
> +# Even though 'u' and 'has_*' are forbidden as struct member names, they
> +# should still be valid as a type or union branch name. And although
> +# '*Kind' and '*List' are forbidden as type names, they should not be
> +# forbidden as a member or branch name.
>  { 'struct': 'has_a', 'data': { 'MyKind': 'int', 'MyList': ['int'] } }
>  { 'union': 'u', 'data': { 'u': 'uint8', 'myKind': 'has_a',
>                            'myList': 'has_a' } }
> diff --git a/tests/qapi-schema/struct-member-u.err 
> b/tests/qapi-schema/struct-member-u.err
> index e69de29..5610310 100644
> --- a/tests/qapi-schema/struct-member-u.err
> +++ b/tests/qapi-schema/struct-member-u.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/struct-member-u.json:6: Member of 'data' for struct 'Oops' 
> uses reserved name 'u'
> diff --git a/tests/qapi-schema/struct-member-u.exit 
> b/tests/qapi-schema/struct-member-u.exit
> index 573541a..d00491f 100644
> --- a/tests/qapi-schema/struct-member-u.exit
> +++ b/tests/qapi-schema/struct-member-u.exit
> @@ -1 +1 @@
> -0
> +1
> diff --git a/tests/qapi-schema/struct-member-u.json 
> b/tests/qapi-schema/struct-member-u.json
> index d72023d..a0e05b5 100644
> --- a/tests/qapi-schema/struct-member-u.json
> +++ b/tests/qapi-schema/struct-member-u.json
> @@ -1,6 +1,6 @@
>  # Potential C member name collision
> -# FIXME - This parses and compiles, but would cause a collision if this
> -# struct is later reworked to be part of a flat union, once unions hide
> -# their tag values under a C union named 'u'. We should reject the use
> -# of this identifier to reserve it for internal use.
> +# We reject use of 'u' as a member name, to allow it for internal use in
> +# putting union branch members in a separate namespace from QMP members.
> +# This is true even for non-unions, because it is possible to convert a
> +# struct to flat union while remaining backwards compatible in QMP.
>  { 'struct': 'Oops', 'data': { 'u': 'str' } }
> diff --git a/tests/qapi-schema/struct-member-u.out 
> b/tests/qapi-schema/struct-member-u.out
> index aa53e7f..e69de29 100644
> --- a/tests/qapi-schema/struct-member-u.out
> +++ b/tests/qapi-schema/struct-member-u.out
> @@ -1,3 +0,0 @@
> -object :empty
> -object Oops
> -    member u: str optional=False