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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 05/02/2016 03:15 AM, Markus Armbruster wrote:
>> Title: s/json/JSON/
>> 
>> Eric Blake <address@hidden> 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 **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?

I'd drop it.

In general, I consider asserting "pointer not null" right before the
pointer is dereferenced a waste of space, except when it does
double-duty as documentation.  Matter of taste, of course.

Here, the assertion does double-duty confusing me: how can this be null?
Are my ideas on qobject_to_json()'s behavior mistaken?  Dig, dig, aha,
I'm not mistaken, it can't return null.  In short, it wastes programmer
time in addition to space.

>>> +/* Finish building, and return the resulting string. Will not be NULL. */
>>> +char *json_output_get_string(JsonOutputVisitor *jov)
>>> +{
>>> +    char *result;
>>> +
>>> +    assert(!jov->depth);
>>> +    result = g_strdup(qstring_get_str(jov->str));
>>> +    json_output_visitor_reset(jov);
>> 
>> Could avoid the strdup() if we wanted to.  Needs a way to convert
>> jov->str to char * destructively, like g_string_free() can do for a
>> GString.  Your choice.
>
> May be a nice pre-req patch to add; not sure if there are any other
> places already in tree that would benefit from it.

Just don't do it with a flag parameter, like g_string_free() does.
Mashing two operations as different as those into one function shows a
deplorable lack of taste.

>>> +    for (i = 0; i < ENUM_ONE__MAX; i++) {
>>> +        visit_type_EnumOne(data->ov, "unused", &i, &error_abort);
>>> +
>>> +        out = json_output_get_string(data->jov);
>>> +        g_assert(*out == '"');
>>> +        len = strlen(out);
>>> +        g_assert(out[len - 1] == '"');
>>> +        tmp = out + 1;
>>> +        out[len - 1] = 0;
>>> +        g_assert_cmpstr(tmp, ==, EnumOne_lookup[i]);
>>> +        g_free(out);
>> 
>> Unlike in test-qmp-output-visitor.c, you don't need
>> qmp_output_visitor_reset() here, because json_output_get_string() does
>> it automatically.
>> 
>> Is the difference between json_output_get_string() and
>> qmp_output_get_qobject() a good idea?
>
> No, and it probably means I have a bug for NOT requiring the reset.
>
>>> +    output_visitor_test_add("/visitor/json/any", test_visitor_out_any);
>>> +    output_visitor_test_add("/visitor/json/union-flat",
>>> +                            test_visitor_out_union_flat);
>>> +    output_visitor_test_add("/visitor/json/alternate",
>>> +                            test_visitor_out_alternate);
>>> +    output_visitor_test_add("/visitor/json/null", test_visitor_out_null);
>>> +
>>> +    g_test_run();
>>> +
>>> +    return 0;
>>> +}
>> 
>> This is obviously patterned after test-qmp-output-visitor.c.  Should we
>> link the two with comments, to improve our chances of them being kept in
>> sync?
>
> Sure.