From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44335) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bLBkm-0001Gf-3X for qemu-devel@nongnu.org; Thu, 07 Jul 2016 12:02:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bLBkh-0008Jd-UP for qemu-devel@nongnu.org; Thu, 07 Jul 2016 12:02:15 -0400 Received: from mx1.redhat.com ([209.132.183.28]:41424) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bLBkh-0008JR-M1 for qemu-devel@nongnu.org; Thu, 07 Jul 2016 12:02:11 -0400 References: <1467514729-29366-1-git-send-email-eblake@redhat.com> <1467514729-29366-9-git-send-email-eblake@redhat.com> <87vb0hpvoj.fsf@dusky.pond.sub.org> From: Eric Blake Message-ID: <577E7CFF.4000408@redhat.com> Date: Thu, 7 Jul 2016 10:02:07 -0600 MIME-Version: 1.0 In-Reply-To: <87vb0hpvoj.fsf@dusky.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="lVH0vvpn8tsRuVsMoJhStfKjEt02J4pqi" Subject: Re: [Qemu-devel] [PATCH v8 08/16] qapi: Implement boxed types for commands/events 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) --lVH0vvpn8tsRuVsMoJhStfKjEt02J4pqi From: Eric Blake To: Markus Armbruster Cc: qemu-devel@nongnu.org, Michael Roth Message-ID: <577E7CFF.4000408@redhat.com> Subject: Re: [Qemu-devel] [PATCH v8 08/16] qapi: Implement boxed types for commands/events References: <1467514729-29366-1-git-send-email-eblake@redhat.com> <1467514729-29366-9-git-send-email-eblake@redhat.com> <87vb0hpvoj.fsf@dusky.pond.sub.org> In-Reply-To: <87vb0hpvoj.fsf@dusky.pond.sub.org> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 07/07/2016 04:52 AM, Markus Armbruster wrote: > Eric Blake writes: >=20 >> Turn on the ability to pass command and event arguments in >> a single boxed parameter, which must name a non-empty type >> (although the type can be a struct with all optional members). >> For structs, it makes it possible to pass a single qapi type >> instead of a breakout of all struct members (useful if the >> arguments are already in a struct or if the number of members >> is large); for other complex types, it is now possible to use >> a union or alternate as the data for a command or event. >> >> The empty type may be technically feasible if needed down the >> road, but it's easier to forbid it now and relax things to allow >> it later, than it is to allow it now and have to special case >> how the generated 'q_empty' type is handled (see commit 7ce106a9 >> for reasons why nothing is generated for the empty type). An >> alternate type is never considered empty. >> >> Generated code is unchanged, as long as no client uses the >> new feature. >> >> Signed-off-by: Eric Blake >> >> @@ -1180,9 +1195,18 @@ class QAPISchemaCommand(QAPISchemaEntity): >> def check(self, schema): >> if self._arg_type_name: >> self.arg_type =3D schema.lookup_type(self._arg_type_name)= >> - assert isinstance(self.arg_type, QAPISchemaObjectType) >> - assert not self.arg_type.variants # not implemented >> - assert not self.box # not implemented >> + assert (isinstance(self.arg_type, QAPISchemaObjectType) o= r >> + isinstance(self.arg_type, QAPISchemaAlternateType= )) >> + self.arg_type.check(schema) >=20 > qapi.py recurses .check() only when necessary, because undisciplined > recursion can easily become cyclic. >=20 > I think you need self.arg_type.check() here so you can > self.arg_type.is_empty() below. Correct? Correct. And should not be unbounded - a command depends on a type, but no type depends on a command, so this does not introduce new recursion. >=20 >> + if self.box: >> + if self.arg_type.is_empty(): >> + raise QAPIExprError(self.info, >> + "Cannot use 'box' with empty = type") >> + else: >> + assert not self.arg_type.variants >=20 > Lost: assert isinstance(self.arg_type, QAPISchemaObjectType), or the > equivalent assert not isinstance(self.arg_type, QAPISchemaAlternateType= ). Or rather, implicitly hidden. Only QAPISchemaObjectType and QAPISchemaAlternateType have a .is_empty() or .variants, so if any other type is passed, python will complain about a missing attribute (which is just as effective as the assert() used to be). >=20 >> + elif self.box: >> + raise QAPIExprError(self.info, >> + "Use of 'box' requires 'data'") >> if self._ret_type_name: >> self.ret_type =3D schema.lookup_type(self._ret_type_name)= >> assert isinstance(self.ret_type, QAPISchemaType) >> @@ -1204,9 +1228,18 @@ class QAPISchemaEvent(QAPISchemaEntity): >> def check(self, schema): >> if self._arg_type_name: >> self.arg_type =3D schema.lookup_type(self._arg_type_name)= >> - assert isinstance(self.arg_type, QAPISchemaObjectType) >> - assert not self.arg_type.variants # not implemented >> - assert not self.box # not implemented >> + assert (isinstance(self.arg_type, QAPISchemaObjectType) o= r >> + isinstance(self.arg_type, QAPISchemaAlternateType= )) >> + self.arg_type.check(schema) >> + if self.box: >> + if self.arg_type.is_empty(): >> + raise QAPIExprError(self.info, >> + "Cannot use 'box' with empty = type") >> + else: >> + assert not self.arg_type.variants >=20 > Likewise. And same argument about being implicitly correct. I can either document this in the commit message (to call it out as intentional) or restore the asserts; up to you which is cleaner. >> =3D=3D Client JSON Protocol introspection =3D=3D >> > [Tests snipped, they look good...] >=20 > Having read PATCH 07+08 another time, I got one more stylistic remark: > I'd name the flag @boxed, not @box. It's not a box, it's a flag that > tells us that whatever it applies to is boxed. Sounds reasonable, so looks like a v9 is worth posting. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --lVH0vvpn8tsRuVsMoJhStfKjEt02J4pqi 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/ iQEcBAEBCAAGBQJXfnz/AAoJEKeha0olJ0NqZ8UH/jI5yewZ9/jlnhE1bd7S2Bcz +EhHZR57GDnzbeyRporCcxth6OjL/PB+NjTdCwSdBx7jTx5Fjpq1Yo6o9dPdzukk Dp4H/9vaZ2Y3K69Qd5TUJrIeMwvesuhP9Ra0ktXs/R9qi93uDiifwq7RBfDungfv 8RQAZSTHBfTUPWxQmufyWtSE9wgENL4XSt63G5oi2BBon8BjuBH06oIxS14t2zHb lDKcfpUUdOl5Zob/v9UJM+zLHAN3ipBUie8WPtS8EaJcbQ+tDe1MNN5kbFOfVKgr KhTtpliVHa2b8nG5QXp3+Bku03G42l2+pSo4YTbDbg/3iBJCSTo7MfrX2A8VCqo= =y2nS -----END PGP SIGNATURE----- --lVH0vvpn8tsRuVsMoJhStfKjEt02J4pqi--