On 10/23/2015 06:33 AM, Markus Armbruster wrote: > Eric Blake <ebl...@redhat.com> writes: > >> We are failing to detect a collision between a QMP member and >> the implicit 'has_*' flag for another optional QMP member. The >> easiest fix would be for a future patch to reserve the entire >> "has[-_]" namespace for member names (the collision is also >> possible for branch names within flat unions, but only as long >> as branch names can collide with QMP names; since future >> patches are about to remove that, it is not worth testing here). > > This is args-has-clash.json. > >> A similar collision exists between a QMP member where c_name() >> munges what might otherwise be a reserved name to start with >> 'q_', and another member explicitly starts with "q[-_]". Again, >> the easiest task for a future patch will be reserving the entire > > s/task/solution/ ? >
Sounds better. >> namespace, but here for commands as well as members. > > This is reserved-member.json. And reserve-commands.json > >> Our current representation of qapi arrays is done by appending >> 'List' to the element name; but we are not preventing the >> creation of a non-array type with the same name. > > This is struct-name-list.json. > >> Finally, our testsuite does not have any compilation coverage >> of struct inheritance with empty qapi structs. > > This is qapi-schema-test.json. > > Left undescribed: reserved-commands.json :) No, the 'q_' paragraph covered both. But yes, mentioning the actual test names in the commit body can't hurt. > >> Add tests to cover these issues. >> >> On the other hand, there is currently no technical reason to >> forbid type name patterns from member names, or member name >> patterns from types, since the two are not in the same namespace >> in C and won't collide (but it's not worth adding positive tests >> of these corner cases, especially while there is other churn >> pending in patches that rearrange which collisions actually >> happen). >> >> Signed-off-by: Eric Blake <ebl...@redhat.com> >> >> --- >> +++ b/tests/qapi-schema/reserved-command.json >> @@ -0,0 +1,7 @@ >> +# 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. >> +{ 'command': 'unix' } >> +{ 'command': 'q-unix' } > > Note that mangling 'unix' to 'q-unix' is pretty pointless for command > names, because their C name occurs only in identifiers qmp_CNAME() and > qmp_marshal_CNAME(). > Probably true, but it is the current implementation, and we DO have a compile time collision until (unless?) we bother to fix it. >> +++ b/tests/qapi-schema/reserved-member.json >> @@ -0,0 +1,6 @@ >> +# 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. >> +{ 'struct': 'Foo', 'data': { 'unix':'int', 'q-unix':'bool' } } > > Unlike command names, member names actually occur by themselves, so some > name mangling is required. > > A less ham-fisted approach would mangle *complete* identifiers, i.e. > c_name('qmp_' + name) instead of 'qmp_' + c_name(name). > > Please feel free to stick to ham-fisted for now. We need to make > progress flushing the queue. Indeed - cleaning up command name mangling can come at a later date, and maybe at that point we can decide 'q_' reservations for command names doesn't add any power, and relax it at that point. >> +++ b/tests/qapi-schema/struct-name-list.json >> @@ -0,0 +1,5 @@ >> +# Potential C name collision >> +# FIXME - This parses and compiles on its own, but prevents the user from >> +# creating a type named 'Foo' and using ['Foo'] for an array. We should >> +# reject the use of any non-array type names ending in 'List'. >> +{ 'struct': 'FooList', 'data': { 's': 'str' } } > > "Non-array type name" makes no sense when talking about the QAPI schema, > because arrays don't have names there. Suggestions? Maybe s/non-array//? The wording changes in 2/25 when the test starts working. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature