Re: [Qemu-devel] [PATCH v10 05/25] qapi: Reserve 'q_*' and 'has_*' member names

[Markus Armbruster]

Eric Blake <address@hidden> 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 <address@hidden>
>
> ---
> 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