Re: [Qemu-devel] [PATCH v4 24/28] qapi: Add JSON output visitor

[Markus Armbruster]

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.
>
> 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, support for visit_type_any(),
> and conversion of 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, via abort().
> 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 <address@hidden>
>
> ---
> v4: retitle, split off inf/NaN handling, rebase to visit_complete
> and visit_free changes, defer type_any, address other findings from
> Markus
> 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             |  33 ++--
>  include/qapi/json-output-visitor.h |  28 +++
>  qapi/json-output-visitor.c         | 193 +++++++++++++++++++
>  tests/test-json-output-visitor.c   | 383 
> +++++++++++++++++++++++++++++++++++++
>  tests/test-qmp-output-visitor.c    |   5 +
>  qapi/Makefile.objs                 |   2 +-
>  tests/.gitignore                   |   1 +
>  tests/Makefile                     |   5 +-
>  8 files changed, 632 insertions(+), 18 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 3f46921..c097507 100644
> --- a/include/qapi/visitor.h
> +++ b/include/qapi/visitor.h
> @@ -26,17 +26,17 @@
>   *
>   * There are four 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, the
> - * dealloc visitor can take a QAPI graph (possibly partially
> - * constructed) and recursively free its resources, and the clone
> - * visitor performs a deep clone of one QAPI object to another.  While
> - * the dealloc and QMP input/output visitors are general, the string,
> - * QemuOpts, and clone 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, the dealloc visitor can take a QAPI graph (possibly
> + * partially constructed) and recursively free its resources, and the
> + * clone visitor performs a deep clone of one QAPI object to another.
> + * While the dealloc, JSON output, and QMP input/output visitors are
> + * general, the string, QemuOpts, and clone 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 of the visitors are created via:
>   *
> @@ -59,11 +59,12 @@
>   * visitors are declared here; the remaining visitors are generated in
>   * qapi-visit.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.
> + * The @name parameter of visit_type_FOO() and visit_start_OBJECT()
> + * 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.

Not related to this patch, just came up in review of v3.  Splitting it
out into its own patch doesn't seem worthwhile, so let's keep it here.

>   *
>   * FIXME: Clients must pass NULL for @name when visiting a member of a
>   * list, but this leads to poor error messages; it might be nicer to
> diff --git a/include/qapi/json-output-visitor.h 
> b/include/qapi/json-output-visitor.h
> new file mode 100644
> index 0000000..41c79f4
> --- /dev/null
> +++ b/include/qapi/json-output-visitor.h
> @@ -0,0 +1,28 @@
> +/*
> + * JSON Output Visitor
> + *
> + * Copyright (C) 2015-2016 Red Hat, Inc.
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + *
> + */
> +
> +#ifndef JSON_OUTPUT_VISITOR_H
> +#define JSON_OUTPUT_VISITOR_H
> +
> +#include "qapi/visitor.h"
> +
> +typedef struct JsonOutputVisitor JsonOutputVisitor;
> +
> +/*
> + * Create a new JSON output visitor.
> + *
> + * If everything else succeeds, pass @result to visit_complete() to
> + * collect the result of the visit.
> + *
> + * For now, this cannot be used to visit the 'any' type.
> + */
> +Visitor *json_output_visitor_new(char **result);
> +
> +#endif
> diff --git a/qapi/json-output-visitor.c b/qapi/json-output-visitor.c
> new file mode 100644
> index 0000000..7010ff8
> --- /dev/null
> +++ b/qapi/json-output-visitor.c
> @@ -0,0 +1,193 @@
> +/*
> + * Convert QAPI to JSON
> + *
> + * Copyright (C) 2015-2016 Red Hat, Inc.
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + *
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/json-output-visitor.h"
> +#include "qapi/visitor-impl.h"
> +#include "qapi/qmp/qstring.h"
> +#include "qapi/qmp/qobject-json.h"
> +
> +struct JsonOutputVisitor {
> +    Visitor visitor;
> +    QString *str;
> +    bool comma;
> +    unsigned int depth;
> +    char **result;
> +};
> +
> +static JsonOutputVisitor *to_jov(Visitor *v)
> +{
> +    return container_of(v, JsonOutputVisitor, visitor);
> +}
> +
> +static void json_output_name(JsonOutputVisitor *jov, const char *name)
> +{
> +    if (jov->comma) {
> +        qstring_append(jov->str, ", ");
> +    } else {
> +        jov->comma = true;
> +    }
> +    if (name && jov->depth) {
> +        qstring_append_json_string(jov->str, name);
> +        qstring_append(jov->str, ": ");
> +    }
> +}
> +
> +static void json_output_start_struct(Visitor *v, const char *name, void 
> **obj,
> +                                     size_t unused, Error **errp)
> +{
> +    JsonOutputVisitor *jov = to_jov(v);
> +
> +    json_output_name(jov, name);
> +    qstring_append(jov->str, "{");
> +    jov->comma = false;
> +    jov->depth++;
> +}
> +
> +static void json_output_end_struct(Visitor *v, void **obj)
> +{
> +    JsonOutputVisitor *jov = to_jov(v);
> +
> +    assert(jov->depth);
> +    jov->depth--;
> +    qstring_append(jov->str, "}");
> +    jov->comma = true;
> +}
> +
> +static void json_output_start_list(Visitor *v, const char *name,
> +                                   GenericList **listp, size_t size,
> +                                   Error **errp)
> +{
> +    JsonOutputVisitor *jov = to_jov(v);
> +
> +    json_output_name(jov, name);
> +    qstring_append(jov->str, "[");
> +    jov->comma = false;
> +    jov->depth++;
> +}
> +
> +static GenericList *json_output_next_list(Visitor *v, GenericList *tail,
> +                                          size_t size)
> +{
> +    return tail->next;
> +}
> +
> +static void json_output_end_list(Visitor *v, void **obj)
> +{
> +    JsonOutputVisitor *jov = to_jov(v);
> +
> +    assert(jov->depth);
> +    jov->depth--;
> +    qstring_append(jov->str, "]");
> +    jov->comma = true;
> +}
> +
> +static void json_output_type_int64(Visitor *v, const char *name, int64_t 
> *obj,
> +                                   Error **errp)
> +{
> +    JsonOutputVisitor *jov = to_jov(v);
> +
> +    json_output_name(jov, name);
> +    qstring_append_printf(jov->str, "%" PRId64, *obj);
> +}
> +
> +static void json_output_type_uint64(Visitor *v, const char *name,
> +                                    uint64_t *obj, Error **errp)
> +{
> +    JsonOutputVisitor *jov = to_jov(v);
> +
> +    json_output_name(jov, name);
> +    qstring_append_printf(jov->str, "%" PRIu64, *obj);
> +}
> +
> +static void json_output_type_bool(Visitor *v, const char *name, bool *obj,
> +                                  Error **errp)
> +{
> +    JsonOutputVisitor *jov = to_jov(v);
> +
> +    json_output_name(jov, name);
> +    qstring_append(jov->str, *obj ? "true" : "false");
> +}
> +
> +static void json_output_type_str(Visitor *v, const char *name, char **obj,
> +                                 Error **errp)
> +{
> +    JsonOutputVisitor *jov = to_jov(v);
> +
> +    assert(*obj);
> +    json_output_name(jov, name);
> +    /* FIXME: report invalid UTF-8 encoding */
> +    qstring_append_json_string(jov->str, *obj);
> +}
> +
> +static void json_output_type_number(Visitor *v, const char *name, double 
> *obj,
> +                                    Error **errp)
> +{
> +    JsonOutputVisitor *jov = to_jov(v);
> +
> +    json_output_name(jov, name);
> +    /* FIXME: report Inf/NaN problems */
> +    qstring_append_json_number(jov->str, *obj);
> +}
> +
> +static void json_output_type_null(Visitor *v, const char *name, Error **errp)
> +{
> +    JsonOutputVisitor *jov = to_jov(v);
> +
> +    json_output_name(jov, name);
> +    qstring_append(jov->str, "null");
> +}
> +
> +static void json_output_complete(Visitor *v, void *result)
> +{
> +    JsonOutputVisitor *jov = to_jov(v);
> +
> +    assert(!jov->depth);
> +    assert(qstring_get_length(jov->str));
> +    assert(jov->result == result);
> +    *jov->result = qstring_consume_str(jov->str);
> +    jov->str = NULL;
> +    jov->result = NULL;
> +}

Related: discussion of complete() semantics in review of PATCH 12.
Non-idempotent semantics save us a copy, like in
string_output_complete().

> +
> +static void json_output_free(Visitor *v)
> +{
> +    JsonOutputVisitor *jov = to_jov(v);
> +
> +    QDECREF(jov->str);
> +    g_free(jov);

I'm afraid this leaks jov->result when the visitor is destroyed without
calling its complete() method.

string_output_free() appears to have the same leak.

> +}
> +
> +Visitor *json_output_visitor_new(char **result)
> +{
> +    JsonOutputVisitor *v;
> +
> +    v = g_malloc0(sizeof(*v));
> +    v->result = result;
> +    *result = NULL;
> +    v->str = qstring_new();
> +
> +    v->visitor.type = VISITOR_OUTPUT;
> +    v->visitor.start_struct = json_output_start_struct;
> +    v->visitor.end_struct = json_output_end_struct;
> +    v->visitor.start_list = json_output_start_list;
> +    v->visitor.next_list = json_output_next_list;
> +    v->visitor.end_list = json_output_end_list;
> +    v->visitor.type_int64 = json_output_type_int64;
> +    v->visitor.type_uint64 = json_output_type_uint64;
> +    v->visitor.type_bool = json_output_type_bool;
> +    v->visitor.type_str = json_output_type_str;
> +    v->visitor.type_number = json_output_type_number;
> +    v->visitor.type_null = json_output_type_null;
> +    v->visitor.complete = json_output_complete;
> +    v->visitor.free = json_output_free;
> +
> +    return &v->visitor;
> +}
[...]