From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54319) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZwXgI-0001uJ-Dv for qemu-devel@nongnu.org; Wed, 11 Nov 2015 10:53:12 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZwXea-0002Hb-LE for qemu-devel@nongnu.org; Wed, 11 Nov 2015 10:51:30 -0500 Received: from mx1.redhat.com ([209.132.183.28]:49836) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZwXea-0002HW-D4 for qemu-devel@nongnu.org; Wed, 11 Nov 2015 10:49:44 -0500 References: <1447224690-9743-1-git-send-email-eblake@redhat.com> <1447224690-9743-12-git-send-email-eblake@redhat.com> <878u64d54n.fsf@blackfin.pond.sub.org> From: Eric Blake Message-ID: <56436396.60601@redhat.com> Date: Wed, 11 Nov 2015 08:49:42 -0700 MIME-Version: 1.0 In-Reply-To: <878u64d54n.fsf@blackfin.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="o2RPXaTODEbP3c3I2MeaLqHsbssPVgJks" Subject: Re: [Qemu-devel] [PATCH v11 11/28] qapi: Check for qapi collisions of flat union branches 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) --o2RPXaTODEbP3c3I2MeaLqHsbssPVgJks Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 11/11/2015 06:42 AM, Markus Armbruster wrote: > Eric Blake writes: > Your patch actually does two things: >=20 > 1. Extend the "object type cannot contain itself" check to cover its > variant types in addition to base type. This is the two-liner chang= e > to QAPISchemaObjectTypeVariants.check(). >=20 > 2. Extend the "object type cannot contain members with clashing names" > to cover variant members. >=20 > They're related: you need the former's side effect to compute .members > to be able to do the latter. >=20 > Doing it in two separate patches *might* make explaining it in the > commit message easier. See below. Interesting observation. I can split if you still think it is worth it; otherwise, I think your wording makes both changes obvious: > Let me have a stab at the commit message of the *unsplit* patch, use as= > you see fit: >=20 > qapi: Check for collisions involving variant members >=20 > Right now, our ad hoc parser ensures that we cannot have a flat union > that introduces any members that would clash with non-variant members > inherited from the union's base type (see flat-union-clash-member.json)= =2E > We want QAPISchemaObjectType.check() to make the same check, so we can > later reduce some of the ad hoc checks. >=20 > We already have a map 'seen' of all non-variant members. We still need= > to check for collisions between each variant type's members and the > non-variant ones. >=20 > To know the variant type's members, we need to call > variant.type.check(). This also detects when a type contains itself in= > a variant, exactly like the existing base.check() detects when a type > contains itself as a base. If I split, this paragraph would go in the first patch (adding the =2Echeck() for cycle detection). >=20 > Slight complication: an alternate's variant can have arbitrary type, bu= t > only an object type's check() may be called outside QAPISchema.check().= > We could either skip the call for variants of alternates, or skip it fo= r > non-object types. Do the latter, because it's easier. I actually have some churn on this; later in 22/28, I'm stuck making Variants.check() do something for unions only, and had to reintroduce an 'if seen:' conditional, at which point I moved the v.type.check() back to unions only. I guess your review of that patch may determine whether I minimize churn back here, or add a 'for now' phrase to the commit message, or just not worry about it. >=20 > Then we can call each variant member's check_clash() with the > appropriate 'seen' map. Since members of different variants can't > clash, We have to clone a fresh seen for each variant. Wrap this in th= e > new helper method QAPISchemaObjectTypeVariants.check_clash(). >=20 > Note that cloning 'seen' inside Variants.check_clash() > resembles the one we just removed from Variants.check(); the > difference here is that we are now checking for clashes > among the qapi members of the variant type, rather than for > a single clash with the variant tag name itself. >=20 > Note that by construction collisions can't actually happen for simple > unions: each variant's type is a wrapper with a single member 'data', > which will never collide with the only non-variant member 'type'. >=20 > For alternates, there's nothing for a variant object type's members to > clash with, and therefore no need to call variants.check_clash(). >=20 > No change to generated code. >=20 --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --o2RPXaTODEbP3c3I2MeaLqHsbssPVgJks 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/ iQEcBAEBCAAGBQJWQ2OWAAoJEKeha0olJ0NqX8kIAJGDKqj7P8qKcdMEmTLZCjBw IkxtH03mf1T/sojBMcr720tP5hVT+GSTN5OYBlXCd5vGMAWp78wXaJzVRM2QFQwx HEq2e/24mmzLv1ulc7QC6QKk+lGsvENPq1g63myH5HzVL6DKvlw1HiYwFUn43SpQ IHgH/+AeTifxTHa0VI7duPP1IWUpznvZ+A6bPAfoKvh4OkCnfHOh8gYO74Ajd6R0 2Mi7pRK+H6ZIQug98WOJkXoVy3yrqaOim8zYq28PVOqcPstYRg8ijadFN87P+lBj p1O+PthYGuBTEK2pAHdqkGrvEy2/rSqM2Em3Q7IKUHYW2HLBGKghbFBX04dtv0Y= =20Cf -----END PGP SIGNATURE----- --o2RPXaTODEbP3c3I2MeaLqHsbssPVgJks--