[v3,07/18] qapi: Add json output visitor

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 <eblake@redhat.com>

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

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?

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 <eblake@redhat.com>
>
> ---
> 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 0000000..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 *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 0000000..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 "qemu-common.h"

qemu/queue.h and qemu-common.h are superfluous.

> +#include "qapi/qmp/qstring.h"
> +#include "qapi/qmp/qobject-json.h"
> +
> +struct JsonOutputVisitor {
> +    Visitor visitor;
> +    QString *str;
> +    bool comma;
> +    unsigned int depth;
> +};
> +
> +static JsonOutputVisitor *to_jov(Visitor *v)
> +{
> +    return container_of(v, JsonOutputVisitor, visitor);
> +}
> +
> +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().

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

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

> +    }
> +    if (jov->comma) {
> +        qstring_append(jov->str, ", ");

Must be in an array or object.

We get here only when called multiple times without a reset of
jov->comma in between, i.e. a call of json_output_start_struct(),
json_output_start_list(), or json_output_visitor_reset().

In other words, if we get here outside an array or object, the caller
screwed up.  Okay, although the Visitor contract is silent on it.

> +    } else {
> +        jov->comma = true;
> +    }
> +    if (name && jov->depth) {
> +        qstring_append_json_string(jov->str, name);
> +        qstring_append(jov->str, ": ");

Must be in an object.  Covered by the Visitor contract quoted above.

> +    }
> +}
> +
> +static void json_output_start_struct(Visitor *v, const char *name, void **obj,
> +                                     size_t unused, Error **errp)
> +{
> +    JsonOutputVisitor *jov = to_jov(v);

Blank line between declarations and statements, please.  More of the
same below.

> +    json_output_name(jov, name);
> +    qstring_append(jov->str, "{");
> +    jov->comma = false;
> +    jov->depth++;
> +}
> +
> +static void json_output_end_struct(Visitor *v)
> +{
> +    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)
> +{
> +    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.

> +
> +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_format(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_format(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);
> +    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);
> +    qstring_append_json_number(jov->str, *obj, errp);
> +}
> +
> +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.

> +    json_output_name(jov, name);
> +    qstring_append(jov->str, qstring_get_str(str));
> +    QDECREF(str);
> +}
> +
> +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");
> +}
> +
> +/* 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.

> +    return result;
> +}
> +
> +Visitor *json_output_get_visitor(JsonOutputVisitor *v)
> +{
> +    return &v->visitor;
> +}
> +
> +void json_output_visitor_reset(JsonOutputVisitor *v)
> +{
> +    QDECREF(v->str);
> +    v->str = NULL;
> +    v->depth = 0;
> +    v->comma = false;
> +}
> +
> +void json_output_visitor_cleanup(JsonOutputVisitor *v)
> +{
> +    json_output_visitor_reset(v);
> +    g_free(v);
> +}
> +
> +JsonOutputVisitor *json_output_visitor_new(void)
> +{
> +    JsonOutputVisitor *v;
> +
> +    v = g_malloc0(sizeof(*v));
> +
> +    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_any = json_output_type_any;
> +    v->visitor.type_null = json_output_type_null;
> +
> +    return v;
> +}
> diff --git a/tests/test-json-output-visitor.c b/tests/test-json-output-visitor.c
> new file mode 100644
> index 0000000..57fe1c6
> --- /dev/null
> +++ b/tests/test-json-output-visitor.c
> @@ -0,0 +1,418 @@
> +/*
> + * JSON Output Visitor unit-tests.
> + *
> + * 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 <glib.h>
> +#include <math.h>
> +
> +#include "qemu-common.h"
> +#include "qapi/json-output-visitor.h"
> +#include "test-qapi-types.h"
> +#include "test-qapi-visit.h"
> +#include "qapi/qmp/types.h"
> +
> +typedef struct TestOutputVisitorData {
> +    JsonOutputVisitor *jov;
> +    Visitor *ov;
> +} TestOutputVisitorData;
> +
> +static void visitor_output_setup(TestOutputVisitorData *data,
> +                                 const void *unused)
> +{
> +    data->jov = json_output_visitor_new();
> +    g_assert(data->jov);
> +
> +    data->ov = json_output_get_visitor(data->jov);
> +    g_assert(data->ov);
> +}
> +
> +static void visitor_output_teardown(TestOutputVisitorData *data,
> +                                    const void *unused)
> +{
> +    json_output_visitor_cleanup(data->jov);
> +    data->jov = NULL;
> +    data->ov = NULL;
> +}
> +
> +static void test_visitor_out_int(TestOutputVisitorData *data,
> +                                 const void *unused)
> +{
> +    int64_t value = -42;
> +    char *out;
> +
> +    visit_type_int(data->ov, NULL, &value, &error_abort);
> +
> +    out = json_output_get_string(data->jov);
> +    g_assert_cmpstr(out, ==, "-42");
> +    g_free(out);
> +}
> +
> +static void test_visitor_out_bool(TestOutputVisitorData *data,
> +                                  const void *unused)
> +{
> +    bool value = true;
> +    char *out;
> +
> +    visit_type_bool(data->ov, NULL, &value, &error_abort);
> +
> +    out = json_output_get_string(data->jov);
> +    g_assert_cmpstr(out, ==, "true");
> +    g_free(out);
> +}
> +
> +static void test_visitor_out_number(TestOutputVisitorData *data,
> +                                    const void *unused)
> +{
> +    double value = 3.14;
> +    char *out;
> +    Error *err = NULL;
> +
> +    visit_type_number(data->ov, NULL, &value, &error_abort);
> +
> +    out = json_output_get_string(data->jov);
> +    g_assert_cmpstr(out, ==, "3.14");
> +    g_free(out);
> +
> +    /* JSON requires finite numbers */
> +    value = INFINITY;
> +    visit_type_number(data->ov, NULL, &value, &err);
> +    g_assert(err);
> +    error_free(err);

error_free_or_abort()

> +}
> +
> +static void test_visitor_out_string(TestOutputVisitorData *data,
> +                                    const void *unused)
> +{
> +    char *string = (char *) "Q E M U";
> +    char *out;
> +
> +    visit_type_str(data->ov, NULL, &string, &error_abort);
> +
> +    out = json_output_get_string(data->jov);
> +    g_assert_cmpstr(out, ==, "\"Q E M U\"");
> +    g_free(out);
> +}
> +
> +static void test_visitor_out_enum(TestOutputVisitorData *data,
> +                                  const void *unused)
> +{
> +    char *out;
> +    char *tmp;
> +    EnumOne i;
> +    size_t len;
> +
> +    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?

> +    }
> +}
> +
> +static void test_visitor_out_enum_errors(TestOutputVisitorData *data,
> +                                         const void *unused)
> +{
> +    EnumOne i, bad_values[] = { ENUM_ONE__MAX, -1 };
> +    Error *err;
> +
> +    for (i = 0; i < ARRAY_SIZE(bad_values) ; i++) {
> +        err = NULL;
> +        visit_type_EnumOne(data->ov, "unused", &bad_values[i], &err);
> +        g_assert(err);
> +        error_free(err);

error_free_or_abort()

Why no json_output_visitor_reset() here?

Running out of time, skipping the rest of the test for now.

> +    }
> +}
> +
> +
> +static void test_visitor_out_struct(TestOutputVisitorData *data,
> +                                    const void *unused)
> +{
> +    TestStruct test_struct = { .integer = 42,
> +                               .boolean = false,
> +                               .string = (char *) "foo"};
> +    TestStruct *p = &test_struct;
> +    char *out;
> +
> +    visit_type_TestStruct(data->ov, NULL, &p, &error_abort);
> +
> +    out = json_output_get_string(data->jov);
> +    g_assert_cmpstr(out, ==,
> +                    "{"
> +                     "\"integer\": 42, "
> +                     "\"boolean\": false, "
> +                     "\"string\": \"foo\""
> +                    "}");
> +    g_free(out);
> +}
> +
> +static void test_visitor_out_struct_nested(TestOutputVisitorData *data,
> +                                           const void *unused)
> +{
> +    int64_t value = 42;
> +    UserDefTwo *ud2;
> +    const char *string = "user def string";
> +    const char *strings[] = { "forty two", "forty three", "forty four",
> +                              "forty five" };
> +    char *out;
> +
> +    ud2 = g_malloc0(sizeof(*ud2));
> +    ud2->string0 = g_strdup(strings[0]);
> +
> +    ud2->dict1 = g_malloc0(sizeof(*ud2->dict1));
> +    ud2->dict1->string1 = g_strdup(strings[1]);
> +
> +    ud2->dict1->dict2 = g_malloc0(sizeof(*ud2->dict1->dict2));
> +    ud2->dict1->dict2->userdef = g_new0(UserDefOne, 1);
> +    ud2->dict1->dict2->userdef->string = g_strdup(string);
> +    ud2->dict1->dict2->userdef->integer = value;
> +    ud2->dict1->dict2->string = g_strdup(strings[2]);
> +
> +    ud2->dict1->dict3 = g_malloc0(sizeof(*ud2->dict1->dict3));
> +    ud2->dict1->has_dict3 = true;
> +    ud2->dict1->dict3->userdef = g_new0(UserDefOne, 1);
> +    ud2->dict1->dict3->userdef->string = g_strdup(string);
> +    ud2->dict1->dict3->userdef->integer = value;
> +    ud2->dict1->dict3->string = g_strdup(strings[3]);
> +
> +    visit_type_UserDefTwo(data->ov, "unused", &ud2, &error_abort);
> +
> +    out = json_output_get_string(data->jov);
> +    g_assert_cmpstr(out, ==,
> +                    "{"
> +                     "\"string0\": \"forty two\", "
> +                     "\"dict1\": {"
> +                      "\"string1\": \"forty three\", "
> +                      "\"dict2\": {"
> +                       "\"userdef\": {"
> +                        "\"integer\": 42, "
> +                        "\"string\": \"user def string\""
> +                        "}, "
> +                       "\"string\": \"forty four\""
> +                       "}, "
> +                      "\"dict3\": {"
> +                       "\"userdef\": {"
> +                        "\"integer\": 42, "
> +                        "\"string\": \"user def string\""
> +                        "}, "
> +                      "\"string\": \"forty five\""
> +                      "}"
> +                     "}"
> +                    "}");
> +    qapi_free_UserDefTwo(ud2);
> +    g_free(out);
> +}
> +
> +static void test_visitor_out_struct_errors(TestOutputVisitorData *data,
> +                                           const void *unused)
> +{
> +    EnumOne bad_values[] = { ENUM_ONE__MAX, -1 };
> +    UserDefOne u = { .string = (char *)"" };
> +    UserDefOne *pu = &u;
> +    Error *err;
> +    int i;
> +
> +    for (i = 0; i < ARRAY_SIZE(bad_values) ; i++) {
> +        err = NULL;
> +        u.has_enum1 = true;
> +        u.enum1 = bad_values[i];
> +        visit_type_UserDefOne(data->ov, "unused", &pu, &err);
> +        g_assert(err);
> +        error_free(err);

error_free_or_abort()

> +        json_output_visitor_reset(data->jov);
> +    }
> +}
> +
> +
> +static void test_visitor_out_list(TestOutputVisitorData *data,
> +                                  const void *unused)
> +{
> +    const char *value_str = "list value";
> +    TestStructList *p, *head = NULL;
> +    const int max_items = 10;
> +    bool value_bool = true;
> +    int value_int = 10;
> +    int i;
> +    char *out;
> +
> +    for (i = 0; i < max_items; i++) {
> +        p = g_malloc0(sizeof(*p));
> +        p->value = g_malloc0(sizeof(*p->value));
> +        p->value->integer = value_int + (max_items - i - 1);
> +        p->value->boolean = value_bool;
> +        p->value->string = g_strdup(value_str);
> +
> +        p->next = head;
> +        head = p;
> +    }
> +
> +    visit_type_TestStructList(data->ov, NULL, &head, &error_abort);
> +
> +    out = json_output_get_string(data->jov);
> +    g_assert_cmpstr(out, ==,
> +                    "["
> +                     "{\"integer\": 10, \"boolean\": true, "
> +                      "\"string\": \"list value\"}, "
> +                     "{\"integer\": 11, \"boolean\": true, "
> +                      "\"string\": \"list value\"}, "
> +                     "{\"integer\": 12, \"boolean\": true, "
> +                      "\"string\": \"list value\"}, "
> +                     "{\"integer\": 13, \"boolean\": true, "
> +                      "\"string\": \"list value\"}, "
> +                     "{\"integer\": 14, \"boolean\": true, "
> +                      "\"string\": \"list value\"}, "
> +                     "{\"integer\": 15, \"boolean\": true, "
> +                      "\"string\": \"list value\"}, "
> +                     "{\"integer\": 16, \"boolean\": true, "
> +                      "\"string\": \"list value\"}, "
> +                     "{\"integer\": 17, \"boolean\": true, "
> +                      "\"string\": \"list value\"}, "
> +                     "{\"integer\": 18, \"boolean\": true, "
> +                      "\"string\": \"list value\"}, "
> +                     "{\"integer\": 19, \"boolean\": true, "
> +                      "\"string\": \"list value\"}"
> +                    "]");
> +    qapi_free_TestStructList(head);
> +    g_free(out);
> +}
> +
> +static void test_visitor_out_any(TestOutputVisitorData *data,
> +                                 const void *unused)
> +{
> +    QObject *qobj;
> +    QDict *qdict;
> +    char *out;
> +
> +    qobj = QOBJECT(qint_from_int(-42));
> +    visit_type_any(data->ov, NULL, &qobj, &error_abort);
> +    out = json_output_get_string(data->jov);
> +    g_assert_cmpstr(out, ==, "-42");
> +    qobject_decref(qobj);
> +    g_free(out);
> +
> +    qdict = qdict_new();
> +    qdict_put(qdict, "integer", qint_from_int(-42));
> +    qdict_put(qdict, "boolean", qbool_from_bool(true));
> +    qdict_put(qdict, "string", qstring_from_str("foo"));
> +    qobj = QOBJECT(qdict);
> +    visit_type_any(data->ov, NULL, &qobj, &error_abort);
> +    qobject_decref(qobj);
> +    out = json_output_get_string(data->jov);
> +    g_assert_cmpstr(out, ==,
> +                    "{"
> +                     "\"integer\": -42, "
> +                     "\"boolean\": true, "
> +                     "\"string\": \"foo\""
> +                    "}");
> +    g_free(out);
> +}
> +
> +static void test_visitor_out_union_flat(TestOutputVisitorData *data,
> +                                        const void *unused)
> +{
> +    char *out;
> +    UserDefFlatUnion *tmp = g_malloc0(sizeof(UserDefFlatUnion));
> +
> +    tmp->enum1 = ENUM_ONE_VALUE1;
> +    tmp->string = g_strdup("str");
> +    tmp->integer = 41;
> +    tmp->u.value1.boolean = true;
> +
> +    visit_type_UserDefFlatUnion(data->ov, NULL, &tmp, &error_abort);
> +    out = json_output_get_string(data->jov);
> +    g_assert_cmpstr(out, ==,
> +                    "{"
> +                     "\"integer\": 41, "
> +                     "\"string\": \"str\", "
> +                     "\"enum1\": \"value1\", "
> +                     "\"boolean\": true"
> +                    "}");
> +    g_free(out);
> +    qapi_free_UserDefFlatUnion(tmp);
> +}
> +
> +static void test_visitor_out_alternate(TestOutputVisitorData *data,
> +                                       const void *unused)
> +{
> +    UserDefAlternate *tmp;
> +    char *out;
> +
> +    tmp = g_new0(UserDefAlternate, 1);
> +    tmp->type = QTYPE_QINT;
> +    tmp->u.i = 42;
> +
> +    visit_type_UserDefAlternate(data->ov, NULL, &tmp, &error_abort);
> +    out = json_output_get_string(data->jov);
> +    g_assert_cmpstr(out, ==, "42");
> +    g_free(out);
> +    qapi_free_UserDefAlternate(tmp);
> +
> +    tmp = g_new0(UserDefAlternate, 1);
> +    tmp->type = QTYPE_QSTRING;
> +    tmp->u.s = g_strdup("hello");
> +
> +    visit_type_UserDefAlternate(data->ov, NULL, &tmp, &error_abort);
> +    out = json_output_get_string(data->jov);
> +    g_assert_cmpstr(out, ==, "\"hello\"");
> +    g_free(out);
> +    qapi_free_UserDefAlternate(tmp);
> +}
> +
> +static void test_visitor_out_null(TestOutputVisitorData *data,
> +                                  const void *unused)
> +{
> +    char *out;
> +
> +    visit_type_null(data->ov, NULL, &error_abort);
> +    out = json_output_get_string(data->jov);
> +    g_assert_cmpstr(out, ==, "null");
> +    g_free(out);
> +}
> +
> +static void output_visitor_test_add(const char *testpath,
> +                                    void (*test_func)(TestOutputVisitorData *,
> +                                                      const void *))
> +{
> +    g_test_add(testpath, TestOutputVisitorData, NULL, visitor_output_setup,
> +               test_func, visitor_output_teardown);
> +}
> +
> +int main(int argc, char **argv)
> +{
> +    g_test_init(&argc, &argv, NULL);
> +
> +    output_visitor_test_add("/visitor/json/int", test_visitor_out_int);
> +    output_visitor_test_add("/visitor/json/bool", test_visitor_out_bool);
> +    output_visitor_test_add("/visitor/json/number", test_visitor_out_number);
> +    output_visitor_test_add("/visitor/json/string", test_visitor_out_string);
> +    output_visitor_test_add("/visitor/json/enum", test_visitor_out_enum);
> +    output_visitor_test_add("/visitor/json/enum-errors",
> +                            test_visitor_out_enum_errors);
> +    output_visitor_test_add("/visitor/json/struct", test_visitor_out_struct);
> +    output_visitor_test_add("/visitor/json/struct-nested",
> +                            test_visitor_out_struct_nested);
> +    output_visitor_test_add("/visitor/json/struct-errors",
> +                            test_visitor_out_struct_errors);
> +    output_visitor_test_add("/visitor/json/list", test_visitor_out_list);
> +    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?

> diff --git a/qapi/Makefile.objs b/qapi/Makefile.objs
> index 2278970..b60e11b 100644
> --- a/qapi/Makefile.objs
> +++ b/qapi/Makefile.objs
> @@ -1,6 +1,6 @@
>  util-obj-y = qapi-visit-core.o qapi-dealloc-visitor.o qmp-input-visitor.o
>  util-obj-y += qmp-output-visitor.o qmp-registry.o qmp-dispatch.o
>  util-obj-y += string-input-visitor.o string-output-visitor.o
> -util-obj-y += opts-visitor.o
> +util-obj-y += opts-visitor.o json-output-visitor.o
>  util-obj-y += qmp-event.o
>  util-obj-y += qapi-util.o
> diff --git a/tests/.gitignore b/tests/.gitignore
> index 2f8c7ea..c2aad79 100644
> --- a/tests/.gitignore
> +++ b/tests/.gitignore
> @@ -40,6 +40,7 @@ test-io-channel-file.txt
>  test-io-channel-socket
>  test-io-channel-tls
>  test-io-task
> +test-json-output-visitor
>  test-logging
>  test-mul64
>  test-opts-visitor
> diff --git a/tests/Makefile b/tests/Makefile
> index f71ed1c..0b5a7a8 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -22,6 +22,8 @@ check-unit-y += tests/check-qobject-json$(EXESUF)
>  gcov-files-check-qobject-json-y = qobject/qobject-json.c
>  check-unit-y += tests/test-qmp-output-visitor$(EXESUF)
>  gcov-files-test-qmp-output-visitor-y = qapi/qmp-output-visitor.c
> +check-unit-y += tests/test-json-output-visitor$(EXESUF)
> +gcov-files-test-json-output-visitor-y = qapi/json-output-visitor.c
>  check-unit-y += tests/test-qmp-input-visitor$(EXESUF)
>  gcov-files-test-qmp-input-visitor-y = qapi/qmp-input-visitor.c
>  check-unit-y += tests/test-qmp-input-strict$(EXESUF)
> @@ -388,6 +390,7 @@ test-obj-y = tests/check-qint.o tests/check-qstring.o tests/check-qdict.o \
>  	tests/check-qobject-json.o \
>  	tests/test-coroutine.o tests/test-string-output-visitor.o \
>  	tests/test-string-input-visitor.o tests/test-qmp-output-visitor.o \
> +	tests/test-json-output-visitor.o \
>  	tests/test-qmp-input-visitor.o tests/test-qmp-input-strict.o \
>  	tests/test-qmp-commands.o tests/test-visitor-serialization.o \
>  	tests/test-x86-cpuid.o tests/test-mul64.o tests/test-int128.o \
> @@ -478,6 +481,7 @@ tests/test-string-output-visitor$(EXESUF): tests/test-string-output-visitor.o $(
>  tests/test-string-input-visitor$(EXESUF): tests/test-string-input-visitor.o $(test-qapi-obj-y)
>  tests/test-qmp-event$(EXESUF): tests/test-qmp-event.o $(test-qapi-obj-y)
>  tests/test-qmp-output-visitor$(EXESUF): tests/test-qmp-output-visitor.o $(test-qapi-obj-y)
> +tests/test-json-output-visitor$(EXESUF): tests/test-json-output-visitor.o $(test-qapi-obj-y)
>  tests/test-qmp-input-visitor$(EXESUF): tests/test-qmp-input-visitor.o $(test-qapi-obj-y)
>  tests/test-qmp-input-strict$(EXESUF): tests/test-qmp-input-strict.o $(test-qapi-obj-y)
>  tests/test-qmp-commands$(EXESUF): tests/test-qmp-commands.o tests/test-qmp-marshal.o $(test-qapi-obj-y)

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.
>
> 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 <eblake@redhat.com>
>
> ---
> 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:
[...]

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> writes:

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

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.

Markus Armbruster <armbru@redhat.com> writes:

> Eric Blake <eblake@redhat.com> 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.

[...]

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, &local_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(&ret);
    object_property_get(obj, v, name, &local_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> 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, &local_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(&ret);
>     object_property_get(obj, v, name, &local_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.

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

where the completion function asserts that opaque matches the same
pointer as passed in with correct type during new().

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

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?

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.

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