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

[Eric Blake]

On 05/18/2016 09:16 AM, Eric Blake wrote:

>>> +static void json_output_type_any(Visitor *v, const char *name, QObject 
>>> **obj,
>>> + Error **errp)
>>> +{
>>> +JsonOutputVisitor *jov = to_jov(v);
>>> +QString *str = qobject_to_json(*obj);
>>> +assert(str);
>>
>> Can't happen.
> 
> Can too.  From tests/check-qobject-json.c:
> 
> obj = QOBJECT(qstring_from_str(utf8_in));
> str = qobject_to_json(obj);
> if (json_out) {
> g_assert(str);
> g_assert_cmpstr(qstring_get_str(str), ==, json_out);
> } else {
> g_assert(!str);
> }
> 
> where the failures occur when it is impossible to output proper UTF-8
> due to invalid encoding in a string.

Correction - that test has dead code, because:

json_out = test_cases[i].json_out ?: test_cases[i].json_in;

so you are right after all - we currently cannot fail on a conversion to
JSON (although the result might not be valid JSON).  At any rate, my
conclusion remains:

>  Arguably, that case would be nicer
> if it could set an Error* (and would make my argument for setting an
> error on Inf/NaN for numbers also a bit more tenable), but that is
> additional work that I haven't tackled yet. I'm trying to get the series
> posted for another round of review, where I've done some major
> reshuffling (such as doing the clone visitor first, not second, in the
> series), so hopefully later today.
> 

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

On 05/02/2016 03:15 AM, Markus Armbruster wrote:
> Title: s/json/JSON/
> 
> Eric Blake  writes:
> 
>> We have several places that want to go from qapi to JSON; right now,
>> they have to create an intermediate QObject to do the work.  That
>> also has the drawback that the JSON formatting of a QDict will
>> rearrange keys (according to a deterministic, but unpredictable,
>> hash), when humans have an easier time if dicts are produced in
>> the same order as the qapi type.
> 
> There's a drawback, though: more code.
> 
> Could the JSON output visitor replace the QMP output visitor?

Yes, it turned out quite nicely to write qobject_to_json() on top of a
JSON output visitor. And in fact, doing so makes it trivial to write a
QObject deep-cloner - pass the QObject to the qmp-output-visitor instead
of the json-output-visitor!


>> +static void json_output_type_any(Visitor *v, const char *name, QObject 
>> **obj,
>> + Error **errp)
>> +{
>> +JsonOutputVisitor *jov = to_jov(v);
>> +QString *str = qobject_to_json(*obj);
>> +assert(str);
> 
> Can't happen.

Can too.  From tests/check-qobject-json.c:

obj = QOBJECT(qstring_from_str(utf8_in));
str = qobject_to_json(obj);
if (json_out) {
g_assert(str);
g_assert_cmpstr(qstring_get_str(str), ==, json_out);
} else {
g_assert(!str);
}

where the failures occur when it is impossible to output proper UTF-8
due to invalid encoding in a string.  Arguably, that case would be nicer
if it could set an Error* (and would make my argument for setting an
error on Inf/NaN for numbers also a bit more tenable), but that is
additional work that I haven't tackled yet. I'm trying to get the series
posted for another round of review, where I've done some major
reshuffling (such as doing the clone visitor first, not second, in the
series), so hopefully later today.

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

On 05/06/2016 08:08 AM, Eric Blake wrote:
> 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.

In fact, it's easier to replace that part in the existing qapi-next
branch than it is to revert the addition of qmp_output_visitor_reset();
see patch posted in the other thread (v16A 17/24).

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

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();
>> object_property_get(obj, v, name, _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();
...
visit_complete(v, );

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

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

[Markus Armbruster]

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, _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();
> object_property_get(obj, v, name, _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]

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, _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();
object_property_get(obj, v, name, _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

[Markus Armbruster]

Markus Armbruster  writes:

> Eric Blake  writes:
>
>> On 05/02/2016 03:15 AM, Markus Armbruster wrote:
[...]
 +/*
 + * The JSON output visitor does not accept Infinity or NaN to
 + * visit_type_number().
 + */
 +JsonOutputVisitor *json_output_visitor_new(void);
 +void json_output_visitor_cleanup(JsonOutputVisitor *v);
 +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.

[...]

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

[Markus Armbruster]

Eric Blake  writes:

> On 05/02/2016 03:15 AM, Markus Armbruster wrote:
>> Title: s/json/JSON/
>> 
>> Eric Blake  writes:
>> 
>>> We have several places that want to go from qapi to JSON; right now,
>>> they have to create an intermediate QObject to do the work.  That
>>> also has the drawback that the JSON formatting of a QDict will
>>> rearrange keys (according to a deterministic, but unpredictable,
>>> hash), when humans have an easier time if dicts are produced in
>>> the same order as the qapi type.
>> 
>> There's a drawback, though: more code.
>> 
>> Could the JSON output visitor replace the QMP output visitor?
>
> Hmm. As written here, the JSON output visitor _reuses_ the QMP output
> visitor, for outputting an 'any' object.  Maybe the QMP output visitor
> could do a virtual visit through the JSON visitor, though (as in rather
> than directly outputting to JSON, it instead opens a JSON visitor under
> the hood, for some recursion when doing an 'any' visit).  I can try it
> as followup patches, but don't want to make the original checkin any
> more complex than it has to be.

Explorative followup patches are okay.  The decision whether the JSON
visitor's additional code is worthwhile is easier when we know whether
it can replace the QMP output visitor.

>> Aside: the QMP visitors are really QObject visitors.
>
> Yeah, particularly since they are also used by QGA. Is it worth renaming
> them?

Let's consider the rename when the current visitors rework settles.
Perhaps we have less to rename then.

>>> +/*
>>> + * The JSON output visitor does not accept Infinity or NaN to
>>> + * visit_type_number().
>>> + */
>>> +JsonOutputVisitor *json_output_visitor_new(void);
>>> +void json_output_visitor_cleanup(JsonOutputVisitor *v);
>>> +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.

>>> +
>>> +#include "qemu/osdep.h"
>>> +#include "qapi/json-output-visitor.h"
>>> +#include "qapi/visitor-impl.h"
>>> +#include "qemu/queue.h"
>>> +#include "qemu-common.h"
>> 
>> qemu/queue.h and qemu-common.h are superfluous.
>
> Rebase churn, I first wrote the patches before the header cleanups.
> Will fix.
>
>>> +
>>> +static void json_output_name(JsonOutputVisitor *jov, const char *name)
>> 
>> This is called for every value, by its visit_start_FOO() or
>> visit_type_FOO() function.  Quote visitor.h:
>> 
>>  * The @name parameter of visit_type_FOO() describes the relation
>>  * between this QAPI value and its parent container.  When visiting
>>  * the root of a tree, @name is ignored; when visiting a member of an
>>  * object, @name is the key associated with the value; and when
>>  * visiting a member of a list, @name is NULL.
>> 
>> Aside: it should mention visit_start_FOO() in addition to
>> visit_type_FOO().
>> 
>
> Separate cleanup, but sounds useful. I can add it.
>
>>> +{
>>> +if (!jov->str) {
>>> +jov->str = qstring_new();
>> 
>> This happens on the first call after creation or reset of jov.
>> 
>> If you call json_output_get_string() after an empty visit, it chokes on
>> null jov->str.  Could be declared a feature.  The Visitor contract is
>> silent on it, but the QMP output visitor rejects it since "qmp: Tighten
>> output visitor rules".
>
> I think feature, so yes, I should probably make the Visitor contract
> explicit that at least something has to be visited via visit_type_FOO()
> or visit_start_XXX().

To do that right, you have to make reset a method.

>> Regardless: why not create jov->str in json_output_visitor_new(), and
>> truncate it in json_output_visitor_reset()?
>> 
>> To retain the "feature", you'd assert qstring_get_length(jov->str).
>
> Sounds reasonable.
>
> ...
>>> +static void json_output_end_list(Visitor *v)
>>> +{
>>> +JsonOutputVisitor *jov = to_jov(v);
>>> +assert(jov->depth);
>>> +jov->depth--;
>>> +qstring_append(jov->str, "]");
>>> +jov->comma = true;
>>> +}
>> 
>> The nesting checks are less thorough than the QMP visitor's, because we
>> don't use a stack.  Okay.
>
> And at times, I've debated about giving qapi-visit-core.c a stack, if
> only to centralize some of the sanity checking for all visitors rather
> than just the particular visitors that need a stack.

Doesn't feel worthwhile, unless you can *replace* the visitors' stacks.
Too much code just for catching a kind of bug that seems rather
unlikely.

If the stack is just for checking nesting, it could be a bit vector.

>>> +static void json_output_type_any(Visitor *v, const char *name, QObject 
>>> **obj,
>>> + Error 

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

[Eric Blake]

On 05/02/2016 03:15 AM, Markus Armbruster wrote:
> Title: s/json/JSON/
> 
> Eric Blake  writes:
> 
>> We have several places that want to go from qapi to JSON; right now,
>> they have to create an intermediate QObject to do the work.  That
>> also has the drawback that the JSON formatting of a QDict will
>> rearrange keys (according to a deterministic, but unpredictable,
>> hash), when humans have an easier time if dicts are produced in
>> the same order as the qapi type.
> 
> There's a drawback, though: more code.
> 
> Could the JSON output visitor replace the QMP output visitor?

Hmm. As written here, the JSON output visitor _reuses_ the QMP output
visitor, for outputting an 'any' object.  Maybe the QMP output visitor
could do a virtual visit through the JSON visitor, though (as in rather
than directly outputting to JSON, it instead opens a JSON visitor under
the hood, for some recursion when doing an 'any' visit).  I can try it
as followup patches, but don't want to make the original checkin any
more complex than it has to be.

> 
> Aside: the QMP visitors are really QObject visitors.

Yeah, particularly since they are also used by QGA. Is it worth renaming
them?


>> +/*
>> + * The JSON output visitor does not accept Infinity or NaN to
>> + * visit_type_number().
>> + */
>> +JsonOutputVisitor *json_output_visitor_new(void);
>> +void json_output_visitor_cleanup(JsonOutputVisitor *v);
>> +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).

>> +
>> +#include "qemu/osdep.h"
>> +#include "qapi/json-output-visitor.h"
>> +#include "qapi/visitor-impl.h"
>> +#include "qemu/queue.h"
>> +#include "qemu-common.h"
> 
> qemu/queue.h and qemu-common.h are superfluous.

Rebase churn, I first wrote the patches before the header cleanups.
Will fix.

>> +
>> +static void json_output_name(JsonOutputVisitor *jov, const char *name)
> 
> This is called for every value, by its visit_start_FOO() or
> visit_type_FOO() function.  Quote visitor.h:
> 
>  * The @name parameter of visit_type_FOO() describes the relation
>  * between this QAPI value and its parent container.  When visiting
>  * the root of a tree, @name is ignored; when visiting a member of an
>  * object, @name is the key associated with the value; and when
>  * visiting a member of a list, @name is NULL.
> 
> Aside: it should mention visit_start_FOO() in addition to
> visit_type_FOO().
> 

Separate cleanup, but sounds useful. I can add it.

>> +{
>> +if (!jov->str) {
>> +jov->str = qstring_new();
> 
> This happens on the first call after creation or reset of jov.
> 
> If you call json_output_get_string() after an empty visit, it chokes on
> null jov->str.  Could be declared a feature.  The Visitor contract is
> silent on it, but the QMP output visitor rejects it since "qmp: Tighten
> output visitor rules".

I think feature, so yes, I should probably make the Visitor contract
explicit that at least something has to be visited via visit_type_FOO()
or visit_start_XXX().

> 
> Regardless: why not create jov->str in json_output_visitor_new(), and
> truncate it in json_output_visitor_reset()?
> 
> To retain the "feature", you'd assert qstring_get_length(jov->str).

Sounds reasonable.

...
>> +static void json_output_end_list(Visitor *v)
>> +{
>> +JsonOutputVisitor *jov = to_jov(v);
>> +assert(jov->depth);
>> +jov->depth--;
>> +qstring_append(jov->str, "]");
>> +jov->comma = true;
>> +}
> 
> The nesting checks are less thorough than the QMP visitor's, because we
> don't use a stack.  Okay.

And at times, I've debated about giving qapi-visit-core.c a stack, if
only to centralize some of the sanity checking for all visitors rather
than just the particular visitors that need a stack.

>> +static void json_output_type_any(Visitor *v, const char *name, QObject 
>> **obj,
>> + Error **errp)
>> +{
>> +JsonOutputVisitor *jov = to_jov(v);
>> +QString *str = qobject_to_json(*obj);
>> +assert(str);
> 
> Can't happen.

Can't happen now, but COULD happen if we teach qobject_to_json() to fail
on an attempt to visit Inf or NaN as a number (since that is not valid
JSON).  Then again, if we teach it to fail, we'd want to add an Error
parameter.  May also be impacted by how I refactor detection of invalid
numbers in response to your comments on 4/18.  Should I keep or drop the
assert?

>> +/* Finish building, and return the resulting string. Will not be NULL. */
>> +char *json_output_get_string(JsonOutputVisitor *jov)
>> +{
>> +char *result;
>> +
>> +assert(!jov->depth);
>> +result = 

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

[Markus Armbruster]

Eric Blake  writes:

> We have several places that want to go from qapi to JSON; right now,
> they have to create an intermediate QObject to do the work.  That
> also has the drawback that the JSON formatting of a QDict will
> rearrange keys (according to a deterministic, but unpredictable,
> hash), when humans have an easier time if dicts are produced in
> the same order as the qapi type.
>
> For these reasons, it is time to add a new JSON output visitor.
> This patch just adds the basic visitor and tests that it works;
> later patches will add pretty-printing, and convert clients to
> use the visitor.
>
> Design choices: Unlike the QMP output visitor, the JSON visitor
> refuses to visit a required string with a NULL value (abort), as
> well as a non-finite number (raises an error message).  Reusing
> QString to grow the contents means that we easily share code with
> both qobject-json.c and qjson.c; although it might be nice to
> enhance things to take an optional output callback function so
> that the output can truly be streamed instead of collected in
> memory.
>
> Signed-off-by: Eric Blake 
>
> ---
> v3: retitle, rebase to master, minor cleanups
> v2: rebase to qapi subset E v8; add test of error outputting
> infinity; use unsigned depth
> ---
>  include/qapi/visitor.h |  20 +-
>  include/qapi/json-output-visitor.h |  29 +++
>  qapi/json-output-visitor.c | 202 ++
>  tests/test-json-output-visitor.c   | 418 
> +
>  qapi/Makefile.objs |   2 +-
>  tests/.gitignore   |   1 +
>  tests/Makefile |   4 +
>  7 files changed, 665 insertions(+), 11 deletions(-)
>  create mode 100644 include/qapi/json-output-visitor.h
>  create mode 100644 qapi/json-output-visitor.c
>  create mode 100644 tests/test-json-output-visitor.c
>
> diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
> index a430c19..e8a4403 100644
> --- a/include/qapi/visitor.h
> +++ b/include/qapi/visitor.h
> @@ -26,16 +26,16 @@
>   *
>   * There are three kinds of visitor classes: input visitors (QMP,
>   * string, and QemuOpts) parse an external representation and build
> - * the corresponding QAPI graph, output visitors (QMP and string) take
> - * a completed QAPI graph and generate an external representation, and
> - * the dealloc visitor can take a QAPI graph (possibly partially
> - * constructed) and recursively free its resources.  While the dealloc
> - * and QMP input/output visitors are general, the string and QemuOpts
> - * visitors have some implementation limitations; see the
> - * documentation for each visitor for more details on what it
> - * supports.  Also, see visitor-impl.h for the callback contracts
> - * implemented by each visitor, and docs/qapi-code-gen.txt for more
> - * about the QAPI code generator.
> + * the corresponding QAPI graph, output visitors (QMP, string, and
> + * JSON) take a completed QAPI graph and generate an external
> + * representation, and the dealloc visitor can take a QAPI graph
> + * (possibly partially constructed) and recursively free its
> + * resources.  While the dealloc and QMP input/output visitors are

and the JSON output visitor?

> + * general, the string and QemuOpts visitors have some implementation
> + * limitations; see the documentation for each visitor for more
> + * details on what it supports.  Also, see visitor-impl.h for the
> + * callback contracts implemented by each visitor, and
> + * docs/qapi-code-gen.txt for more about the QAPI code generator.
>   *
>   * All QAPI types have a corresponding function with a signature
>   * roughly compatible with this:
[...]

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

[Markus Armbruster]

Title: s/json/JSON/

Eric Blake  writes:

> We have several places that want to go from qapi to JSON; right now,
> they have to create an intermediate QObject to do the work.  That
> also has the drawback that the JSON formatting of a QDict will
> rearrange keys (according to a deterministic, but unpredictable,
> hash), when humans have an easier time if dicts are produced in
> the same order as the qapi type.

There's a drawback, though: more code.

Could the JSON output visitor replace the QMP output visitor?

Aside: the QMP visitors are really QObject visitors.

> For these reasons, it is time to add a new JSON output visitor.
> This patch just adds the basic visitor and tests that it works;
> later patches will add pretty-printing, and convert clients to
> use the visitor.
>
> Design choices: Unlike the QMP output visitor, the JSON visitor
> refuses to visit a required string with a NULL value (abort), as
> well as a non-finite number (raises an error message).  Reusing
> QString to grow the contents means that we easily share code with
> both qobject-json.c and qjson.c; although it might be nice to
> enhance things to take an optional output callback function so
> that the output can truly be streamed instead of collected in
> memory.
>
> Signed-off-by: Eric Blake 
>
> ---
> v3: retitle, rebase to master, minor cleanups
> v2: rebase to qapi subset E v8; add test of error outputting
> infinity; use unsigned depth
> ---
>  include/qapi/visitor.h |  20 +-
>  include/qapi/json-output-visitor.h |  29 +++
>  qapi/json-output-visitor.c | 202 ++
>  tests/test-json-output-visitor.c   | 418 
> +
>  qapi/Makefile.objs |   2 +-
>  tests/.gitignore   |   1 +
>  tests/Makefile |   4 +
>  7 files changed, 665 insertions(+), 11 deletions(-)
>  create mode 100644 include/qapi/json-output-visitor.h
>  create mode 100644 qapi/json-output-visitor.c
>  create mode 100644 tests/test-json-output-visitor.c
>
> diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
> index a430c19..e8a4403 100644
> --- a/include/qapi/visitor.h
> +++ b/include/qapi/visitor.h
> @@ -26,16 +26,16 @@
>   *
>   * There are three kinds of visitor classes: input visitors (QMP,
>   * string, and QemuOpts) parse an external representation and build
> - * the corresponding QAPI graph, output visitors (QMP and string) take
> - * a completed QAPI graph and generate an external representation, and
> - * the dealloc visitor can take a QAPI graph (possibly partially
> - * constructed) and recursively free its resources.  While the dealloc
> - * and QMP input/output visitors are general, the string and QemuOpts
> - * visitors have some implementation limitations; see the
> - * documentation for each visitor for more details on what it
> - * supports.  Also, see visitor-impl.h for the callback contracts
> - * implemented by each visitor, and docs/qapi-code-gen.txt for more
> - * about the QAPI code generator.
> + * the corresponding QAPI graph, output visitors (QMP, string, and
> + * JSON) take a completed QAPI graph and generate an external
> + * representation, and the dealloc visitor can take a QAPI graph
> + * (possibly partially constructed) and recursively free its
> + * resources.  While the dealloc and QMP input/output visitors are
> + * general, the string and QemuOpts visitors have some implementation
> + * limitations; see the documentation for each visitor for more
> + * details on what it supports.  Also, see visitor-impl.h for the
> + * callback contracts implemented by each visitor, and
> + * docs/qapi-code-gen.txt for more about the QAPI code generator.
>   *
>   * All QAPI types have a corresponding function with a signature
>   * roughly compatible with this:
> diff --git a/include/qapi/json-output-visitor.h 
> b/include/qapi/json-output-visitor.h
> new file mode 100644
> index 000..ac03da1
> --- /dev/null
> +++ b/include/qapi/json-output-visitor.h
> @@ -0,0 +1,29 @@
> +/*
> + * JSON Output Visitor
> + *
> + * Copyright (C) 2015-2016 Red Hat, Inc.
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.1 or 
> later.
> + * See the COPYING.LIB file in the top-level directory.
> + *
> + */
> +
> +#ifndef JSON_OUTPUT_VISITOR_H
> +#define JSON_OUTPUT_VISITOR_H
> +
> +#include "qapi/visitor.h"
> +
> +typedef struct JsonOutputVisitor JsonOutputVisitor;
> +
> +/*
> + * The JSON output visitor does not accept Infinity or NaN to
> + * visit_type_number().
> + */
> +JsonOutputVisitor *json_output_visitor_new(void);
> +void json_output_visitor_cleanup(JsonOutputVisitor *v);
> +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.

> +
> +char *json_output_get_string(JsonOutputVisitor *v);
> +Visitor 

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

[Eric Blake]

We have several places that want to go from qapi to JSON; right now,
they have to create an intermediate QObject to do the work.  That
also has the drawback that the JSON formatting of a QDict will
rearrange keys (according to a deterministic, but unpredictable,
hash), when humans have an easier time if dicts are produced in
the same order as the qapi type.

For these reasons, it is time to add a new JSON output visitor.
This patch just adds the basic visitor and tests that it works;
later patches will add pretty-printing, and convert clients to
use the visitor.

Design choices: Unlike the QMP output visitor, the JSON visitor
refuses to visit a required string with a NULL value (abort), as
well as a non-finite number (raises an error message).  Reusing
QString to grow the contents means that we easily share code with
both qobject-json.c and qjson.c; although it might be nice to
enhance things to take an optional output callback function so
that the output can truly be streamed instead of collected in
memory.

Signed-off-by: Eric Blake 

---
v3: retitle, rebase to master, minor cleanups
v2: rebase to qapi subset E v8; add test of error outputting
infinity; use unsigned depth
---
 include/qapi/visitor.h |  20 +-
 include/qapi/json-output-visitor.h |  29 +++
 qapi/json-output-visitor.c | 202 ++
 tests/test-json-output-visitor.c   | 418 +
 qapi/Makefile.objs |   2 +-
 tests/.gitignore   |   1 +
 tests/Makefile |   4 +
 7 files changed, 665 insertions(+), 11 deletions(-)
 create mode 100644 include/qapi/json-output-visitor.h
 create mode 100644 qapi/json-output-visitor.c
 create mode 100644 tests/test-json-output-visitor.c

diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index a430c19..e8a4403 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -26,16 +26,16 @@
  *
  * There are three kinds of visitor classes: input visitors (QMP,
  * string, and QemuOpts) parse an external representation and build
- * the corresponding QAPI graph, output visitors (QMP and string) take
- * a completed QAPI graph and generate an external representation, and
- * the dealloc visitor can take a QAPI graph (possibly partially
- * constructed) and recursively free its resources.  While the dealloc
- * and QMP input/output visitors are general, the string and QemuOpts
- * visitors have some implementation limitations; see the
- * documentation for each visitor for more details on what it
- * supports.  Also, see visitor-impl.h for the callback contracts
- * implemented by each visitor, and docs/qapi-code-gen.txt for more
- * about the QAPI code generator.
+ * the corresponding QAPI graph, output visitors (QMP, string, and
+ * JSON) take a completed QAPI graph and generate an external
+ * representation, and the dealloc visitor can take a QAPI graph
+ * (possibly partially constructed) and recursively free its
+ * resources.  While the dealloc and QMP input/output visitors are
+ * general, the string and QemuOpts visitors have some implementation
+ * limitations; see the documentation for each visitor for more
+ * details on what it supports.  Also, see visitor-impl.h for the
+ * callback contracts implemented by each visitor, and
+ * docs/qapi-code-gen.txt for more about the QAPI code generator.
  *
  * All QAPI types have a corresponding function with a signature
  * roughly compatible with this:
diff --git a/include/qapi/json-output-visitor.h 
b/include/qapi/json-output-visitor.h
new file mode 100644
index 000..ac03da1
--- /dev/null
+++ b/include/qapi/json-output-visitor.h
@@ -0,0 +1,29 @@
+/*
+ * JSON Output Visitor
+ *
+ * Copyright (C) 2015-2016 Red Hat, Inc.
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ */
+
+#ifndef JSON_OUTPUT_VISITOR_H
+#define JSON_OUTPUT_VISITOR_H
+
+#include "qapi/visitor.h"
+
+typedef struct JsonOutputVisitor JsonOutputVisitor;
+
+/*
+ * The JSON output visitor does not accept Infinity or NaN to
+ * visit_type_number().
+ */
+JsonOutputVisitor *json_output_visitor_new(void);
+void json_output_visitor_cleanup(JsonOutputVisitor *v);
+void json_output_visitor_reset(JsonOutputVisitor *v);
+
+char *json_output_get_string(JsonOutputVisitor *v);
+Visitor *json_output_get_visitor(JsonOutputVisitor *v);
+
+#endif
diff --git a/qapi/json-output-visitor.c b/qapi/json-output-visitor.c
new file mode 100644
index 000..3539db5
--- /dev/null
+++ b/qapi/json-output-visitor.c
@@ -0,0 +1,202 @@
+/*
+ * Convert QAPI to JSON
+ *
+ * Copyright (C) 2015-2016 Red Hat, Inc.
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
+ * See the COPYING.LIB file in the top-level directory.
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/json-output-visitor.h"
+#include "qapi/visitor-impl.h"
+#include "qemu/queue.h"
+#include