From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54615) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zi20Z-0000K5-4o for qemu-devel@nongnu.org; Fri, 02 Oct 2015 11:12:30 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zi20U-0001sO-Me for qemu-devel@nongnu.org; Fri, 02 Oct 2015 11:12:26 -0400 Received: from mx1.redhat.com ([209.132.183.28]:52668) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zi20U-0001oy-68 for qemu-devel@nongnu.org; Fri, 02 Oct 2015 11:12:22 -0400 References: <1443760312-656-1-git-send-email-eblake@redhat.com> <1443760312-656-8-git-send-email-eblake@redhat.com> <87bnch76hs.fsf@blackfin.pond.sub.org> From: Eric Blake Message-ID: <560E9ECE.9050904@redhat.com> Date: Fri, 2 Oct 2015 09:12:14 -0600 MIME-Version: 1.0 In-Reply-To: <87bnch76hs.fsf@blackfin.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="TjTLejksTQ6XoeNNExG2r3AFC48rW3sRF" Subject: Re: [Qemu-devel] [PATCH v6 07/12] qapi: Detect collisions in C member names List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: marcandre.lureau@redhat.com, qemu-devel@nongnu.org, ehabkost@redhat.com, Michael Roth This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --TjTLejksTQ6XoeNNExG2r3AFC48rW3sRF Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 10/02/2015 07:19 AM, Markus Armbruster wrote: > Eric Blake writes: >=20 >> Detect attempts to declare two object members that would result >> in the same C member name, by keying the 'seen' dictionary off >> of the C name rather than the qapi name. Doing this was made >> easier by adding a member.c_name() helper function. >=20 > This gains protection against colliding C names. It keeps protection > against colliding QMP names as long as QMP name collision implies C nam= e > collision. I think that's the case, but it's definitely worth spelling= > out in the code, and possibly in the commit message. >=20 >> As this is the first error raised within the QAPISchema*.check() >> methods, we also have to pass 'info' through the call stack, and >> fix the overall 'try' to display errors detected during the >> check() phase. >=20 > Could also be a separate patch, if the parts are easier to review. I'm still debating about that. >> + def check(self, schema, info, all_members, seen): >> + if self.c_name() in seen: >> + raise QAPIExprError(info, >> + "%s collides with %s" >> + % (self.describe(), >> + seen[self.c_name()].describe())) >> self.type =3D schema.lookup_type(self._type_name) >> assert self.type >> all_members.append(self) >> - seen[self.name] =3D self >> + seen[self.c_name()] =3D self >> + >> + def c_name(self): >> + return c_name(self.name) >=20 > Why wrap function c_name() in a method? Why not simply call the > function? 'self.c_name()' is shorter than 'c_name(self.name)'. And I already had long lines with that seen[self.c_name()].describe() pattern. >=20 > It's method in QAPISchemaEntity only because this lets us add special > cases in a neat way. True, but I _did_ mention in the commit message that I did it for less typing. But as to special cases, yes, I have one in mind (although I have not played with it yet). In v5 19/46 Simplify visiting of alternate types, I want to change alternates to have variants.tag_member =3D=3D None, and = the generated C code to use 'qtype_code type;' as the tag variable. In the v5 representation, it led to a lot of special-casing (many uses of QAPISchemaObjectTypeVariants became more complex because tag_member was not always defined). But now that I've been working on these front-end patches, my idea was to do something like: class QAPISchemaVariantTag(QAPISchemaObjectTypeMember): def __init__(self, info): QAPISchemaObjectTypeMember(self, 'type', None, False) def c_type(self): return 'qtype_code' and then, we WOULD need member.c_type() rather than member.type.c_type() (at which point having member.c_name() to match member.c_type() makes more sense). >> @@ -1028,23 +1037,24 @@ class QAPISchemaObjectTypeVariants(object): >> self.tag_member =3D tag_member >> self.variants =3D variants >> >> - def check(self, schema, members, seen): >> + def check(self, schema, info, members, seen): >> if self.tag_name: >> - self.tag_member =3D seen[self.tag_name] >> + self.tag_member =3D seen[c_name(self.tag_name)] >> + assert self.tag_name =3D=3D self.tag_member.name >=20 > Assertion should hold before the patch, but it's kind of trivial then. My worry here was that if I have: { 'enum': 'Enum', 'data': ['a'] } { 'base': 'Base', 'data': { 'b-c': 'Enum' } } { 'union': 'Flat', 'base': 'Base', 'discriminator': 'b_c', 'data': { 'a': 'Struct...' } } the old ad hoc parser rejects 'b_c' as not being a member of Enum (bad discriminator). But once we convert from the old parser to the check() method (basically, anywhere the check() methods now have an assert will become if statements that raise an exception), we need to make sure that we don't get confused by the fact that seen[c_name('b_c')] maps to the same value as seen[c_name('b-c')], so that we are still flagging the flat union as invalid. >=20 > I'm fine with not splitting this patch. I'd be also fine with splittin= g > it up. Your choice. I'm still thinking about it; may depend on how much other refactoring I d= o. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --TjTLejksTQ6XoeNNExG2r3AFC48rW3sRF 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/ iQEcBAEBCAAGBQJWDp7OAAoJEKeha0olJ0NqQ+QIAJdqChvFslapPpMewOtYOKrs DhcOjl7u7qjAxybix3t0d6qAeEdsI+5AfrVgWRQbGc3iLCArrUsvF97bR6/hsgsz K4pxqahGvUCFplxDpcriJlO0/gBUoCkNvFGlOOqmvdBqQgcEy5evnXfKnR9XwHkW REU8+l1Q+96KDWUA2OeGy75SZ/sR+XNO7liOs6mB1jwHrSmM4WCRkcdixPBvITwd jsBdMzr4ArlR6dl6Xt/ONz0jSiSVKa2TvI+k4KOx5szVQq38Hyv0m7bmHHSecFkO 1m/7PzmwPQir2EX9RPxDbbLxAoc+kU3RJwxhqbhk1rnslxFAf71kSCdibuvrHz0= =rWUB -----END PGP SIGNATURE----- --TjTLejksTQ6XoeNNExG2r3AFC48rW3sRF--