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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3289 bytes --]

On 10/23/2015 07:05 AM, Markus Armbruster wrote:
> Eric Blake <eblake@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 <eblake@redhat.com>
>>

>> +++ 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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]