From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58844) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zpbdh-0004hi-FJ for qemu-devel@nongnu.org; Fri, 23 Oct 2015 08:40:15 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zpbde-0007v2-2o for qemu-devel@nongnu.org; Fri, 23 Oct 2015 08:40:09 -0400 Received: from mx1.redhat.com ([209.132.183.28]:53711) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zpbdd-0007uV-Rk for qemu-devel@nongnu.org; Fri, 23 Oct 2015 08:40:06 -0400 References: <1445576998-2921-1-git-send-email-eblake@redhat.com> <1445576998-2921-2-git-send-email-eblake@redhat.com> <87twphhih6.fsf@blackfin.pond.sub.org> From: Eric Blake Message-ID: <562A2A9F.60004@redhat.com> Date: Fri, 23 Oct 2015 06:39:59 -0600 MIME-Version: 1.0 In-Reply-To: <87twphhih6.fsf@blackfin.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="elHqqkPhais38XbtWgQNE1a0kI6tDAjva" Subject: Re: [Qemu-devel] [PATCH v10 01/25] tests/qapi-schema: Test for reserved names, empty struct List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: qemu-devel@nongnu.org, Michael Roth This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --elHqqkPhais38XbtWgQNE1a0kI6tDAjva Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 10/23/2015 06:33 AM, Markus Armbruster wrote: > Eric Blake writes: >=20 >> 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). >=20 > This is args-has-clash.json. >=20 >> 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 >=20 > s/task/solution/ ? >=20 Sounds better. >> namespace, but here for commands as well as members. >=20 > This is reserved-member.json. And reserve-commands.json >=20 >> 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. >=20 > This is struct-name-list.json. >=20 >> Finally, our testsuite does not have any compilation coverage >> of struct inheritance with empty qapi structs. >=20 > This is qapi-schema-test.json. >=20 > 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. >=20 >> 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 >> >> --- >> +++ 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 d= eclare >> +# two 'qmp_q_unix' functions (one for 'q-unix', the other because c_n= ame() >> +# munges 'unix' to 'q_unix' to avoid reserved word collisions). We sh= ould >> +# reject attempts to explicitly use 'q_' names, to reserve it for qap= i. >> +{ 'command': 'unix' } >> +{ 'command': 'q-unix' } >=20 > 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(). >=20 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 d= eclare >> +# two 'q_unix' members (one for 'q-unix', the other because c_name() >> +# munges 'unix' to 'q_unix' to avoid reserved word collisions). We sh= ould >> +# reject attempts to explicitly use 'q_' names, to reserve it for qap= i. >> +{ 'struct': 'Foo', 'data': { 'unix':'int', 'q-unix':'bool' } } >=20 > Unlike command names, member names actually occur by themselves, so som= e > name mangling is required. >=20 > A less ham-fisted approach would mangle *complete* identifiers, i.e. > c_name('qmp_' + name) instead of 'qmp_' + c_name(name). >=20 > 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 sho= uld >> +# reject the use of any non-array type names ending in 'List'. >> +{ 'struct': 'FooList', 'data': { 's': 'str' } } >=20 > "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. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --elHqqkPhais38XbtWgQNE1a0kI6tDAjva Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJWKiqfAAoJEKeha0olJ0NqoM0H/iNcokVp3hESCTkyURMnLvqQ gxf8GpNcr/iDr22PqaCmWtuM+5ytsuUrd1qpJi+3KbzE+YjGAcA34tr8+4wWGmeI dDXScTy9JETt0M20cwQc49eQ8Yo5RTXpNnFSI0SbkbDmaRHbBlGd5cJO+t0ft4gw hwoy/110wFoccQ+xo3cAfTkfigxHEzvzPUcpBE6yolY3wXGPFsPoZq+fGkF3BA6t 797BxgfJusZznKPOjEQQzLKkGK1nxNCc8E8dUCTOTQyoc9J1vSglau+HAoCLuPv1 JPvBJeKkJFjbly/K68sRVIFPzo/jY8sUldRjK79qHkHM/5JZJHi9jyjKDTFb33I= =Wzen -----END PGP SIGNATURE----- --elHqqkPhais38XbtWgQNE1a0kI6tDAjva--