From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45324) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zgm3h-0004Nn-Ky for qemu-devel@nongnu.org; Mon, 28 Sep 2015 23:58:30 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zgm3d-0007ye-R3 for qemu-devel@nongnu.org; Mon, 28 Sep 2015 23:58:29 -0400 Received: from mx1.redhat.com ([209.132.183.28]:46552) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zgm3d-0007yU-Ip for qemu-devel@nongnu.org; Mon, 28 Sep 2015 23:58:25 -0400 References: <1442872682-6523-1-git-send-email-eblake@redhat.com> <1442872682-6523-12-git-send-email-eblake@redhat.com> <87r3lihfyw.fsf@blackfin.pond.sub.org> From: Eric Blake Message-ID: <560A0C5B.80008@redhat.com> Date: Mon, 28 Sep 2015 21:58:19 -0600 MIME-Version: 1.0 In-Reply-To: <87r3lihfyw.fsf@blackfin.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="7o6husueX6eAs5CG7n7faMBdI8OveDjqB" Subject: Re: [Qemu-devel] [PATCH v5 11/46] qapi: Don't use info as witness of implicit object type List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: marcandre.lureau@redhat.com, DirtY.iCE.hu@gmail.com, qemu-devel@nongnu.org, ehabkost@redhat.com, Michael Roth This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --7o6husueX6eAs5CG7n7faMBdI8OveDjqB Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 09/28/2015 06:43 AM, Markus Armbruster wrote: > Eric Blake writes: >=20 >> A future patch will enable error reporting from the various >> check() methods. But to report an error on an implicit type, >> we'll need to associate a location with the type (the same >> location as the top-level entity that is causing the creation >> of the implicit type), and once we do that, keying off of >> whether foo.info exists is no longer a viable way to determine >> if foo is an implicit type. >=20 > Ensuring error messages are good even for implicit types could be hard.= > But pretty much anything's better than error messages without location > information. Especially since the current implementation crashes hard when trying to report an error with info=3DNone. >=20 >> Rename the info member to _info (so that sub-classes can still >> use it, but external code should not), add an is_implicit() >> method to QAPISchemaObjectType, and adjust the visitor to pass >> another parameter about whether the type is implicit. >=20 > I have doubts on the rename. Fair enough; the proposal to separate it into its own patch, so we can then discard or easily revert it, sounds like the right approach. >> class QAPISchemaObjectType(QAPISchemaType): >> @@ -961,21 +961,25 @@ class QAPISchemaObjectType(QAPISchemaType): >> self.variants.check(schema, members, seen) >> self.members =3D members >> >> + def is_implicit(self): >> + return self.name[0] =3D=3D ':' >> + >=20 > The predicate could be defined on any QAPISchemaType, or even any > QAPISchemaEntity, but right now we only ever want to test it for > objects. Okay. Yeah, I thought about that. All builtin types are implicit, all array types are implicit, no commands or events are implicit, and we didn't make any different generated output based on whether enums were explicit or implicit, so that leaves just objects. >> +++ b/tests/qapi-schema/test-qapi.py >> @@ -22,7 +22,7 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor): >> if prefix: >> print ' prefix %s' % prefix >> >> - def visit_object_type(self, name, info, base, members, variants):= >> + def visit_object_type(self, name, info, base, members, variants, = implicit): >> print 'object %s' % name >> if base: >> print ' base %s' % base.name >=20 > Three of our visitors implement visit_object_type(): Another idea would be change the signature from: def visit_object_type(self (QAPISchemaVisitor), name (str), info (dict), base (QEMUSchemaObjectType), members (list of QEMUSchemaObjectTypeMember), variants (QAPISchemaObjectTypeVariants), implicit (bool)) to: def visit_object_type(self, object (QEMUSchemaObjectType)) and let callers dereference object.name, object.info, object.base, object.members (or object.local_members), object.variants, and object.is_implicit() as they see fit. (In fact, in one of my later patches, I already wished I had access to the actual QEMUSchemaObjectType object rather than its breakdown of parts to begin with, and ended up doing a schema.lookupType(name) just to get back to that piece of information). >=20 > * test-qapi.py doesn't care about implicit (implicitness is obvious > enough from the name here). >=20 > * qapi-types.py and qapi-visit.py ignore implicit object types. Hmm. >=20 > qapi-introspect.py has a similar need: it wants to ignore *all* types= =2E > Two ways to ignore entities seem one too many. Preexisting, but your= > patch makes it stand out a bit more. >=20 > Could we reuse the existing mechanism somehow (and keep method > visit_object_type() simple)? >=20 > To reuse it without changes, we'd have to make implicit object types = a > separate class, so that QAPISchema.visit()'s isinstance() test can be= > put to work. Maybe. Would also make implementing is_implicit() easy (which type did I instantiate) rather than hacky (does name start with ':'). >=20 > Another option is generalizing QAPISchema's filter. How? >=20 > A third option is to abandon QAPISchema's filter, and make > qapi-introspect.py filter in the visitor methods, just like we filter= > implicit objects. I'm still thinking about this one. >=20 > Patch could be split into >=20 > A. Encapsulate the "is implicit" predicate in a method, i.e. replace > not o.info by o.is_implicit(). >=20 > B. Clean up how we filter out implicit objects. May better go before A= , > not sure. >=20 > C. Rename .info to ._info. Not sure we even want this part. Yes, I'll go along with a split somewhere along these lines before reposting this patch for v6, although I'm going to have to sleep on it before deciding how to clean up the filtering. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --7o6husueX6eAs5CG7n7faMBdI8OveDjqB 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/ iQEcBAEBCAAGBQJWCgxbAAoJEKeha0olJ0NqkhcH/1T1HfF7MIRkkCmzVCkIOJ5U t22fs66jy4usbXiG/whDRRbfPJW13laxepmk4UGU9pxPgaD9P/LrV6oJ55JTUcWk jC6Z+L3DFugMMjzrKQRvxKoSUiWGHSjHtr9fG4xWJ127hqWbXCQKWCmRfUMgyCCA bbJL3g4dJacuuks2pP/CLQIOovZKva3Lm134H90S6WKJ+CXcSic9G8iumY+w7v5d 8hNKzuSQ5lFv5g0b59ucb5mZJO633BWP8RATCH4+uP6fLu2tzoQ0KjrycOnrjfdN hvhLczBSSCibMLAkIAODTeR4G7E5v2AQ0dGTzK5dpQ/GkyOk/qiOy1FNiMga2j8= =qaH2 -----END PGP SIGNATURE----- --7o6husueX6eAs5CG7n7faMBdI8OveDjqB--