[Qemu-devel] [PATCH V6 0/2] Add JSON output to qemu-img info

[Qemu-devel] [PATCH V6 0/2] Add JSON output to qemu-img info

[Benoît Canet]

This patchset add a JSON output mode to the qemu-img info command.
It's a rewrite from scratch of the original patchset by Wenchao Xia
following Anthony Liguori advices on JSON formating.

the --output=(json|human) option is now mandatory on the command line.

Benoît Canet (3):
  qapi: Add SnapshotInfo.
  qapi: Add ImageInfo.
  qemu-img: Add json output option to the info command.

in v2:

eblake: make some field optionals
        squash the two qapi patchs together
        fix a typo on vm_clock_nsec

bcanet: fix a potential memory leak

in v3: 

lcapitulino: 
       remove unneeded test
       put '\n' at the end of json in printf statement
       drop the uneeded head pointer in collect_snapshots

in v4:

Wenchao Xia && Kevin Wolf: -Refactor to separate rate ImageInfo
                           collection from human printing.

Kevin Wolf: -Use --output=(json|human).
            -make the two choice exclusive and print a message
            if none is specified.
            -cosmetic '=' alignement in collect snapshots.

Benoît Canet: -add full-backing-filename to the ImageInfo structure
              (needed for human printing)
              -make ImageInfo->actual_size optional depending on the
              context.
in v5:

Eric Blake: -use a constant for getopt parsing to avoid future
	     short options collision.
            -make the command default to --output=human.
            -fix spurious whitespace change.
            -split vm-clock-nsec in two fields vm-clock-sec and
             vm-clock-nsec.
            -declare JSON structure as "Since 1.3" 
  
in v6:

Blue Swirl: -Add missing const in getopt structure declaration.

Eric Blake: -Remove spurious undef.
            -Use an enum instead of two boolean.

Benoît Canet (2):
  qapi: Add SnapshotInfo and ImageInfo.
  qemu-img: Add json output option to the info command.

 Makefile         |    3 +-
 qapi-schema.json |   64 ++++++++++++++
 qemu-img.c       |  259 +++++++++++++++++++++++++++++++++++++++++++++---------
 3 files changed, 282 insertions(+), 44 deletions(-)

-- 
1.7.9.5

[Qemu-devel] [PATCH V6 1/2] qapi: Add SnapshotInfo and ImageInfo.

[Benoît Canet]

Signed-off-by: Benoit Canet <benoit@irqsave.net>
---
 qapi-schema.json |   64 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 64 insertions(+)

diff --git a/qapi-schema.json b/qapi-schema.json
index a92adb1..ffe3a0a 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -126,6 +126,70 @@
             'running', 'save-vm', 'shutdown', 'suspended', 'watchdog' ] }
 
 ##
+# @SnapshotInfo
+#
+# @id: unique snapshot id
+#
+# @name: user chosen name
+#
+# @vm-state-size: size of the VM state
+#
+# @date-sec: UTC date of the snapshot in seconds
+#
+# @date-nsec: fractional part in nano seconds to be used with date-sec
+#
+# @vm-clock-sec: VM clock relative to boot in seconds
+#
+# @vm-clock-nsec: fractional part in nano seconds to be used with vm-clock-sec
+#
+# Since: 1.3
+#
+##
+
+{ 'type': 'SnapshotInfo',
+  'data': { 'id': 'str', 'name': 'str', 'vm-state-size': 'int',
+            'date-sec': 'int', 'date-nsec': 'int',
+            'vm-clock-sec': 'int', 'vm-clock-nsec': 'int' } }
+
+##
+# @ImageInfo:
+#
+# Information about a QEMU image file
+#
+# @filename: name of the image file
+#
+# @format: format of the image file
+#
+# @virtual-size: maximum capacity in bytes of the image
+#
+# @actual-size: #optional actual size on disk in bytes of the image
+#
+# @dirty-flag: #optional true if image is not cleanly closed
+#
+# @cluster-size: #optional size of a cluster in bytes
+#
+# @encrypted: #optional true if the image is encrypted
+#
+# @backing-filename: #optional name of the backing file
+#
+# @full-backing-filename: #optional full path of the backing file
+#
+# @backing-filename-format: #optional the format of the backing file
+#
+# @snapshots: #optional list of VM snapshots
+#
+# Since: 1.3
+#
+##
+
+{ 'type': 'ImageInfo',
+  'data': {'filename': 'str', 'format': 'str', '*dirty-flag': 'bool',
+           '*actual-size': 'int', 'virtual-size': 'int',
+           '*cluster-size': 'int', '*encrypted': 'bool',
+           '*backing-filename': 'str', '*full-backing-filename': 'str',
+           '*backing-filename-format': 'str', '*snapshots': ['SnapshotInfo'] } }
+
+##
 # @StatusInfo:
 #
 # Information about VCPU run state
-- 
1.7.9.5

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

[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>
---
 Makefile   |    3 +-
 qemu-img.c |  259 ++++++++++++++++++++++++++++++++++++++++++++++++++----------
 2 files changed, 218 insertions(+), 44 deletions(-)

diff --git a/Makefile b/Makefile
index ab82ef3..9ba064b 100644
--- a/Makefile
+++ b/Makefile
@@ -160,7 +160,8 @@ tools-obj-y = $(oslib-obj-y) $(trace-obj-y) qemu-tool.o qemu-timer.o \
 	iohandler.o cutils.o iov.o async.o
 tools-obj-$(CONFIG_POSIX) += compatfd.o
 
-qemu-img$(EXESUF): qemu-img.o $(tools-obj-y) $(block-obj-y)
+qemu-img$(EXESUF): qemu-img.o $(tools-obj-y) $(block-obj-y) $(qapi-obj-y) \
+                              qapi-visit.o qapi-types.o
 qemu-nbd$(EXESUF): qemu-nbd.o $(tools-obj-y) $(block-obj-y)
 qemu-io$(EXESUF): qemu-io.o cmd.o $(tools-obj-y) $(block-obj-y)
 
diff --git a/qemu-img.c b/qemu-img.c
index 80cfb9b..fe4a4fc 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -21,12 +21,16 @@
  * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
  * THE SOFTWARE.
  */
+#include "qapi-visit.h"
+#include "qapi/qmp-output-visitor.h"
+#include "qjson.h"
 #include "qemu-common.h"
 #include "qemu-option.h"
 #include "qemu-error.h"
 #include "osdep.h"
 #include "sysemu.h"
 #include "block_int.h"
+#include <getopt.h>
 #include <stdio.h>
 
 #ifdef _WIN32
@@ -84,6 +88,7 @@ static void help(void)
            "  '-p' show progress of command (only certain commands)\n"
            "  '-S' indicates the consecutive number of bytes that must contain only zeros\n"
            "       for qemu-img to create a sparse image during conversion\n"
+           "  '--output' takes the format in which the output must be done (human or json)\n"
            "\n"
            "Parameters to check subcommand:\n"
            "  '-r' tries to repair any inconsistencies that are found during the check.\n"
@@ -1102,21 +1107,196 @@ static void dump_snapshots(BlockDriverState *bs)
     g_free(sn_tab);
 }
 
-static int img_info(int argc, char **argv)
+static void collect_snapshots(BlockDriverState *bs , ImageInfo *info)
+{
+    int i, sn_count;
+    QEMUSnapshotInfo *sn_tab = NULL;
+    SnapshotInfoList *info_list, *cur_item = NULL;
+    sn_count = bdrv_snapshot_list(bs, &sn_tab);
+
+    for (i = 0; i < sn_count; i++) {
+        info->has_snapshots = true;
+        info_list = g_new0(SnapshotInfoList, 1);
+
+        info_list->value                = g_new0(SnapshotInfo, 1);
+        info_list->value->id            = g_strdup(sn_tab[i].id_str);
+        info_list->value->name          = g_strdup(sn_tab[i].name);
+        info_list->value->vm_state_size = sn_tab[i].vm_state_size;
+        info_list->value->date_sec      = sn_tab[i].date_sec;
+        info_list->value->date_nsec     = sn_tab[i].date_nsec;
+        info_list->value->vm_clock_sec  = sn_tab[i].vm_clock_nsec / 1000000000;
+        info_list->value->vm_clock_nsec = sn_tab[i].vm_clock_nsec % 1000000000;
+
+        /* XXX: waiting for the qapi to support GSList */
+        if (!cur_item) {
+            info->snapshots = cur_item = info_list;
+        } else {
+            cur_item->next = info_list;
+            cur_item = info_list;
+        }
+
+    }
+
+    g_free(sn_tab);
+}
+
+static void dump_json_image_info(ImageInfo *info)
+{
+    Error *errp = NULL;
+    QString *str;
+    QmpOutputVisitor *ov = qmp_output_visitor_new();
+    QObject *obj;
+    visit_type_ImageInfo(qmp_output_get_visitor(ov),
+                         &info, NULL, &errp);
+    obj = qmp_output_get_qobject(ov);
+    str = qobject_to_json_pretty(obj);
+    assert(str != NULL);
+    printf("%s\n", qstring_get_str(str));
+    qobject_decref(obj);
+    qmp_output_visitor_cleanup(ov);
+    QDECREF(str);
+}
+
+static void collect_backing_file_format(ImageInfo *info, char *filename)
+{
+    BlockDriverState *bs = NULL;
+    bs = bdrv_new_open(filename, NULL,
+                       BDRV_O_FLAGS | BDRV_O_NO_BACKING);
+    if (!bs) {
+        return;
+    }
+    info->backing_filename_format =
+                    g_strdup(bdrv_get_format_name(bs));
+    bdrv_delete(bs);
+    info->has_backing_filename_format = true;
+}
+
+static void collect_image_info(BlockDriverState *bs,
+                   ImageInfo *info,
+                   const char *filename,
+                   const char *fmt)
 {
-    int c;
-    const char *filename, *fmt;
-    BlockDriverState *bs;
-    char size_buf[128], dsize_buf[128];
     uint64_t total_sectors;
-    int64_t allocated_size;
     char backing_filename[1024];
     char backing_filename2[1024];
     BlockDriverInfo bdi;
+    struct stat st;
+
+    bdrv_get_geometry(bs, &total_sectors);
+
+    info->filename        = g_strdup(filename);
+    info->format          = g_strdup(bdrv_get_format_name(bs));
+    info->virtual_size    = total_sectors * 512;
+    info->actual_size     = bdrv_get_allocated_file_size(bs);
+    info->has_actual_size = info->actual_size >= 0;
+    if (bdrv_is_encrypted(bs)) {
+        info->encrypted = true;
+        info->has_encrypted = true;
+    }
+    if (bdrv_get_info(bs, &bdi) >= 0) {
+        if (bdi.cluster_size != 0) {
+            info->cluster_size = bdi.cluster_size;
+            info->has_cluster_size = true;
+        }
+        if (bdi.is_dirty) {
+            info->dirty_flag = true;
+        } else {
+            info->dirty_flag = false;
+        }
+        info->has_dirty_flag = true;
+    }
+    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;
+        }
+
+        if (access(backing_filename2, R_OK) == 0 &&
+            !stat(backing_filename2, &st) &&
+            S_ISREG(st.st_mode)) {
+                collect_backing_file_format(info, backing_filename2);
+
+        }
+    }
+}
+
+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);
+    }
+
+    if (info->has_backing_filename) {
+        putchar('\n');
+    }
+}
+
+enum {OPTION_OUTPUT = 256};
+
+typedef enum Format {
+    FORMAT_JSON,
+    FORMAT_HUMAN,
+} Format;
+
+static int img_info(int argc, char **argv)
+{
+    int c;
+    Format output_format;
+    const char *filename, *fmt, *output;
+    BlockDriverState *bs;
+    ImageInfo *info;
 
     fmt = NULL;
+    output = NULL;
     for(;;) {
-        c = getopt(argc, argv, "f:h");
+        int option_index = 0;
+        static const struct option long_options[] = {
+            {"help", no_argument, 0, 'h'},
+            {"format", required_argument, 0, 'f'},
+            {"output", required_argument, 0, OPTION_OUTPUT},
+            {0, 0, 0, 0}
+        };
+        c = getopt_long(argc, argv, "f:h",
+                        long_options, &option_index);
         if (c == -1) {
             break;
         }
@@ -1128,6 +1308,9 @@ static int img_info(int argc, char **argv)
         case 'f':
             fmt = optarg;
             break;
+        case OPTION_OUTPUT:
+            output = optarg;
+            break;
         }
     }
     if (optind >= argc) {
@@ -1135,48 +1318,38 @@ static int img_info(int argc, char **argv)
     }
     filename = argv[optind++];
 
-    bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS | BDRV_O_NO_BACKING);
-    if (!bs) {
+    if (output && !strcmp(output, "json")) {
+        output_format = FORMAT_JSON;
+    } else if (output && !strcmp(output, "human")) {
+        output_format = FORMAT_HUMAN;
+    } else if (output) {
+        fprintf(stderr,
+            "Error: --output must be used with human or json as argument.\n");
         return 1;
-    }
-    bdrv_get_geometry(bs, &total_sectors);
-    get_human_readable_size(size_buf, sizeof(size_buf), total_sectors * 512);
-    allocated_size = bdrv_get_allocated_file_size(bs);
-    if (allocated_size < 0) {
-        snprintf(dsize_buf, sizeof(dsize_buf), "unavailable");
     } else {
-        get_human_readable_size(dsize_buf, sizeof(dsize_buf),
-                                allocated_size);
+        output_format = FORMAT_HUMAN;
     }
-    printf("image: %s\n"
-           "file format: %s\n"
-           "virtual size: %s (%" PRId64 " bytes)\n"
-           "disk size: %s\n",
-           filename, bdrv_get_format_name(bs), size_buf,
-           (total_sectors * 512),
-           dsize_buf);
-    if (bdrv_is_encrypted(bs)) {
-        printf("encrypted: yes\n");
+
+    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 (bdrv_get_info(bs, &bdi) >= 0) {
-        if (bdi.cluster_size != 0) {
-            printf("cluster_size: %d\n", bdi.cluster_size);
-        }
-        if (bdi.is_dirty) {
-            printf("cleanly shut down: no\n");
-        }
+
+    collect_image_info(bs, info, filename, fmt);
+
+    if (output_format == FORMAT_HUMAN) {
+        dump_human_image_info(info);
+        dump_snapshots(bs);
     }
-    bdrv_get_backing_filename(bs, backing_filename, sizeof(backing_filename));
-    if (backing_filename[0] != '\0') {
-        bdrv_get_full_backing_filename(bs, backing_filename2,
-                                       sizeof(backing_filename2));
-        printf("backing file: %s", backing_filename);
-        if (strcmp(backing_filename, backing_filename2) != 0) {
-            printf(" (actual path: %s)", backing_filename2);
-        }
-        putchar('\n');
+
+    if (output_format == FORMAT_JSON) {
+        collect_snapshots(bs, info);
+        dump_json_image_info(info);
     }
-    dump_snapshots(bs);
+
+    qapi_free_ImageInfo(info);
     bdrv_delete(bs);
     return 0;
 }
-- 
1.7.9.5

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

[Kevin Wolf]

Am 27.08.2012 09:15, 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>
> ---
>  Makefile   |    3 +-
>  qemu-img.c |  259 ++++++++++++++++++++++++++++++++++++++++++++++++++----------
>  2 files changed, 218 insertions(+), 44 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index ab82ef3..9ba064b 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -160,7 +160,8 @@ tools-obj-y = $(oslib-obj-y) $(trace-obj-y) qemu-tool.o qemu-timer.o \
>  	iohandler.o cutils.o iov.o async.o
>  tools-obj-$(CONFIG_POSIX) += compatfd.o
>  
> -qemu-img$(EXESUF): qemu-img.o $(tools-obj-y) $(block-obj-y)
> +qemu-img$(EXESUF): qemu-img.o $(tools-obj-y) $(block-obj-y) $(qapi-obj-y) \
> +                              qapi-visit.o qapi-types.o
>  qemu-nbd$(EXESUF): qemu-nbd.o $(tools-obj-y) $(block-obj-y)
>  qemu-io$(EXESUF): qemu-io.o cmd.o $(tools-obj-y) $(block-obj-y)
>  
> diff --git a/qemu-img.c b/qemu-img.c
> index 80cfb9b..fe4a4fc 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -21,12 +21,16 @@
>   * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
>   * THE SOFTWARE.
>   */
> +#include "qapi-visit.h"
> +#include "qapi/qmp-output-visitor.h"
> +#include "qjson.h"
>  #include "qemu-common.h"
>  #include "qemu-option.h"
>  #include "qemu-error.h"
>  #include "osdep.h"
>  #include "sysemu.h"
>  #include "block_int.h"
> +#include <getopt.h>
>  #include <stdio.h>
>  
>  #ifdef _WIN32
> @@ -84,6 +88,7 @@ static void help(void)
>             "  '-p' show progress of command (only certain commands)\n"
>             "  '-S' indicates the consecutive number of bytes that must contain only zeros\n"
>             "       for qemu-img to create a sparse image during conversion\n"
> +           "  '--output' takes the format in which the output must be done (human or json)\n"
>             "\n"
>             "Parameters to check subcommand:\n"
>             "  '-r' tries to repair any inconsistencies that are found during the check.\n"

qemu-img-cmds.hx and qemu-img.texi should be updated with documentation
of the new option as well.

> @@ -1102,21 +1107,196 @@ static void dump_snapshots(BlockDriverState *bs)
>      g_free(sn_tab);
>  }
>  
> -static int img_info(int argc, char **argv)
> +static void collect_snapshots(BlockDriverState *bs , ImageInfo *info)
> +{
> +    int i, sn_count;
> +    QEMUSnapshotInfo *sn_tab = NULL;
> +    SnapshotInfoList *info_list, *cur_item = NULL;
> +    sn_count = bdrv_snapshot_list(bs, &sn_tab);
> +
> +    for (i = 0; i < sn_count; i++) {
> +        info->has_snapshots = true;
> +        info_list = g_new0(SnapshotInfoList, 1);
> +
> +        info_list->value                = g_new0(SnapshotInfo, 1);
> +        info_list->value->id            = g_strdup(sn_tab[i].id_str);
> +        info_list->value->name          = g_strdup(sn_tab[i].name);
> +        info_list->value->vm_state_size = sn_tab[i].vm_state_size;
> +        info_list->value->date_sec      = sn_tab[i].date_sec;
> +        info_list->value->date_nsec     = sn_tab[i].date_nsec;
> +        info_list->value->vm_clock_sec  = sn_tab[i].vm_clock_nsec / 1000000000;
> +        info_list->value->vm_clock_nsec = sn_tab[i].vm_clock_nsec % 1000000000;
> +
> +        /* XXX: waiting for the qapi to support GSList */

No, GSList isn't typesafe and therefore to be avoided. If anything, the
support we want is for qemu-queue.h types.

> +        if (!cur_item) {
> +            info->snapshots = cur_item = info_list;
> +        } else {
> +            cur_item->next = info_list;
> +            cur_item = info_list;
> +        }
> +
> +    }
> +
> +    g_free(sn_tab);
> +}
> +
> +static void dump_json_image_info(ImageInfo *info)
> +{
> +    Error *errp = NULL;
> +    QString *str;
> +    QmpOutputVisitor *ov = qmp_output_visitor_new();
> +    QObject *obj;
> +    visit_type_ImageInfo(qmp_output_get_visitor(ov),
> +                         &info, NULL, &errp);
> +    obj = qmp_output_get_qobject(ov);
> +    str = qobject_to_json_pretty(obj);
> +    assert(str != NULL);
> +    printf("%s\n", qstring_get_str(str));
> +    qobject_decref(obj);
> +    qmp_output_visitor_cleanup(ov);
> +    QDECREF(str);
> +}
> +
> +static void collect_backing_file_format(ImageInfo *info, char *filename)
> +{
> +    BlockDriverState *bs = NULL;
> +    bs = bdrv_new_open(filename, NULL,
> +                       BDRV_O_FLAGS | BDRV_O_NO_BACKING);
> +    if (!bs) {
> +        return;
> +    }
> +    info->backing_filename_format =
> +                    g_strdup(bdrv_get_format_name(bs));
> +    bdrv_delete(bs);
> +    info->has_backing_filename_format = true;
> +}

This is new functionality and should be added in a separate patch.

It is also wrong because it uses NULL instead of the explicitly
specified format to open the image.

> +
> +static void collect_image_info(BlockDriverState *bs,
> +                   ImageInfo *info,
> +                   const char *filename,
> +                   const char *fmt)
>  {
> -    int c;
> -    const char *filename, *fmt;
> -    BlockDriverState *bs;
> -    char size_buf[128], dsize_buf[128];
>      uint64_t total_sectors;
> -    int64_t allocated_size;
>      char backing_filename[1024];
>      char backing_filename2[1024];
>      BlockDriverInfo bdi;
> +    struct stat st;
> +
> +    bdrv_get_geometry(bs, &total_sectors);
> +
> +    info->filename        = g_strdup(filename);
> +    info->format          = g_strdup(bdrv_get_format_name(bs));
> +    info->virtual_size    = total_sectors * 512;
> +    info->actual_size     = bdrv_get_allocated_file_size(bs);
> +    info->has_actual_size = info->actual_size >= 0;
> +    if (bdrv_is_encrypted(bs)) {
> +        info->encrypted = true;
> +        info->has_encrypted = true;
> +    }
> +    if (bdrv_get_info(bs, &bdi) >= 0) {
> +        if (bdi.cluster_size != 0) {
> +            info->cluster_size = bdi.cluster_size;
> +            info->has_cluster_size = true;
> +        }
> +        if (bdi.is_dirty) {
> +            info->dirty_flag = true;
> +        } else {
> +            info->dirty_flag = false;
> +        }

Shorter written as:

info->dirty_flag = bdi.is_dirty;

> +        info->has_dirty_flag = true;
> +    }
> +    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;
> +        }
> +
> +        if (access(backing_filename2, R_OK) == 0 &&
> +            !stat(backing_filename2, &st) &&
> +            S_ISREG(st.st_mode)) {

What are you doing here? This is most certainly wrong because it doesn't
consider protocols supported by qemu.

Also, I believe what you really want to expose is the backing file
format as specified in the image file (if any), i.e. bs->backing_format.
Opening the backing isn't even necessary then.

> +                collect_backing_file_format(info, backing_filename2);
> +
> +        }
> +    }
> +}
> +
> +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);
> +    }
> +
> +    if (info->has_backing_filename) {
> +        putchar('\n');
> +    }

The full filename is only present if the "normal" name is present
(otherwise the output would come out wrong anyway). So:

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);
    }
    putchar('\n');
}

> +}
> +
> +enum {OPTION_OUTPUT = 256};
> +
> +typedef enum Format {
> +    FORMAT_JSON,
> +    FORMAT_HUMAN,
> +} Format;

Hm, calling this Format is asking for naming conflicts. Maybe OutputFormat?

> +
> +static int img_info(int argc, char **argv)
> +{
> +    int c;
> +    Format output_format;

Format output_format = FORMAT_HUMAN;

> +    const char *filename, *fmt, *output;
> +    BlockDriverState *bs;
> +    ImageInfo *info;
>  
>      fmt = NULL;
> +    output = NULL;
>      for(;;) {
> -        c = getopt(argc, argv, "f:h");
> +        int option_index = 0;
> +        static const struct option long_options[] = {
> +            {"help", no_argument, 0, 'h'},
> +            {"format", required_argument, 0, 'f'},
> +            {"output", required_argument, 0, OPTION_OUTPUT},
> +            {0, 0, 0, 0}
> +        };
> +        c = getopt_long(argc, argv, "f:h",
> +                        long_options, &option_index);
>          if (c == -1) {
>              break;
>          }
> @@ -1128,6 +1308,9 @@ static int img_info(int argc, char **argv)
>          case 'f':
>              fmt = optarg;
>              break;
> +        case OPTION_OUTPUT:
> +            output = optarg;
> +            break;
>          }
>      }
>      if (optind >= argc) {
> @@ -1135,48 +1318,38 @@ static int img_info(int argc, char **argv)
>      }
>      filename = argv[optind++];
>  
> -    bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS | BDRV_O_NO_BACKING);
> -    if (!bs) {
> +    if (output && !strcmp(output, "json")) {
> +        output_format = FORMAT_JSON;
> +    } else if (output && !strcmp(output, "human")) {
> +        output_format = FORMAT_HUMAN;
> +    } else if (output) {
> +        fprintf(stderr,
> +            "Error: --output must be used with human or json as argument.\n");

error_report()?

>          return 1;
> -    }
> -    bdrv_get_geometry(bs, &total_sectors);
> -    get_human_readable_size(size_buf, sizeof(size_buf), total_sectors * 512);
> -    allocated_size = bdrv_get_allocated_file_size(bs);
> -    if (allocated_size < 0) {
> -        snprintf(dsize_buf, sizeof(dsize_buf), "unavailable");
>      } else {
> -        get_human_readable_size(dsize_buf, sizeof(dsize_buf),
> -                                allocated_size);
> +        output_format = FORMAT_HUMAN;
>      }

When you add the default above, you don't need the else branch here.

> -    printf("image: %s\n"
> -           "file format: %s\n"
> -           "virtual size: %s (%" PRId64 " bytes)\n"
> -           "disk size: %s\n",
> -           filename, bdrv_get_format_name(bs), size_buf,
> -           (total_sectors * 512),
> -           dsize_buf);
> -    if (bdrv_is_encrypted(bs)) {
> -        printf("encrypted: yes\n");
> +
> +    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 (bdrv_get_info(bs, &bdi) >= 0) {
> -        if (bdi.cluster_size != 0) {
> -            printf("cluster_size: %d\n", bdi.cluster_size);
> -        }
> -        if (bdi.is_dirty) {
> -            printf("cleanly shut down: no\n");
> -        }
> +
> +    collect_image_info(bs, info, filename, fmt);
> +
> +    if (output_format == FORMAT_HUMAN) {
> +        dump_human_image_info(info);
> +        dump_snapshots(bs);
>      }
> -    bdrv_get_backing_filename(bs, backing_filename, sizeof(backing_filename));
> -    if (backing_filename[0] != '\0') {
> -        bdrv_get_full_backing_filename(bs, backing_filename2,
> -                                       sizeof(backing_filename2));
> -        printf("backing file: %s", backing_filename);
> -        if (strcmp(backing_filename, backing_filename2) != 0) {
> -            printf(" (actual path: %s)", backing_filename2);
> -        }
> -        putchar('\n');
> +
> +    if (output_format == FORMAT_JSON) {
> +        collect_snapshots(bs, info);
> +        dump_json_image_info(info);
>      }

Use a switch instead of two independent ifs?

> -    dump_snapshots(bs);
> +
> +    qapi_free_ImageInfo(info);
>      bdrv_delete(bs);
>      return 0;
>  }
> 

Kevin

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

[Benoît Canet]

Thanks for reviewing.
I will correct the series tomorow.

Benoît

Le Tuesday 04 Sep 2012 à 14:54:18 (+0200), Kevin Wolf a écrit :
> Am 27.08.2012 09:15, 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>
> > ---
> >  Makefile   |    3 +-
> >  qemu-img.c |  259 ++++++++++++++++++++++++++++++++++++++++++++++++++----------
> >  2 files changed, 218 insertions(+), 44 deletions(-)
> > 
> > diff --git a/Makefile b/Makefile
> > index ab82ef3..9ba064b 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -160,7 +160,8 @@ tools-obj-y = $(oslib-obj-y) $(trace-obj-y) qemu-tool.o qemu-timer.o \
> >  	iohandler.o cutils.o iov.o async.o
> >  tools-obj-$(CONFIG_POSIX) += compatfd.o
> >  
> > -qemu-img$(EXESUF): qemu-img.o $(tools-obj-y) $(block-obj-y)
> > +qemu-img$(EXESUF): qemu-img.o $(tools-obj-y) $(block-obj-y) $(qapi-obj-y) \
> > +                              qapi-visit.o qapi-types.o
> >  qemu-nbd$(EXESUF): qemu-nbd.o $(tools-obj-y) $(block-obj-y)
> >  qemu-io$(EXESUF): qemu-io.o cmd.o $(tools-obj-y) $(block-obj-y)
> >  
> > diff --git a/qemu-img.c b/qemu-img.c
> > index 80cfb9b..fe4a4fc 100644
> > --- a/qemu-img.c
> > +++ b/qemu-img.c
> > @@ -21,12 +21,16 @@
> >   * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> >   * THE SOFTWARE.
> >   */
> > +#include "qapi-visit.h"
> > +#include "qapi/qmp-output-visitor.h"
> > +#include "qjson.h"
> >  #include "qemu-common.h"
> >  #include "qemu-option.h"
> >  #include "qemu-error.h"
> >  #include "osdep.h"
> >  #include "sysemu.h"
> >  #include "block_int.h"
> > +#include <getopt.h>
> >  #include <stdio.h>
> >  
> >  #ifdef _WIN32
> > @@ -84,6 +88,7 @@ static void help(void)
> >             "  '-p' show progress of command (only certain commands)\n"
> >             "  '-S' indicates the consecutive number of bytes that must contain only zeros\n"
> >             "       for qemu-img to create a sparse image during conversion\n"
> > +           "  '--output' takes the format in which the output must be done (human or json)\n"
> >             "\n"
> >             "Parameters to check subcommand:\n"
> >             "  '-r' tries to repair any inconsistencies that are found during the check.\n"
> 
> qemu-img-cmds.hx and qemu-img.texi should be updated with documentation
> of the new option as well.
> 
> > @@ -1102,21 +1107,196 @@ static void dump_snapshots(BlockDriverState *bs)
> >      g_free(sn_tab);
> >  }
> >  
> > -static int img_info(int argc, char **argv)
> > +static void collect_snapshots(BlockDriverState *bs , ImageInfo *info)
> > +{
> > +    int i, sn_count;
> > +    QEMUSnapshotInfo *sn_tab = NULL;
> > +    SnapshotInfoList *info_list, *cur_item = NULL;
> > +    sn_count = bdrv_snapshot_list(bs, &sn_tab);
> > +
> > +    for (i = 0; i < sn_count; i++) {
> > +        info->has_snapshots = true;
> > +        info_list = g_new0(SnapshotInfoList, 1);
> > +
> > +        info_list->value                = g_new0(SnapshotInfo, 1);
> > +        info_list->value->id            = g_strdup(sn_tab[i].id_str);
> > +        info_list->value->name          = g_strdup(sn_tab[i].name);
> > +        info_list->value->vm_state_size = sn_tab[i].vm_state_size;
> > +        info_list->value->date_sec      = sn_tab[i].date_sec;
> > +        info_list->value->date_nsec     = sn_tab[i].date_nsec;
> > +        info_list->value->vm_clock_sec  = sn_tab[i].vm_clock_nsec / 1000000000;
> > +        info_list->value->vm_clock_nsec = sn_tab[i].vm_clock_nsec % 1000000000;
> > +
> > +        /* XXX: waiting for the qapi to support GSList */
> 
> No, GSList isn't typesafe and therefore to be avoided. If anything, the
> support we want is for qemu-queue.h types.
> 
> > +        if (!cur_item) {
> > +            info->snapshots = cur_item = info_list;
> > +        } else {
> > +            cur_item->next = info_list;
> > +            cur_item = info_list;
> > +        }
> > +
> > +    }
> > +
> > +    g_free(sn_tab);
> > +}
> > +
> > +static void dump_json_image_info(ImageInfo *info)
> > +{
> > +    Error *errp = NULL;
> > +    QString *str;
> > +    QmpOutputVisitor *ov = qmp_output_visitor_new();
> > +    QObject *obj;
> > +    visit_type_ImageInfo(qmp_output_get_visitor(ov),
> > +                         &info, NULL, &errp);
> > +    obj = qmp_output_get_qobject(ov);
> > +    str = qobject_to_json_pretty(obj);
> > +    assert(str != NULL);
> > +    printf("%s\n", qstring_get_str(str));
> > +    qobject_decref(obj);
> > +    qmp_output_visitor_cleanup(ov);
> > +    QDECREF(str);
> > +}
> > +
> > +static void collect_backing_file_format(ImageInfo *info, char *filename)
> > +{
> > +    BlockDriverState *bs = NULL;
> > +    bs = bdrv_new_open(filename, NULL,
> > +                       BDRV_O_FLAGS | BDRV_O_NO_BACKING);
> > +    if (!bs) {
> > +        return;
> > +    }
> > +    info->backing_filename_format =
> > +                    g_strdup(bdrv_get_format_name(bs));
> > +    bdrv_delete(bs);
> > +    info->has_backing_filename_format = true;
> > +}
> 
> This is new functionality and should be added in a separate patch.
> 
> It is also wrong because it uses NULL instead of the explicitly
> specified format to open the image.
> 
> > +
> > +static void collect_image_info(BlockDriverState *bs,
> > +                   ImageInfo *info,
> > +                   const char *filename,
> > +                   const char *fmt)
> >  {
> > -    int c;
> > -    const char *filename, *fmt;
> > -    BlockDriverState *bs;
> > -    char size_buf[128], dsize_buf[128];
> >      uint64_t total_sectors;
> > -    int64_t allocated_size;
> >      char backing_filename[1024];
> >      char backing_filename2[1024];
> >      BlockDriverInfo bdi;
> > +    struct stat st;
> > +
> > +    bdrv_get_geometry(bs, &total_sectors);
> > +
> > +    info->filename        = g_strdup(filename);
> > +    info->format          = g_strdup(bdrv_get_format_name(bs));
> > +    info->virtual_size    = total_sectors * 512;
> > +    info->actual_size     = bdrv_get_allocated_file_size(bs);
> > +    info->has_actual_size = info->actual_size >= 0;
> > +    if (bdrv_is_encrypted(bs)) {
> > +        info->encrypted = true;
> > +        info->has_encrypted = true;
> > +    }
> > +    if (bdrv_get_info(bs, &bdi) >= 0) {
> > +        if (bdi.cluster_size != 0) {
> > +            info->cluster_size = bdi.cluster_size;
> > +            info->has_cluster_size = true;
> > +        }
> > +        if (bdi.is_dirty) {
> > +            info->dirty_flag = true;
> > +        } else {
> > +            info->dirty_flag = false;
> > +        }
> 
> Shorter written as:
> 
> info->dirty_flag = bdi.is_dirty;
> 
> > +        info->has_dirty_flag = true;
> > +    }
> > +    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;
> > +        }
> > +
> > +        if (access(backing_filename2, R_OK) == 0 &&
> > +            !stat(backing_filename2, &st) &&
> > +            S_ISREG(st.st_mode)) {
> 
> What are you doing here? This is most certainly wrong because it doesn't
> consider protocols supported by qemu.
> 
> Also, I believe what you really want to expose is the backing file
> format as specified in the image file (if any), i.e. bs->backing_format.
> Opening the backing isn't even necessary then.
> 
> > +                collect_backing_file_format(info, backing_filename2);
> > +
> > +        }
> > +    }
> > +}
> > +
> > +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);
> > +    }
> > +
> > +    if (info->has_backing_filename) {
> > +        putchar('\n');
> > +    }
> 
> The full filename is only present if the "normal" name is present
> (otherwise the output would come out wrong anyway). So:
> 
> 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);
>     }
>     putchar('\n');
> }
> 
> > +}
> > +
> > +enum {OPTION_OUTPUT = 256};
> > +
> > +typedef enum Format {
> > +    FORMAT_JSON,
> > +    FORMAT_HUMAN,
> > +} Format;
> 
> Hm, calling this Format is asking for naming conflicts. Maybe OutputFormat?
> 
> > +
> > +static int img_info(int argc, char **argv)
> > +{
> > +    int c;
> > +    Format output_format;
> 
> Format output_format = FORMAT_HUMAN;
> 
> > +    const char *filename, *fmt, *output;
> > +    BlockDriverState *bs;
> > +    ImageInfo *info;
> >  
> >      fmt = NULL;
> > +    output = NULL;
> >      for(;;) {
> > -        c = getopt(argc, argv, "f:h");
> > +        int option_index = 0;
> > +        static const struct option long_options[] = {
> > +            {"help", no_argument, 0, 'h'},
> > +            {"format", required_argument, 0, 'f'},
> > +            {"output", required_argument, 0, OPTION_OUTPUT},
> > +            {0, 0, 0, 0}
> > +        };
> > +        c = getopt_long(argc, argv, "f:h",
> > +                        long_options, &option_index);
> >          if (c == -1) {
> >              break;
> >          }
> > @@ -1128,6 +1308,9 @@ static int img_info(int argc, char **argv)
> >          case 'f':
> >              fmt = optarg;
> >              break;
> > +        case OPTION_OUTPUT:
> > +            output = optarg;
> > +            break;
> >          }
> >      }
> >      if (optind >= argc) {
> > @@ -1135,48 +1318,38 @@ static int img_info(int argc, char **argv)
> >      }
> >      filename = argv[optind++];
> >  
> > -    bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS | BDRV_O_NO_BACKING);
> > -    if (!bs) {
> > +    if (output && !strcmp(output, "json")) {
> > +        output_format = FORMAT_JSON;
> > +    } else if (output && !strcmp(output, "human")) {
> > +        output_format = FORMAT_HUMAN;
> > +    } else if (output) {
> > +        fprintf(stderr,
> > +            "Error: --output must be used with human or json as argument.\n");
> 
> error_report()?
> 
> >          return 1;
> > -    }
> > -    bdrv_get_geometry(bs, &total_sectors);
> > -    get_human_readable_size(size_buf, sizeof(size_buf), total_sectors * 512);
> > -    allocated_size = bdrv_get_allocated_file_size(bs);
> > -    if (allocated_size < 0) {
> > -        snprintf(dsize_buf, sizeof(dsize_buf), "unavailable");
> >      } else {
> > -        get_human_readable_size(dsize_buf, sizeof(dsize_buf),
> > -                                allocated_size);
> > +        output_format = FORMAT_HUMAN;
> >      }
> 
> When you add the default above, you don't need the else branch here.
> 
> > -    printf("image: %s\n"
> > -           "file format: %s\n"
> > -           "virtual size: %s (%" PRId64 " bytes)\n"
> > -           "disk size: %s\n",
> > -           filename, bdrv_get_format_name(bs), size_buf,
> > -           (total_sectors * 512),
> > -           dsize_buf);
> > -    if (bdrv_is_encrypted(bs)) {
> > -        printf("encrypted: yes\n");
> > +
> > +    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 (bdrv_get_info(bs, &bdi) >= 0) {
> > -        if (bdi.cluster_size != 0) {
> > -            printf("cluster_size: %d\n", bdi.cluster_size);
> > -        }
> > -        if (bdi.is_dirty) {
> > -            printf("cleanly shut down: no\n");
> > -        }
> > +
> > +    collect_image_info(bs, info, filename, fmt);
> > +
> > +    if (output_format == FORMAT_HUMAN) {
> > +        dump_human_image_info(info);
> > +        dump_snapshots(bs);
> >      }
> > -    bdrv_get_backing_filename(bs, backing_filename, sizeof(backing_filename));
> > -    if (backing_filename[0] != '\0') {
> > -        bdrv_get_full_backing_filename(bs, backing_filename2,
> > -                                       sizeof(backing_filename2));
> > -        printf("backing file: %s", backing_filename);
> > -        if (strcmp(backing_filename, backing_filename2) != 0) {
> > -            printf(" (actual path: %s)", backing_filename2);
> > -        }
> > -        putchar('\n');
> > +
> > +    if (output_format == FORMAT_JSON) {
> > +        collect_snapshots(bs, info);
> > +        dump_json_image_info(info);
> >      }
> 
> Use a switch instead of two independent ifs?
> 
> > -    dump_snapshots(bs);
> > +
> > +    qapi_free_ImageInfo(info);
> >      bdrv_delete(bs);
> >      return 0;
> >  }
> > 
> 
> Kevin

Re: [Qemu-devel] [PATCH V6 0/2] Add JSON output to qemu-img info

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 848 bytes --]

On 08/27/2012 01:15 AM, Benoît Canet wrote:
> This patchset add a JSON output mode to the qemu-img info command.
> It's a rewrite from scratch of the original patchset by Wenchao Xia
> following Anthony Liguori advices on JSON formating.
> 
> the --output=(json|human) option is now mandatory on the command line.

This statement is not true, but doesn't affect the series itself.

> in v6:
> 
> Blue Swirl: -Add missing const in getopt structure declaration.
> 
> Eric Blake: -Remove spurious undef.
>             -Use an enum instead of two boolean.

You have now addressed my complaints about the interface, but I would
feel comfortable if someone more familiar with qemu-img itself gives
final review and/or ack.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 620 bytes --]

Re: [Qemu-devel] [PATCH V6 0/2] Add JSON output to qemu-img info

[Benoît Canet]

Le Monday 27 Aug 2012 à 11:52:59 (-0600), Eric Blake a écrit :
> On 08/27/2012 01:15 AM, Benoît Canet wrote:
> > This patchset add a JSON output mode to the qemu-img info command.
> > It's a rewrite from scratch of the original patchset by Wenchao Xia
> > following Anthony Liguori advices on JSON formating.
> > 
> > the --output=(json|human) option is now mandatory on the command line.
> 
> This statement is not true, but doesn't affect the series itself.
> 
> > in v6:
> > 
> > Blue Swirl: -Add missing const in getopt structure declaration.
> > 
> > Eric Blake: -Remove spurious undef.
> >             -Use an enum instead of two boolean.
> 
> You have now addressed my complaints about the interface, but I would
> feel comfortable if someone more familiar with qemu-img itself gives
> final review and/or ack.

gentle ping

Benoît

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

Re: [Qemu-devel] [PATCH V6 0/2] Add JSON output to qemu-img info

[Richard W.M. Jones]

[-- Attachment #1: Type: text/plain, Size: 1416 bytes --]

On Mon, Aug 27, 2012 at 09:15:50AM +0200, Benoît Canet wrote:
> This patchset add a JSON output mode to the qemu-img info command.
> It's a rewrite from scratch of the original patchset by Wenchao Xia
> following Anthony Liguori advices on JSON formating.

I won't comment on the patch set, but something like this patch seems
essential to avoid unintended security flaws in parsing the output of
'qemu-img info'.

$ qemu-img create -f qcow2 test.qcow2 10M
$ mv test.qcow2 "$(echo -e '/tmp/foo\ncluster_size: bar')"
$ qemu-img info /tmp/foo*
image: /tmp/foo
cluster_size: bar                   <---- note
file format: qcow2
virtual size: 10M (10485760 bytes)
disk size: 136K
cluster_size: 65536

See the attached script which tries to exploit the backing file too:

$ ./bad-qemu-img-info.sh 
Formatting 'test.qcow2', fmt=qcow2 size=10485760 backing_file='backing
file formatx foo' encryption=off cluster_size=65536 
image: test.qcow2
file format: qcow2
virtual size: 10M (10485760 bytes)
disk size: 196K
cluster_size: 65536
backing file: backing
file format: foo                    <---- note

I think this change should be backported to stable branches as well.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

[-- Attachment #2: bad-qemu-img-info.sh --]
[-- Type: application/x-sh, Size: 761 bytes --]