From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49040) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a3nXu-0001o8-7b for qemu-devel@nongnu.org; Tue, 01 Dec 2015 11:12:51 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a3nXp-0004kv-Pu for qemu-devel@nongnu.org; Tue, 01 Dec 2015 11:12:50 -0500 Received: from mx1.redhat.com ([209.132.183.28]:46693) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a3nXp-0004kq-JT for qemu-devel@nongnu.org; Tue, 01 Dec 2015 11:12:45 -0500 References: <1448040300-968-1-git-send-email-eblake@redhat.com> <1448040300-968-13-git-send-email-eblake@redhat.com> <877fl3aicq.fsf@blackfin.pond.sub.org> From: Eric Blake Message-ID: <565DC6F7.5040801@redhat.com> Date: Tue, 1 Dec 2015 09:12:39 -0700 MIME-Version: 1.0 In-Reply-To: <877fl3aicq.fsf@blackfin.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="D6Up4ocHK9aWfcmKMW2VHqdI50N7iHh6B" Subject: Re: [Qemu-devel] [PATCH v13 12/14] qapi: Enforce (or whitelist) case conventions on qapi members 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) --D6Up4ocHK9aWfcmKMW2VHqdI50N7iHh6B Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 11/27/2015 02:42 AM, Markus Armbruster wrote: > Eric Blake writes: >=20 >> We document that members of enums and objects should be >> 'lower-case', although we were not enforcing it. We have to >> whitelist a few pre-existing entities that violate the norms. >> Add three new tests to expose the new error message, each of >> which first uses the whitelisted name 'UuidInfo' to prove the >> whitelist works, then triggers the failure. >> >> Note that by adding this check, we have effectively forbidden >> an entity with a case-insensitive clash of member names, for >> any entity that is not on the whitelist (although there is >> still the possibility to clash via '-' vs. '_'). >> >> Signed-off-by: Eric Blake > [...] >> @@ -1039,6 +1054,10 @@ class QAPISchemaMember(object): >> >> def check_clash(self, info, seen): >> cname =3D c_name(self.name) >> + if cname.lower() !=3D cname and info['name'] not in case_whit= elist: >> + raise QAPIExprError(info, >> + "Member '%s' of '%s' should use lower= case" >> + % (self.name, info['name'])) >> if cname in seen: >> raise QAPIExprError(info, >> "%s collides with %s" >=20 > As far as I can tell, this is the only use of info['name'] in this > series. Yes, although I may find more uses for it later. >=20 > Can you give an example where info['name'] !=3D self.owner? Sure; this triggers lots of debug lines before crashing[1]: diff --git i/scripts/qapi.py w/scripts/qapi.py index 6a77db4..ec59682 100644 --- i/scripts/qapi.py +++ w/scripts/qapi.py @@ -1054,6 +1054,8 @@ class QAPISchemaMember(object): def check_clash(self, info, seen): cname =3D c_name(self.name) + if info['name'] !=3D self.owner: + print ' ** checking differs in %s, owner is %s' % (info['name'], self.owner) if cname.lower() !=3D cname and info['name'] not in case_whiteli= st: raise QAPIExprError(info, "Member '%s' of '%s' should use lowercas= e" The very first one is: ** checking differs in block_passwd, owner is :obj-block_passwd-arg Remember, QAPISchemaMember.owner is the innermost (possibly-implicit) type that owns the member, while info['name'] is the name of the top-level entity that encloses the member. So the two are not always equal. member._pretty_owner() converts from an implicit struct name back to the top-level entity, but not directly (it is a human-readable phrase, not the plain entity name). Furthermore, look at CpuInfo's member 'CPU': there, we have two call paths (one with info['name'] =3D=3D 'CpuInfo', the other with it as 'CpuInfoBase') but both call paths would see only self.owner =3D=3D 'CpuInfoBase'. The whitelist covers both struct names. Perhaps whitelisting only 'self.owner' names would be sufficient; but then the whitelist would have to use implicit type names rather than entity names from the .json file. [1] The crash is "TypeError: 'NoneType' object has no attribute '__getitem__'" at the point where QType is being tested. Normally, QType is well-formed, so even though it is a builtin type and therefore has info =3D=3D None, the 'cname.lower() !=3D cname' test never fails and= we short-circuit past an attempt to dereference None; but not so with my temporary print hack. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --D6Up4ocHK9aWfcmKMW2VHqdI50N7iHh6B 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/ iQEcBAEBCAAGBQJWXcb3AAoJEKeha0olJ0NqzUYH/1VZx4spcX5IErEBz+e1IKmC rJEKWgPdiYsXmU8GfBt2b4HoyuIZtKjjxeTft4xHheSF4UuxrKC3/2Ij4s8aAydB 1+24YVDrJ7ZLJ8UDc10wGBQVTDfELIpr89s87tTk8NXVxsZh4z8hntakkvE+BLik fmdgbKJytcrCQGRyxjVcURdr2Hdt+YJ+4OGeXmykk6Ki1kN6Tczm+hLHA1Iji3JN 1hU12hNX2U43W9M4Y7IBz3Ght/+U7lDFONOhMIdRzY0hDhWEc/uEt5m37aTGbXqZ DNHJMkPxqMMJ3Dt2DxBB9Uer2NbANUO4SUyhdKPJ1jo21uwccHCL1uvRZcfoHnk= =5bPy -----END PGP SIGNATURE----- --D6Up4ocHK9aWfcmKMW2VHqdI50N7iHh6B--