[Qemu-devel] [PATCH v2 0/6] Stable output of blockdev format specific info

[Qemu-devel] [PATCH v2 0/6] Stable output of blockdev format specific info

[Daniel P. Berrange]

This is an update of the bits of this previous
series which were not merged

  https://lists.gnu.org/archive/html/qemu-devel/2016-06/msg01723.html

The problem addressed in this series is that the
'qemu-img info' command does not have a stable
output format for the image specific info objects.

The QAPI types are first converted to QDicts,
and then printed with custom code to recurse over
the QDicts. This causes information about the object
field ordering to be thrown away, and fields are
printed in whatever order they appear in the QDict
hash buckets. This is not a big deal historically
since none of the image formats had nested data
structures, but with the LUKS blockdev this style
of random ordering looks very unpleasant.

To address this, the patch series introduces a
TextOutputVisitor class that is designed to be
able to print out arbitrarily nested QAPI types
directly, in a format that is identical to that
currently used with 'qemu-img info'. Consult
the patch in question to actually see the output
format, and compare test-string-output-visitor.c
with test-text-output-visitor.c to see how we
really do have 2 completely distinct output formats
that don't share any significant characteristics
beyond both being "plain text".

Changed in v2:

 - Pulled code for printing sized value out into the
   cutils file via qemu_sztostr* methods
 - Added unit test for text output visitor
 - Convert block code to use qemu_sztostr* methods
 - Convert string output visitor code to use
   qemu_sztostr* methods
 - Document the string output visitor format against
   its constructor
 - Document the text otput visitor format against
   its constructor.
 - Adjustments to block i/o tests to take account of
   stable field ordering
 - Adjustments to block i/o tests to take account of
   simpified size format when ending in a .0

Daniel P. Berrange (6):
  cutils: add helpers for formatting sized values
  qapi: convert StringOutputVisitor to use qemu_szutostr
  qapi: assert that visitor impls have required callbacks
  qapi: add a text output visitor for pretty printing types
  block: convert to use the qemu_szutostr functions
  block: convert to use qapi_stringify_ImageInfoSpecific

 block/qapi.c                         | 163 +++-------------
 include/qapi/string-output-visitor.h |  28 ++-
 include/qapi/text-output-visitor.h   |  77 ++++++++
 include/qemu/cutils.h                |  12 ++
 qapi/Makefile.objs                   |   1 +
 qapi/qapi-visit-core.c               |  15 ++
 qapi/string-output-visitor.c         |  20 +-
 qapi/text-output-visitor.c           | 349 +++++++++++++++++++++++++++++++++
 tests/Makefile.include               |   5 +-
 tests/qemu-iotests/095.out           |   2 +-
 tests/qemu-iotests/104.out           |   2 +-
 tests/test-cutils.c                  |  96 +++++++++
 tests/test-string-output-visitor.c   |  22 +++
 tests/test-text-output-visitor.c     | 366 +++++++++++++++++++++++++++++++++++
 util/cutils.c                        |  78 ++++++++
 15 files changed, 1079 insertions(+), 157 deletions(-)
 create mode 100644 include/qapi/text-output-visitor.h
 create mode 100644 qapi/text-output-visitor.c
 create mode 100644 tests/test-text-output-visitor.c

-- 
2.7.4

[Qemu-devel] [PATCH v2 1/6] cutils: add helpers for formatting sized values

[Daniel P. Berrange]

Introduce qemu_sztostr which takes an int and turns it
into a sized string. Variants are added for both signed
and unsigned integers.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 include/qemu/cutils.h | 12 +++++++
 tests/test-cutils.c   | 96 +++++++++++++++++++++++++++++++++++++++++++++++++++
 util/cutils.c         | 78 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 186 insertions(+)

diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h
index 3e4ea23..31f6bff 100644
--- a/include/qemu/cutils.h
+++ b/include/qemu/cutils.h
@@ -158,6 +158,18 @@ int64_t qemu_strtosz_suffix(const char *nptr, char **end,
                             const char default_suffix);
 int64_t qemu_strtosz_suffix_unit(const char *nptr, char **end,
                             const char default_suffix, int64_t unit);
+
+char *qemu_sztostr(int64_t val);
+char *qemu_sztostr_full(int64_t val,
+                        const char default_suffix,
+                        bool long_suffix,
+                        const char *separator);
+char *qemu_szutostr(uint64_t val);
+char *qemu_szutostr_full(uint64_t val,
+                         const char default_suffix,
+                         bool long_suffix,
+                         const char *separator);
+
 #define K_BYTE     (1ULL << 10)
 #define M_BYTE     (1ULL << 20)
 #define G_BYTE     (1ULL << 30)
diff --git a/tests/test-cutils.c b/tests/test-cutils.c
index 20b0f59..b067c68 100644
--- a/tests/test-cutils.c
+++ b/tests/test-cutils.c
@@ -1437,8 +1437,94 @@ static void test_qemu_strtosz_suffix_unit(void)
     g_assert_cmpint(res, ==, 12345000);
 }
 
+
+static struct SzToStrData {
+    const char *path;
+    int64_t val;
+    const char *expected;
+    const char default_suffix;
+    bool long_suffix;
+    const char *separator;
+} test_qemu_sztostr_data[] = {
+    { "/cutils/sztostr/simple", -42, "-42B", '\0', true, NULL },
+    { "/cutils/sztostr/simple-suffix", -1729, "-1.69KiB", '\0', true, NULL },
+    { "/cutils/sztostr/default-suffix", 1729, "1.69",
+      QEMU_STRTOSZ_DEFSUFFIX_KB, true, NULL },
+    { "/cutils/sztostr/short-suffix", 1729, "1.69 K", '\0', false, " " },
+    { "/cutils/sztostr/simple-sepator", -1729, "-1.69",
+      QEMU_STRTOSZ_DEFSUFFIX_KB, true, " " },
+    { "/cutils/sztostr/simple-suffix-sepator", -1729, "-1.69 KiB",
+      '\0', true, " " },
+    { "/cutils/sztostr/ulong-max", ULLONG_MAX, "-1B", '\0', true, NULL },
+    { "/cutils/sztostr/long-max", LONG_MAX, "8EiB", '\0', true, NULL },
+    { "/cutils/sztostr/long-min", -LONG_MAX - 1, "-8EiB", '\0', true, NULL },
+};
+
+static void test_qemu_sztostr(const void *opaque)
+{
+    const struct SzToStrData *data = opaque;
+    char *actual;
+
+    if (data->separator || data->default_suffix) {
+        actual = qemu_sztostr_full(data->val,
+                                   data->default_suffix,
+                                   data->long_suffix,
+                                   data->separator);
+    } else {
+        actual = qemu_sztostr(data->val);
+    }
+
+    g_assert_cmpstr(actual, ==, data->expected);
+
+    g_free(actual);
+}
+
+
+static struct SzUToStrData {
+    const char *path;
+    uint64_t val;
+    const char *expected;
+    const char default_suffix;
+    bool long_suffix;
+    const char *separator;
+} test_qemu_szutostr_data[] = {
+    { "/cutils/szutostr/simple", 42, "42B", '\0', true, NULL },
+    { "/cutils/szutostr/simple-suffix", 1729, "1.69KiB", '\0', true, NULL },
+    { "/cutils/szutostr/default-suffix", 1729, "1.69",
+      QEMU_STRTOSZ_DEFSUFFIX_KB, true, NULL },
+    { "/cutils/szutostr/short-suffix", 1729, "1.69 K", '\0', false, " " },
+    { "/cutils/szutostr/simple-sepator", 1729, "1.69",
+      QEMU_STRTOSZ_DEFSUFFIX_KB, true, " " },
+    { "/cutils/szutostr/simple-suffix-sepator", 1729, "1.69 KiB",
+      '\0', true, " " },
+    { "/cutils/szutostr/ulong-max", ULLONG_MAX, "16EiB", '\0', true, NULL },
+    { "/cutils/szutostr/long-max", LONG_MAX, "8EiB", '\0', true, NULL },
+};
+
+static void test_qemu_szutostr(const void *opaque)
+{
+    const struct SzUToStrData *data = opaque;
+    char *actual;
+
+    if (data->separator || data->default_suffix) {
+        actual = qemu_szutostr_full(data->val,
+                                    data->default_suffix,
+                                    data->long_suffix,
+                                    data->separator);
+    } else {
+        actual = qemu_szutostr(data->val);
+    }
+
+    g_assert_cmpstr(actual, ==, data->expected);
+
+    g_free(actual);
+}
+
+
 int main(int argc, char **argv)
 {
+    gsize i;
+
     g_test_init(&argc, &argv, NULL);
 
     g_test_add_func("/cutils/parse_uint/null", test_parse_uint_null);
@@ -1598,5 +1684,15 @@ int main(int argc, char **argv)
     g_test_add_func("/cutils/strtosz/suffix-unit",
                     test_qemu_strtosz_suffix_unit);
 
+    for (i = 0; i < G_N_ELEMENTS(test_qemu_sztostr_data); i++) {
+        g_test_add_data_func(test_qemu_sztostr_data[i].path,
+                             &(test_qemu_sztostr_data[i]),
+                             test_qemu_sztostr);
+    }
+    for (i = 0; i < G_N_ELEMENTS(test_qemu_szutostr_data); i++) {
+        g_test_add_data_func(test_qemu_szutostr_data[i].path,
+                             &(test_qemu_szutostr_data[i]),
+                             test_qemu_szutostr);
+    }
     return g_test_run();
 }
diff --git a/util/cutils.c b/util/cutils.c
index 7505fda..bdc3fc1 100644
--- a/util/cutils.c
+++ b/util/cutils.c
@@ -503,6 +503,84 @@ int64_t qemu_strtosz(const char *nptr, char **end)
     return qemu_strtosz_suffix(nptr, end, QEMU_STRTOSZ_DEFSUFFIX_MB);
 }
 
+
+static char *qemu_sztostr_impl(uint64_t val,
+                               const char default_suffix,
+                               bool long_suffix,
+                               const char *separator,
+                               bool allowSigned)
+{
+    static const char *suffixes[] = {
+        "B", "K", "M", "G", "T", "P", "E" };
+    const char *extra_suffix = "";
+    uint64_t div;
+    int i;
+    const char *sign = "";
+    const char *suffix = "";
+
+    /* The exponent (returned in i) minus one gives us
+     * floor(log2(val * 1024 / 1000).  The correction makes us
+     * switch to the higher power when the integer part is >= 1000.
+     */
+    if (allowSigned && ((int64_t)val < 0)) {
+        val = ((int64_t)val) * -1;
+        sign = "-";
+    }
+
+    frexp(val / (1000.0 / 1024.0), &i);
+    i = (i - 1) / 10;
+    assert(i < ARRAY_SIZE(suffixes));
+    div = 1ULL << (i * 10);
+
+    if (suffixes[i][0] != default_suffix) {
+        suffix = suffixes[i];
+        if (i > 0 && long_suffix) {
+            extra_suffix = "iB";
+        }
+    } else {
+        separator = NULL;
+    }
+
+    return g_strdup_printf("%s%0.3g%s%s%s",
+                           sign,
+                           (double)val / div,
+                           separator ? separator : "",
+                           suffix, extra_suffix);
+}
+
+
+char *qemu_sztostr(int64_t val)
+{
+    return qemu_sztostr_impl((uint64_t)val, '\0', true, "", true);
+}
+
+
+char *qemu_sztostr_full(int64_t val,
+                        const char default_suffix,
+                        bool long_suffix,
+                        const char *separator)
+{
+    return qemu_sztostr_impl((uint64_t)val, default_suffix,
+                             long_suffix, separator, true);
+}
+
+
+char *qemu_szutostr(uint64_t val)
+{
+    return qemu_sztostr_impl(val, '\0', true, "", false);
+}
+
+
+char *qemu_szutostr_full(uint64_t val,
+                         const char default_suffix,
+                         bool long_suffix,
+                         const char *separator)
+{
+    return qemu_sztostr_impl(val, default_suffix,
+                             long_suffix, separator, false);
+}
+
+
 /**
  * Helper function for qemu_strto*l() functions.
  */
-- 
2.7.4

[Qemu-devel] [PATCH v2 2/6] qapi: convert StringOutputVisitor to use qemu_szutostr

[Daniel P. Berrange]

Instead inlining code for formatting sized integers,
call out to the new qemu_szutostr function.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 qapi/string-output-visitor.c       | 20 +++++---------------
 tests/test-string-output-visitor.c | 22 ++++++++++++++++++++++
 2 files changed, 27 insertions(+), 15 deletions(-)

diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c
index 94ac821..dfb15b5 100644
--- a/qapi/string-output-visitor.c
+++ b/qapi/string-output-visitor.c
@@ -15,6 +15,7 @@
 #include "qapi/string-output-visitor.h"
 #include "qapi/visitor-impl.h"
 #include "qemu/host-utils.h"
+#include "qemu/cutils.h"
 #include <math.h>
 #include "qemu/range.h"
 
@@ -211,10 +212,8 @@ static void print_type_size(Visitor *v, const char *name, uint64_t *obj,
                             Error **errp)
 {
     StringOutputVisitor *sov = to_sov(v);
-    static const char suffixes[] = { 'B', 'K', 'M', 'G', 'T', 'P', 'E' };
-    uint64_t div, val;
+    char *szval;
     char *out;
-    int i;
 
     if (!sov->human) {
         out = g_strdup_printf("%"PRIu64, *obj);
@@ -222,20 +221,11 @@ static void print_type_size(Visitor *v, const char *name, uint64_t *obj,
         return;
     }
 
-    val = *obj;
+    szval = qemu_szutostr_full(*obj, '\0', true, " ");
 
-    /* The exponent (returned in i) minus one gives us
-     * floor(log2(val * 1024 / 1000).  The correction makes us
-     * switch to the higher power when the integer part is >= 1000.
-     */
-    frexp(val / (1000.0 / 1024.0), &i);
-    i = (i - 1) / 10;
-    assert(i < ARRAY_SIZE(suffixes));
-    div = 1ULL << (i * 10);
-
-    out = g_strdup_printf("%"PRIu64" (%0.3g %c%s)", val,
-                          (double)val/div, suffixes[i], i ? "iB" : "");
+    out = g_strdup_printf("%"PRIu64" (%s)", *obj, szval);
     string_output_set(sov, out);
+    g_free(szval);
 }
 
 static void print_type_bool(Visitor *v, const char *name, bool *obj,
diff --git a/tests/test-string-output-visitor.c b/tests/test-string-output-visitor.c
index 444844a..dd6e00f 100644
--- a/tests/test-string-output-visitor.c
+++ b/tests/test-string-output-visitor.c
@@ -87,6 +87,24 @@ static void test_visitor_out_int(TestOutputVisitorData *data,
     }
 }
 
+static void test_visitor_out_size(TestOutputVisitorData *data,
+                                  const void *unused)
+{
+    uint64_t value = 1729;
+    Error *err = NULL;
+    char *str;
+
+    visit_type_size(data->ov, NULL, &value, &err);
+    g_assert(!err);
+
+    str = visitor_get(data);
+    if (data->human) {
+        g_assert_cmpstr(str, ==, "1729 (1.69 KiB)");
+    } else {
+        g_assert_cmpstr(str, ==, "1729");
+    }
+}
+
 static void test_visitor_out_intList(TestOutputVisitorData *data,
                                      const void *unused)
 {
@@ -240,6 +258,10 @@ int main(int argc, char **argv)
                             &out_visitor_data, test_visitor_out_int, false);
     output_visitor_test_add("/string-visitor/output/int-human",
                             &out_visitor_data, test_visitor_out_int, true);
+    output_visitor_test_add("/string-visitor/output/size",
+                            &out_visitor_data, test_visitor_out_size, false);
+    output_visitor_test_add("/string-visitor/output/size-human",
+                            &out_visitor_data, test_visitor_out_size, true);
     output_visitor_test_add("/string-visitor/output/bool",
                             &out_visitor_data, test_visitor_out_bool, false);
     output_visitor_test_add("/string-visitor/output/bool-human",
-- 
2.7.4

[Qemu-devel] [PATCH v2 3/6] qapi: assert that visitor impls have required callbacks

[Daniel P. Berrange]

Not all visitor implementations supply the full set of
visitor callback functions. For example, the string
output visitor does not provide 'start_struct' and
friends. If you don't know this and feed it an object
that uses structs, you'll get a crash:

  Segmentation fault (core dumped)

Crashing is fine, because this is a programmer mistake,
but we can improve the error message upon crash to make
it obvious what failed by adding assert()s:

  qapi/qapi-visit-core.c:32: visit_start_struct: Assertion `v->start_struct != ((void *)0)' failed.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 qapi/qapi-visit-core.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index 55f5876..df199b7 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -44,6 +44,7 @@ void visit_start_struct(Visitor *v, const char *name, void **obj,
         assert(size);
         assert(!(v->type & VISITOR_OUTPUT) || *obj);
     }
+    assert(v->start_struct != NULL);
     v->start_struct(v, name, obj, size, &err);
     if (obj && (v->type & VISITOR_INPUT)) {
         assert(!err != !*obj);
@@ -60,6 +61,7 @@ void visit_check_struct(Visitor *v, Error **errp)
 
 void visit_end_struct(Visitor *v, void **obj)
 {
+    assert(v->end_struct != NULL);
     v->end_struct(v, obj);
 }
 
@@ -69,6 +71,7 @@ void visit_start_list(Visitor *v, const char *name, GenericList **list,
     Error *err = NULL;
 
     assert(!list || size >= sizeof(GenericList));
+    assert(v->start_list != NULL);
     v->start_list(v, name, list, size, &err);
     if (list && (v->type & VISITOR_INPUT)) {
         assert(!(err && *list));
@@ -79,11 +82,13 @@ void visit_start_list(Visitor *v, const char *name, GenericList **list,
 GenericList *visit_next_list(Visitor *v, GenericList *tail, size_t size)
 {
     assert(tail && size >= sizeof(GenericList));
+    assert(v->next_list != NULL);
     return v->next_list(v, tail, size);
 }
 
 void visit_end_list(Visitor *v, void **obj)
 {
+    assert(v->end_list != NULL);
     v->end_list(v, obj);
 }
 
@@ -127,6 +132,7 @@ bool visit_is_input(Visitor *v)
 void visit_type_int(Visitor *v, const char *name, int64_t *obj, Error **errp)
 {
     assert(obj);
+    assert(v->type_int64 != NULL);
     v->type_int64(v, name, obj, errp);
 }
 
@@ -136,6 +142,7 @@ static void visit_type_uintN(Visitor *v, uint64_t *obj, const char *name,
     Error *err = NULL;
     uint64_t value = *obj;
 
+    assert(v->type_uint64 != NULL);
     v->type_uint64(v, name, &value, &err);
     if (err) {
         error_propagate(errp, err);
@@ -175,6 +182,7 @@ void visit_type_uint64(Visitor *v, const char *name, uint64_t *obj,
                        Error **errp)
 {
     assert(obj);
+    assert(v->type_uint64 != NULL);
     v->type_uint64(v, name, obj, errp);
 }
 
@@ -185,6 +193,7 @@ static void visit_type_intN(Visitor *v, int64_t *obj, const char *name,
     Error *err = NULL;
     int64_t value = *obj;
 
+    assert(v->type_int64 != NULL);
     v->type_int64(v, name, &value, &err);
     if (err) {
         error_propagate(errp, err);
@@ -233,6 +242,7 @@ void visit_type_size(Visitor *v, const char *name, uint64_t *obj,
     if (v->type_size) {
         v->type_size(v, name, obj, errp);
     } else {
+        assert(v->type_uint64 != NULL);
         v->type_uint64(v, name, obj, errp);
     }
 }
@@ -240,6 +250,7 @@ void visit_type_size(Visitor *v, const char *name, uint64_t *obj,
 void visit_type_bool(Visitor *v, const char *name, bool *obj, Error **errp)
 {
     assert(obj);
+    assert(v->type_bool != NULL);
     v->type_bool(v, name, obj, errp);
 }
 
@@ -252,6 +263,7 @@ void visit_type_str(Visitor *v, const char *name, char **obj, Error **errp)
      * can enable:
     assert(!(v->type & VISITOR_OUTPUT) || *obj);
      */
+    assert(v->type_str != NULL);
     v->type_str(v, name, obj, &err);
     if (v->type & VISITOR_INPUT) {
         assert(!err != !*obj);
@@ -263,6 +275,7 @@ void visit_type_number(Visitor *v, const char *name, double *obj,
                        Error **errp)
 {
     assert(obj);
+    assert(v->type_number != NULL);
     v->type_number(v, name, obj, errp);
 }
 
@@ -272,6 +285,7 @@ void visit_type_any(Visitor *v, const char *name, QObject **obj, Error **errp)
 
     assert(obj);
     assert(v->type != VISITOR_OUTPUT || *obj);
+    assert(v->type_any != NULL);
     v->type_any(v, name, obj, &err);
     if (v->type == VISITOR_INPUT) {
         assert(!err != !*obj);
@@ -281,6 +295,7 @@ void visit_type_any(Visitor *v, const char *name, QObject **obj, Error **errp)
 
 void visit_type_null(Visitor *v, const char *name, Error **errp)
 {
+    assert(v->type_null != NULL);
     v->type_null(v, name, errp);
 }
 
-- 
2.7.4

[Qemu-devel] [PATCH v2 4/6] qapi: add a text output visitor for pretty printing types

[Daniel P. Berrange]

The current approach for pretty-printing QAPI types is to
convert them to JSON using the QMP output visitor and then
pretty-print the JSON document. This has an unfixable problem
that structs get their keys printed out in random order, since
JSON dicts do not contain any key ordering information.

To address this, introduce a text output visitor that can
directly pretty print a QAPI type into a string.

While this might sound superficially similar to the existing
string-output-visitor, it is formatting data in a completely
different way. The string-output-visitor only deals with
scalar values, or arrays of scalar values and formats the
values only into a single line, separated with commas. eg

    foo,bar,wizz

This new text-output-visitor deals with all arbitrarily
nested structs and arrays, and formats the names and value
into multi-line strings, so that previous example would
instead appear as

   foo
   bar
   wizz

Or, if a name was provided

   name: foo
   name: bar
   name: wizz

A more complex multi-level nested example is:

  name: hello
  num: 1729
  accounts:
      [0]:
          num: 1729
          name: hello
      [1]:
          num: 1729
          name: hello
      [2]:
          num: 1729
          name: hello
          info:
              help: world
      [3]:
          num: 1729
          name: hello
      [4]:
          num: 1729
          name: hello
          payments:
              [0]: 1729
              [1]: 1729
              [2]: 1729

As such there is no useful amount of code that can be shared
between the two output visitors and it thus is clearer to
read the code if they are kept separate. The docs for the
string-input-visitor are expanded to make the difference in
output formats clearer.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 include/qapi/string-output-visitor.h |  28 ++-
 include/qapi/text-output-visitor.h   |  77 ++++++++
 qapi/Makefile.objs                   |   1 +
 qapi/text-output-visitor.c           | 349 +++++++++++++++++++++++++++++++++
 tests/Makefile.include               |   5 +-
 tests/test-text-output-visitor.c     | 366 +++++++++++++++++++++++++++++++++++
 6 files changed, 823 insertions(+), 3 deletions(-)
 create mode 100644 include/qapi/text-output-visitor.h
 create mode 100644 qapi/text-output-visitor.c
 create mode 100644 tests/test-text-output-visitor.c

diff --git a/include/qapi/string-output-visitor.h b/include/qapi/string-output-visitor.h
index 268dfe9..920cd7a 100644
--- a/include/qapi/string-output-visitor.h
+++ b/include/qapi/string-output-visitor.h
@@ -21,14 +21,38 @@ typedef struct StringOutputVisitor StringOutputVisitor;
  * Create a new string output visitor.
  *
  * Using @human creates output that is a bit easier for humans to read
- * (for example, showing integer values in both decimal and hex).
+ * (for example, showing integer values in both decimal and hex). The
+ * output will only contain the values for the visited items, not the
+ * field names. When encountering lists of integers, order of the list
+ * elements will not be preserved in the output format. They will be
+ * re-arranged in numerical order and contiguous values merged into
+ * ranges. Strings will have double quotes added. If @human is set
+ * to true, then integers will be printed in both decimal and hexidecimal
+ * format. Some example outputs:
  *
- * If everything else succeeds, pass @result to visit_complete() to
+ * - Single integer (human friendly)
+ *    42 (0x2a)
+ *
+ * - List of integers
+ *   0-1,3-6,9-16,21-22,9223372036854775806-9223372036854775807
+ *
+ * - Boolean
+ *   true
+ *
+ * - Sring
+ *   "QEMU"
+ *
+ * If everything succeeds, pass @result to visit_complete() to
  * collect the result of the visit.
  *
  * The string output visitor does not implement support for visiting
  * QAPI structs, alternates, null, or arbitrary QTypes.  It also
  * requires a non-null list argument to visit_start_list().
+ *
+ * For outputting of complex types, including the field names, the
+ * TextOutputVisitor implementation is likely to be a better choice,
+ * as it can deal with arbitrary nesting and will preserve ordering
+ * of lists of integers.
  */
 Visitor *string_output_visitor_new(bool human, char **result);
 
diff --git a/include/qapi/text-output-visitor.h b/include/qapi/text-output-visitor.h
new file mode 100644
index 0000000..7996efb
--- /dev/null
+++ b/include/qapi/text-output-visitor.h
@@ -0,0 +1,77 @@
+/*
+ * Text pretty printing Visitor
+ *
+ * Copyright Red Hat, Inc. 2016
+ *
+ * Author: Daniel Berrange <berrange@redhat.com>
+ *
+ * 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 TEXT_OUTPUT_VISITOR_H
+#define TEXT_OUTPUT_VISITOR_H
+
+#include "qapi/visitor.h"
+
+typedef struct TextOutputVisitor TextOutputVisitor;
+
+/**
+ * text_output_visitor_new:
+ * @extraIndent: number of extra levels of indent to apply
+ * @skipLevel: skip output of nodes less than depth @skipLevel
+ *
+ * Create a new output visitor for displaying objects
+ * in a pretty-printed text format. The @extraIdent
+ * parameter can be used to add extra levels of whitespace
+ * indentation on the output text. If there are some nodes
+ * at the top level of the QAPI object that should not be
+ * displayed, the @skipLevel parameter can be set to a
+ * non-zero value to hide them.
+ *
+ * The objects are printed in a multi-line indented
+ * fashion, such that each line contains a single
+ * value. Extra indentation is added each time a
+ * compound type (list, struct) is entered.
+ *
+ * To obtain the formatted string, call visit_complete()
+ * passing a pointer to a "char *".
+ *
+ * <example>
+ *   <title>Print of a complex type</title>
+ *   <programlisting>
+ *  name: hello
+ *  num: 1729
+ *  accounts:
+ *      [0]:
+ *          num: 1729
+ *          name: hello
+ *      [1]:
+ *          num: 1729
+ *          name: hello
+ *      [2]:
+ *          num: 1729
+ *          name: hello
+ *          info:
+ *              help: world
+ *      [3]:
+ *          num: 1729
+ *          name: hello
+ *      [4]:
+ *          num: 1729
+ *          name: hello
+ *          payments:
+ *              [0]: 1729
+ *              [1]: 1729
+ *              [2]: 1729
+ *   </programlisting>
+ * </example>
+ *
+ * Returns: a pointer to new output visitor
+ */
+Visitor *text_output_visitor_new(int extraIndent,
+                                 int skipLevel);
+
+
+#endif
diff --git a/qapi/Makefile.objs b/qapi/Makefile.objs
index 7ea4aeb..e702f07 100644
--- a/qapi/Makefile.objs
+++ b/qapi/Makefile.objs
@@ -2,5 +2,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 qapi-clone-visitor.o
+util-obj-y += text-output-visitor.o
 util-obj-y += qmp-event.o
 util-obj-y += qapi-util.o
diff --git a/qapi/text-output-visitor.c b/qapi/text-output-visitor.c
new file mode 100644
index 0000000..6d499d0
--- /dev/null
+++ b/qapi/text-output-visitor.c
@@ -0,0 +1,349 @@
+/*
+ * Text pretty printing Visitor
+ *
+ * Copyright Red Hat, Inc. 2016
+ *
+ * Author: Daniel Berrange <berrange@redhat.com>
+ *
+ * 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 "qemu/cutils.h"
+#include "qemu-common.h"
+#include "qapi/text-output-visitor.h"
+#include "qapi/visitor-impl.h"
+#include <math.h>
+
+struct TextOutputVisitorState {
+    bool isList;
+    size_t listIndex;
+
+    QSIMPLEQ_ENTRY(TextOutputVisitorState) next;
+};
+
+struct TextOutputVisitor {
+    Visitor visitor;
+    GString *string;
+    int level;
+    int skipLevel;
+    int extraIndent;
+
+    QSIMPLEQ_HEAD(TextOutputVisitorStateHead, TextOutputVisitorState) state;
+};
+
+#define INDENT (tov->extraIndent + ((tov->level - tov->skipLevel) * 4))
+
+static TextOutputVisitor *
+text_output_visitor_from_visitor(Visitor *v)
+{
+    return container_of(v, TextOutputVisitor, visitor);
+}
+
+
+static void
+text_output_visitor_open_compound_type(struct TextOutputVisitor *tov,
+                                       bool isList)
+{
+    struct TextOutputVisitorState *currstate = QSIMPLEQ_FIRST(&tov->state);
+    struct TextOutputVisitorState *state =
+        g_new0(struct TextOutputVisitorState, 1);
+
+    if (currstate && currstate->isList) {
+        g_string_append_printf(tov->string, "\n");
+    }
+    state->isList = isList;
+
+    QSIMPLEQ_INSERT_HEAD(&tov->state, state, next);
+
+    tov->level++;
+}
+
+
+static void
+text_output_visitor_close_compound_type(struct TextOutputVisitor *tov)
+{
+    struct TextOutputVisitorState *state = QSIMPLEQ_FIRST(&tov->state);
+
+    tov->level--;
+
+    QSIMPLEQ_REMOVE_HEAD(&tov->state, next);
+
+    g_free(state);
+}
+
+
+static void
+text_output_visitor_print_list_index(struct TextOutputVisitor *tov)
+{
+    struct TextOutputVisitorState *state = QSIMPLEQ_FIRST(&tov->state);
+
+    if (!state || !state->isList) {
+        return;
+    }
+
+    if (tov->level >= tov->skipLevel) {
+        g_string_append_printf(tov->string,
+                               "%*s[%zu]:",
+                               INDENT, "", state->listIndex++);
+    }
+}
+
+
+static char *
+text_output_visitor_format_name(const char *name)
+{
+    if (!name) {
+        return g_strdup("<anon>");
+    } else {
+        char *ret = g_strdup(name);
+        gsize i;
+        for (i = 0; ret[i]; i++) {
+            if (ret[i] == '-') {
+                ret[i] = ' ';
+            }
+        }
+        return ret;
+    }
+}
+
+static void
+text_output_visitor_print_scalar(TextOutputVisitor *tov,
+                                 const char *name,
+                                 const char *fmt,
+                                 ...)
+{
+    struct TextOutputVisitorState *state = QSIMPLEQ_FIRST(&tov->state);
+    va_list vargs;
+    char *val;
+    char *key;
+
+    va_start(vargs, fmt);
+
+    text_output_visitor_print_list_index(tov);
+
+    val = g_strdup_vprintf(fmt, vargs);
+
+    if ((!state && !name) ||
+        (state && state->isList)) {
+        g_string_append_printf(tov->string,
+                               !state ? "%s\n" : " %s\n", val);
+    } else {
+        key = text_output_visitor_format_name(name);
+        g_string_append_printf(tov->string,
+                               "%*s%s: %s\n",
+                               INDENT, "", key, val);
+        g_free(key);
+    }
+
+    g_free(val);
+    va_end(vargs);
+}
+
+
+static void
+text_output_visitor_print_type_int64(Visitor *v, const char *name, int64_t *obj,
+                                     Error **errp)
+{
+    TextOutputVisitor *tov = text_output_visitor_from_visitor(v);
+
+    if (tov->level < tov->skipLevel) {
+        return;
+    }
+
+    text_output_visitor_print_scalar(tov, name, "%" PRIu64, *obj);
+}
+
+
+static void
+text_output_visitor_print_type_uint64(Visitor *v, const char *name,
+                                      uint64_t *obj, Error **errp)
+{
+    TextOutputVisitor *tov = text_output_visitor_from_visitor(v);
+
+    if (tov->level < tov->skipLevel) {
+        return;
+    }
+
+    text_output_visitor_print_scalar(tov, name, "%" PRIi64, *obj);
+}
+
+
+static void
+text_output_visitor_print_type_size(Visitor *v, const char *name, uint64_t *obj,
+                                    Error **errp)
+{
+    TextOutputVisitor *tov = text_output_visitor_from_visitor(v);
+    char *szval;
+
+    if (tov->level < tov->skipLevel) {
+        return;
+    }
+
+    szval = qemu_szutostr_full(*obj, '\0', true, " ");
+    text_output_visitor_print_scalar(tov, name, "%" PRIu64 " (%s)",
+                                     *obj, szval);
+    g_free(szval);
+}
+
+
+static void
+text_output_visitor_print_type_bool(Visitor *v, const char *name, bool *obj,
+                                    Error **errp)
+{
+    TextOutputVisitor *tov = text_output_visitor_from_visitor(v);
+
+    if (tov->level < tov->skipLevel) {
+        return;
+    }
+
+    text_output_visitor_print_scalar(tov, name, "%s", *obj ? "true" : "false");
+}
+
+
+static void
+text_output_visitor_print_type_str(Visitor *v, const char *name, char **obj,
+                                   Error **errp)
+{
+    TextOutputVisitor *tov = text_output_visitor_from_visitor(v);
+
+    if (tov->level < tov->skipLevel) {
+        return;
+    }
+
+    text_output_visitor_print_scalar(tov, name, "%s", *obj ? *obj : "<null>");
+}
+
+
+static void
+text_output_visitor_print_type_number(Visitor *v, const char *name, double *obj,
+                                      Error **errp)
+{
+    TextOutputVisitor *tov = text_output_visitor_from_visitor(v);
+
+    if (tov->level < tov->skipLevel) {
+        return;
+    }
+
+    text_output_visitor_print_scalar(tov, name, "%f", *obj);
+}
+
+
+static void
+text_output_visitor_start_list(Visitor *v, const char *name, GenericList **list,
+                               size_t size, Error **errp)
+{
+    TextOutputVisitor *tov = text_output_visitor_from_visitor(v);
+    char *key;
+
+    if (tov->level >= tov->skipLevel && name) {
+        key = text_output_visitor_format_name(name);
+        g_string_append_printf(tov->string,
+                               "%*s%s:\n",
+                               INDENT, "",
+                               key);
+        g_free(key);
+    }
+    text_output_visitor_open_compound_type(tov, true);
+}
+
+
+static GenericList *
+text_output_visitor_next_list(Visitor *v, GenericList *tail, size_t size)
+{
+    GenericList *ret = tail->next;
+    return ret;
+}
+
+
+static void
+text_output_visitor_end_list(Visitor *v, void **obj)
+{
+    TextOutputVisitor *tov = text_output_visitor_from_visitor(v);
+
+    text_output_visitor_close_compound_type(tov);
+}
+
+
+static void
+text_output_visitor_start_struct(Visitor *v, const char *name, void **obj,
+                                 size_t size, Error **errp)
+{
+    TextOutputVisitor *tov = text_output_visitor_from_visitor(v);
+    struct TextOutputVisitorState *state = QSIMPLEQ_FIRST(&tov->state);
+
+    if (tov->level >= tov->skipLevel && name &&
+        state && !state->isList) {
+        g_string_append_printf(tov->string,
+                               "%*s%s:\n",
+                               INDENT, "",
+                               name);
+    }
+    text_output_visitor_print_list_index(tov);
+    text_output_visitor_open_compound_type(tov, false);
+}
+
+
+static void
+text_output_visitor_end_struct(Visitor *v, void **obj)
+{
+    TextOutputVisitor *tov = text_output_visitor_from_visitor(v);
+    text_output_visitor_close_compound_type(tov);
+}
+
+
+static void
+text_output_visitor_complete(Visitor *v, void *opaque)
+{
+    TextOutputVisitor *tov = text_output_visitor_from_visitor(v);
+    char **result = opaque;
+
+    *result = g_string_free(tov->string, false);
+    tov->string = NULL;
+}
+
+
+static void
+text_output_visitor_free(Visitor *v)
+{
+    TextOutputVisitor *tov = text_output_visitor_from_visitor(v);
+
+    if (tov->string) {
+        g_string_free(tov->string, true);
+    }
+    g_free(tov);
+}
+
+
+Visitor *
+text_output_visitor_new(int extraIndent,
+                        int skipLevel)
+{
+    TextOutputVisitor *v;
+
+    v = g_malloc0(sizeof(*v));
+
+    v->extraIndent = extraIndent;
+    v->skipLevel = skipLevel;
+    v->string = g_string_new(NULL);
+    v->visitor.type = VISITOR_OUTPUT;
+    v->visitor.type_int64 = text_output_visitor_print_type_int64;
+    v->visitor.type_uint64 = text_output_visitor_print_type_uint64;
+    v->visitor.type_size = text_output_visitor_print_type_size;
+    v->visitor.type_bool = text_output_visitor_print_type_bool;
+    v->visitor.type_str = text_output_visitor_print_type_str;
+    v->visitor.type_number = text_output_visitor_print_type_number;
+    v->visitor.start_list = text_output_visitor_start_list;
+    v->visitor.next_list = text_output_visitor_next_list;
+    v->visitor.end_list = text_output_visitor_end_list;
+    v->visitor.start_struct = text_output_visitor_start_struct;
+    v->visitor.end_struct = text_output_visitor_end_struct;
+    v->visitor.complete = text_output_visitor_complete;
+    v->visitor.free = text_output_visitor_free;
+
+    QSIMPLEQ_INIT(&v->state);
+
+    return &v->visitor;
+}
diff --git a/tests/Makefile.include b/tests/Makefile.include
index e3a3266..aa1188f 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -33,6 +33,8 @@ check-unit-y += tests/test-string-input-visitor$(EXESUF)
 gcov-files-test-string-input-visitor-y = qapi/string-input-visitor.c
 check-unit-y += tests/test-string-output-visitor$(EXESUF)
 gcov-files-test-string-output-visitor-y = qapi/string-output-visitor.c
+check-unit-y += tests/test-text-output-visitor$(EXESUF)
+gcov-files-test-text-output-visitor-y = qapi/text-output-visitor.c
 check-unit-y += tests/test-qmp-event$(EXESUF)
 gcov-files-test-qmp-event-y += qapi/qmp-event.c
 check-unit-y += tests/test-opts-visitor$(EXESUF)
@@ -433,7 +435,7 @@ test-obj-y = tests/check-qint.o tests/check-qstring.o tests/check-qdict.o \
 	tests/check-qjson.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-clone-visitor.o \
+	tests/test-clone-visitor.o tests/test-text-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 \
@@ -530,6 +532,7 @@ $(SRC_PATH)/tests/qapi-schema/qapi-schema-test.json $(SRC_PATH)/scripts/qapi-int
 
 tests/test-string-output-visitor$(EXESUF): tests/test-string-output-visitor.o $(test-qapi-obj-y)
 tests/test-string-input-visitor$(EXESUF): tests/test-string-input-visitor.o $(test-qapi-obj-y)
+tests/test-text-output-visitor$(EXESUF): tests/test-text-output-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-clone-visitor$(EXESUF): tests/test-clone-visitor.o $(test-qapi-obj-y)
diff --git a/tests/test-text-output-visitor.c b/tests/test-text-output-visitor.c
new file mode 100644
index 0000000..6382732
--- /dev/null
+++ b/tests/test-text-output-visitor.c
@@ -0,0 +1,366 @@
+/*
+ * String Output Visitor unit-tests.
+ *
+ * Copyright (C) 2012 Red Hat Inc.
+ *
+ * Authors:
+ *  Paolo Bonzini <pbonzini@redhat.com> (based on test-qmp-output-visitor)
+ *
+ * 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 "qemu-common.h"
+#include "qapi/error.h"
+#include "qapi/text-output-visitor.h"
+#include "test-qapi-types.h"
+#include "test-qapi-visit.h"
+#include "qapi/qmp/types.h"
+
+
+static void test_visitor_out_int(void)
+{
+    int64_t value = 42;
+    char *str;
+    Visitor *v;
+
+    v = text_output_visitor_new(0, 0);
+    g_assert(v);
+
+    visit_type_int(v, NULL, &value, &error_abort);
+
+    visit_complete(v, &str);
+    g_assert_cmpstr(str, ==, "42\n");
+    g_free(str);
+    visit_free(v);
+}
+
+
+static void test_visitor_out_size(void)
+{
+    uint64_t value = 1729;
+    char *str;
+    Visitor *v;
+
+    v = text_output_visitor_new(0, 0);
+    g_assert(v);
+
+    visit_type_size(v, NULL, &value, &error_abort);
+
+    visit_complete(v, &str);
+    g_assert_cmpstr(str, ==, "1729 (1.69 KiB)\n");
+    g_free(str);
+    visit_free(v);
+}
+
+static void test_visitor_out_intList(void)
+{
+    int64_t value[] = {0, 1, 9, 10, 16, 15, 14,
+        3, 4, 5, 6, 11, 12, 13, 21, 22, INT64_MAX - 1, INT64_MAX};
+    intList *list = NULL, **tmp = &list;
+    int i;
+    char *str;
+    Visitor *v;
+
+    v = text_output_visitor_new(0, 0);
+    g_assert(v);
+
+    for (i = 0; i < sizeof(value) / sizeof(value[0]); i++) {
+        *tmp = g_malloc0(sizeof(**tmp));
+        (*tmp)->value = value[i];
+        tmp = &(*tmp)->next;
+    }
+
+    visit_type_intList(v, NULL, &list, &error_abort);
+
+    visit_complete(v, &str);
+    g_assert_cmpstr(str, ==,
+                    "    [0]: 0\n"
+                    "    [1]: 1\n"
+                    "    [2]: 9\n"
+                    "    [3]: 10\n"
+                    "    [4]: 16\n"
+                    "    [5]: 15\n"
+                    "    [6]: 14\n"
+                    "    [7]: 3\n"
+                    "    [8]: 4\n"
+                    "    [9]: 5\n"
+                    "    [10]: 6\n"
+                    "    [11]: 11\n"
+                    "    [12]: 12\n"
+                    "    [13]: 13\n"
+                    "    [14]: 21\n"
+                    "    [15]: 22\n"
+                    "    [16]: 9223372036854775806\n"
+                    "    [17]: 9223372036854775807\n");
+    qapi_free_intList(list);
+    g_free(str);
+    visit_free(v);
+}
+
+static void test_visitor_out_bool(void)
+{
+    bool value = true;
+    char *str;
+    Visitor *v;
+
+    v = text_output_visitor_new(0, 0);
+    g_assert(v);
+
+    visit_type_bool(v, NULL, &value, &error_abort);
+
+    visit_complete(v, &str);
+    g_assert_cmpstr(str, ==, "true\n");
+    g_free(str);
+    visit_free(v);
+}
+
+static void test_visitor_out_number(void)
+{
+    double value = 3.14;
+    char *str;
+    Visitor *v;
+
+    v = text_output_visitor_new(0, 0);
+    g_assert(v);
+
+    visit_type_number(v, NULL, &value, &error_abort);
+
+    visit_complete(v, &str);
+    g_assert_cmpstr(str, ==, "3.140000\n");
+    g_free(str);
+    visit_free(v);
+}
+
+static void test_visitor_out_string(void)
+{
+    const char *string = "Q E M U";
+    char *str;
+    Visitor *v;
+
+    v = text_output_visitor_new(0, 0);
+    g_assert(v);
+
+    visit_type_str(v, NULL, (char **)&string, &error_abort);
+
+    visit_complete(v, &str);
+    g_assert_cmpstr(str, ==, "Q E M U\n");
+    g_free(str);
+    visit_free(v);
+}
+
+static void test_visitor_out_no_string(void)
+{
+    char *string = NULL;
+    char *str;
+    Visitor *v;
+
+    v = text_output_visitor_new(0, 0);
+    g_assert(v);
+
+    /* A null string should return "" */
+    visit_type_str(v, NULL, &string, &error_abort);
+
+    visit_complete(v, &str);
+    g_assert_cmpstr(str, ==, "<null>\n");
+    g_free(str);
+    visit_free(v);
+}
+
+static void test_visitor_out_enum(void)
+{
+    char *actual, *expected;
+    EnumOne i;
+    Visitor *v;
+
+    for (i = 0; i < ENUM_ONE__MAX; i++) {
+        v = text_output_visitor_new(0, 0);
+        g_assert(v);
+
+        visit_type_EnumOne(v, "val", &i, &error_abort);
+
+        visit_complete(v, &actual);
+        expected = g_strdup_printf("val: %s\n", EnumOne_lookup[i]);
+
+        g_assert_cmpstr(actual, ==, expected);
+        g_free(expected);
+        g_free(actual);
+        visit_free(v);
+    }
+}
+
+static void test_visitor_out_enum_errors(void)
+{
+    EnumOne i, bad_values[] = { ENUM_ONE__MAX, -1 };
+    Visitor *v;
+
+    for (i = 0; i < ARRAY_SIZE(bad_values) ; i++) {
+        v = text_output_visitor_new(0, 0);
+        g_assert(v);
+
+        Error *err = NULL;
+        visit_type_EnumOne(v, "unused", &bad_values[i], &err);
+        error_free_or_abort(&err);
+        visit_free(v);
+    }
+}
+
+
+static void test_visitor_out_struct_named(void)
+{
+    const char *string = "hello";
+    int64_t i = 1729;
+    char *str;
+    Visitor *v;
+
+    v = text_output_visitor_new(0, 0);
+    g_assert(v);
+
+    visit_start_struct(v, NULL, NULL, 0, &error_abort);
+
+    visit_type_str(v, "name", (char **)&string, &error_abort);
+
+    visit_type_int(v, "num", &i, &error_abort);
+
+    visit_end_struct(v, NULL);
+    visit_complete(v, &str);
+    g_assert_cmpstr(str, ==,
+                    "    name: hello\n"
+                    "    num: 1729\n");
+    g_free(str);
+    visit_free(v);
+}
+
+
+static void test_visitor_out_struct_anon(void)
+{
+    const char *string = "hello";
+    int64_t i = 1729;
+    char *str;
+    Visitor *v;
+
+    v = text_output_visitor_new(0, 1);
+    g_assert(v);
+
+    visit_start_struct(v, NULL, NULL, 0, &error_abort);
+
+    visit_type_str(v, NULL, (char **)&string, &error_abort);
+
+    visit_type_int(v, NULL, &i, &error_abort);
+
+    visit_end_struct(v, NULL);
+    visit_complete(v, &str);
+    g_assert_cmpstr(str, ==,
+                    "<anon>: hello\n"
+                    "<anon>: 1729\n");
+    g_free(str);
+    visit_free(v);
+}
+
+
+static void test_visitor_out_complex(void)
+{
+    const char *string = "hello";
+    const char *string2 = "world";
+    int64_t n = 1729;
+    char *str;
+    Visitor *v;
+    gsize i;
+
+    v = text_output_visitor_new(0, 0);
+    g_assert(v);
+
+    visit_type_str(v, "full-name", (char **)&string, &error_abort);
+
+    visit_type_int(v, "num", &n, &error_abort);
+
+    visit_start_list(v, "accounts", NULL, 0, &error_abort);
+
+    for (i = 0; i < 5; i++) {
+        visit_start_struct(v, "account", NULL, 0, &error_abort);
+
+        visit_type_int(v, "num", &n, &error_abort);
+        visit_type_str(v, "name", (char **)&string, &error_abort);
+
+        if (i == 2) {
+            visit_start_struct(v, "info", NULL, 0, &error_abort);
+            visit_type_str(v, "help", (char **)&string2, &error_abort);
+            visit_end_struct(v, NULL);
+        } else if (i == 4) {
+            visit_start_list(v, "payment-info", NULL, 0, &error_abort);
+            visit_type_int(v, "num", &n, &error_abort);
+            visit_type_int(v, "num", &n, &error_abort);
+            visit_type_int(v, "num", &n, &error_abort);
+            visit_end_list(v, NULL);
+        }
+
+        visit_end_struct(v, NULL);
+    }
+
+    visit_end_list(v, NULL);
+
+    visit_complete(v, &str);
+    g_assert_cmpstr(str, ==,
+                    "full name: hello\n"
+                    "num: 1729\n"
+                    "accounts:\n"
+                    "    [0]:\n"
+                    "        num: 1729\n"
+                    "        name: hello\n"
+                    "    [1]:\n"
+                    "        num: 1729\n"
+                    "        name: hello\n"
+                    "    [2]:\n"
+                    "        num: 1729\n"
+                    "        name: hello\n"
+                    "        info:\n"
+                    "            help: world\n"
+                    "    [3]:\n"
+                    "        num: 1729\n"
+                    "        name: hello\n"
+                    "    [4]:\n"
+                    "        num: 1729\n"
+                    "        name: hello\n"
+                    "        payment info:\n"
+                    "            [0]: 1729\n"
+                    "            [1]: 1729\n"
+                    "            [2]: 1729\n");
+    g_free(str);
+    visit_free(v);
+}
+
+
+int main(int argc, char **argv)
+{
+    g_test_init(&argc, &argv, NULL);
+
+    g_test_add_func("/string-visitor/output/int",
+                    test_visitor_out_int);
+    g_test_add_func("/string-visitor/output/size",
+                    test_visitor_out_size);
+    g_test_add_func("/string-visitor/output/bool",
+                    test_visitor_out_bool);
+    g_test_add_func("/string-visitor/output/number",
+                    test_visitor_out_number);
+    g_test_add_func("/string-visitor/output/string",
+                    test_visitor_out_string);
+    g_test_add_func("/string-visitor/output/no-string",
+                    test_visitor_out_no_string);
+    g_test_add_func("/string-visitor/output/enum",
+                    test_visitor_out_enum);
+    g_test_add_func("/string-visitor/output/enum-errors",
+                    test_visitor_out_enum_errors);
+    g_test_add_func("/string-visitor/output/intList",
+                    test_visitor_out_intList);
+    g_test_add_func("/string-visitor/output/struct-named",
+                    test_visitor_out_struct_named);
+    g_test_add_func("/string-visitor/output/struct-anon",
+                    test_visitor_out_struct_anon);
+    g_test_add_func("/string-visitor/output/complex",
+                    test_visitor_out_complex);
+    g_test_run();
+
+    return 0;
+}
-- 
2.7.4

[Qemu-devel] [PATCH v2 5/6] block: convert to use the qemu_szutostr functions

[Daniel P. Berrange]

Remove the get_human_readable_size() method in favour of using
the new common qemu_szutostr() method. There are two slight
differences in output format

 - the new code will also deal with petabyte and exabyte suffixes,
   where as old code stopped at the terrabyte level.
 - if the value has a trailing '.0' the decimal point will be
   omitted altogether. ie,  '5M' instead of '5.0M'

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 block/qapi.c               | 63 +++++++++++++++-------------------------------
 tests/qemu-iotests/095.out |  2 +-
 tests/qemu-iotests/104.out |  2 +-
 3 files changed, 22 insertions(+), 45 deletions(-)

diff --git a/block/qapi.c b/block/qapi.c
index 6f947e3..0f59d9c 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -535,43 +535,15 @@ BlockStatsList *qmp_query_blockstats(bool has_query_nodes,
     return head;
 }
 
-#define NB_SUFFIXES 4
-
-static char *get_human_readable_size(char *buf, int buf_size, int64_t size)
-{
-    static const char suffixes[NB_SUFFIXES] = {'K', 'M', 'G', 'T'};
-    int64_t base;
-    int i;
-
-    if (size <= 999) {
-        snprintf(buf, buf_size, "%" PRId64, size);
-    } else {
-        base = 1024;
-        for (i = 0; i < NB_SUFFIXES; i++) {
-            if (size < (10 * base)) {
-                snprintf(buf, buf_size, "%0.1f%c",
-                         (double)size / base,
-                         suffixes[i]);
-                break;
-            } else if (size < (1000 * base) || i == (NB_SUFFIXES - 1)) {
-                snprintf(buf, buf_size, "%" PRId64 "%c",
-                         ((size + (base >> 1)) / base),
-                         suffixes[i]);
-                break;
-            }
-            base = base * 1024;
-        }
-    }
-    return buf;
-}
 
 void bdrv_snapshot_dump(fprintf_function func_fprintf, void *f,
                         QEMUSnapshotInfo *sn)
 {
-    char buf1[128], date_buf[128], clock_buf[128];
+    char date_buf[128], clock_buf[128];
     struct tm tm;
     time_t ti;
     int64_t secs;
+    char *szval;
 
     if (!sn) {
         func_fprintf(f,
@@ -589,13 +561,14 @@ void bdrv_snapshot_dump(fprintf_function func_fprintf, void *f,
                  (int)((secs / 60) % 60),
                  (int)(secs % 60),
                  (int)((sn->vm_clock_nsec / 1000000) % 1000));
+        szval = qemu_szutostr_full(sn->vm_state_size,
+                                   QEMU_STRTOSZ_DEFSUFFIX_B,
+                                   false, "");
         func_fprintf(f,
                      "%-10s%-20s%7s%20s%15s",
-                     sn->id_str, sn->name,
-                     get_human_readable_size(buf1, sizeof(buf1),
-                                             sn->vm_state_size),
-                     date_buf,
-                     clock_buf);
+                     sn->id_str, sn->name, szval,
+                     date_buf, clock_buf);
+        g_free(szval);
     }
 }
 
@@ -704,22 +677,26 @@ void bdrv_image_info_specific_dump(fprintf_function func_fprintf, void *f,
 void bdrv_image_info_dump(fprintf_function func_fprintf, void *f,
                           ImageInfo *info)
 {
-    char size_buf[128], dsize_buf[128];
+    char *szval, *dszval;
     if (!info->has_actual_size) {
-        snprintf(dsize_buf, sizeof(dsize_buf), "unavailable");
+        dszval = g_strdup("unavailable");
     } else {
-        get_human_readable_size(dsize_buf, sizeof(dsize_buf),
-                                info->actual_size);
+        dszval = qemu_szutostr_full(info->actual_size,
+                                    QEMU_STRTOSZ_DEFSUFFIX_B,
+                                    false, "");
     }
-    get_human_readable_size(size_buf, sizeof(size_buf), info->virtual_size);
+    szval = qemu_szutostr_full(info->virtual_size,
+                               QEMU_STRTOSZ_DEFSUFFIX_B,
+                               false, "");
     func_fprintf(f,
                  "image: %s\n"
                  "file format: %s\n"
                  "virtual size: %s (%" PRId64 " bytes)\n"
                  "disk size: %s\n",
-                 info->filename, info->format, size_buf,
-                 info->virtual_size,
-                 dsize_buf);
+                 info->filename, info->format, szval,
+                 info->virtual_size, dszval);
+    g_free(szval);
+    g_free(dszval);
 
     if (info->has_encrypted && info->encrypted) {
         func_fprintf(f, "encrypted: yes\n");
diff --git a/tests/qemu-iotests/095.out b/tests/qemu-iotests/095.out
index 73875ca..96c02f6 100644
--- a/tests/qemu-iotests/095.out
+++ b/tests/qemu-iotests/095.out
@@ -6,7 +6,7 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=104857600 backing_file=TEST_DIR/
 === Base image info before commit and resize ===
 image: TEST_DIR/t.IMGFMT.base
 file format: IMGFMT
-virtual size: 5.0M (5242880 bytes)
+virtual size: 5M (5242880 bytes)
 
 === Running QEMU Live Commit Test ===
 
diff --git a/tests/qemu-iotests/104.out b/tests/qemu-iotests/104.out
index ab8d892..f40b629 100644
--- a/tests/qemu-iotests/104.out
+++ b/tests/qemu-iotests/104.out
@@ -4,7 +4,7 @@ QA output created by 104
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1024
 image: TEST_DIR/t.IMGFMT
 file format: IMGFMT
-virtual size: 1.0K (1024 bytes)
+virtual size: 1K (1024 bytes)
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1234
 image: TEST_DIR/t.IMGFMT
 file format: IMGFMT
-- 
2.7.4

[Qemu-devel] [PATCH v2 6/6] block: convert to use qapi_stringify_ImageInfoSpecific

[Daniel P. Berrange]

When 'qemu-img info' prints out format specific information,
it first converts the QAPI object into a JSON based QObject
data structure. Unfortunately structs have to be turned into
dicts, which looses all information about field ordering,
so the data printed appears in a semi-random order.

Converting this to use the TextOutputVisitor allows the QAPI
type to be directly pretty-printed without any conversion,
thus preserving struct field ordering in the output.

The output format structure to the old code, only the ordering
of fields is changed.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 block/qapi.c | 100 ++++-------------------------------------------------------
 1 file changed, 6 insertions(+), 94 deletions(-)

diff --git a/block/qapi.c b/block/qapi.c
index 0f59d9c..ff98ac8 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -29,7 +29,7 @@
 #include "block/write-threshold.h"
 #include "qmp-commands.h"
 #include "qapi-visit.h"
-#include "qapi/qmp-output-visitor.h"
+#include "qapi/text-output-visitor.h"
 #include "qapi/qmp/types.h"
 #include "sysemu/block-backend.h"
 #include "qemu/cutils.h"
@@ -572,105 +572,17 @@ void bdrv_snapshot_dump(fprintf_function func_fprintf, void *f,
     }
 }
 
-static void dump_qdict(fprintf_function func_fprintf, void *f, int indentation,
-                       QDict *dict);
-static void dump_qlist(fprintf_function func_fprintf, void *f, int indentation,
-                       QList *list);
-
-static void dump_qobject(fprintf_function func_fprintf, void *f,
-                         int comp_indent, QObject *obj)
-{
-    switch (qobject_type(obj)) {
-        case QTYPE_QINT: {
-            QInt *value = qobject_to_qint(obj);
-            func_fprintf(f, "%" PRId64, qint_get_int(value));
-            break;
-        }
-        case QTYPE_QSTRING: {
-            QString *value = qobject_to_qstring(obj);
-            func_fprintf(f, "%s", qstring_get_str(value));
-            break;
-        }
-        case QTYPE_QDICT: {
-            QDict *value = qobject_to_qdict(obj);
-            dump_qdict(func_fprintf, f, comp_indent, value);
-            break;
-        }
-        case QTYPE_QLIST: {
-            QList *value = qobject_to_qlist(obj);
-            dump_qlist(func_fprintf, f, comp_indent, value);
-            break;
-        }
-        case QTYPE_QFLOAT: {
-            QFloat *value = qobject_to_qfloat(obj);
-            func_fprintf(f, "%g", qfloat_get_double(value));
-            break;
-        }
-        case QTYPE_QBOOL: {
-            QBool *value = qobject_to_qbool(obj);
-            func_fprintf(f, "%s", qbool_get_bool(value) ? "true" : "false");
-            break;
-        }
-        default:
-            abort();
-    }
-}
-
-static void dump_qlist(fprintf_function func_fprintf, void *f, int indentation,
-                       QList *list)
-{
-    const QListEntry *entry;
-    int i = 0;
-
-    for (entry = qlist_first(list); entry; entry = qlist_next(entry), i++) {
-        QType type = qobject_type(entry->value);
-        bool composite = (type == QTYPE_QDICT || type == QTYPE_QLIST);
-        func_fprintf(f, "%*s[%i]:%c", indentation * 4, "", i,
-                     composite ? '\n' : ' ');
-        dump_qobject(func_fprintf, f, indentation + 1, entry->value);
-        if (!composite) {
-            func_fprintf(f, "\n");
-        }
-    }
-}
-
-static void dump_qdict(fprintf_function func_fprintf, void *f, int indentation,
-                       QDict *dict)
-{
-    const QDictEntry *entry;
-
-    for (entry = qdict_first(dict); entry; entry = qdict_next(dict, entry)) {
-        QType type = qobject_type(entry->value);
-        bool composite = (type == QTYPE_QDICT || type == QTYPE_QLIST);
-        char *key = g_malloc(strlen(entry->key) + 1);
-        int i;
-
-        /* replace dashes with spaces in key (variable) names */
-        for (i = 0; entry->key[i]; i++) {
-            key[i] = entry->key[i] == '-' ? ' ' : entry->key[i];
-        }
-        key[i] = 0;
-        func_fprintf(f, "%*s%s:%c", indentation * 4, "", key,
-                     composite ? '\n' : ' ');
-        dump_qobject(func_fprintf, f, indentation + 1, entry->value);
-        if (!composite) {
-            func_fprintf(f, "\n");
-        }
-        g_free(key);
-    }
-}
 
 void bdrv_image_info_specific_dump(fprintf_function func_fprintf, void *f,
                                    ImageInfoSpecific *info_spec)
 {
-    QObject *obj, *data;
-    Visitor *v = qmp_output_visitor_new(&obj);
+    Visitor *v = text_output_visitor_new(4, 2);
+    char *str;
 
     visit_type_ImageInfoSpecific(v, NULL, &info_spec, &error_abort);
-    visit_complete(v, &obj);
-    assert(qobject_type(obj) == QTYPE_QDICT);
-    data = qdict_get(qobject_to_qdict(obj), "data");
-    dump_qobject(func_fprintf, f, 1, data);
+    visit_complete(v, &str);
+    func_fprintf(f, "%s", str);
+    g_free(str);
     visit_free(v);
 }
 
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 0/6] Stable output of blockdev format specific info

[Markus Armbruster]

"Daniel P. Berrange" <berrange@redhat.com> writes:

> This is an update of the bits of this previous
> series which were not merged
>
>   https://lists.gnu.org/archive/html/qemu-devel/2016-06/msg01723.html
>
> The problem addressed in this series is that the
> 'qemu-img info' command does not have a stable
> output format for the image specific info objects.
>
> The QAPI types are first converted to QDicts,
> and then printed with custom code to recurse over
> the QDicts. This causes information about the object
> field ordering to be thrown away, and fields are
> printed in whatever order they appear in the QDict
> hash buckets. This is not a big deal historically
> since none of the image formats had nested data
> structures, but with the LUKS blockdev this style
> of random ordering looks very unpleasant.

QDict could be changed to support iteration in insertion order.

> To address this, the patch series introduces a
> TextOutputVisitor class that is designed to be
> able to print out arbitrarily nested QAPI types
> directly,

What order does it use?

>           in a format that is identical to that
> currently used with 'qemu-img info'. Consult
> the patch in question to actually see the output
> format, and compare test-string-output-visitor.c
> with test-text-output-visitor.c to see how we
> really do have 2 completely distinct output formats
> that don't share any significant characteristics
> beyond both being "plain text".

I haven't done that, yet, but I take your word for it.  Begs the
question whether we really *need* two different output formats!  I guess
we better discuss that in the context of the patch, where we can
actually see the formats.

Re: [Qemu-devel] [PATCH v2 0/6] Stable output of blockdev format specific info

[Daniel P. Berrange]

On Tue, Sep 13, 2016 at 03:44:21PM +0200, Markus Armbruster wrote:
> "Daniel P. Berrange" <berrange@redhat.com> writes:
> 
> > This is an update of the bits of this previous
> > series which were not merged
> >
> >   https://lists.gnu.org/archive/html/qemu-devel/2016-06/msg01723.html
> >
> > The problem addressed in this series is that the
> > 'qemu-img info' command does not have a stable
> > output format for the image specific info objects.
> >
> > The QAPI types are first converted to QDicts,
> > and then printed with custom code to recurse over
> > the QDicts. This causes information about the object
> > field ordering to be thrown away, and fields are
> > printed in whatever order they appear in the QDict
> > hash buckets. This is not a big deal historically
> > since none of the image formats had nested data
> > structures, but with the LUKS blockdev this style
> > of random ordering looks very unpleasant.
> 
> QDict could be changed to support iteration in insertion order.

True, at the cost of extra data storage for each QDict
to maintain details of the insertion order of its keys.

> > To address this, the patch series introduces a
> > TextOutputVisitor class that is designed to be
> > able to print out arbitrarily nested QAPI types
> > directly,
> 
> What order does it use?

It uses whatever order the generated visitor visits
in, which AFAICT, is the same order in which fields
are declared in the QAPI schema struct definitions.

> 
> >           in a format that is identical to that
> > currently used with 'qemu-img info'. Consult
> > the patch in question to actually see the output
> > format, and compare test-string-output-visitor.c
> > with test-text-output-visitor.c to see how we
> > really do have 2 completely distinct output formats
> > that don't share any significant characteristics
> > beyond both being "plain text".
> 
> I haven't done that, yet, but I take your word for it.  Begs the
> question whether we really *need* two different output formats!  I guess
> we better discuss that in the context of the patch, where we can
> actually see the formats.

Since posting this I have been looking at where string-{input,output}-visitor
is used, and I think there is probably scope for eliminating many uses,
and converting other uses to text-output-visitor. It might even be possible
to get everything able to use text-output-visitor with a little work, at
which point we might as well call it string-output-visitor again ;-)
Investigation is ongoing.... The main point is all around the code which
special cases lists of ints in string input/ouput visitor :-(

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|