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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 7002 bytes --]

On 05/02/2016 03:15 AM, Markus Armbruster wrote:
> Title: s/json/JSON/
> 
> Eric Blake <eblake@redhat.com> 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 = 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.

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

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]