Eric Blake <ebl...@redhat.com> 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 <ebl...@redhat.com>
>
> ---
> 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
