Re: [Qemu-devel] [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 <address@hidden>
> Signed-off-by: Fam Zheng <address@hidden>
> ---
>  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