From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38059) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XWSV9-0000te-OM for qemu-devel@nongnu.org; Tue, 23 Sep 2014 11:59:44 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XWSV4-00054l-Ds for qemu-devel@nongnu.org; Tue, 23 Sep 2014 11:59:39 -0400 Received: from mx1.redhat.com ([209.132.183.28]:43008) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XWSV4-000542-6Y for qemu-devel@nongnu.org; Tue, 23 Sep 2014 11:59:34 -0400 Message-ID: <542198DE.4030504@redhat.com> Date: Tue, 23 Sep 2014 09:59:26 -0600 From: Eric Blake MIME-Version: 1.0 References: <1411165504-18198-1-git-send-email-eblake@redhat.com> <1411165504-18198-7-git-send-email-eblake@redhat.com> <874mvyiffi.fsf@blackfin.pond.sub.org> In-Reply-To: <874mvyiffi.fsf@blackfin.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="9iTiLabsxewJ5HwSUAUmhr6VhkTIuNLrl" Subject: Re: [Qemu-devel] [PATCH v4 06/19] qapi: Better error messages for bad enums List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: Fam Zheng , qemu-devel@nongnu.org, wenchaoqemu@gmail.com, Luiz Capitulino This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --9iTiLabsxewJ5HwSUAUmhr6VhkTIuNLrl Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 09/23/2014 08:23 AM, Markus Armbruster wrote: > Eric Blake writes: >=20 >> The previous commit demonstrated that the generator had several >> flaws with less-than-perfect enums: >> - an enum that listed the same string twice (or two variant >> strings that map to the same C enum) ended up generating an >> invalid C enum >> - because the generator adds a _MAX terminator to each enum, >> the use of an enum member 'max' can also cause this clash >> - if an enum omits 'data', the generator left a python stack >> trace rather than a graceful message >> - an enum used a non-array 'data' was silently accepted by >> the parser >> - an enum that used non-string members in the 'data' member >> was silently accepted by the parser >> >> Add check_enum to cover these situations, and update testcases >> to match. While valid .json files won't trigger any of these >> cases, we might as well be nicer to developers that make a typo >> while trying to add new QAPI code. >> >> Signed-off-by: Eric Blake >> --- >> --- a/tests/qapi-schema/enum-clash-member.err >> +++ b/tests/qapi-schema/enum-clash-member.err >> @@ -0,0 +1 @@ >> +tests/qapi-schema/enum-clash-member.json:2: enum 'MyEnum' member 'ONE= ' clashes with 'one' >> diff --git a/tests/qapi-schema/enum-clash-member.exit b/tests/qapi-sch= ema/enum-clash-member.exit >> index 573541a..d00491f 100644 >> --- a/tests/qapi-schema/enum-clash-member.exit >> +++ b/tests/qapi-schema/enum-clash-member.exit >> @@ -1 +1 @@ >> -0 >> +1 >> diff --git a/tests/qapi-schema/enum-clash-member.json b/tests/qapi-sch= ema/enum-clash-member.json >> index cb4b428..c668ff5 100644 >> --- a/tests/qapi-schema/enum-clash-member.json >> +++ b/tests/qapi-schema/enum-clash-member.json >> @@ -1,2 +1,2 @@ >> -# FIXME: we should reject enums where members will clash in C switch >> +# we reject enums where members will clash in C switch >> { 'enum': 'MyEnum', 'data': [ 'one', 'ONE' ] } >=20 > Actually in PATCH 05 already: "clash in C switch". I guess you mean we= > generate an enum with clashing enumeration constants. In the test case= , > we'd generate MY_ENUM_ONE (I think) both for 'one' and 'ONE'. Correct?= Correct; the generated C code would include an invalid switch statement with two repetitions of the same spelling of a case label. In patch 5, the test case demonstrates that the parser was silently accepting code that resulted in a clash in the generated C code; this patch updates both qapi.py to make it a hard error, and the testsuite to change from (accidental) pass to (intentional) error detection, so that we no longer have to worry about the issue in the generated C code. The same principle applies throughout my series - I first introduced new tests in isolation for existing pre-patch behavior, with FIXME comments where the tests expose bogus behavior, then in later patches fix the parser to reject bogus behavior and update the test to match that it now covers the new error message. >> +++ b/tests/qapi-schema/enum-dict-member.err >> @@ -0,0 +1 @@ >> +tests/qapi-schema/enum-dict-member.json:2: enum 'MyEnum' member 'Orde= redDict([('value', 'str')])' is not a string >=20 > Error message is in terms of implementation instead of source. Since > this is merely a development tool, it'll do. Same elsewhere, and I'm > not going to flag it. Precedents in master quite possible. Yeah, I couldn't figure a way to get back at the original text typed by the user. I'm open to suggestions, but I'm (obviously) okay with leaving it as is. >> +++ b/tests/qapi-schema/enum-max-member.json >> @@ -1,3 +1,3 @@ >> -# FIXME: we should either reject user-supplied 'max', or munge the im= plicit >> -# max value we generate at the end of an array >> +# we reject user-supplied 'max' for clashing with implicit enum end >> +# FIXME: should we instead munge the the implicit value to avoid the = clash? >=20 > Or pick an identifier for the max member so that it cannot clash with > the ones we generate for the user's members. >=20 > The generator picks identifiers pretty much thoughtlessly in general. > If something clashes, the C compiler spits it out, and you get to fiddl= e > with the .json. Well, hopefully by hoisting the error message away from C compilation time (late) to qapi.py runtime (early), we have made it easier for anyone actually needing the name 'max' to identify what still needs fixing. At any rate, I'm always a fan of erroring out as early as possible, rather than waiting for an obscure crash later on in the C compilation :) >=20 > [...] >=20 > Reviewed-by: Markus Armbruster Thanks. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --9iTiLabsxewJ5HwSUAUmhr6VhkTIuNLrl Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg iQEcBAEBCAAGBQJUIZjeAAoJEKeha0olJ0NqqwAH/3D38P1bx+O9E3IJFqf7PVoh d5EubmtZ272d62wlaVY9JcwnqGcpL9HUImf1sZfJ7rf2SfBEyL1cImEsrJBeN4Ay TpY6KtSxwBlAd0mH0MBbWtyMdHh3zXEbh8LLnLix2eCEYaD0MEHaRSzgteZJUU+8 AuFbejrx2H+ApOJxfZHdOYQTnwOVXWdkw4yidWyCGgHbuauL00UTbefQK7TjxG7A KtiaVd1tZ9JD4wSSI1f3BL5edXSnaW/FPj8QEO6bVdPLhoPSoSZM/SvASiXkOSQq RKM+dMLAX9NgDF8G5adRXGcNQ9gYgYMmjwMjo7cFFrBVkpXT37N8ccR8V9s1FmI= =2XBz -----END PGP SIGNATURE----- --9iTiLabsxewJ5HwSUAUmhr6VhkTIuNLrl--