Re: [Qemu-devel] [PATCH v3 07/18] qapi: Add json output visitor

[Eric Blake]

On 05/06/2016 06:31 AM, Markus Armbruster wrote:
>>  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.

That part's (relatively) easy, so it will be in the next spin.


>> 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.

But at least you can assert that the right visitor was used.

> 
> You can let visit_complete() return the output (if any) as void *.
> Drawback: now the compiler lets you misinterpret the output's type.

And you lose any chance to assert things.

Another (hybrid?) option:

void visit_complete(Visitor *v, void *opaque);
Visitor *qmp_output_visitor_new(QObject **ret);

called as:

v = qmp_output_visitor_new(&ret);
...
visit_complete(v, &ret);

where the completion function asserts that opaque matches the same
pointer as passed in with correct type during new().

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature