[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v12 21/36] qapi: Tighten the regex on valid name
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v12 21/36] qapi: Tighten the regex on valid names |
Date: |
Wed, 18 Nov 2015 12:51:19 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
Eric Blake <address@hidden> writes:
> We already documented that qapi names should match specific
> patterns (such as starting with a letter unless it was an enum
> value or a downstream extension). Tighten that from a suggestion
> into a hard requirement, which frees up names beginning with a
> single underscore for qapi internal usage.
If we care enough about naming conventions to document them, enforcing
them makes only sense.
> Also restrict things
> to avoid 'a__b' or 'a--b' (that is, dash and underscore must not
> occur consecutively).
I feel this is entering "foolish names" territory, where things like
"IcAnFiNdNaMeSeVeNlEsSrEaDaBlEtHaNStudlyCaps" live. Catching fools
automatically is generally futile, they're too creative :)
Let's drop this part.
> Add a new test, reserved-member-underscore, to demonstrate the
> tighter checking.
>
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> v12: new patch
> ---
> docs/qapi-code-gen.txt | 19 ++++++++++---------
> scripts/qapi.py | 12 +++++++-----
> tests/Makefile | 1 +
> tests/qapi-schema/reserved-member-underscore.err | 1 +
> tests/qapi-schema/reserved-member-underscore.exit | 1 +
> tests/qapi-schema/reserved-member-underscore.json | 4 ++++
> tests/qapi-schema/reserved-member-underscore.out | 0
> 7 files changed, 24 insertions(+), 14 deletions(-)
> create mode 100644 tests/qapi-schema/reserved-member-underscore.err
> create mode 100644 tests/qapi-schema/reserved-member-underscore.exit
> create mode 100644 tests/qapi-schema/reserved-member-underscore.json
> create mode 100644 tests/qapi-schema/reserved-member-underscore.out
>
> diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
> index ceb9a78..ec225d1 100644
> --- a/docs/qapi-code-gen.txt
> +++ b/docs/qapi-code-gen.txt
> @@ -118,17 +118,18 @@ tracking optional fields.
>
> Any name (command, event, type, field, or enum value) beginning with
> "x-" is marked experimental, and may be withdrawn or changed
> -incompatibly in a future release. Downstream vendors may add
> -extensions; such extensions should begin with a prefix matching
> +incompatibly in a future release. All names must begin with a letter,
> +and contain only ASCII letters, digits, dash, and underscore, where
> +dash and underscore cannot occur consecutively. There are two
> +exceptions: enum values may start with a digit, and any extensions
> +added by downstream vendors should start with a prefix matching
> "__RFQDN_" (for the reverse-fully-qualified-domain-name of the
> 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.
> -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.
> +__com.redhat_drive-mirror). 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 4a30bc0..e6d014b 100644
> --- a/scripts/qapi.py
> +++ b/scripts/qapi.py
> @@ -353,9 +353,11 @@ def discriminator_find_enum_define(expr):
> return find_enum(discriminator_type)
>
>
> -# FIXME should enforce "other than downstream extensions [...], all
> -# names should begin with a letter".
> -valid_name = re.compile('^[a-zA-Z_][a-zA-Z0-9_.-]*$')
> +# Names must be letters, numbers, -, and _. They must start with letter,
> +# except for downstream extensions which must start with __RFQDN_.
> +# Dots are only valid in the downstream extension prefix.
> +valid_name = re.compile('^(__[a-zA-Z][a-zA-Z0-9.]*_)?'
> + '[a-zA-Z][a-zA-Z0-9]*([_-][a-zA-Z0-9]+)*$')
This regexp consists of two parts: optional __RFQDN_ prefix and the name
proper.
The latter stays simpler if we don't attempt to catch adjacent [-_].
The former isn't quite right. According to RFC 822 Appendix 1 - Domain
Name Syntax Specification:
* We must accept '-' in addition to digits.
* We require RFQDN to start with a letter. This is still a loose match.
The tight match is "labels separated by dots", where labels consist of
letters, digits and '-', starting with a letter. I wouldn't bother
checking first characters are letters at all.
Recommend
valid_name = re.compile('^(__[a-zA-Z0-9.-]+_)?'
'[a-zA-Z][a-zA-Z0-9_-]*$')
>
>
> def check_name(expr_info, source, name, allow_optional=False,
> @@ -374,8 +376,8 @@ def check_name(expr_info, source, name,
> allow_optional=False,
> % (source, name))
> # Enum members can start with a digit, because the generated C
> # code always prefixes it with the enum name
> - if enum_member:
> - membername = '_' + membername
> + if enum_member and membername[0].isdigit():
What's wrong with the old condition?
> + membername = 'D' + membername
> # Reserve the entire 'q_' namespace for c_name()
> if not valid_name.match(membername) or \
> c_name(membername, False).startswith('q_'):
> diff --git a/tests/Makefile b/tests/Makefile
> index 1c2c8ee..b70d246 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -320,6 +320,7 @@ qapi-schema += reserved-command-q.json
> qapi-schema += reserved-member-has.json
> qapi-schema += reserved-member-q.json
> qapi-schema += reserved-member-u.json
> +qapi-schema += reserved-member-underscore.json
> qapi-schema += reserved-type-kind.json
> qapi-schema += reserved-type-list.json
> qapi-schema += returns-alternate.json
> diff --git a/tests/qapi-schema/reserved-member-underscore.err
> b/tests/qapi-schema/reserved-member-underscore.err
> new file mode 100644
> index 0000000..65ff0da
> --- /dev/null
> +++ b/tests/qapi-schema/reserved-member-underscore.err
> @@ -0,0 +1 @@
> +tests/qapi-schema/reserved-member-underscore.json:4: Member of 'data' for
> struct 'Oops' uses invalid name '_oops'
> diff --git a/tests/qapi-schema/reserved-member-underscore.exit
> b/tests/qapi-schema/reserved-member-underscore.exit
> new file mode 100644
> index 0000000..d00491f
> --- /dev/null
> +++ b/tests/qapi-schema/reserved-member-underscore.exit
> @@ -0,0 +1 @@
> +1
> diff --git a/tests/qapi-schema/reserved-member-underscore.json
> b/tests/qapi-schema/reserved-member-underscore.json
> new file mode 100644
> index 0000000..4a3a017
> --- /dev/null
> +++ b/tests/qapi-schema/reserved-member-underscore.json
> @@ -0,0 +1,4 @@
> +# C member name collision
> +# We reject use of a single leading underscore in all names (names must
> +# begin with a letter or a downstream extension double-underscore prefix).
> +{ 'struct': 'Oops', 'data': { '_oops': 'str' } }
> diff --git a/tests/qapi-schema/reserved-member-underscore.out
> b/tests/qapi-schema/reserved-member-underscore.out
> new file mode 100644
> index 0000000..e69de29
- [Qemu-devel] [PATCH v12 03/36] qapi-types: Simplify gen_struct_field[s], (continued)
- [Qemu-devel] [PATCH v12 03/36] qapi-types: Simplify gen_struct_field[s], Eric Blake, 2015/11/18
- [Qemu-devel] [PATCH v12 11/36] qapi: Check for qapi collisions involving variant members, Eric Blake, 2015/11/18
- [Qemu-devel] [PATCH v12 01/36] qapi: Track simple union tag in object.local_members, Eric Blake, 2015/11/18
- [Qemu-devel] [PATCH v12 02/36] qapi-types: Consolidate gen_struct() and gen_union(), Eric Blake, 2015/11/18
- [Qemu-devel] [PATCH v12 08/36] qapi: Eliminate QAPISchemaObjectType.check() variable members, Eric Blake, 2015/11/18
- [Qemu-devel] [PATCH v12 15/36] qapi: Track owner of each object member, Eric Blake, 2015/11/18
- [Qemu-devel] [PATCH v12 25/36] qapi: Add alias for ErrorClass, Eric Blake, 2015/11/18
- [Qemu-devel] [PATCH v12 09/36] qapi: Factor out QAPISchemaObjectTypeMember.check_clash(), Eric Blake, 2015/11/18
- [Qemu-devel] [PATCH v12 21/36] qapi: Tighten the regex on valid names, Eric Blake, 2015/11/18
- Re: [Qemu-devel] [PATCH v12 21/36] qapi: Tighten the regex on valid names,
Markus Armbruster <=
- [Qemu-devel] [PATCH] fixup! qapi: Tighten the regex on valid names, Eric Blake, 2015/11/18
[Qemu-devel] [PATCH v12 06/36] qapi: Clean up after previous commit, Eric Blake, 2015/11/18
[Qemu-devel] [PATCH v12 07/36] qapi: Fix up commit 7618b91's clash sanity checking change, Eric Blake, 2015/11/18
[Qemu-devel] [PATCH v12 12/36] qapi: Factor out QAPISchemaObjectType.check_clash(), Eric Blake, 2015/11/18
[Qemu-devel] [PATCH v12 18/36] qapi: Remove dead visitor code, Eric Blake, 2015/11/18