Re: [Qemu-block] [PATCH v3 14/15] qemu-img: Use QAPI visitor to generate JSON

[Eric Blake]

On 11/25/2015 10:05 PM, Fam Zheng wrote:
> A visible improvement is that "filename" is now included in the output
> if it's valid.
> 
> Reviewed-by: Eric Blake 
> Signed-off-by: Fam Zheng 
> ---
>  qemu-img.c | 34 ++--
>  tests/qemu-iotests/122.out | 96 
> ++
>  2 files changed, 77 insertions(+), 53 deletions(-)
> 
> @@ -2157,20 +2163,26 @@ static void dump_map_entry(OutputFormat 
> output_format, MapEntry *e,
>  }
>  break;
>  case OFORMAT_JSON:
> -printf("%s{ \"start\": %"PRId64", \"length\": %"PRId64","

The space after '{' here is interesting below [1]

> -   " \"depth\": %"PRId64", \"zero\": %s, \"data\": %s",
> -   (e->start == 0 ? "[" : ",\n"),
> -   e->start, e->length, e->depth,
> -   e->zero ? "true" : "false",
> -   e->data ? "true" : "false");
> -if (e->has_offset) {
> -printf(", \"offset\": %"PRId64"", e->offset);
> -}
> -putchar('}');
> +ov = qmp_output_visitor_new();
> +visit_type_MapEntry(qmp_output_get_visitor(ov),
> +&e, NULL, &error_abort);
> +obj = qmp_output_get_qobject(ov);
> +str = qobject_to_json(obj);
> +assert(str != NULL);

For what it's worth, I have a patch series on my local tree that adds
json_output_visitor_new(), and which would not only bypass the wasted
QObject by doing a multi-step qapi=>QObject=>JSON, but it also has the
benefit of outputting JSON that is in qapi order (under your control)
rather than QDict hash order (deterministic and still valid JSON, but
hard to predict and not always the nicest results for human readers).

> +++ b/tests/qemu-iotests/122.out
> @@ -49,12 +49,13 @@ read 65536/65536 bytes at offset 4194304
>  64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>  read 65536/65536 bytes at offset 8388608
>  64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> -[{ "start": 0, "length": 65536, "depth": 0, "zero": false, "data": true},
> -{ "start": 65536, "length": 4128768, "depth": 0, "zero": true, "data": 
> false},
> -{ "start": 4194304, "length": 65536, "depth": 0, "zero": false, "data": 
> true},
> -{ "start": 4259840, "length": 4128768, "depth": 0, "zero": true, "data": 
> false},
> -{ "start": 8388608, "length": 65536, "depth": 0, "zero": false, "data": 
> true},
> -{ "start": 8454144, "length": 4128768, "depth": 0, "zero": true, "data": 
> false}]
> +[{"length": 65536, "start": 0, "zero": false, "depth": 0, "data": true}

Thus, my series conflicts with your patch, and one of us will have to
rebase on the other.  But with my series, we'd have yet more churn here,
looking like:

> {"start": 65536, "length": 4128768, "data": true, "zero": true, "depth": 0},

unless we change patch 13/15 to lay out the qapi struct in the same
order as the existing output.  But some churn is inevitable; both the
QObject formatter and my pending patches output a leading "{KEY", rather
than "{ KEY" (back to point [1] above), regardless of which key gets
displayed first.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

[Qemu-block] [PATCH v3 14/15] qemu-img: Use QAPI visitor to generate JSON

[Fam Zheng]

A visible improvement is that "filename" is now included in the output
if it's valid.

Reviewed-by: Eric Blake 
Signed-off-by: Fam Zheng 
---
 qemu-img.c | 34 ++--
 tests/qemu-iotests/122.out | 96 ++
 2 files changed, 77 insertions(+), 53 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 1d19c86..fccddc0 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -25,6 +25,8 @@
 #include "qapi/qmp-output-visitor.h"
 #include "qapi/qmp/qerror.h"
 #include "qapi/qmp/qjson.h"
+#include "qapi/qmp/qint.h"
+#include "qapi/qmp/qbool.h"
 #include "qemu-common.h"
 #include "qemu/option.h"
 #include "qemu/error-report.h"
@@ -2136,10 +2138,14 @@ static int img_info(int argc, char **argv)
 static void dump_map_entry(OutputFormat output_format, MapEntry *e,
MapEntry *next)
 {
+QString *str;
+QObject *obj;
+QmpOutputVisitor *ov;
+
 switch (output_format) {
 case OFORMAT_HUMAN:
 if (e->data && !e->has_offset) {
-error_report("File contains external, encrypted or compressed 
clusters.");
+error_report("File contains encrypted or compressed clusters.");
 exit(1);
 }
 if (e->data && !e->zero) {
@@ -2157,20 +2163,26 @@ static void dump_map_entry(OutputFormat output_format, 
MapEntry *e,
 }
 break;
 case OFORMAT_JSON:
-printf("%s{ \"start\": %"PRId64", \"length\": %"PRId64","
-   " \"depth\": %"PRId64", \"zero\": %s, \"data\": %s",
-   (e->start == 0 ? "[" : ",\n"),
-   e->start, e->length, e->depth,
-   e->zero ? "true" : "false",
-   e->data ? "true" : "false");
-if (e->has_offset) {
-printf(", \"offset\": %"PRId64"", e->offset);
-}
-putchar('}');
+ov = qmp_output_visitor_new();
+visit_type_MapEntry(qmp_output_get_visitor(ov),
+&e, NULL, &error_abort);
+obj = qmp_output_get_qobject(ov);
+str = qobject_to_json(obj);
+assert(str != NULL);
 
+if (e->start == 0) {
+printf("[");
+} else {
+printf(",");
+}
+printf("%s\n", qstring_get_str(str));
 if (!next) {
 printf("]\n");
 }
+
+qobject_decref(obj);
+qmp_output_visitor_cleanup(ov);
+QDECREF(str);
 break;
 }
 }
diff --git a/tests/qemu-iotests/122.out b/tests/qemu-iotests/122.out
index 0068e96..760759e 100644
--- a/tests/qemu-iotests/122.out
+++ b/tests/qemu-iotests/122.out
@@ -49,12 +49,13 @@ read 65536/65536 bytes at offset 4194304
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 read 65536/65536 bytes at offset 8388608
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-[{ "start": 0, "length": 65536, "depth": 0, "zero": false, "data": true},
-{ "start": 65536, "length": 4128768, "depth": 0, "zero": true, "data": false},
-{ "start": 4194304, "length": 65536, "depth": 0, "zero": false, "data": true},
-{ "start": 4259840, "length": 4128768, "depth": 0, "zero": true, "data": 
false},
-{ "start": 8388608, "length": 65536, "depth": 0, "zero": false, "data": true},
-{ "start": 8454144, "length": 4128768, "depth": 0, "zero": true, "data": 
false}]
+[{"length": 65536, "start": 0, "zero": false, "depth": 0, "data": true}
+,{"length": 4128768, "start": 65536, "zero": true, "depth": 0, "data": false}
+,{"length": 65536, "start": 4194304, "zero": false, "depth": 0, "data": true}
+,{"length": 4128768, "start": 4259840, "zero": true, "depth": 0, "data": false}
+,{"length": 65536, "start": 8388608, "zero": false, "depth": 0, "data": true}
+,{"length": 4128768, "start": 8454144, "zero": true, "depth": 0, "data": false}
+]
 read 65536/65536 bytes at offset 0
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 read 65536/65536 bytes at offset 4194304
@@ -76,12 +77,13 @@ wrote 1024/1024 bytes at offset 1046528
 1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 wrote 1024/1024 bytes at offset 0
 1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-[{ "start": 0, "length": 65536, "depth": 0, "zero": false, "data": true},
-{ "start": 65536, "length": 65536, "depth": 0, "zero": true, "data": false},
-{ "start": 131072, "length": 196608, "depth": 0, "zero": false, "data": true},
-{ "start": 327680, "length": 655360, "depth": 0, "zero": true, "data": false},
-{ "start": 983040, "length": 65536, "depth": 0, "zero": false, "data": true},
-{ "start": 1048576, "length": 1046528, "depth": 0, "zero": true, "data": 
false}]
+[{"length": 65536, "start": 0, "zero": false, "depth": 0, "data": true}
+,{"length": 65536, "start": 65536, "zero": true, "depth": 0, "data": false}
+,{"length": 196608, "start": 131072, "zero": false, "depth": 0, "data": true}
+,{"length": 655360, "start": 327680, "zero": true, "depth": 0, "data": false}
+,{"length": 65536, "start": 983040, "zero": false, "depth": 0, "data": true}
+,{