From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47763) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zi1dC-0005OR-RS for qemu-devel@nongnu.org; Fri, 02 Oct 2015 10:48:20 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zi1d7-0007Lq-Nx for qemu-devel@nongnu.org; Fri, 02 Oct 2015 10:48:18 -0400 Received: from mx1.redhat.com ([209.132.183.28]:53514) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zi1d7-0007Li-GY for qemu-devel@nongnu.org; Fri, 02 Oct 2015 10:48:13 -0400 References: <1443760312-656-1-git-send-email-eblake@redhat.com> <1443760312-656-7-git-send-email-eblake@redhat.com> <871tddfvjx.fsf@blackfin.pond.sub.org> From: Eric Blake Message-ID: <560E9927.5060102@redhat.com> Date: Fri, 2 Oct 2015 08:48:07 -0600 MIME-Version: 1.0 In-Reply-To: <871tddfvjx.fsf@blackfin.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="GoRbFP37nPaNqC7Clov7A7l45L0gCLv3W" Subject: Re: [Qemu-devel] [PATCH v6 06/12] qapi: Track owner of each object member 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) --GoRbFP37nPaNqC7Clov7A7l45L0gCLv3W Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 10/02/2015 03:50 AM, Markus Armbruster wrote: > Eric Blake writes: >=20 >> Future commits will migrate semantic checking away from parsing >> and over to the various QAPISchema*.check() methods. But to >> report an error message about an incorrect semantic use of a >> member of an object type, we need to know which type, command, >> or event owns the member. Rather than making all the check() >> methods have to pass around additional information, it is easier >> to have each member track who owns it in the first place. >> >> The source information is intended for human consumption in >> error messages, and a new describe() method is added to access >> the resulting information. For example, given the qapi: >> { 'command': 'foo', 'data': { 'string': 'str' } } >> an implementation of visit_command() that calls >> arg_type.members[0].describe() >> will see "'string' (member of foo arguments)". >=20 > Peeking ahead a bit, I see two describe(), one for ordinary members > returning >=20 > "'%s' (member of %s)" % (self.name, self._owner) >=20 > and one for variant members returning >=20 > "'%s' (branch of %s)" % (self.name, self._owner) >=20 > The name _owner makes me expect it's the owning types name, but that's > not always the case, as we shall see. How is it related to info and to= > the owning type's name then? >=20 > In your example (implicit arguments type): >=20 > arg_type.members[0]._owner is 'foo arguments'. >=20 > arg_type.members[0] has no info. Well, none of the members have info - they are not subclassed from QAPISchemaEntity. >=20 > arg_type.name is ':obj-foo-arg' >=20 > arg_type.info is something like >=20 > info {'line': 10, 'parent': None, 'file': 'example-schema.json'= } >=20 > pointing to the definition of command 'foo'. It's actually the > command's info, inherited by its implicit argument type. >=20 > Here, _owner is merely a variation of the owning type's name geared for= > human readers. Oh, I think I see where you are going with this - why is 'owner' a string, rather than the actual QAPISchemaType python object? And is it worth following the pattern used in other classes, where __init__ gets a string naming the type, and then check() resolves that name to the actual type? At which point, we could do member.owner.info to access the info of the type that owns the member? But there's a chicken-and-egg situation - we don't know what the type will be named until we call _make_implicit_object_type(), but that function requires that we have already pre-constructed the QAPISchemaObjectTypeMembers array (which means we can't pre-construct the members with the type name embedded). We'd have to refactor things to generate the type name, then construct the members, then construct the type (doable, but probably involves splitting this patch for ease of review). >=20 > Example of explicit arguments type: >=20 > { 'struct': 'BarArgs', 'data': { 'string': 'str' } } > { 'command': 'bar', 'data': 'BarArgs' } >=20 > Here, we get: >=20 > arg_type.members[0]._owner is 'BarArgs'. >=20 > arg_type.members[0] has no info. Again, because NO members have info. >=20 > arg_type.name is 'BarArgs' >=20 > arg_type.info is something like >=20 > info {'line': 12, 'parent': None, 'file': 'example-schema.json'= } >=20 > pointing to the definition of command 'bar. Again, it's the > command's info, inherited by its implicit argument type. >=20 > Here, _owner *is* the owning type's name. >=20 > So, _owner is a more readable name we make up when the other name for > the same thing isn't readable. However, we make up that other name, > too! Begs the question why we don't simply make it readable right away= =2E >=20 > Naturally, we still need to make up names collision-free. But as far a= s > I can tell, nothing stops us from picking ':obj-foo arguments' instead > of ':obj-foo-arg', and when we talk to users strip off the common prefi= x > ':obj-' we prepend to avoid collisions. Might be doable, but then we'd have to generate the implicit object name prior to creating its Member objects (thus splitting _make_implicit_object_type() into two parts). >=20 >> Where implicit types are involved, the code intentionally tries >> to pick the name of the owner of that implicit type, rather than >> the type name itself (a user reading the error message should be >> able to grep for the problem in their original file, but will not >> be able to locate a generated implicit name). >> >> No change to generated code. >> >> Signed-off-by: Eric Blake >=20 > Let's discuss the above before I review the actual patch closely. What do you think - would that refactoring be worth it? --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --GoRbFP37nPaNqC7Clov7A7l45L0gCLv3W 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/ iQEcBAEBCAAGBQJWDpknAAoJEKeha0olJ0NqCzsH/jySlCy6MjIYcp8ddf/zSD/6 IJ690q+A3hKx7qGt1cPT7IYDQ/4mZhXgCH3F7peqg0rUqx9skwDXIhn0oCO3f12A 0W7qfOypWL6zRyPC9DDDp+HB8qP9Lbz+iyhVtj4DsjDxO6wnVXhF+Z2IVMtxUGGy c3D5J18sILBVtpiiBggdn9HAWUAQvrHUy03OVK7iK0wCApA/aHtwncIoXllGiwvM jfLuo7cgrYLQU412HRCxd5CaL6bLlzbMoLeIZPEnOR4HxPwgWJ0+W2kmUsm/XywQ kjj6E428xCMcIyQcvI7d7tshlUgNPfUQWqXfGTOCKmhFn6MZvsDBRo28rWYasVM= =LpdE -----END PGP SIGNATURE----- --GoRbFP37nPaNqC7Clov7A7l45L0gCLv3W--