[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: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v3 07/18] qapi: Add json output visitor |
Date: |
Fri, 06 May 2016 14:31:51 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
Eric Blake <address@hidden> 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.
Re: [Qemu-devel] [PATCH v3 07/18] qapi: Add json output visitor, Eric Blake, 2016/05/18