From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42012) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zi2dk-0001im-Et for qemu-devel@nongnu.org; Fri, 02 Oct 2015 11:52:57 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zi2dg-0007l7-AN for qemu-devel@nongnu.org; Fri, 02 Oct 2015 11:52:56 -0400 Received: from mx1.redhat.com ([209.132.183.28]:32801) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zi2dg-0007ko-2y for qemu-devel@nongnu.org; Fri, 02 Oct 2015 11:52:52 -0400 References: <1443760312-656-1-git-send-email-eblake@redhat.com> <1443760312-656-9-git-send-email-eblake@redhat.com> <87mvw15q11.fsf@blackfin.pond.sub.org> From: Eric Blake Message-ID: <560EA84D.2080502@redhat.com> Date: Fri, 2 Oct 2015 09:52:45 -0600 MIME-Version: 1.0 In-Reply-To: <87mvw15q11.fsf@blackfin.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="Ps74CwKlSxSuq2VN0q0waFMHcxN6NVPl3" Subject: Re: [Qemu-devel] [PATCH v6 08/12] qapi: Defer duplicate member checks to schema check() 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) --Ps74CwKlSxSuq2VN0q0waFMHcxN6NVPl3 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 10/02/2015 08:00 AM, Markus Armbruster wrote: > Eric Blake writes: >=20 >> With the previous commit, we have two different locations for >> detecting member name clashes - one at parse time, and another >> at QAPISchema*.check() time. Consolidate some of the checks >> into a single place, which is also in line with our TODO to >> eventually defer all of the parse time semantic checking into >> the newer schema code. The check_member_clash() function is >> no longer needed. >> >> The wording of several error messages has changed, but in many >> cases feels like an improvement rather than a regression. The >> recent change to avoid an assertion failure when a flat union >> branch name collides with its discriminator name is also >=20 > Which patch was that again? v7a 6/18, http://repo.or.cz/qemu/ericb.git/commit/ede42e8e [uggh - gnu.org archives are still quite lagging, or I'd point to a mail]= >=20 >> handled nicely by this code; but there is more work needed >> before we can detect all collisions in simple union branch >> names done by the old code. >> >> No change to generated code. >> >> Signed-off-by: Eric Blake >> >> @@ -611,11 +590,6 @@ def check_union(expr, expr_info): > # If the discriminator names an enum type, then all members > # of 'data' must also be members of the enum type, which in = turn > # must not collide with the discriminator name. >=20 > Should this comment be updated for the code removal below? Oh, right, revert both hunks of the earlier commit since we are now doing it in a less hacky manner. >> + v.check(schema, info, self.tag_member.type, vseen, >> + self.tag_name is not None) >> >> >> class QAPISchemaObjectTypeVariant(QAPISchemaObjectTypeMember): >> def __init__(self, name, typ, owner): >> QAPISchemaObjectTypeMember.__init__(self, name, typ, False, o= wner) >> >> - def check(self, schema, info, tag_type, seen): >> + def check(self, schema, info, tag_type, seen, flat): >> QAPISchemaObjectTypeMember.check(self, schema, info, [], seen= ) >> assert self.name in tag_type.values >> + if flat: >=20 > Any way to avoid the conditional? Not that I could come up with when I first tried, but it's been a while, so I can try again. I remember running into an assertion failure about nested ObjectType.check() calls if I did it unconditionally, since we allow the following: { 'struct': 'Fork', 'data': { '*fork': 'Union' } } { 'union': 'Union', 'data': { 'branch': 'Fork' } } We must NOT call self.type.check() on Fork (because we are not embedding the members of Fork into the simple union) - but then again, what we are really doing in the QAPISchema is visiting 'branch':':obj-Union-branch-wrapper', not 'Fork'. And then there's alternates, where branches aren't necessarily QAPISchemaObjectType in the first place. >=20 > I've tried hard to make simple unions mere sugar for flat ones. There > are a few special cases left where we need to distinguish the two, but > they're all marked TODO. >=20 > Can we have a brief comment explaing what we're checking here? We > generally don't have such comments in check() methods so far. Sort of > okay as long as they merely assert, but you're now starting to move > semantic analysis into them, which raises the bar. Yeah, I agree with adding more documentation about the checks. >=20 >> + self.type.check(schema) >=20 > Uh, careful. >=20 > It's always okay for a check() method to call the check() of a child in= > the abstract syntax tree, e.g. QAPISchemaObjectType.check() calling > m.check() for its members. A tree has no cycles. >=20 > Calling it on anything else requires a non-trivial correctness argument= =2E > Example: QAPISchemaObjectType.check() calls self.base.check(). Okay, > because no type may be a base of itself, and therefore a cycle would be= > an error. The code takes care to detect cycles. >=20 > I think the correctness argument here would be "no type contain itself > as variant member, and therefore a cycle would be an error." Do we > detect cycles? Not in this patch, but the assertion failure I ran into is the very reason I wrote a cycle detection patch next (11/12 in this series); maybe I should rebase that patch first? But you are correct that a flat union must not include itself as one of the branch types. I originally tried to test that in v5 3/46; your review convinced me it is no different than any other branch type that injects the same QMP members into the flat union, so I revamped the test in v6 to be a self-inheritance test instead, and then we dropped it after v7. (Search for "fork" in https://lists.gnu.org/archive/html/qemu-devel/2015-09/msg05733.html) >=20 >> + assert isinstance(self.type.members, list) >=20 > Implied by QAPISchemaObjectType.check()'s postcondition. Feels > redundant here, in particular since you iterate over it next anyway. Probably redundant, but I added it when I hit a cycle loop, to make sure that I wasn't still hitting a cycle (the 'if flat:' condition was the only way I could figure at the time to ensure I didn't hit this assertion= ). --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --Ps74CwKlSxSuq2VN0q0waFMHcxN6NVPl3 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/ iQEcBAEBCAAGBQJWDqhNAAoJEKeha0olJ0NqoYIH/RodLTeNyvV9lP5MQT4X0B+s JI4f9I3N7fvkoVyuPvjp7+gQEhSDTzlRCOBsSFulikxrB+NlIbUHSxBZ/ul4bNgY OWWfrhLYOPhaTOkyj3pOBQXP6IYHh1CuJBEt8onZ2whV46XM1LtklqJMma4l8844 QStGHVVOiJTLc5w+MSBLsulgsdNlbMUZnoT+q38Hzwo6lK+NugT+W9u3o8rTRNrm yD4XSL1Z4Lc3i/KH3Z4rY2vqi1Fiiw2i9kU2t+L/zzyqLZPBdSU+G543LdgCGOcd enxSGnytDGzMtiargi5IRhHjr95ctPPlcjNZGwDnJR0Dko00a5HKXFs/3vlab9o= =jqqB -----END PGP SIGNATURE----- --Ps74CwKlSxSuq2VN0q0waFMHcxN6NVPl3--