Re: [Qemu-devel] [PATCH V7 2/2] qemu-img: Add json output option to the info command.

[Kevin Wolf <kwolf@redhat.com>]

Am 05.09.2012 11:25, schrieb Benoît Canet:
> This option --output=[human|json] make qemu-img info output on
> human or JSON representation at the choice of the user.
> 
> example:
> {
>     "snapshots": [
>         {
>             "vm-clock-nsec": 637102488,
>             "name": "vm-20120821145509",
>             "date-sec": 1345553709,
>             "date-nsec": 220289000,
>             "vm-clock-sec": 20,
>             "id": "1",
>             "vm-state-size": 96522745
>         },
>         {
>             "vm-clock-nsec": 28210866,
>             "name": "vm-20120821154059",
>             "date-sec": 1345556459,
>             "date-nsec": 171392000,
>             "vm-clock-sec": 46,
>             "id": "2",
>             "vm-state-size": 101208714
>         }
>     ],
>     "virtual-size": 1073741824,
>     "filename": "snap.qcow2",
>     "cluster-size": 65536,
>     "format": "qcow2",
>     "actual-size": 985587712,
>     "dirty-flag": false
> }
> 
> Signed-off-by: Benoit Canet <benoit@irqsave.net>

Looks much better, but I still have a few comments:

> +    bdrv_get_backing_filename(bs, backing_filename, sizeof(backing_filename));
> +    if (backing_filename[0] != '\0') {
> +        info->backing_filename = g_strdup(backing_filename);
> +        info->has_backing_filename = true;
> +        bdrv_get_full_backing_filename(bs, backing_filename2,
> +                                       sizeof(backing_filename2));
> +
> +        if (strcmp(backing_filename, backing_filename2) != 0) {
> +            info->full_backing_filename =
> +                        g_strdup(backing_filename2);
> +             info->has_full_backing_filename = true;
> +        }
> +
> +        info->backing_filename_format = bs->backing_format;
> +        info->has_backing_filename_format = true;

I believe we should check bs->backing_format[0] first. If it's empty,
don't include the format.

> +    }
> +}
> +
> +static void dump_human_image_info(ImageInfo *info)
> +{
> +    char size_buf[128], dsize_buf[128];
> +    if (!info->has_actual_size) {
> +        snprintf(dsize_buf, sizeof(dsize_buf), "unavailable");
> +    } else {
> +        get_human_readable_size(dsize_buf, sizeof(dsize_buf),
> +                                info->actual_size);
> +    }
> +    get_human_readable_size(size_buf, sizeof(size_buf), info->virtual_size);
> +    printf("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);
> +
> +    if (info->has_encrypted && info->encrypted) {
> +        printf("encrypted: yes\n");
> +    }
> +
> +    if (info->has_cluster_size) {
> +        printf("cluster_size: %" PRId64 "\n", info->cluster_size);
> +    }
> +
> +    if (info->has_dirty_flag && info->dirty_flag) {
> +        printf("cleanly shut down: no\n");
> +    }
> +
> +    if (info->has_backing_filename) {
> +        printf("backing file: %s", info->backing_filename);
> +        if (info->has_full_backing_filename) {
> +            printf(" (actual path: %s)", info->full_backing_filename);
> +        }

Maybe add the backing file format here if it's available?

> @@ -1135,48 +1291,36 @@ static int img_info(int argc, char **argv)
>      }
>      filename = argv[optind++];
>  
> +    if (output && !strcmp(output, "json")) {
> +        output_format = OFORMAT_JSON;
> +    } else if (output && !strcmp(output, "human")) {
> +        output_format = OFORMAT_HUMAN;
> +    } else if (output) {
> +        error_report("--output must be used with human or json as argument.");
> +        return 1;
> +    }
> +
> +    info = g_new0(ImageInfo, 1);
>      bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS | BDRV_O_NO_BACKING);
>      if (!bs) {
> +        g_free(info);
>          return 1;
>      }

If you move thee g_new0 below, you don't have to free here.

> diff --git a/qemu-img.texi b/qemu-img.texi
> index 6b42e35..75a7c8b 100644
> --- a/qemu-img.texi
> +++ b/qemu-img.texi
> @@ -129,12 +129,13 @@ created as a copy on write image of the specified base image; the
>  @var{backing_file} should have the same content as the input's base image,
>  however the path, image format, etc may differ.
>  
> -@item info [-f @var{fmt}] @var{filename}
> +@item info [-f @var{fmt}] [--output=@var{ofmt}] @var{filename}
>  
>  Give information about the disk image @var{filename}. Use it in
>  particular to know the size reserved on disk which can be different
>  from the displayed size. If VM snapshots are stored in the disk image,
> -they are displayed too.
> +they are displayed too. The command can output in the format @var{ofmt}
> +which is either human or json.

"...@code{human} or @code{json}" is better, I think.

Kevin