From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41326) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ayevY-0007a6-QM for qemu-devel@nongnu.org; Fri, 06 May 2016 08:32:23 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ayevM-0003U8-TE for qemu-devel@nongnu.org; Fri, 06 May 2016 08:32:11 -0400 Received: from mx1.redhat.com ([209.132.183.28]:39572) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ayevM-0003Qt-MG for qemu-devel@nongnu.org; Fri, 06 May 2016 08:32:04 -0400 From: Markus Armbruster 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> <572C1AB8.1060705@redhat.com> Date: Fri, 06 May 2016 14:31:51 +0200 In-Reply-To: <572C1AB8.1060705@redhat.com> (Eric Blake's message of "Thu, 5 May 2016 22:16:56 -0600") Message-ID: <87k2j74bbc.fsf@dusky.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain 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: Eric Blake Cc: famz@redhat.com, qemu-devel@nongnu.org, Michael Roth Eric Blake writes: > 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: Tighten >>>>> 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-output). >>> >>> Yes, please. >> >> 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? The two output functions are QObject *qmp_output_get_qobject(QmpOutputVisitor *v); char *string_output_get_string(StringOutputVisitor *v); > 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. I'm okay with replacing reset by destroy + new in the test suite. > 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 = NULL; > Error *local_err = NULL; > QmpOutputVisitor *qov; > > qov = qmp_output_visitor_new(); > object_property_get(obj, qmp_output_get_visitor(qov), name, &local_err); > if (!local_err) { > ret = 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 = NULL; > Error *local_err = NULL; > Visitor *v; > > v = 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. I like not having to deal with the QmpOutputVisitor type, but like you, I don't like action at a distance. > Maybe we need to keep the FOO_new() functions returning FOO* rather > than Visitor*, along with the FOO_get_visitor() functions, after all. I can think of a other ways to hide the QmpOutputVisitor type, but they have drawbacks, too. You can let the two output functions take Visitor *. Drawback: now the compiler lets you pass the wrong kind of visitor. You can let visit_complete() return the output (if any) as void *. Drawback: now the compiler lets you misinterpret the output's type.