Eric Blake <ebl...@redhat.com> writes:
> c_name() produces names starting with 'q_' when protecting
> a QMP member name that would fail to directly compile, but
> in doing so can cause clashes with any QMP name already
> beginning with 'q-' or 'q_'. Likewise, we create a C name
> 'has_' for any optional member, that can clash with any QMP
> name beginning with 'has-' or 'has_'.
>
> Technically, rather than blindly reserving the namespace,
> we could try to complain about user names only when an actual
> collision occurs, or even teach c_name() how to munge names
> to avoid collisions. But it is not trivial, especially when
> collisions can occur across multiple types (such as via
> inheritance or flat unions). Besides, no existing .json
> files are trying to use these names. So it's easier to just
> outright forbid the potential for collision. We can always
> relax things in the future if a real need arises for QMP to
> express member names that have been forbidden here.
>
> 'has_' only has to be reserved for struct/union member names,
> while 'q_' is reserved everywhere (matching the fact that
> we only have optional members, but use c_name() everywhere).
>
> Update and add tests to cover the new error messages.
>
> Signed-off-by: Eric Blake <ebl...@redhat.com>
>
> ---
> v10: retitle, improve comments, defer 'u' changes for later, use
> c_name(name).startswith('has_') rather than open coding
> v9: new patch
> ---
> docs/qapi-code-gen.txt | 11 +++++++----
> scripts/qapi.py | 8 +++++++-
> tests/qapi-schema/args-has-clash.err | 1 +
> tests/qapi-schema/args-has-clash.exit | 2 +-
> tests/qapi-schema/args-has-clash.json | 8 ++++----
> tests/qapi-schema/args-has-clash.out | 6 ------
> tests/qapi-schema/reserved-command.err | 1 +
> tests/qapi-schema/reserved-command.exit | 2 +-
> tests/qapi-schema/reserved-command.json | 7 +++----
> tests/qapi-schema/reserved-command.out | 5 -----
> tests/qapi-schema/reserved-member.err | 1 +
> tests/qapi-schema/reserved-member.exit | 2 +-
> tests/qapi-schema/reserved-member.json | 7 +++----
> tests/qapi-schema/reserved-member.out | 4 ----
> 14 files changed, 30 insertions(+), 35 deletions(-)
All right, this is simple enough to be done now. And as you say, we can
always relax things again later.
> diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
> index c4264a8..4d8c2fc 100644
> --- a/docs/qapi-code-gen.txt
> +++ b/docs/qapi-code-gen.txt
> @@ -112,7 +112,9 @@ 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 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
> @@ -123,9 +125,10 @@ vendor), even if the rest of the name uses dash (example:
> __com.redhat_drive-mirror). Other than downstream extensions (with
> leading underscore and the use of dots), all names should begin with a
> letter, and contain only ASCII letters, digits, dash, and underscore.
> -It is okay to reuse names that match C keywords; the generator will
> -rename a field named "default" in the QAPI to "q_default" in the
> -generated C code.
> +Names beginning with 'q_' are reserved for the generator: QMP names
> +that resemble C keywords or other problematic strings will be munged
> +in C to use this prefix. For example, a field named "default" in
> +qapi becomes "q_default" in the generated C code.
>
> In the rest of this document, usage lines are given for each
> expression type, with literal strings written in lower case and
> diff --git a/scripts/qapi.py b/scripts/qapi.py
> index d53b5c4..04bcbf7 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -376,7 +376,9 @@ def check_name(expr_info, source, name,
> allow_optional=False,
> # code always prefixes it with the enum name
> if enum_member:
> membername = '_' + membername
> - if not valid_name.match(membername):
> + # Reserve the entire 'q_' namespace for c_name()
> + if not valid_name.match(membername) or \
> + membername.replace('-', '_').startswith('q_'):
Why not c_name(membername).startswith('q_')?
> raise QAPIExprError(expr_info,
> "%s uses invalid name '%s'" % (source, name))
>
> @@ -488,6 +490,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 c_name(key).startswith('has_'):
> + 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,
> diff --git a/tests/qapi-schema/args-has-clash.err
> b/tests/qapi-schema/args-has-clash.err
> index e69de29..595fe93 100644
> --- a/tests/qapi-schema/args-has-clash.err
> +++ b/tests/qapi-schema/args-has-clash.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/args-has-clash.json:6: Member of 'data' for command 'oops'
> uses reserved name 'has-a'
> diff --git a/tests/qapi-schema/args-has-clash.exit
> b/tests/qapi-schema/args-has-clash.exit
> index 573541a..d00491f 100644
> --- a/tests/qapi-schema/args-has-clash.exit
> +++ b/tests/qapi-schema/args-has-clash.exit
> @@ -1 +1 @@
> -0
> +1
> diff --git a/tests/qapi-schema/args-has-clash.json
> b/tests/qapi-schema/args-has-clash.json
> index a2197de..38453e7 100644
> --- a/tests/qapi-schema/args-has-clash.json
> +++ b/tests/qapi-schema/args-has-clash.json
> @@ -1,6 +1,6 @@
> # 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'; so
> +# instead we reject any member with a name that would collide with 'has_'.
Suggest something like:
# We reject names like 'has-a', because they can collide with the flag
# for an optional 'a' in generated C.
> +# TODO we could munge the optional flag name to avoid the collision.
> { 'command': 'oops', 'data': { '*a': 'str', 'has-a': 'str' } }
> diff --git a/tests/qapi-schema/args-has-clash.out
> b/tests/qapi-schema/args-has-clash.out
> index 5a18b6b..e69de29 100644
> --- a/tests/qapi-schema/args-has-clash.out
> +++ b/tests/qapi-schema/args-has-clash.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/reserved-command.err
> b/tests/qapi-schema/reserved-command.err
> index e69de29..a70856b 100644
> --- a/tests/qapi-schema/reserved-command.err
> +++ b/tests/qapi-schema/reserved-command.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/reserved-command.json:6: 'command' uses invalid name
> 'q-unix'
> diff --git a/tests/qapi-schema/reserved-command.exit
> b/tests/qapi-schema/reserved-command.exit
> index 573541a..d00491f 100644
> --- a/tests/qapi-schema/reserved-command.exit
> +++ b/tests/qapi-schema/reserved-command.exit
> @@ -1 +1 @@
> -0
> +1
> diff --git a/tests/qapi-schema/reserved-command.json
> b/tests/qapi-schema/reserved-command.json
> index be9944c..d662a96 100644
> --- a/tests/qapi-schema/reserved-command.json
> +++ b/tests/qapi-schema/reserved-command.json
> @@ -1,7 +1,6 @@
> # C entity name collision
> -# FIXME - This parses, but fails to compile, because it attempts to declare
> -# two 'qmp_q_unix' functions (one for 'q-unix', the other because c_name()
> -# munges 'unix' to 'q_unix' to avoid reserved word collisions). We should
> -# reject attempts to explicitly use 'q_' names, to reserve it for qapi.
> +# This would attempt to declare two 'qmp_q_unix' functions (one for 'q-unix',
> +# the other because c_name() munges 'unix' to 'q_unix' to avoid reserved word
> +# collisions). Therefore, we reserve 'q_' names for qapi.
Suggest something like:
# We reject names like 'q-unix', because they can collide with the
# mangled name for 'unix' in generated C.
> { 'command': 'unix' }
> { 'command': 'q-unix' }
> diff --git a/tests/qapi-schema/reserved-command.out
> b/tests/qapi-schema/reserved-command.out
> index b31b38f..e69de29 100644
> --- a/tests/qapi-schema/reserved-command.out
> +++ b/tests/qapi-schema/reserved-command.out
> @@ -1,5 +0,0 @@
> -object :empty
> -command q-unix None -> None
> - gen=True success_response=True
> -command unix None -> None
> - gen=True success_response=True
> diff --git a/tests/qapi-schema/reserved-member.err
> b/tests/qapi-schema/reserved-member.err
> index e69de29..8752c5b 100644
> --- a/tests/qapi-schema/reserved-member.err
> +++ b/tests/qapi-schema/reserved-member.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/reserved-member.json:5: Member of 'data' for struct 'Foo'
> uses invalid name 'q-unix'
> diff --git a/tests/qapi-schema/reserved-member.exit
> b/tests/qapi-schema/reserved-member.exit
> index 573541a..d00491f 100644
> --- a/tests/qapi-schema/reserved-member.exit
> +++ b/tests/qapi-schema/reserved-member.exit
> @@ -1 +1 @@
> -0
> +1
> diff --git a/tests/qapi-schema/reserved-member.json
> b/tests/qapi-schema/reserved-member.json
> index 1602ed3..a8dac22 100644
> --- a/tests/qapi-schema/reserved-member.json
> +++ b/tests/qapi-schema/reserved-member.json
> @@ -1,6 +1,5 @@
> # C member name collision
> -# FIXME - This parses, but fails to compile, because it attempts to declare
> -# two 'q_unix' members (one for 'q-unix', the other because c_name()
> -# munges 'unix' to 'q_unix' to avoid reserved word collisions). We should
> -# reject attempts to explicitly use 'q_' names, to reserve it for qapi.
> +# This would attempt to declare two 'q_unix' members (one for 'q-unix',
> +# the other because c_name() munges 'unix' to 'q_unix' to avoid reserved word
> +# collisions). Therefore, we reserve 'q_' names for qapi.
> { 'struct': 'Foo', 'data': { 'unix':'int', 'q-unix':'bool' } }
Likewise.
Remind me, why do we need both reserved-command.json and
reserved-member.json?
> diff --git a/tests/qapi-schema/reserved-member.out
> b/tests/qapi-schema/reserved-member.out
> index 0d8685a..e69de29 100644
> --- a/tests/qapi-schema/reserved-member.out
> +++ b/tests/qapi-schema/reserved-member.out
> @@ -1,4 +0,0 @@
> -object :empty
> -object Foo
> - member unix: int optional=False
> - member q-unix: bool optional=False
