Re: [Qemu-devel] [PATCH v10 23/25] qapi: Reserve 'u' member name

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> Now that we have separated union tag values from colliding with
> non-variant C names, by naming the union 'u', we should reserve
> this name for our use.  Note that we want to forbid 'u' even in
> a struct with no variants, because it is possible for a future
> qemu release to extend QMP in a backwards-compatible manner while
> converting from a struct to a flat union.  Fortunately, no
> existing clients were using this member name.  If we ever find
> the need for QMP to have a member 'u', we could at that time
> relax things, perhaps by having c_name() munge the QMP member to
> 'q_u'.
>
> Note that we cannot forbid 'u' everywhere (by adding the
> rejection code to check_name()), because the existing QKeyCode
> enum already uses it.

Suggest to add something like: Therefore, we only reserve it as a struct
type member name.

>                        And it is still okay to use it as a
> branch name within a union, although not worth adding a
> positive test to qapi-schema-test for this corner case.

Not sure this sentence is needed.

> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> v10: new patch, split from 1/17 and 3/17; by deferring this until
> after the rename was complete, it reduces churn due to the new feature
> ---
>  scripts/qapi.py                        | 2 +-
>  tests/Makefile                         | 1 +
>  tests/qapi-schema/struct-member-u.err  | 1 +
>  tests/qapi-schema/struct-member-u.exit | 1 +
>  tests/qapi-schema/struct-member-u.json | 7 +++++++
>  tests/qapi-schema/struct-member-u.out  | 0
>  6 files changed, 11 insertions(+), 1 deletion(-)
>  create mode 100644 tests/qapi-schema/struct-member-u.err
>  create mode 100644 tests/qapi-schema/struct-member-u.exit
>  create mode 100644 tests/qapi-schema/struct-member-u.json
>  create mode 100644 tests/qapi-schema/struct-member-u.out
>
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index 8b9f5f7..b941ccf 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -490,7 +490,7 @@ 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 c_name(key).startswith('has_'):
> +        if key == 'u' or c_name(key).startswith('has_'):

Perhaps c_name(key) == 'u' would be very slightly cleaner.  No
difference in practice, of course.

>              raise QAPIExprError(expr_info,
>                                  "Member of %s uses reserved name '%s'"
>                                  % (source, key))
> diff --git a/tests/Makefile b/tests/Makefile
> index 9194264..8c239f8 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -326,6 +326,7 @@ qapi-schema += struct-base-clash-deep.json
>  qapi-schema += struct-base-clash.json
>  qapi-schema += struct-data-invalid.json
>  qapi-schema += struct-member-invalid.json
> +qapi-schema += struct-member-u.json

The closely related test for the reserved 'has_' prefix is called
args-has-clash.json.

More losely related tests for other reserved names are called
reserved-command.json, reserved-member.json.

Should these all be called like reserved-WHATEVER-WHERE.json?

>  qapi-schema += struct-name-list.json
>  qapi-schema += trailing-comma-list.json
>  qapi-schema += trailing-comma-object.json
> diff --git a/tests/qapi-schema/struct-member-u.err 
> b/tests/qapi-schema/struct-member-u.err
> new file mode 100644
> index 0000000..0dc354c
> --- /dev/null
> +++ b/tests/qapi-schema/struct-member-u.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/struct-member-u.json:7: 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
> new file mode 100644
> index 0000000..d00491f
> --- /dev/null
> +++ b/tests/qapi-schema/struct-member-u.exit
> @@ -0,0 +1 @@
> +1
> diff --git a/tests/qapi-schema/struct-member-u.json 
> b/tests/qapi-schema/struct-member-u.json
> new file mode 100644
> index 0000000..1eaf0f3
> --- /dev/null
> +++ b/tests/qapi-schema/struct-member-u.json
> @@ -0,0 +1,7 @@
> +# Potential C member name collision
> +# 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.
> +# TODO - we could munge the member name to 'q_u' to avoid the collision
> +{ 'struct': 'Oops', 'data': { 'u': 'str' } }
> diff --git a/tests/qapi-schema/struct-member-u.out 
> b/tests/qapi-schema/struct-member-u.out
> new file mode 100644
> index 0000000..e69de29