[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 07/18] qapi: Add json output visitor
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v3 07/18] qapi: Add json output visitor |
Date: |
Thu, 5 May 2016 22:16:56 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 |
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?
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 = 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.
Maybe we need to keep the FOO_new() functions returning FOO* rather
than Visitor*, along with the FOO_get_visitor() functions, after all.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-devel] [PATCH v3 07/18] qapi: Add json output visitor, Eric Blake, 2016/05/18