Re: [Qemu-devel] [PATCH v6 12/15] qapi: Support pretty printing in JSON output visitor

[Marc-André Lureau]

On Mon, Oct 10, 2016 at 5:44 PM Eric Blake <address@hidden> wrote:

> Add pretty printing, where the format intentionally matches that of
> qobject_to_json() (a later patch will then rework qobject-json.c to
> work on top of the JSON visitor).  The trickiest part is probably
> that the testsuite now has to honor parameterization on whether
> pretty printing is enabled.
>
> Signed-off-by: Eric Blake <address@hidden>
>
> ---
> v6: tweak commit message, use of 'bool pretty' in testsuite
> [no v5 due to series split]
> v4: rebase to earlier changes, defer type_any() to later
> v3: rebase to earlier changes
> v2: rebase to earlier changes
> ---
>  include/qapi/json-output-visitor.h |   5 +-
>  qapi/json-output-visitor.c         |  15 ++++-
>  tests/test-json-output-visitor.c   | 124
> +++++++++++++++++++++++++------------
>  3 files changed, 101 insertions(+), 43 deletions(-)
>
> diff --git a/include/qapi/json-output-visitor.h
> b/include/qapi/json-output-visitor.h
> index 41c79f4..94c9e0f 100644
> --- a/include/qapi/json-output-visitor.h
> +++ b/include/qapi/json-output-visitor.h
> @@ -21,8 +21,11 @@ typedef struct JsonOutputVisitor JsonOutputVisitor;
>   * If everything else succeeds, pass @result to visit_complete() to
>   * collect the result of the visit.
>   *
> + * If @pretty, make the output legible with newlines and indentation;
> + * otherwise the output uses a single line.
> + *
>   * For now, this cannot be used to visit the 'any' type.
>   */
> -Visitor *json_output_visitor_new(char **result);
> +Visitor *json_output_visitor_new(bool pretty, char **result);
>
>  #endif
> diff --git a/qapi/json-output-visitor.c b/qapi/json-output-visitor.c
> index 805154a..7d12879 100644
> --- a/qapi/json-output-visitor.c
> +++ b/qapi/json-output-visitor.c
> @@ -17,6 +17,7 @@
>  struct JsonOutputVisitor {
>      Visitor visitor;
>      QString *str;
> +    bool pretty;
>      bool comma;
>      unsigned int depth;
>      char **result;
> @@ -30,10 +31,13 @@ static JsonOutputVisitor *to_jov(Visitor *v)
>  static void json_output_name(JsonOutputVisitor *jov, const char *name)
>  {
>      if (jov->comma) {
> -        qstring_append(jov->str, ", ");
> +        qstring_append(jov->str, jov->pretty ? "," : ", ");
>      } else {
>          jov->comma = true;
>      }
> +    if (jov->pretty && jov->depth) {
> +        qstring_append_printf(jov->str, "\n%*s", 4 * jov->depth, "");
> +    }
>      if (name && jov->depth) {
>          qstring_append_json_string(jov->str, name);
>          qstring_append(jov->str, ": ");
> @@ -57,6 +61,9 @@ static void json_output_end_struct(Visitor *v, void
> **obj)
>
>      assert(jov->depth);
>      jov->depth--;
> +    if (jov->pretty) {
> +        qstring_append_printf(jov->str, "\n%*s", 4 * jov->depth, "");
> +    }
>      qstring_append(jov->str, "}");
>      jov->comma = true;
>  }
> @@ -85,6 +92,9 @@ static void json_output_end_list(Visitor *v, void **obj)
>
>      assert(jov->depth);
>      jov->depth--;
> +    if (jov->pretty) {
> +        qstring_append_printf(jov->str, "\n%*s", 4 * jov->depth, "");
> +    }
>      qstring_append(jov->str, "]");
>      jov->comma = true;
>  }
> @@ -165,11 +175,12 @@ static void json_output_free(Visitor *v)
>      g_free(jov);
>  }
>
> -Visitor *json_output_visitor_new(char **result)
> +Visitor *json_output_visitor_new(bool pretty, char **result)
>  {
>      JsonOutputVisitor *v;
>
>      v = g_malloc0(sizeof(*v));
> +    v->pretty = pretty;
>      v->result = result;
>      *result = NULL;
>      v->str = qstring_new();
> diff --git a/tests/test-json-output-visitor.c
> b/tests/test-json-output-visitor.c
> index 3c77a61..849cf2b 100644
> --- a/tests/test-json-output-visitor.c
> +++ b/tests/test-json-output-visitor.c
> @@ -24,13 +24,17 @@
>
>  typedef struct TestOutputVisitorData {
>      Visitor *ov;
> +    bool pretty;
>      char *str;
>  } TestOutputVisitorData;
>
>  static void visitor_output_setup(TestOutputVisitorData *data,
> -                                 const void *unused)
> +                                 const void *arg)
>  {
> -    data->ov = json_output_visitor_new(&data->str);
> +    bool pretty = *(bool *)arg;
> +
>

Have you considered GPOINTER_TO_INT/GINT_TO_POINTER instead of pointers to
the stack frame?

either way, it's fine.


> +    data->pretty = pretty;
> +    data->ov = json_output_visitor_new(pretty, &data->str);
>      g_assert(data->ov);
>  }
>
> @@ -52,8 +56,10 @@ static const char *visitor_get(TestOutputVisitorData
> *data)
>
>  static void visitor_reset(TestOutputVisitorData *data)
>  {
> +    bool pretty = data->pretty;
> +
>      visitor_output_teardown(data, NULL);
> -    visitor_output_setup(data, NULL);
> +    visitor_output_setup(data, &pretty);
>  }
>
>  static void test_visitor_out_int(TestOutputVisitorData *data,
> @@ -161,8 +167,9 @@ static void
> test_visitor_out_struct(TestOutputVisitorData *data,
>  }
>
>  static void test_visitor_out_struct_nested(TestOutputVisitorData *data,
> -                                           const void *unused)
> +                                           const void *arg)
>  {
> +    bool pretty = *(bool *)arg;
>      int64_t value = 42;
>      UserDefTwo *ud2;
>      const char *string = "user def string";
> @@ -192,27 +199,51 @@ static void
> test_visitor_out_struct_nested(TestOutputVisitorData *data,
>      visit_type_UserDefTwo(data->ov, "unused", &ud2, &error_abort);
>
>      out = visitor_get(data);
> -    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\""
> -                      "}"
> -                     "}"
> -                    "}");
> +    if (pretty) {
> +        g_assert_cmpstr(out, ==,
> +                        "{\n"
> +                        "    \"string0\": \"forty two\",\n"
> +                        "    \"dict1\": {\n"
> +                        "        \"string1\": \"forty three\",\n"
> +                        "        \"dict2\": {\n"
> +                        "            \"userdef\": {\n"
> +                        "                \"integer\": 42,\n"
> +                        "                \"string\": \"user def
> string\"\n"
> +                        "            },\n"
> +                        "            \"string\": \"forty four\"\n"
> +                        "        },\n"
> +                        "        \"dict3\": {\n"
> +                        "            \"userdef\": {\n"
> +                        "                \"integer\": 42,\n"
> +                        "                \"string\": \"user def
> string\"\n"
> +                        "            },\n"
> +                        "            \"string\": \"forty five\"\n"
> +                        "        }\n"
> +                        "    }\n"
> +                        "}");
> +    } else {
> +        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);
>  }
>
> @@ -346,36 +377,49 @@ static void
> test_visitor_out_null(TestOutputVisitorData *data,
>      g_assert_cmpstr(out, ==, "null");
>  }
>
> -static void output_visitor_test_add(const char *testpath,
> +static void output_visitor_test_add(const char *testpath, bool *data,
>                                      void
> (*test_func)(TestOutputVisitorData *,
>                                                        const void *))
>  {
> -    g_test_add(testpath, TestOutputVisitorData, NULL,
> visitor_output_setup,
> +    g_test_add(testpath, TestOutputVisitorData, data,
> visitor_output_setup,
>                 test_func, visitor_output_teardown);
>  }
>
>  int main(int argc, char **argv)
>  {
> +    bool plain = false;
> +    bool pretty = true;
> +
>      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",
> +    output_visitor_test_add("/visitor/json/int", &plain,
> +                            test_visitor_out_int);
> +    output_visitor_test_add("/visitor/json/bool", &plain,
> +                            test_visitor_out_bool);
> +    output_visitor_test_add("/visitor/json/number", &plain,
> +                            test_visitor_out_number);
> +    output_visitor_test_add("/visitor/json/string", &plain,
> +                            test_visitor_out_string);
> +    output_visitor_test_add("/visitor/json/enum", &plain,
> +                            test_visitor_out_enum);
> +    output_visitor_test_add("/visitor/json/enum-errors", &plain,
>                              test_visitor_out_enum_errors);
> -    output_visitor_test_add("/visitor/json/struct",
> test_visitor_out_struct);
> -    output_visitor_test_add("/visitor/json/struct-nested",
> +    output_visitor_test_add("/visitor/json/struct", &plain,
> +                            test_visitor_out_struct);
> +    output_visitor_test_add("/visitor/json/struct-nested", &plain,
>                              test_visitor_out_struct_nested);
> -    output_visitor_test_add("/visitor/json/struct-errors",
> +    output_visitor_test_add("/visitor/json-pretty/struct-nested", &pretty,
> +                            test_visitor_out_struct_nested);
> +    output_visitor_test_add("/visitor/json/struct-errors", &plain,
>                              test_visitor_out_struct_errors);
> -    output_visitor_test_add("/visitor/json/list", test_visitor_out_list);
> -    output_visitor_test_add("/visitor/json/union-flat",
> +    output_visitor_test_add("/visitor/json/list", &plain,
> +                            test_visitor_out_list);
> +    output_visitor_test_add("/visitor/json/union-flat", &plain,
>                              test_visitor_out_union_flat);
> -    output_visitor_test_add("/visitor/json/alternate",
> +    output_visitor_test_add("/visitor/json/alternate", &plain,
>                              test_visitor_out_alternate);
> -    output_visitor_test_add("/visitor/json/null", test_visitor_out_null);
> +    output_visitor_test_add("/visitor/json/null", &plain,
> +                            test_visitor_out_null);
>
>      g_test_run();
>
> --
> 2.7.4
>
>
> --
Marc-André Lureau