From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51483) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ayXKh-00013u-Jf for qemu-devel@nongnu.org; Fri, 06 May 2016 00:25:53 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ayXKR-00033y-Gz for qemu-devel@nongnu.org; Fri, 06 May 2016 00:25:36 -0400 Received: from mx1.redhat.com ([209.132.183.28]:54865) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ayXKR-0002zC-8i for qemu-devel@nongnu.org; Fri, 06 May 2016 00:25:27 -0400 References: <1461903820-3092-1-git-send-email-eblake@redhat.com> <1461903820-3092-8-git-send-email-eblake@redhat.com> <87lh3som6o.fsf@dusky.pond.sub.org> <57276E26.5020006@redhat.com> <87r3dja6ue.fsf@dusky.pond.sub.org> <87wpn9esj5.fsf@dusky.pond.sub.org> From: Eric Blake Message-ID: <572C1AB8.1060705@redhat.com> Date: Thu, 5 May 2016 22:16:56 -0600 MIME-Version: 1.0 In-Reply-To: <87wpn9esj5.fsf@dusky.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="30rHF2CJ5Q41dBsDEb8XWH2IMnCLw1jrp" Subject: Re: [Qemu-devel] [PATCH v3 07/18] qapi: Add json output visitor List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: famz@redhat.com, qemu-devel@nongnu.org, Michael Roth This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --30rHF2CJ5Q41dBsDEb8XWH2IMnCLw1jrp Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 05/04/2016 09:45 AM, Markus Armbruster wrote: >>>>> +void json_output_visitor_reset(JsonOutputVisitor *v); >>>> >>>> Hmm. Why is "reset" not a Visitor method? >>>> >>>> I think this would let us put the things enforced by your "qmp: Tigh= ten >>>> output visitor rules" in the Visitor contract. >>> >>> I thought about that, and now that you've mentioned it, I'll probably= >>> give it a try (that is, make visit_reset() a top-level construct that= >>> ALL visitors must support, rather than just qmp-output and json-outpu= t). >> >> Yes, please. >=20 > Same question for "cleanup". Stupid name for a destructor, by the way.= Interface question - all of the FOO_visitor_new() functions return a subtype pointer Foo*, rather than Visitor*; along with a Visitor *FOO_get_visitor(FOO*) for up-casting, so that FOO* can be used in the per-type cleanup function; the FOO* pointers are also useful for two additional output functions in the two output visitors. We're proposing hiding the per-type cleanup function behind a simpler visit_free(Visitor *v). So all that's left are the two output functions. Can we get rid of those, and make Visitor* the only public interface, rather than making every caller have to do upcasts? It looks like outside of the testsuite, all uses of these visitors are local to a single function; and improving the testsuite is not the end of the world. Particularly if only the testsuite is using reset, it may be easier to just patch the testsuite to use a new visitor in the places where it currently does a reset. How ugly would it be to require that the caller pass in a pointer to the result as part of creating the visitor, with the promise that the result is populated at the end of a successful visit, and left NULL if the visit is reset early? Or maybe a visit_complete() function that is a no-op on input visitors, but populates the parameter passed at creation for output visitors. If we do that, we could rewrite things like the existing: QObject *object_property_get_qobject(Object *obj, const char *name, Error **errp) { QObject *ret =3D NULL; Error *local_err =3D NULL; QmpOutputVisitor *qov; qov =3D qmp_output_visitor_new(); object_property_get(obj, qmp_output_get_visitor(qov), name, &local_er= r); if (!local_err) { ret =3D qmp_output_get_qobject(qov); } error_propagate(errp, local_err); qmp_output_visitor_cleanup(qov); return ret; } to instead be: QObject *object_property_get_qobject(Object *obj, const char *name, Error **errp) { QObject *ret =3D NULL; Error *local_err =3D NULL; Visitor *v; v =3D qmp_output_visitor_new(&ret); object_property_get(obj, v, name, &local_err); if (!local_err) { visit_complete(v); /* populates ret */ } error_propagate(errp, local_err); visit_free(v); return ret; } Slightly shorter, but populating 'ret' at a distance feels a bit weird. Maybe we need to keep the FOO_new() functions returning FOO* rather than Visitor*, along with the FOO_get_visitor() functions, after all. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --30rHF2CJ5Q41dBsDEb8XWH2IMnCLw1jrp 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/ iQEcBAEBCAAGBQJXLBq4AAoJEKeha0olJ0NqTOMIAKl+8dqWlcORhjHENs6hRK3y LCoMz3PvMv+SK+Bft0izb0miqEJIC0XZujR1I1n01s4ItJ+UnaEYXFdJIlxBQiaS y2sLJsfenf55Inp9vxZnXWXvx5ZPW8J5B/yo5ND2keO7hrISsajGIufkO/bik0r3 l9bbvq4jAud26cFOHnN8MntvS1P8HQ7ma9Lnufq8oxDtxJ+FduzRoQEa2uIWTimn d+Gb92OVp8LB05Krc/abIIsl1wc0NIGgd8kJpTd1NbW+a8FPV9nAS5vBXJeOtHs1 kTuUKsqWFQHqwrmrd3OZDsK5Kn4fNzlFfYYN766WlJS++ZGb8KEYZb2rYY7q0uU= =xTbd -----END PGP SIGNATURE----- --30rHF2CJ5Q41dBsDEb8XWH2IMnCLw1jrp--