From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40735) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZKnEZ-0003la-1v for qemu-devel@nongnu.org; Thu, 30 Jul 2015 08:46:52 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZKnEU-00064F-4H for qemu-devel@nongnu.org; Thu, 30 Jul 2015 08:46:51 -0400 Received: from mx1.redhat.com ([209.132.183.28]:38428) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZKnET-00063k-TL for qemu-devel@nongnu.org; Thu, 30 Jul 2015 08:46:46 -0400 References: <1435782155-31412-1-git-send-email-armbru@redhat.com> <1435782155-31412-27-git-send-email-armbru@redhat.com> <55B95DAA.3040302@redhat.com> <87oaiu5eox.fsf@blackfin.pond.sub.org> From: Eric Blake Message-ID: <55BA1CAE.8030405@redhat.com> Date: Thu, 30 Jul 2015 06:46:38 -0600 MIME-Version: 1.0 In-Reply-To: <87oaiu5eox.fsf@blackfin.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="xuQgiK9PPxFj10v66e6BrxFX2VK4iuSrJ" Subject: Re: [Qemu-devel] [PATCH RFC v2 26/47] qapi-types: Convert to QAPISchemaVisitor, fixing flat unions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: kwolf@redhat.com, berto@igalia.com, qemu-devel@nongnu.org, mdroth@linux.vnet.ibm.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --xuQgiK9PPxFj10v66e6BrxFX2VK4iuSrJ Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 07/30/2015 12:42 AM, Markus Armbruster wrote: >> But what happens if the 0th branch is mapped to a different parser, as= >> would be the case if one of the alternate's branches is 'number'? In >> particular, qmp_input_type_number() accepts BOTH QFloat and QInt types= =2E >> So, if we have this qapi: >> { 'alternate': 'Foo', 'data': { 'a': 'str', 'b': 'number' } } >> but pass in an integer, visit_get_next_type() will see a qtype of QInt= , >> but Foo_qtypes[QTYPE_QINT] will be 0 (due to default initialization) a= nd >> we will wrongly try to visit the 0th branch (FOO_KIND_A) and fail (the= >> string parser doesn't like ints) even though the parse should succeed = by >> using the FOO_KIND_B branch. >=20 > Yup, bug. And it's an order-dependent bug - merely declaring 'b' first makes it appear to work correctly. >=20 >> Interestingly, this means that if we ever write an alternate type that= >> accepts both 'int' and 'number' (we have not attempted that so far), >> then the number branch will only be taken for inputs that don't also >> look like ints (normally, 'number' accepts anything numeric). Maybe th= at >> means we should document and enforce that 'number' and 'int' cannot be= >> mixed in the same alternate? >=20 > Even if we outlaw mixing the two, I'm afraid we still have this bug: an= > alternate with a 'number' member rejects input that gets parsed as > QTYPE_QINT. >=20 > Let's simply make alternates behave sanely: >=20 > alternate has case selected for > 'int' 'number' QTYPE_QINT QTYPE_QFLOAT > no no error error > no yes 'number' 'number' > yes no 'int' error > yes yes 'int' 'number' Works for me. >=20 > + 1 works, because the element type is int, not BlockdevRefKind. It's > int so it can serve as argument for visit_get_next_type()'s parameter > const int *qtypes. >=20 > The + 1, - 1 business could be mildly confusing. We could set all > unused elements to -1 instead: Or, we could ditch the qtypes lookup altogether, and merely create the alternate enum as a non-consecutive QTYPE mapping, for one less level of indirection, as in: typedef enum BlockdevRefKind { BLOCKDEV_REF_DEFINITION =3D QTYPE_QOBJECT, BLOCKDEV_REF_REFERENCE =3D QTYPE_QSTRING, }; then rewrite visit_get_next_type() to directly return the qtype, as well as rewrite the generated switch statement in visit_type_BlockdevRef() to directly inspect the qtypes it cares about. In fact, that's the approach I'm currently playing with. > Add test eight test cases from my table above, then fix the generator t= o > make them pass. I hope to post an RFC followup patch along those lines later today. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --xuQgiK9PPxFj10v66e6BrxFX2VK4iuSrJ 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/ iQEcBAEBCAAGBQJVuhyuAAoJEKeha0olJ0NqWwMH/30mpGFipifsI6paLcHqeTOE fvG1mk3q+Kcy7F2DS7CmVT99bc32ZldKZYbhBGa3ulCMvk2QG+X5JlOw2QBdVNpP PULyF4hCypVNkorWu8ZLZvsxMRyNIy9nP1tuqk1VFoT5Q0jGve70/iLLRfMDCuyX 3GIYylUb32JMLQYcn/HyFOPEsHgtIT1uuqk7DrzAffkhvy1LTQmqxVdtYSgmoI3N GQ8W1vgGMK8qtw6vzT+bOkqMBX7cCkFb3SOmabe38FjlxYFOETrA/cbpbUpgB5yA BsRG/oWAJg0sWR4izi0EIFxq7YEA6CGozxb71iUvS2GGtR7a7P/k+DwGcmUhOw4= =eAC9 -----END PGP SIGNATURE----- --xuQgiK9PPxFj10v66e6BrxFX2VK4iuSrJ--