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

[Qemu-devel] [PATCH V4 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.

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

 Makefile         |    3 +-
 qapi-schema.json |   62 ++++++++++++++
 qemu-img.c       |  252 ++++++++++++++++++++++++++++++++++++++++++++----------
 3 files changed, 272 insertions(+), 45 deletions(-)

-- 
1.7.9.5

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

[Benoît Canet]

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

diff --git a/qapi-schema.json b/qapi-schema.json
index a92adb1..4c4b9b9 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -126,6 +126,68 @@
             'running', 'save-vm', 'shutdown', 'suspended', 'watchdog' ] }
 
 ##
+# @SnapshotInfo
+#
+# @id: unique snapshot id
+#
+# @name: user choosen name
+#
+# @vm-state-size: size of the VM state
+#
+# @date-sec: UTC date of the snapshot
+#
+# @date-nsec: date in nano seconds
+#
+# @vm-clock-nsec: VM clock relative to boot in nano seconds
+#
+# Since: 1.2
+#
+##
+
+{ 'type': 'SnapshotInfo',
+  'data': { 'id': 'str', 'name': 'str', 'vm-state-size': 'int',
+            'date-sec': 'int', 'date-nsec': '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.2
+#
+##
+
+{ '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 V4 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": 20637102488,
            "name": "vm-20120821145509",
            "date-sec": 1345553709,
            "date-nsec": 220289000,
            "id": "1",
            "vm-state-size": 96522745
        },
        {
            "vm-clock-nsec": 46028210866,
            "name": "vm-20120821154059",
            "date-sec": 1345556459,
            "date-nsec": 171392000,
            "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 |  252 +++++++++++++++++++++++++++++++++++++++++++++++++-----------
 2 files changed, 210 insertions(+), 45 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..1591898 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"
@@ -1083,7 +1088,6 @@ out:
     return 0;
 }
 
-
 static void dump_snapshots(BlockDriverState *bs)
 {
     QEMUSnapshotInfo *sn_tab, *sn;
@@ -1102,21 +1106,188 @@ 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 *sn_info_list, *cur_item = NULL;
+    sn_count = bdrv_snapshot_list(bs, &sn_tab);
+
+    for (i = 0; i < sn_count; i++) {
+        info->has_snapshots = true;
+        sn_info_list = g_new0(SnapshotInfoList, 1);
+
+        sn_info_list->value                = g_new0(SnapshotInfo, 1);
+        sn_info_list->value->id            = g_strdup(sn_tab[i].id_str);
+        sn_info_list->value->name          = g_strdup(sn_tab[i].name);
+        sn_info_list->value->vm_state_size = sn_tab[i].vm_state_size;
+        sn_info_list->value->date_sec      = sn_tab[i].date_sec;
+        sn_info_list->value->date_nsec     = sn_tab[i].date_nsec;
+        sn_info_list->value->vm_clock_nsec = sn_tab[i].vm_clock_nsec;
+
+        /* XXX: waiting for the qapi to support GSList */
+        if (!cur_item) {
+            info->snapshots = cur_item = sn_info_list;
+        } else {
+            cur_item->next = sn_info_list;
+            cur_item = sn_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');
+    }
+}
+
+static int img_info(int argc, char **argv)
+{
+    int c;
+    bool human = false, json = false;
+    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 struct option long_options[] = {
+            {"help", no_argument, 0, 'h'},
+            {"format", required_argument, 0, 'f'},
+            {"output", required_argument, 0, 'm'},
+            {0, 0, 0, 0}
+        };
+        c = getopt_long(argc, argv, "f:h",
+                        long_options, &option_index);
         if (c == -1) {
             break;
         }
@@ -1128,6 +1299,9 @@ static int img_info(int argc, char **argv)
         case 'f':
             fmt = optarg;
             break;
+        case 'm':
+            output = optarg;
+            break;
         }
     }
     if (optind >= argc) {
@@ -1135,52 +1309,42 @@ static int img_info(int argc, char **argv)
     }
     filename = argv[optind++];
 
+    if (output && !strncmp(output, "json", strlen("json"))) {
+        json = true;
+    } else if (output && !strncmp(output, "human", strlen("human"))) {
+        human = true;
+    } else {
+        fprintf(stderr,
+            "Error: --output must be used with human or json as argument.\n");
+        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;
     }
-    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);
-    }
-    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");
-    }
-    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 (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 (json) {
+        collect_snapshots(bs, info);
+        dump_json_image_info(info);
     }
-    dump_snapshots(bs);
+
+    qapi_free_ImageInfo(info);
     bdrv_delete(bs);
     return 0;
 }
 
+#undef PRINTH
+
 #define SNAPSHOT_LIST   1
 #define SNAPSHOT_CREATE 2
 #define SNAPSHOT_APPLY  3
-- 
1.7.9.5

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

[Eric Blake]

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

On 08/22/2012 06:45 AM, Benoît Canet wrote:
> Signed-off-by: Benoit Canet <benoit@irqsave.net>
> ---
>  qapi-schema.json |   62 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 62 insertions(+)
> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index a92adb1..4c4b9b9 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -126,6 +126,68 @@
>              'running', 'save-vm', 'shutdown', 'suspended', 'watchdog' ] }
>  
>  ##
> +# @SnapshotInfo
> +#
> +# @id: unique snapshot id
> +#
> +# @name: user choosen name

s/choosen/chosen/

> +#
> +# @vm-state-size: size of the VM state
> +#
> +# @date-sec: UTC date of the snapshot
> +#
> +# @date-nsec: date in nano seconds
> +#
> +# @vm-clock-nsec: VM clock relative to boot in nano seconds

Since we have two fields named *-nsec, it might be worth clarifying that
date-nsec is merely the fractional portion to be combined with date-sec
(always less than 1000000000), while vm-clock-nsec includes seconds if
the drift is that large.

For that matter, should we even be exposing things in this manner?  I
know the internal struct has seconds and nanos separate for date,
because it maps to struct timespec; but why can't we combine them into
one giant number for JSON?  Or are we worried about exceeding int64_t if
we take seconds * 1000000000 when dealing with the timestamp the
snapshot was created, even though we aren't worried about the VM clock
being that far away from boot?

> +#
> +# Since: 1.2

Probably too late for 1.2, so this should be 1.3.

> +##
> +# @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

That seems backwards - with a raw file, don't you know the actual size,
but have no idea what it can further grow to?

> +#
> +# @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.2

Again, 1.3.

-- 
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 V4 2/2] qemu-img: Add json output option to the info command.

[Eric Blake]

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

On 08/22/2012 06:45 AM, Benoît Canet wrote:
> This option --output=[human|json] make qemu-img info output on
> human or JSON representation at the choice of the user.
> 

> @@ -1083,7 +1088,6 @@ out:
>      return 0;
>  }
>  
> -
>  static void dump_snapshots(BlockDriverState *bs)
>  {

Spurious whitespace change.

> +static int img_info(int argc, char **argv)
> +{
> +    int c;
> +    bool human = false, json = false;
> +    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 struct option long_options[] = {
> +            {"help", no_argument, 0, 'h'},
> +            {"format", required_argument, 0, 'f'},
> +            {"output", required_argument, 0, 'm'},

I would define a constant, such as 'enum {OPTION_FORMAT=256};', so that
you don't risk future confusion...

> +            {0, 0, 0, 0}
> +        };
> +        c = getopt_long(argc, argv, "f:h",

...if we later do add an 'm' short option.

> +                        long_options, &option_index);
>          if (c == -1) {
>              break;
>          }
> @@ -1128,6 +1299,9 @@ static int img_info(int argc, char **argv)
>          case 'f':
>              fmt = optarg;
>              break;
> +        case 'm':

Given the above, this would reuse your new named value.

> @@ -1135,52 +1309,42 @@ static int img_info(int argc, char **argv)
>      }
>      filename = argv[optind++];
>  
> +    if (output && !strncmp(output, "json", strlen("json"))) {

Why strncmp?  It ignores trailing garbage (as in --output=jsonoops).
Stick to strcmp.

> +        json = true;
> +    } else if (output && !strncmp(output, "human", strlen("human"))) {

And again.

> +        human = true;
> +    } else {
> +        fprintf(stderr,
> +            "Error: --output must be used with human or json as argument.\n");
> +        return 1;
> +    }

If we get here, and --output=... was not given, then both human and json
are false.  That's a problem, since...

> +
> +    if (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 (json) {
> +        collect_snapshots(bs, info);
> +        dump_json_image_info(info);
>      }
> -    dump_snapshots(bs);
> +
> +    qapi_free_ImageInfo(info);

...you will end up with no output.  You want to default to human output
for back-compat to older usage.

-- 
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 V4 1/2] qapi: Add SnapshotInfo and ImageInfo.

[Benoît Canet]

Le Wednesday 22 Aug 2012 à 08:03:14 (-0600), Eric Blake a écrit :
> On 08/22/2012 06:45 AM, Benoît Canet wrote:
> > Signed-off-by: Benoit Canet <benoit@irqsave.net>
> > ---
> >  qapi-schema.json |   62 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 62 insertions(+)
> > 
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index a92adb1..4c4b9b9 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -126,6 +126,68 @@
> >              'running', 'save-vm', 'shutdown', 'suspended', 'watchdog' ] }
> >  
> >  ##
> > +# @SnapshotInfo
> > +#
> > +# @id: unique snapshot id
> > +#
> > +# @name: user choosen name
> 
> s/choosen/chosen/
> 
> > +#
> > +# @vm-state-size: size of the VM state
> > +#
> > +# @date-sec: UTC date of the snapshot
> > +#
> > +# @date-nsec: date in nano seconds
> > +#
> > +# @vm-clock-nsec: VM clock relative to boot in nano seconds
> 
> Since we have two fields named *-nsec, it might be worth clarifying that
> date-nsec is merely the fractional portion to be combined with date-sec
> (always less than 1000000000), while vm-clock-nsec includes seconds if
> the drift is that large.
> 
> For that matter, should we even be exposing things in this manner?  I
> know the internal struct has seconds and nanos separate for date,
> because it maps to struct timespec; but why can't we combine them into
> one giant number for JSON?

Wouldn't people working with low level language be annoyed after parsing
this JSON to have to split this combined number in two parts to fit
them back into struct timespec ?

> Or are we worried about exceeding int64_t if
> we take seconds * 1000000000 when dealing with the timestamp the
> snapshot was created, even though we aren't worried about the VM clock
> being that far away from boot?
> 
> > +#
> > +# Since: 1.2
> 
> Probably too late for 1.2, so this should be 1.3.
> 
> > +##
> > +# @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
> 
> That seems backwards - with a raw file, don't you know the actual size,
> but have no idea what it can further grow to?
> 
> > +#
> > +# @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.2
> 
> Again, 1.3.
> 
> -- 
> Eric Blake   eblake@redhat.com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 

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

[Eric Blake]

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

On 08/22/2012 08:32 AM, Benoît Canet wrote:
>> Since we have two fields named *-nsec, it might be worth clarifying that
>> date-nsec is merely the fractional portion to be combined with date-sec
>> (always less than 1000000000), while vm-clock-nsec includes seconds if
>> the drift is that large.
>>
>> For that matter, should we even be exposing things in this manner?  I
>> know the internal struct has seconds and nanos separate for date,
>> because it maps to struct timespec; but why can't we combine them into
>> one giant number for JSON?
> 
> Wouldn't people working with low level language be annoyed after parsing
> this JSON to have to split this combined number in two parts to fit
> them back into struct timespec ?

Perhaps, in which case, why don't we present vm-clock-nsec via two
fields of seconds and fraction, for the same reasoning?  My point is
that we have two different bike shed colors showing in this one API, but
I would prefer we be consistent and pick just one (as to _which_ color,
I can be persuaded either way).

-- 
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 V4 1/2] qapi: Add SnapshotInfo and ImageInfo.

[Benoît Canet]

Le Wednesday 22 Aug 2012 à 08:56:06 (-0600), Eric Blake a écrit :
> On 08/22/2012 08:32 AM, Benoît Canet wrote:
> >> Since we have two fields named *-nsec, it might be worth clarifying that
> >> date-nsec is merely the fractional portion to be combined with date-sec
> >> (always less than 1000000000), while vm-clock-nsec includes seconds if
> >> the drift is that large.
> >>
> >> For that matter, should we even be exposing things in this manner?  I
> >> know the internal struct has seconds and nanos separate for date,
> >> because it maps to struct timespec; but why can't we combine them into
> >> one giant number for JSON?
> > 
> > Wouldn't people working with low level language be annoyed after parsing
> > this JSON to have to split this combined number in two parts to fit
> > them back into struct timespec ?
> 
> Perhaps, in which case, why don't we present vm-clock-nsec via two
> fields of seconds and fraction, for the same reasoning?
I'll do that.

> My point is
> that we have two different bike shed colors showing in this one API, but
> I would prefer we be consistent and pick just one (as to _which_ color,
> I can be persuaded either way).
> 
> -- 
> Eric Blake   eblake@redhat.com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 

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

[Benoît Canet]

> > +##
> > +# @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
> 
> That seems backwards - with a raw file, don't you know the actual size,
> but have no idea what it can further grow to?
Try the current qemy-img info on a raw file you will be surprised of the results.
This field also can have an "unavailable" value.

example result with unpatched qemu-img:

benoit@Laure:~/qemu$ qemu-img info truc1.raw 
image: truc1.raw
file format: raw
virtual size: 1.0G (1073741824 bytes)
disk size: 776M

disk size is actual-size.

Benoît

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

[Benoît Canet]

> That seems backwards - with a raw file, don't you know the actual size,
> but have no idea what it can further grow to?
In raw it commes from fstat() with the field st.st_blocks.

from man:
blkcnt_t  st_blocks;  /* number of 512B blocks allocated */

Benoît