On 05/02/2016 03:15 AM, Markus Armbruster wrote: > Title: s/json/JSON/ > > Eric Blake <ebl...@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
signature.asc
Description: OpenPGP digital signature
