[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v10 05/25] qapi: Reserve 'q_*' and 'has_*' membe
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v10 05/25] qapi: Reserve 'q_*' and 'has_*' member names |
Date: |
Fri, 23 Oct 2015 08:14:41 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 |
On 10/23/2015 07:05 AM, Markus Armbruster wrote:
> 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>
>>
>> +++ 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_')?
Recursion. c_name('unix') is 'q_unix', which would be rejected, even
though we want to accept it. (And yes, that was my first try, before the
approach you see here)
>> +++ 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.
Sure, your wording changes are fine.
>
> Remind me, why do we need both reserved-command.json and
> reserved-member.json?
Because they are different namespaces. Your comment in 1/25 pointed out
that we could change the command munging to allow qmp_q_unix()
independently from qmp_unix(), even when we can't do the same for members.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
[Qemu-devel] [PATCH v10 02/25] qapi: More idiomatic string operations, Eric Blake, 2015/10/23
[Qemu-devel] [PATCH v10 03/25] qapi: More robust conditions for when labels are needed, Eric Blake, 2015/10/23
[Qemu-devel] [PATCH v10 06/25] vnc: Hoist allocation of VncBasicInfo to callers, Eric Blake, 2015/10/23
[Qemu-devel] [PATCH v10 08/25] qapi-types: Refactor base fields output, Eric Blake, 2015/10/23
[Qemu-devel] [PATCH v10 01/25] tests/qapi-schema: Test for reserved names, empty struct, Eric Blake, 2015/10/23