[Qemu-devel] [PATCH V3 00/11] add qmp/hmp interfaces for snapshot info

[Qemu-devel] [PATCH V3 00/11] add qmp/hmp interfaces for snapshot info

[Wenchao Xia]

  This serial of patches does two things: merge some info code
in qemu-img, and add following interfaces:
1) qmp: query-images
2) qmp: query-snapshots
3) hmp: show snapshot info on a single block device
  These patches follows the rule that use qmp to retieve information,
hmp layer just do a translation from qmp object it got, so almost
every hmp interface may have a correlated qmp interface.
  To make code graceful, snapshot retrieving code in qemu and qemu-img
are merged into block.c, and some function name was adjusted to make it
tips better. Now it works as:

   qemu          qemu-img

dump_monitor    dump_stdout
     |--------------| 
            |
           qmp
            |
          block

Note:
  Last two patches need previous sent patches which extend hmp sub command, at:
http://lists.nongnu.org/archive/html/qemu-devel/2012-12/msg03487.html

v2:
  Rename and adjusted qmp interface according to comments from Eric.
  Spelling fix.
  Information retrieving function in block layer goes to seperated patch.
  Free qmp object after usage in hmp.
  Added counterpart in qmp-commands.hx.
  Better tips in qmp-schema.json.

v3:
  Spelling fix in commit message, patch 03/11.
  Spelling fix in code, patch 06/11.
  Add comments that vm-state-size is in bytes, and change size of it in
example to a reasonable number, patch 08/11.

Wenchao Xia (11):
  qemu-img: remove unused parameter in collect_image_info()
  block: add bdrv_get_filename() function
  block: add snapshot and image info query function
  qemu-img: switch image retrieving function
  block: rename bdrv_query_info to bdrv_query_block_info
  qmp: add interface query-images.
  block: export function bdrv_find_snapshot()
  qmp: add interface query-snapshots
  hmp: export function hmp_handle_error()
  hmp: retrieve info from qmp for snapshot info
  hmp: show snapshot on single block device

 block.c               |  200 ++++++++++++++++++++++++++++++++++++++++++++++++-
 hmp.c                 |    2 +-
 hmp.h                 |    2 +
 include/block/block.h |   14 +++-
 monitor.c             |    6 +-
 qapi-schema.json      |   40 ++++++++++
 qemu-img.c            |   87 +---------------------
 qmp-commands.hx       |  129 +++++++++++++++++++++++++++++++
 savevm.c              |  137 +++++++++++++++++----------------
 9 files changed, 458 insertions(+), 159 deletions(-)

[Qemu-devel] [PATCH V3 01/11] qemu-img: remove unused parameter in collect_image_info()

[Wenchao Xia]

  Parameter *fmt was not used, so remove it.

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 qemu-img.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 85d3740..9dab48f 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1186,8 +1186,7 @@ static void dump_json_image_info(ImageInfo *info)
 
 static void collect_image_info(BlockDriverState *bs,
                    ImageInfo *info,
-                   const char *filename,
-                   const char *fmt)
+                   const char *filename)
 {
     uint64_t total_sectors;
     char backing_filename[1024];
@@ -1361,7 +1360,7 @@ static ImageInfoList *collect_image_info_list(const char *filename,
         }
 
         info = g_new0(ImageInfo, 1);
-        collect_image_info(bs, info, filename, fmt);
+        collect_image_info(bs, info, filename);
         collect_snapshots(bs, info);
 
         elem = g_new0(ImageInfoList, 1);
-- 
1.7.1

[Qemu-devel] [PATCH V3 02/11] block: add bdrv_get_filename() function

[Wenchao Xia]

  This function will simply return the uri or filename used
to open the image.

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 block.c               |    5 +++++
 include/block/block.h |    1 +
 2 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/block.c b/block.c
index 4e28c55..5f95da5 100644
--- a/block.c
+++ b/block.c
@@ -2967,6 +2967,11 @@ BlockStatsList *qmp_query_blockstats(Error **errp)
     return head;
 }
 
+const char *bdrv_get_filename(BlockDriverState *bs)
+{
+    return bs->filename;
+}
+
 const char *bdrv_get_encrypted_filename(BlockDriverState *bs)
 {
     if (bs->backing_hd && bs->backing_hd->encrypted)
diff --git a/include/block/block.h b/include/block/block.h
index 0719339..a0fc2a6 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -310,6 +310,7 @@ int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num,
                           const uint8_t *buf, int nb_sectors);
 int bdrv_get_info(BlockDriverState *bs, BlockDriverInfo *bdi);
 
+const char *bdrv_get_filename(BlockDriverState *bs);
 const char *bdrv_get_encrypted_filename(BlockDriverState *bs);
 void bdrv_get_backing_filename(BlockDriverState *bs,
                                char *filename, int filename_size);
-- 
1.7.1

[Qemu-devel] [PATCH V3 03/11] block: add snapshot and image info query function

[Wenchao Xia]

  This patch added function bdrv_query_image_info() and
bdrv_query_snapshot_infolist(), which will return info in qmp object
format. The implementation code are mostly copied from collect_image_info()
and collect_snapshot() in qemu-img.c.
  To help filter out snapshot info not needed, a call back function is
added in bdrv_query_snapshot_infolist().

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block.c               |  117 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/block/block.h |    9 ++++
 2 files changed, 126 insertions(+), 0 deletions(-)

diff --git a/block.c b/block.c
index 5f95da5..81765a3 100644
--- a/block.c
+++ b/block.c
@@ -2846,6 +2846,123 @@ int coroutine_fn bdrv_co_is_allocated_above(BlockDriverState *top,
     return 0;
 }
 
+SnapshotInfoList *bdrv_query_snapshot_infolist(BlockDriverState *bs,
+                                               SnapshotFilterFunc filter,
+                                               void *opaque,
+                                               Error **errp)
+{
+    int i, sn_count;
+    QEMUSnapshotInfo *sn_tab = NULL;
+    SnapshotInfoList *info_list, *cur_item = NULL, *head = NULL;
+    sn_count = bdrv_snapshot_list(bs, &sn_tab);
+    if (sn_count < 0) {
+        if (errp != NULL) {
+            error_setg(errp, "bdrv_snapshot_list: error %d\n", sn_count);
+        }
+        return NULL;
+    }
+
+    for (i = 0; i < sn_count; i++) {
+        if (filter && filter(&sn_tab[i], opaque) != 0) {
+            continue;
+        }
+
+        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 qemu-queue.h types */
+        if (!cur_item) {
+            head = cur_item = info_list;
+        } else {
+            cur_item->next = info_list;
+            cur_item = info_list;
+        }
+
+    }
+
+    g_free(sn_tab);
+    return head;
+}
+
+/* collect all internal snapshot info in a image for ImageInfo */
+static void collect_snapshots_info(BlockDriverState *bs,
+                                   ImageInfo *info,
+                                   Error **errp)
+{
+    SnapshotInfoList *info_list;
+
+    info_list = bdrv_query_snapshot_infolist(bs, NULL, NULL, errp);
+    if (info_list != NULL) {
+        info->has_snapshots = true;
+        info->snapshots = info_list;
+    }
+    return;
+}
+
+static void collect_image_info(BlockDriverState *bs,
+                               ImageInfo *info)
+{
+    uint64_t total_sectors;
+    char backing_filename[1024];
+    char backing_filename2[1024];
+    BlockDriverInfo bdi;
+    const char *filename;
+
+    filename = bdrv_get_filename(bs);
+    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;
+        }
+        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 (bs->backing_format[0]) {
+            info->backing_filename_format = g_strdup(bs->backing_format);
+            info->has_backing_filename_format = true;
+        }
+    }
+}
+
+ImageInfo *bdrv_query_image_info(BlockDriverState *bs, Error **errp)
+{
+    ImageInfo *info = g_new0(ImageInfo, 1);
+    collect_image_info(bs, info);
+    collect_snapshots_info(bs, info, errp);
+    return info;
+}
+
 BlockInfo *bdrv_query_info(BlockDriverState *bs)
 {
     BlockInfo *info = g_malloc0(sizeof(*info));
diff --git a/include/block/block.h b/include/block/block.h
index a0fc2a6..67f0d13 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -316,8 +316,17 @@ void bdrv_get_backing_filename(BlockDriverState *bs,
                                char *filename, int filename_size);
 void bdrv_get_full_backing_filename(BlockDriverState *bs,
                                     char *dest, size_t sz);
+
+typedef int (*SnapshotFilterFunc)(const QEMUSnapshotInfo *sn, void *opaque);
+/* assume bs is already opened, use qapi_free_* to free returned value. */
+SnapshotInfoList *bdrv_query_snapshot_infolist(BlockDriverState *bs,
+                                               SnapshotFilterFunc filter,
+                                               void *opaque,
+                                               Error **errp);
+ImageInfo *bdrv_query_image_info(BlockDriverState *bs, Error **errp);
 BlockInfo *bdrv_query_info(BlockDriverState *s);
 BlockStats *bdrv_query_stats(const BlockDriverState *bs);
+
 int bdrv_can_snapshot(BlockDriverState *bs);
 int bdrv_is_snapshot(BlockDriverState *bs);
 BlockDriverState *bdrv_snapshots(void);
-- 
1.7.1

[Qemu-devel] [PATCH V3 04/11] qemu-img: switch image retrieving function

[Wenchao Xia]

  Now qemu-img call block layer function to get image info.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 qemu-img.c |   86 +----------------------------------------------------------
 1 files changed, 2 insertions(+), 84 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 9dab48f..e20551a 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1134,39 +1134,6 @@ static void dump_json_image_info_list(ImageInfoList *list)
     QDECREF(str);
 }
 
-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 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;
@@ -1184,54 +1151,6 @@ static void dump_json_image_info(ImageInfo *info)
     QDECREF(str);
 }
 
-static void collect_image_info(BlockDriverState *bs,
-                   ImageInfo *info,
-                   const char *filename)
-{
-    uint64_t total_sectors;
-    char backing_filename[1024];
-    char backing_filename2[1024];
-    BlockDriverInfo bdi;
-
-    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;
-        }
-        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 (bs->backing_format[0]) {
-            info->backing_filename_format = g_strdup(bs->backing_format);
-            info->has_backing_filename_format = true;
-        }
-    }
-}
-
 static void dump_human_image_info(ImageInfo *info)
 {
     char size_buf[128], dsize_buf[128];
@@ -1338,6 +1257,7 @@ static ImageInfoList *collect_image_info_list(const char *filename,
     ImageInfoList *head = NULL;
     ImageInfoList **last = &head;
     GHashTable *filenames;
+    Error *err = NULL;
 
     filenames = g_hash_table_new_full(g_str_hash, str_equal_func, NULL, NULL);
 
@@ -1359,9 +1279,7 @@ static ImageInfoList *collect_image_info_list(const char *filename,
             goto err;
         }
 
-        info = g_new0(ImageInfo, 1);
-        collect_image_info(bs, info, filename);
-        collect_snapshots(bs, info);
+        info = bdrv_query_image_info(bs, &err);
 
         elem = g_new0(ImageInfoList, 1);
         elem->value = info;
-- 
1.7.1

[Qemu-devel] [PATCH V3 05/11] block: rename bdrv_query_info to bdrv_query_block_info

[Wenchao Xia]

  Now that we have bdrv_query_image_info, rename this function to make it
more obvious what it is doing.

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 block.c               |    4 ++--
 include/block/block.h |    2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/block.c b/block.c
index 81765a3..502c06e 100644
--- a/block.c
+++ b/block.c
@@ -2963,7 +2963,7 @@ ImageInfo *bdrv_query_image_info(BlockDriverState *bs, Error **errp)
     return info;
 }
 
-BlockInfo *bdrv_query_info(BlockDriverState *bs)
+BlockInfo *bdrv_query_block_info(BlockDriverState *bs)
 {
     BlockInfo *info = g_malloc0(sizeof(*info));
     info->device = g_strdup(bs->device_name);
@@ -3029,7 +3029,7 @@ BlockInfoList *qmp_query_block(Error **errp)
 
     QTAILQ_FOREACH(bs, &bdrv_states, list) {
         BlockInfoList *info = g_malloc0(sizeof(*info));
-        info->value = bdrv_query_info(bs);
+        info->value = bdrv_query_block_info(bs);
 
         *p_next = info;
         p_next = &info->next;
diff --git a/include/block/block.h b/include/block/block.h
index 67f0d13..5ce817a 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -324,7 +324,7 @@ SnapshotInfoList *bdrv_query_snapshot_infolist(BlockDriverState *bs,
                                                void *opaque,
                                                Error **errp);
 ImageInfo *bdrv_query_image_info(BlockDriverState *bs, Error **errp);
-BlockInfo *bdrv_query_info(BlockDriverState *s);
+BlockInfo *bdrv_query_block_info(BlockDriverState *bs);
 BlockStats *bdrv_query_stats(const BlockDriverState *bs);
 
 int bdrv_can_snapshot(BlockDriverState *bs);
-- 
1.7.1

[Qemu-devel] [PATCH V3 06/11] qmp: add interface query-images.

[Wenchao Xia]

  This mirror function will return all image info including
snapshots. Now Qemu have both query-images and query-block
interfaces.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 block.c          |   19 +++++++++++++
 qapi-schema.json |   27 +++++++++++++++++++
 qmp-commands.hx  |   76 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 122 insertions(+), 0 deletions(-)

diff --git a/block.c b/block.c
index 502c06e..8192d8e 100644
--- a/block.c
+++ b/block.c
@@ -2963,6 +2963,25 @@ ImageInfo *bdrv_query_image_info(BlockDriverState *bs, Error **errp)
     return info;
 }
 
+DeviceImageInfoList *qmp_query_images(Error **errp)
+{
+    DeviceImageInfoList *head = NULL, **p_next = &head;
+    BlockDriverState *bs;
+
+    QTAILQ_FOREACH(bs, &bdrv_states, list) {
+        DeviceImageInfo *dii = g_malloc0(sizeof(*dii));
+        dii->device = g_strdup(bs->device_name);
+        dii->info = bdrv_query_image_info(bs, NULL);
+        DeviceImageInfoList *diil = g_malloc0(sizeof(*diil));
+        diil->value = dii;
+
+        *p_next = diil;
+        p_next = &diil->next;
+    }
+
+    return head;
+}
+
 BlockInfo *bdrv_query_block_info(BlockDriverState *bs)
 {
     BlockInfo *info = g_malloc0(sizeof(*info));
diff --git a/qapi-schema.json b/qapi-schema.json
index 5dfa052..565737c 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -245,6 +245,22 @@
            '*backing-filename-format': 'str', '*snapshots': ['SnapshotInfo'] } }
 
 ##
+# @DeviceImageInfo:
+#
+# Information about an image used by QEMU block device
+#
+# @device: name of the block device
+#
+# @info: info of the image used.
+#
+# Since: 1.4
+#
+##
+
+{ 'type': 'DeviceImageInfo',
+  'data': {'device': 'str', 'info': 'ImageInfo' } }
+
+##
 # @StatusInfo:
 #
 # Information about VCPU run state
@@ -720,6 +736,17 @@
 { 'command': 'query-block', 'returns': ['BlockInfo'] }
 
 ##
+# @query-images:
+#
+# Get a list of DeviceImageInfo for all virtual block devices.
+#
+# Returns: a list of @DeviceImageInfo describing each virtual block device
+#
+# Since: 1.4
+##
+{ 'command': 'query-images', 'returns': ['DeviceImageInfo'] }
+
+##
 # @BlockDeviceStats:
 #
 # Statistics of a virtual block device or a block backing device.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 5c692d0..c31a142 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1668,6 +1668,82 @@ EQMP
     },
 
 SQMP
+query-images
+-----------
+
+Show the block devices' images.
+
+Each block image information is stored in a json-object and the returned value
+is a json-array of all devices' images.
+
+Each json-object contain the following:
+
+- "device": device name (json-string)
+- "info": related image information, it is a json-object containing the following:
+         - "filename": image file name (json-string)
+         - "format": image format (json-string)
+         - "virtual-size": maximum capacity in bytes of the image (json-int)
+         - "dirty-flag": true if image is not cleanly closed (json-bool, optional)
+         - "actual-size": actual size on disk in bytes of the image (json-int, optional)
+         - "cluster-size": size of a cluster in bytes (json-int, optional)
+         - "encrypted": true if the image is encrypted (json-bool, optional)
+         - "backing_file": backing file name (json-string, optional)
+         - "full-backing-filename": full path of the backing file (json-string, optional)
+         - "backing-filename-format": the format of the backing file (json-string, optional)
+         - "snapshots": the internal snapshot info, it is an optional list of json-object
+            containing the following:
+             - "id": unique snapshot id (json-string)
+             - "name": internal snapshot name (json-string)
+             - "vm-state-size": size of the VM state in bytes (json-int)
+             - "date-sec": UTC date of the snapshot in seconds (json-int)
+             - "date-nsec": fractional part in nano seconds to be used with date-sec(json-int)
+             - "vm-clock-sec": VM clock relative to boot in seconds (json-int)
+             - "vm-clock-nsec": fractional part in nano seconds to be used with vm-clock-sec (json-int)
+
+Example:
+
+-> { "execute": "query-images" }
+<- {
+      "return":[
+         {
+            "device":"ide0-hd0",
+            "info":{
+               "filename":"disks/test0.img",
+               "format":"qcow2",
+               "virtual-size":1024000
+            }
+         },
+         {
+            "device":"ide0-hd1",
+            "info":{
+               "filename":"disks/test1.img",
+               "format":"qcow2",
+               "virtual-size":2048000,
+               "snapshots":[
+                  {
+                     "id": "1",
+                     "name": "snapshot1",
+                     "vm-state-size": 0,
+                     "date-sec": 10000200,
+                     "date-nsec": 12,
+                     "vm-clock-sec": 206,
+                     "vm-clock-nsec": 30
+                  }
+               ]
+            }
+         }
+      ]
+   }
+
+EQMP
+
+    {
+        .name       = "query-images",
+        .args_type  = "",
+        .mhandler.cmd_new = qmp_marshal_input_query_images,
+    },
+
+SQMP
 query-blockstats
 ----------------
 
-- 
1.7.1

[Qemu-devel] [PATCH V3 07/11] block: export function bdrv_find_snapshot()

[Wenchao Xia]

  This patch move it from savevm.c to block.c and export it.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 block.c               |   23 +++++++++++++++++++++++
 include/block/block.h |    2 ++
 savevm.c              |   22 ----------------------
 3 files changed, 25 insertions(+), 22 deletions(-)

diff --git a/block.c b/block.c
index 8192d8e..b7d2f03 100644
--- a/block.c
+++ b/block.c
@@ -3351,6 +3351,29 @@ int bdrv_snapshot_load_tmp(BlockDriverState *bs,
     return -ENOTSUP;
 }
 
+int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
+                       const char *name)
+{
+    QEMUSnapshotInfo *sn_tab, *sn;
+    int nb_sns, i, ret;
+
+    ret = -ENOENT;
+    nb_sns = bdrv_snapshot_list(bs, &sn_tab);
+    if (nb_sns < 0) {
+        return ret;
+    }
+    for (i = 0; i < nb_sns; i++) {
+        sn = &sn_tab[i];
+        if (!strcmp(sn->id_str, name) || !strcmp(sn->name, name)) {
+            *sn_info = *sn;
+            ret = 0;
+            break;
+        }
+    }
+    g_free(sn_tab);
+    return ret;
+}
+
 /* backing_file can either be relative, or absolute, or a protocol.  If it is
  * relative, it must be relative to the chain.  So, passing in bs->filename
  * from a BDS as backing_file should not be done, as that may be relative to
diff --git a/include/block/block.h b/include/block/block.h
index 5ce817a..c473be5 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -340,6 +340,8 @@ int bdrv_snapshot_list(BlockDriverState *bs,
 int bdrv_snapshot_load_tmp(BlockDriverState *bs,
                            const char *snapshot_name);
 char *bdrv_snapshot_dump(char *buf, int buf_size, QEMUSnapshotInfo *sn);
+int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
+                       const char *name);
 
 char *get_human_readable_size(char *buf, int buf_size, int64_t size);
 int path_is_absolute(const char *path);
diff --git a/savevm.c b/savevm.c
index 021420f..9af2605 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2036,28 +2036,6 @@ out:
     return ret;
 }
 
-static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
-                              const char *name)
-{
-    QEMUSnapshotInfo *sn_tab, *sn;
-    int nb_sns, i, ret;
-
-    ret = -ENOENT;
-    nb_sns = bdrv_snapshot_list(bs, &sn_tab);
-    if (nb_sns < 0)
-        return ret;
-    for(i = 0; i < nb_sns; i++) {
-        sn = &sn_tab[i];
-        if (!strcmp(sn->id_str, name) || !strcmp(sn->name, name)) {
-            *sn_info = *sn;
-            ret = 0;
-            break;
-        }
-    }
-    g_free(sn_tab);
-    return ret;
-}
-
 /*
  * Deletes snapshots of a given name in all opened images.
  */
-- 
1.7.1

[Qemu-devel] [PATCH V3 08/11] qmp: add interface query-snapshots

[Wenchao Xia]

  This interface now return valid internal snapshots.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 block.c          |   32 ++++++++++++++++++++++++++++++++
 qapi-schema.json |   13 +++++++++++++
 qmp-commands.hx  |   53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 98 insertions(+), 0 deletions(-)

diff --git a/block.c b/block.c
index b7d2f03..990a07f 100644
--- a/block.c
+++ b/block.c
@@ -2892,6 +2892,38 @@ SnapshotInfoList *bdrv_query_snapshot_infolist(BlockDriverState *bs,
     return head;
 }
 
+/* check if sn exist on all block devices, 0 means valid */
+static int snapshot_filter_vm(const QEMUSnapshotInfo *sn, void *opaque)
+{
+    BlockDriverState *bs = (BlockDriverState *)opaque, *bs1 = NULL;
+    QEMUSnapshotInfo s, *sn_info = &s;
+    int ret = 0;
+
+    while ((bs1 = bdrv_next(bs1))) {
+        if (bdrv_can_snapshot(bs1) && bs1 != bs) {
+            ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str);
+            if (ret < 0) {
+                ret = -1;
+                break;
+            }
+        }
+    }
+    return ret;
+}
+
+SnapshotInfoList *qmp_query_snapshots(Error **errp)
+{
+    BlockDriverState *bs;
+
+    bs = bdrv_snapshots();
+    if (!bs) {
+        error_setg(errp, "No available block device supports snapshots\n");
+        return NULL;
+    }
+
+    return bdrv_query_snapshot_infolist(bs, snapshot_filter_vm, bs, errp);
+}
+
 /* collect all internal snapshot info in a image for ImageInfo */
 static void collect_snapshots_info(BlockDriverState *bs,
                                    ImageInfo *info,
diff --git a/qapi-schema.json b/qapi-schema.json
index 565737c..012da03 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -747,6 +747,19 @@
 { 'command': 'query-images', 'returns': ['DeviceImageInfo'] }
 
 ##
+# @query-snapshots:
+#
+# Get a list of valid snapshots of virtual machine. Note that only valid
+# internal snapshot will be returned, inconsistent ones will not be returned.
+#
+# Returns: a list of @SnapshotInfo describing all consistent virtual machine
+#          snapshots.
+#
+# Since: 1.4
+##
+{ 'command': 'query-snapshots', 'returns': ['SnapshotInfo'] }
+
+##
 # @BlockDeviceStats:
 #
 # Statistics of a virtual block device or a block backing device.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index c31a142..ab8e869 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1744,6 +1744,59 @@ EQMP
     },
 
 SQMP
+query-snapshots
+-----------
+
+Show the internal consistent snapshot information.
+
+Each snapshot information is stored in a json-object and the returned value
+is a json-array of all snapshots.
+
+Each json-object contain the following:
+
+- "id": unique snapshot id (json-string)
+- "name": internal snapshot name (json-string)
+- "vm-state-size": size of the VM state in bytes (json-int)
+- "date-sec": UTC date of the snapshot in seconds (json-int)
+- "date-nsec": fractional part in nano seconds to be used with date-sec(json-int)
+- "vm-clock-sec": VM clock relative to boot in seconds (json-int)
+- "vm-clock-nsec": fractional part in nano seconds to be used with vm-clock-sec (json-int)
+
+Example:
+
+-> { "execute": "query-snapshots" }
+<- {
+      "return":[
+         {
+            "id": "1",
+            "name": "snapshot1",
+            "vm-state-size": 0,
+            "date-sec": 10000200,
+            "date-nsec": 12,
+            "vm-clock-sec": 206,
+            "vm-clock-nsec": 30
+         },
+         {
+            "id": "2",
+            "name": "snapshot2",
+            "vm-state-size": 34000000,
+            "date-sec": 13000200,
+            "date-nsec": 32,
+            "vm-clock-sec": 406,
+            "vm-clock-nsec": 31
+         }
+      ]
+   }
+
+EQMP
+
+    {
+        .name       = "query-snapshots",
+        .args_type  = "",
+        .mhandler.cmd_new = qmp_marshal_input_query_snapshots,
+    },
+
+SQMP
 query-blockstats
 ----------------
 
-- 
1.7.1

[Qemu-devel] [PATCH V3 09/11] hmp: export function hmp_handle_error()

[Wenchao Xia]

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 hmp.c |    2 +-
 hmp.h |    2 ++
 2 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/hmp.c b/hmp.c
index 2465d9b..89a1a8c 100644
--- a/hmp.c
+++ b/hmp.c
@@ -23,7 +23,7 @@
 #include "monitor/monitor.h"
 #include "ui/console.h"
 
-static void hmp_handle_error(Monitor *mon, Error **errp)
+void hmp_handle_error(Monitor *mon, Error **errp)
 {
     if (error_is_set(errp)) {
         monitor_printf(mon, "%s\n", error_get_pretty(*errp));
diff --git a/hmp.h b/hmp.h
index 5cab9c0..da1a35c 100644
--- a/hmp.h
+++ b/hmp.h
@@ -17,7 +17,9 @@
 #include "qemu-common.h"
 #include "qapi-types.h"
 #include "qapi/qmp/qdict.h"
+#include "qapi/error.h"
 
+void hmp_handle_error(Monitor *mon, Error **errp);
 void hmp_info_name(Monitor *mon, const QDict *qdict);
 void hmp_info_version(Monitor *mon, const QDict *qdict);
 void hmp_info_kvm(Monitor *mon, const QDict *qdict);
-- 
1.7.1

[Qemu-devel] [PATCH V3 10/11] hmp: retrieve info from qmp for snapshot info

[Wenchao Xia]

  With this patch, hmp command info snapshot simply call a block
layer funtion which will return a qmp object, and then translate
it in monitor console. Now snapshot info retrieving code in qemu
and qemu-tool are merged by calling same block layer function,
and then they just translate the qmp to stdout or monitor.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>

---
Note:
  This patch need previous hmp extention patch which enable
info sub command take qdict * as paramter.
---
 savevm.c |   84 +++++++++++++++++++++++++-------------------------------------
 1 files changed, 34 insertions(+), 50 deletions(-)

diff --git a/savevm.c b/savevm.c
index 9af2605..cabdcb6 100644
--- a/savevm.c
+++ b/savevm.c
@@ -48,6 +48,7 @@
 #include "qmp-commands.h"
 #include "trace.h"
 #include "qemu/bitops.h"
+#include "hmp.h"
 
 #define SELF_ANNOUNCE_ROUNDS 5
 
@@ -2311,68 +2312,51 @@ void do_delvm(Monitor *mon, const QDict *qdict)
     }
 }
 
-void do_info_snapshots(Monitor *mon, const QDict *qdict)
+/* assume list is valid */
+static void monitor_dump_snapshotinfolist(Monitor *mon, SnapshotInfoList *list)
 {
-    BlockDriverState *bs, *bs1;
-    QEMUSnapshotInfo *sn_tab, *sn, s, *sn_info = &s;
-    int nb_sns, i, ret, available;
-    int total;
-    int *available_snapshots;
+    SnapshotInfoList *elem;
     char buf[256];
 
-    bs = bdrv_snapshots();
-    if (!bs) {
-        monitor_printf(mon, "No available block device supports snapshots\n");
-        return;
-    }
-
-    nb_sns = bdrv_snapshot_list(bs, &sn_tab);
-    if (nb_sns < 0) {
-        monitor_printf(mon, "bdrv_snapshot_list: error %d\n", nb_sns);
-        return;
-    }
+    monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL));
 
-    if (nb_sns == 0) {
-        monitor_printf(mon, "There is no snapshot available.\n");
-        return;
+    for (elem = list; elem; elem = elem->next) {
+        QEMUSnapshotInfo sn = {
+            .vm_state_size = elem->value->vm_state_size,
+            .date_sec = elem->value->date_sec,
+            .date_nsec = elem->value->date_nsec,
+            .vm_clock_nsec = elem->value->vm_clock_sec * 1000000000ULL +
+                             elem->value->vm_clock_nsec,
+        };
+        pstrcpy(sn.id_str, sizeof(sn.id_str), elem->value->id);
+        pstrcpy(sn.name, sizeof(sn.name), elem->value->name);
+        monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), &sn));
     }
+}
 
-    available_snapshots = g_malloc0(sizeof(int) * nb_sns);
-    total = 0;
-    for (i = 0; i < nb_sns; i++) {
-        sn = &sn_tab[i];
-        available = 1;
-        bs1 = NULL;
-
-        while ((bs1 = bdrv_next(bs1))) {
-            if (bdrv_can_snapshot(bs1) && bs1 != bs) {
-                ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str);
-                if (ret < 0) {
-                    available = 0;
-                    break;
-                }
-            }
-        }
+static void do_info_snapshots_vm(Monitor *mon)
+{
+    Error *err = NULL;
+    SnapshotInfoList *list;
 
-        if (available) {
-            available_snapshots[total] = i;
-            total++;
-        }
+    list = qmp_query_snapshots(&err);
+    if (error_is_set(&err)) {
+        hmp_handle_error(mon, &err);
+        return;
     }
-
-    if (total > 0) {
-        monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL));
-        for (i = 0; i < total; i++) {
-            sn = &sn_tab[available_snapshots[i]];
-            monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), sn));
-        }
-    } else {
+    if (list == NULL) {
         monitor_printf(mon, "There is no suitable snapshot available\n");
+        return;
     }
 
-    g_free(sn_tab);
-    g_free(available_snapshots);
+    monitor_dump_snapshotinfolist(mon, list);
+    qapi_free_SnapshotInfoList(list);
+    return;
+}
 
+void do_info_snapshots(Monitor *mon, const QDict *qdict)
+{
+    do_info_snapshots_vm(mon);
 }
 
 void vmstate_register_ram(MemoryRegion *mr, DeviceState *dev)
-- 
1.7.1

[Qemu-devel] [PATCH V3 11/11] hmp: show snapshot on single block device

[Wenchao Xia]

  This patch use block layer API to qmp snapshot info on a block
device, then use the same code dumping vm snapshot info, to print
in monitor.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
Note:
  This patch need previous hmp extention patch which enable
info sub command take qdict * as paramter.

---
 monitor.c |    6 +++---
 savevm.c  |   43 ++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 45 insertions(+), 4 deletions(-)

diff --git a/monitor.c b/monitor.c
index d186532..cba75a2 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2583,9 +2583,9 @@ static mon_cmd_t info_cmds[] = {
     },
     {
         .name       = "snapshots",
-        .args_type  = "",
-        .params     = "",
-        .help       = "show the currently saved VM snapshots",
+        .args_type  = "device:B?",
+        .params     = "[device]",
+        .help       = "show snapshots of whole vm or a single device",
         .mhandler.cmd = do_info_snapshots,
     },
     {
diff --git a/savevm.c b/savevm.c
index cabdcb6..cd474e9 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2354,9 +2354,50 @@ static void do_info_snapshots_vm(Monitor *mon)
     return;
 }
 
+static void do_info_snapshots_blk(Monitor *mon, const char *device)
+{
+    Error *err = NULL;
+    SnapshotInfoList *list;
+    BlockDriverState *bs;
+
+    /* find the target bs */
+    bs = bdrv_find(device);
+    if (!bs) {
+        monitor_printf(mon, "Device '%s' not found.\n", device);
+        return ;
+    }
+
+    if (!bdrv_can_snapshot(bs)) {
+        monitor_printf(mon, "Device '%s' can't have snapshot.\n", device);
+        return ;
+    }
+
+    list = bdrv_query_snapshot_infolist(bs, NULL, NULL, &err);
+    if (error_is_set(&err)) {
+        hmp_handle_error(mon, &err);
+        return;
+    }
+
+    if (list == NULL) {
+        monitor_printf(mon, "There is no snapshot available.\n");
+        return;
+    }
+
+    monitor_printf(mon, "Device '%s':\n", device);
+    monitor_dump_snapshotinfolist(mon, list);
+    qapi_free_SnapshotInfoList(list);
+    return;
+}
+
 void do_info_snapshots(Monitor *mon, const QDict *qdict)
 {
-    do_info_snapshots_vm(mon);
+    const char *device = qdict_get_try_str(qdict, "device");
+    if (!device) {
+        do_info_snapshots_vm(mon);
+    } else {
+        do_info_snapshots_blk(mon, device);
+    }
+    return;
 }
 
 void vmstate_register_ram(MemoryRegion *mr, DeviceState *dev)
-- 
1.7.1

Re: [Qemu-devel] [PATCH V3 11/11] hmp: show snapshot on single block device

[Pavel Hrdina]

On Mon, 2013-01-14 at 15:09 +0800, Wenchao Xia wrote:
>   This patch use block layer API to qmp snapshot info on a block
> device, then use the same code dumping vm snapshot info, to print
> in monitor.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
> Note:
>   This patch need previous hmp extention patch which enable
> info sub command take qdict * as paramter.

> diff --git a/savevm.c b/savevm.c
> index cabdcb6..cd474e9 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -2354,9 +2354,50 @@ static void do_info_snapshots_vm(Monitor *mon)
>      return;
>  }
>  
> +static void do_info_snapshots_blk(Monitor *mon, const char *device)
> +{
> +    Error *err = NULL;
> +    SnapshotInfoList *list;
> +    BlockDriverState *bs;
> +
> +    /* find the target bs */
> +    bs = bdrv_find(device);
> +    if (!bs) {
> +        monitor_printf(mon, "Device '%s' not found.\n", device);
> +        return ;
> +    }
> +
> +    if (!bdrv_can_snapshot(bs)) {
> +        monitor_printf(mon, "Device '%s' can't have snapshot.\n", device);
> +        return ;
> +    }
> +
> +    list = bdrv_query_snapshot_infolist(bs, NULL, NULL, &err);
> +    if (error_is_set(&err)) {
> +        hmp_handle_error(mon, &err);
> +        return;
> +    }
> +
> +    if (list == NULL) {
> +        monitor_printf(mon, "There is no snapshot available.\n");
> +        return;
> +    }
> +
> +    monitor_printf(mon, "Device '%s':\n", device);
> +    monitor_dump_snapshotinfolist(mon, list);
> +    qapi_free_SnapshotInfoList(list);
> +    return;
> +}
> +
>  void do_info_snapshots(Monitor *mon, const QDict *qdict)
>  {
> -    do_info_snapshots_vm(mon);
> +    const char *device = qdict_get_try_str(qdict, "device");
> +    if (!device) {
> +        do_info_snapshots_vm(mon);
> +    } else {
> +        do_info_snapshots_blk(mon, device);
> +    }
> +    return;
>  }
>  

I think that you should move these functions into hmp.c file. This also
applies to previous patch and according to this changes you don't have
to export hmp_handle_error() function which should be used only in hmp.c

Pavel

Re: [Qemu-devel] [PATCH V3 04/11] qemu-img: switch image retrieving function

[Pavel Hrdina]

On Mon, 2013-01-14 at 15:09 +0800, Wenchao Xia wrote:
>   Now qemu-img call block layer function to get image info.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  qemu-img.c |   86 +----------------------------------------------------------
>  1 files changed, 2 insertions(+), 84 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 9dab48f..e20551a 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1338,6 +1257,7 @@ static ImageInfoList *collect_image_info_list(const char *filename,
>      ImageInfoList *head = NULL;
>      ImageInfoList **last = &head;
>      GHashTable *filenames;
> +    Error *err = NULL;
>  
>      filenames = g_hash_table_new_full(g_str_hash, str_equal_func, NULL, NULL);
>  
> @@ -1359,9 +1279,7 @@ static ImageInfoList *collect_image_info_list(const char *filename,
>              goto err;
>          }
>  
> -        info = g_new0(ImageInfo, 1);
> -        collect_image_info(bs, info, filename);
> -        collect_snapshots(bs, info);
> +        info = bdrv_query_image_info(bs, &err);

You are not using the 'err' variable so you should pass 'NULL' instead. 

	info = bdrv_query_image_info(bs, NULL);

>  
>          elem = g_new0(ImageInfoList, 1);
>          elem->value = info;

Re: [Qemu-devel] [PATCH V3 01/11] qemu-img: remove unused parameter in collect_image_info()

[Luiz Capitulino]

On Mon, 14 Jan 2013 15:09:37 +0800
Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:

>   Parameter *fmt was not used, so remove it.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  qemu-img.c |    5 ++---
>  1 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 85d3740..9dab48f 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -1186,8 +1186,7 @@ static void dump_json_image_info(ImageInfo *info)
>  
>  static void collect_image_info(BlockDriverState *bs,
>                     ImageInfo *info,
> -                   const char *filename,
> -                   const char *fmt)
> +                   const char *filename)

collect_image_info_list() doc reads:

 @fmt: topmost image format (may be NULL to autodetect)

However, right now only fmt=NULL is supported, as collect_image_info()
ignores fmt altogether.

So, if this patch is correct we better update the comment. Otherwise,
we should improve collect_image_info() to actually obey fmt != NULL.

>  {
>      uint64_t total_sectors;
>      char backing_filename[1024];
> @@ -1361,7 +1360,7 @@ static ImageInfoList *collect_image_info_list(const char *filename,
>          }
>  
>          info = g_new0(ImageInfo, 1);
> -        collect_image_info(bs, info, filename, fmt);
> +        collect_image_info(bs, info, filename);
>          collect_snapshots(bs, info);
>  
>          elem = g_new0(ImageInfoList, 1);

Re: [Qemu-devel] [PATCH V3 02/11] block: add bdrv_get_filename() function

[Luiz Capitulino]

On Mon, 14 Jan 2013 15:09:38 +0800
Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:

>   This function will simply return the uri or filename used
> to open the image.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  block.c               |    5 +++++
>  include/block/block.h |    1 +
>  2 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 4e28c55..5f95da5 100644
> --- a/block.c
> +++ b/block.c
> @@ -2967,6 +2967,11 @@ BlockStatsList *qmp_query_blockstats(Error **errp)
>      return head;
>  }
>  
> +const char *bdrv_get_filename(BlockDriverState *bs)
> +{

bs can be const.

> +    return bs->filename;
> +}
> +
>  const char *bdrv_get_encrypted_filename(BlockDriverState *bs)
>  {
>      if (bs->backing_hd && bs->backing_hd->encrypted)
> diff --git a/include/block/block.h b/include/block/block.h
> index 0719339..a0fc2a6 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -310,6 +310,7 @@ int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num,
>                            const uint8_t *buf, int nb_sectors);
>  int bdrv_get_info(BlockDriverState *bs, BlockDriverInfo *bdi);
>  
> +const char *bdrv_get_filename(BlockDriverState *bs);
>  const char *bdrv_get_encrypted_filename(BlockDriverState *bs);
>  void bdrv_get_backing_filename(BlockDriverState *bs,
>                                 char *filename, int filename_size);

Re: [Qemu-devel] [PATCH V3 03/11] block: add snapshot and image info query function

[Luiz Capitulino]

On Mon, 14 Jan 2013 15:09:39 +0800
Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:

>   This patch added function bdrv_query_image_info() and
> bdrv_query_snapshot_infolist(), which will return info in qmp object
> format. The implementation code are mostly copied from collect_image_info()
> and collect_snapshot() in qemu-img.c.
>   To help filter out snapshot info not needed, a call back function is
> added in bdrv_query_snapshot_infolist().
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  block.c               |  117 +++++++++++++++++++++++++++++++++++++++++++++++++
>  include/block/block.h |    9 ++++
>  2 files changed, 126 insertions(+), 0 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 5f95da5..81765a3 100644
> --- a/block.c
> +++ b/block.c
> @@ -2846,6 +2846,123 @@ int coroutine_fn bdrv_co_is_allocated_above(BlockDriverState *top,
>      return 0;
>  }
>  
> +SnapshotInfoList *bdrv_query_snapshot_infolist(BlockDriverState *bs,
> +                                               SnapshotFilterFunc filter,
> +                                               void *opaque,
> +                                               Error **errp)
> +{
> +    int i, sn_count;
> +    QEMUSnapshotInfo *sn_tab = NULL;
> +    SnapshotInfoList *info_list, *cur_item = NULL, *head = NULL;
> +    sn_count = bdrv_snapshot_list(bs, &sn_tab);
> +    if (sn_count < 0) {
> +        if (errp != NULL) {

This check is already done by error_setg(), you don't have to do it.

> +            error_setg(errp, "bdrv_snapshot_list: error %d\n", sn_count);

This doesn't tell much about the error cause. The best thing to do here
is to guarantee that drv->bdrv_snapshot_list() callbacks always set errno
and then use error_setg_errno().

> +        }
> +        return NULL;
> +    }
> +
> +    for (i = 0; i < sn_count; i++) {
> +        if (filter && filter(&sn_tab[i], opaque) != 0) {
> +            continue;
> +        }
> +
> +        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 qemu-queue.h types */
> +        if (!cur_item) {
> +            head = cur_item = info_list;
> +        } else {
> +            cur_item->next = info_list;
> +            cur_item = info_list;
> +        }
> +
> +    }
> +
> +    g_free(sn_tab);
> +    return head;
> +}
> +
> +/* collect all internal snapshot info in a image for ImageInfo */
> +static void collect_snapshots_info(BlockDriverState *bs,
> +                                   ImageInfo *info,
> +                                   Error **errp)
> +{
> +    SnapshotInfoList *info_list;
> +
> +    info_list = bdrv_query_snapshot_infolist(bs, NULL, NULL, errp);
> +    if (info_list != NULL) {
> +        info->has_snapshots = true;
> +        info->snapshots = info_list;
> +    }
> +    return;
> +}
> +
> +static void collect_image_info(BlockDriverState *bs,
> +                               ImageInfo *info)

IMO, this and the next function have to split from this patch.

> +{
> +    uint64_t total_sectors;
> +    char backing_filename[1024];
> +    char backing_filename2[1024];
> +    BlockDriverInfo bdi;
> +    const char *filename;
> +
> +    filename = bdrv_get_filename(bs);
> +    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;
> +        }
> +        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 (bs->backing_format[0]) {
> +            info->backing_filename_format = g_strdup(bs->backing_format);
> +            info->has_backing_filename_format = true;
> +        }
> +    }
> +}
> +
> +ImageInfo *bdrv_query_image_info(BlockDriverState *bs, Error **errp)
> +{
> +    ImageInfo *info = g_new0(ImageInfo, 1);
> +    collect_image_info(bs, info);
> +    collect_snapshots_info(bs, info, errp);
> +    return info;
> +}
> +
>  BlockInfo *bdrv_query_info(BlockDriverState *bs)
>  {
>      BlockInfo *info = g_malloc0(sizeof(*info));
> diff --git a/include/block/block.h b/include/block/block.h
> index a0fc2a6..67f0d13 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -316,8 +316,17 @@ void bdrv_get_backing_filename(BlockDriverState *bs,
>                                 char *filename, int filename_size);
>  void bdrv_get_full_backing_filename(BlockDriverState *bs,
>                                      char *dest, size_t sz);
> +
> +typedef int (*SnapshotFilterFunc)(const QEMUSnapshotInfo *sn, void *opaque);
> +/* assume bs is already opened, use qapi_free_* to free returned value. */
> +SnapshotInfoList *bdrv_query_snapshot_infolist(BlockDriverState *bs,
> +                                               SnapshotFilterFunc filter,
> +                                               void *opaque,
> +                                               Error **errp);
> +ImageInfo *bdrv_query_image_info(BlockDriverState *bs, Error **errp);
>  BlockInfo *bdrv_query_info(BlockDriverState *s);
>  BlockStats *bdrv_query_stats(const BlockDriverState *bs);
> +
>  int bdrv_can_snapshot(BlockDriverState *bs);
>  int bdrv_is_snapshot(BlockDriverState *bs);
>  BlockDriverState *bdrv_snapshots(void);

Re: [Qemu-devel] [PATCH V3 04/11] qemu-img: switch image retrieving function

[Luiz Capitulino]

On Mon, 14 Jan 2013 12:25:08 +0100
Pavel Hrdina <phrdina@redhat.com> wrote:

> On Mon, 2013-01-14 at 15:09 +0800, Wenchao Xia wrote:
> >   Now qemu-img call block layer function to get image info.
> > 
> > Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> > Reviewed-by: Eric Blake <eblake@redhat.com>
> > ---
> >  qemu-img.c |   86 +----------------------------------------------------------
> >  1 files changed, 2 insertions(+), 84 deletions(-)
> > 
> > diff --git a/qemu-img.c b/qemu-img.c
> > index 9dab48f..e20551a 100644
> > --- a/qemu-img.c
> > +++ b/qemu-img.c
> > @@ -1338,6 +1257,7 @@ static ImageInfoList *collect_image_info_list(const char *filename,
> >      ImageInfoList *head = NULL;
> >      ImageInfoList **last = &head;
> >      GHashTable *filenames;
> > +    Error *err = NULL;
> >  
> >      filenames = g_hash_table_new_full(g_str_hash, str_equal_func, NULL, NULL);
> >  
> > @@ -1359,9 +1279,7 @@ static ImageInfoList *collect_image_info_list(const char *filename,
> >              goto err;
> >          }
> >  
> > -        info = g_new0(ImageInfo, 1);
> > -        collect_image_info(bs, info, filename);
> > -        collect_snapshots(bs, info);
> > +        info = bdrv_query_image_info(bs, &err);
> 
> You are not using the 'err' variable so you should pass 'NULL' instead. 

Actually, it's necessary to check for the error.

> 
> 	info = bdrv_query_image_info(bs, NULL);
> 
> >  
> >          elem = g_new0(ImageInfoList, 1);
> >          elem->value = info;
> 
> 

Re: [Qemu-devel] [PATCH V3 06/11] qmp: add interface query-images.

[Luiz Capitulino]

On Mon, 14 Jan 2013 15:09:42 +0800
Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:

>   This mirror function will return all image info including
> snapshots. Now Qemu have both query-images and query-block
> interfaces.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  block.c          |   19 +++++++++++++
>  qapi-schema.json |   27 +++++++++++++++++++
>  qmp-commands.hx  |   76 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 122 insertions(+), 0 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 502c06e..8192d8e 100644
> --- a/block.c
> +++ b/block.c
> @@ -2963,6 +2963,25 @@ ImageInfo *bdrv_query_image_info(BlockDriverState *bs, Error **errp)
>      return info;
>  }
>  
> +DeviceImageInfoList *qmp_query_images(Error **errp)
> +{
> +    DeviceImageInfoList *head = NULL, **p_next = &head;
> +    BlockDriverState *bs;
> +
> +    QTAILQ_FOREACH(bs, &bdrv_states, list) {
> +        DeviceImageInfo *dii = g_malloc0(sizeof(*dii));
> +        dii->device = g_strdup(bs->device_name);
> +        dii->info = bdrv_query_image_info(bs, NULL);
> +        DeviceImageInfoList *diil = g_malloc0(sizeof(*diil));
> +        diil->value = dii;
> +
> +        *p_next = diil;
> +        p_next = &diil->next;
> +    }

When the device doesn't have an image, this returns:

        {
            "device": "ide1-cd0", 
            "info": {
                "virtual-size": 0, 
                "filename": "", 
                "format": ""
            }
        },

But it would be better to return an empty dict or even omit 'info'
completely. One more comment below.

> +
> +    return head;
> +}
> +
>  BlockInfo *bdrv_query_block_info(BlockDriverState *bs)
>  {
>      BlockInfo *info = g_malloc0(sizeof(*info));
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 5dfa052..565737c 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -245,6 +245,22 @@
>             '*backing-filename-format': 'str', '*snapshots': ['SnapshotInfo'] } }
>  
>  ##
> +# @DeviceImageInfo:
> +#
> +# Information about an image used by QEMU block device
> +#
> +# @device: name of the block device
> +#
> +# @info: info of the image used.

Info is too generic, please call it 'image'.

> +#
> +# Since: 1.4
> +#
> +##
> +
> +{ 'type': 'DeviceImageInfo',
> +  'data': {'device': 'str', 'info': 'ImageInfo' } }
> +
> +##
>  # @StatusInfo:
>  #
>  # Information about VCPU run state
> @@ -720,6 +736,17 @@
>  { 'command': 'query-block', 'returns': ['BlockInfo'] }
>  
>  ##
> +# @query-images:
> +#
> +# Get a list of DeviceImageInfo for all virtual block devices.
> +#
> +# Returns: a list of @DeviceImageInfo describing each virtual block device
> +#
> +# Since: 1.4
> +##
> +{ 'command': 'query-images', 'returns': ['DeviceImageInfo'] }
> +
> +##
>  # @BlockDeviceStats:
>  #
>  # Statistics of a virtual block device or a block backing device.
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 5c692d0..c31a142 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -1668,6 +1668,82 @@ EQMP
>      },
>  
>  SQMP
> +query-images
> +-----------
> +
> +Show the block devices' images.
> +
> +Each block image information is stored in a json-object and the returned value
> +is a json-array of all devices' images.
> +
> +Each json-object contain the following:
> +
> +- "device": device name (json-string)
> +- "info": related image information, it is a json-object containing the following:
> +         - "filename": image file name (json-string)
> +         - "format": image format (json-string)
> +         - "virtual-size": maximum capacity in bytes of the image (json-int)
> +         - "dirty-flag": true if image is not cleanly closed (json-bool, optional)
> +         - "actual-size": actual size on disk in bytes of the image (json-int, optional)
> +         - "cluster-size": size of a cluster in bytes (json-int, optional)
> +         - "encrypted": true if the image is encrypted (json-bool, optional)
> +         - "backing_file": backing file name (json-string, optional)
> +         - "full-backing-filename": full path of the backing file (json-string, optional)
> +         - "backing-filename-format": the format of the backing file (json-string, optional)
> +         - "snapshots": the internal snapshot info, it is an optional list of json-object
> +            containing the following:
> +             - "id": unique snapshot id (json-string)
> +             - "name": internal snapshot name (json-string)
> +             - "vm-state-size": size of the VM state in bytes (json-int)
> +             - "date-sec": UTC date of the snapshot in seconds (json-int)
> +             - "date-nsec": fractional part in nano seconds to be used with date-sec(json-int)
> +             - "vm-clock-sec": VM clock relative to boot in seconds (json-int)
> +             - "vm-clock-nsec": fractional part in nano seconds to be used with vm-clock-sec (json-int)
> +
> +Example:
> +
> +-> { "execute": "query-images" }
> +<- {
> +      "return":[
> +         {
> +            "device":"ide0-hd0",
> +            "info":{
> +               "filename":"disks/test0.img",
> +               "format":"qcow2",
> +               "virtual-size":1024000
> +            }
> +         },
> +         {
> +            "device":"ide0-hd1",
> +            "info":{
> +               "filename":"disks/test1.img",
> +               "format":"qcow2",
> +               "virtual-size":2048000,
> +               "snapshots":[
> +                  {
> +                     "id": "1",
> +                     "name": "snapshot1",
> +                     "vm-state-size": 0,
> +                     "date-sec": 10000200,
> +                     "date-nsec": 12,
> +                     "vm-clock-sec": 206,
> +                     "vm-clock-nsec": 30
> +                  }
> +               ]
> +            }
> +         }
> +      ]
> +   }
> +
> +EQMP
> +
> +    {
> +        .name       = "query-images",
> +        .args_type  = "",
> +        .mhandler.cmd_new = qmp_marshal_input_query_images,
> +    },
> +
> +SQMP
>  query-blockstats
>  ----------------
>  

Re: [Qemu-devel] [PATCH V3 09/11] hmp: export function hmp_handle_error()

[Luiz Capitulino]

On Mon, 14 Jan 2013 15:09:45 +0800
Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:

> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>

This function is really a hmp.c-only helper. It does two more or less
unrelated things, I think it's better not to spread it over other
qemu source files.

> ---
>  hmp.c |    2 +-
>  hmp.h |    2 ++
>  2 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/hmp.c b/hmp.c
> index 2465d9b..89a1a8c 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -23,7 +23,7 @@
>  #include "monitor/monitor.h"
>  #include "ui/console.h"
>  
> -static void hmp_handle_error(Monitor *mon, Error **errp)
> +void hmp_handle_error(Monitor *mon, Error **errp)
>  {
>      if (error_is_set(errp)) {
>          monitor_printf(mon, "%s\n", error_get_pretty(*errp));
> diff --git a/hmp.h b/hmp.h
> index 5cab9c0..da1a35c 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -17,7 +17,9 @@
>  #include "qemu-common.h"
>  #include "qapi-types.h"
>  #include "qapi/qmp/qdict.h"
> +#include "qapi/error.h"
>  
> +void hmp_handle_error(Monitor *mon, Error **errp);
>  void hmp_info_name(Monitor *mon, const QDict *qdict);
>  void hmp_info_version(Monitor *mon, const QDict *qdict);
>  void hmp_info_kvm(Monitor *mon, const QDict *qdict);

Re: [Qemu-devel] [PATCH V3 11/11] hmp: show snapshot on single block device

[Luiz Capitulino]

On Mon, 14 Jan 2013 15:09:47 +0800
Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:

>   This patch use block layer API to qmp snapshot info on a block
> device, then use the same code dumping vm snapshot info, to print
> in monitor.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
> Note:
>   This patch need previous hmp extention patch which enable
> info sub command take qdict * as paramter.
> 
> ---
>  monitor.c |    6 +++---
>  savevm.c  |   43 ++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 45 insertions(+), 4 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index d186532..cba75a2 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -2583,9 +2583,9 @@ static mon_cmd_t info_cmds[] = {
>      },
>      {
>          .name       = "snapshots",
> -        .args_type  = "",
> -        .params     = "",
> -        .help       = "show the currently saved VM snapshots",
> +        .args_type  = "device:B?",
> +        .params     = "[device]",
> +        .help       = "show snapshots of whole vm or a single device",
>          .mhandler.cmd = do_info_snapshots,
>      },
>      {
> diff --git a/savevm.c b/savevm.c
> index cabdcb6..cd474e9 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -2354,9 +2354,50 @@ static void do_info_snapshots_vm(Monitor *mon)
>      return;
>  }
>  
> +static void do_info_snapshots_blk(Monitor *mon, const char *device)
> +{
> +    Error *err = NULL;
> +    SnapshotInfoList *list;
> +    BlockDriverState *bs;
> +
> +    /* find the target bs */
> +    bs = bdrv_find(device);
> +    if (!bs) {
> +        monitor_printf(mon, "Device '%s' not found.\n", device);
> +        return ;
> +    }
> +
> +    if (!bdrv_can_snapshot(bs)) {
> +        monitor_printf(mon, "Device '%s' can't have snapshot.\n", device);
> +        return ;
> +    }
> +
> +    list = bdrv_query_snapshot_infolist(bs, NULL, NULL, &err);
> +    if (error_is_set(&err)) {
> +        hmp_handle_error(mon, &err);
> +        return;
> +    }

We try to restrict hmp.c to use the QMP API only (see Pavel's comment on
moving this to hmp.c).

If this can't be implemented using qmp_query_snapshots(), then I'd suggest
to add this capability to qmp_query_snapshots() or add a new QMP command.

> +
> +    if (list == NULL) {
> +        monitor_printf(mon, "There is no snapshot available.\n");
> +        return;
> +    }
> +
> +    monitor_printf(mon, "Device '%s':\n", device);
> +    monitor_dump_snapshotinfolist(mon, list);
> +    qapi_free_SnapshotInfoList(list);
> +    return;
> +}
> +
>  void do_info_snapshots(Monitor *mon, const QDict *qdict)
>  {
> -    do_info_snapshots_vm(mon);
> +    const char *device = qdict_get_try_str(qdict, "device");
> +    if (!device) {
> +        do_info_snapshots_vm(mon);
> +    } else {
> +        do_info_snapshots_blk(mon, device);
> +    }
> +    return;
>  }
>  
>  void vmstate_register_ram(MemoryRegion *mr, DeviceState *dev)

Re: [Qemu-devel] [PATCH V3 11/11] hmp: show snapshot on single block device

[Luiz Capitulino]

On Mon, 14 Jan 2013 12:15:37 +0100
Pavel Hrdina <phrdina@redhat.com> wrote:

> On Mon, 2013-01-14 at 15:09 +0800, Wenchao Xia wrote:
> >   This patch use block layer API to qmp snapshot info on a block
> > device, then use the same code dumping vm snapshot info, to print
> > in monitor.
> > 
> > Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> > ---
> > Note:
> >   This patch need previous hmp extention patch which enable
> > info sub command take qdict * as paramter.
> 
> > diff --git a/savevm.c b/savevm.c
> > index cabdcb6..cd474e9 100644
> > --- a/savevm.c
> > +++ b/savevm.c
> > @@ -2354,9 +2354,50 @@ static void do_info_snapshots_vm(Monitor *mon)
> >      return;
> >  }
> >  
> > +static void do_info_snapshots_blk(Monitor *mon, const char *device)
> > +{
> > +    Error *err = NULL;
> > +    SnapshotInfoList *list;
> > +    BlockDriverState *bs;
> > +
> > +    /* find the target bs */
> > +    bs = bdrv_find(device);
> > +    if (!bs) {
> > +        monitor_printf(mon, "Device '%s' not found.\n", device);
> > +        return ;
> > +    }
> > +
> > +    if (!bdrv_can_snapshot(bs)) {
> > +        monitor_printf(mon, "Device '%s' can't have snapshot.\n", device);
> > +        return ;
> > +    }
> > +
> > +    list = bdrv_query_snapshot_infolist(bs, NULL, NULL, &err);
> > +    if (error_is_set(&err)) {
> > +        hmp_handle_error(mon, &err);
> > +        return;
> > +    }
> > +
> > +    if (list == NULL) {
> > +        monitor_printf(mon, "There is no snapshot available.\n");
> > +        return;
> > +    }
> > +
> > +    monitor_printf(mon, "Device '%s':\n", device);
> > +    monitor_dump_snapshotinfolist(mon, list);
> > +    qapi_free_SnapshotInfoList(list);
> > +    return;
> > +}
> > +
> >  void do_info_snapshots(Monitor *mon, const QDict *qdict)
> >  {
> > -    do_info_snapshots_vm(mon);
> > +    const char *device = qdict_get_try_str(qdict, "device");
> > +    if (!device) {
> > +        do_info_snapshots_vm(mon);
> > +    } else {
> > +        do_info_snapshots_blk(mon, device);
> > +    }
> > +    return;
> >  }
> >  
> 
> I think that you should move these functions into hmp.c file. This also
> applies to previous patch and according to this changes you don't have
> to export hmp_handle_error() function which should be used only in hmp.c

Exactly, I was going to say the same thing. Also note that you'll need
better names for do_info_snapshots_vm() and do_info_snapshots_blk() if
you make them public (or you could move them to hmp.c as well).

Re: [Qemu-devel] [PATCH V3 00/11] add qmp/hmp interfaces for snapshot info

[Luiz Capitulino]

On Mon, 14 Jan 2013 15:09:36 +0800
Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:

>   This serial of patches does two things: merge some info code
> in qemu-img, and add following interfaces:
> 1) qmp: query-images
> 2) qmp: query-snapshots
> 3) hmp: show snapshot info on a single block device
>   These patches follows the rule that use qmp to retieve information,
> hmp layer just do a translation from qmp object it got, so almost
> every hmp interface may have a correlated qmp interface.

I've reviewed this and made some comments, and just to set expectations,
I think this should go in through the block queue.

Re: [Qemu-devel] [PATCH V3 07/11] block: export function bdrv_find_snapshot()

[Eric Blake]

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

On 01/14/2013 12:09 AM, Wenchao Xia wrote:
>   This patch move it from savevm.c to block.c and export it.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  block.c               |   23 +++++++++++++++++++++++
>  include/block/block.h |    2 ++
>  savevm.c              |   22 ----------------------
>  3 files changed, 25 insertions(+), 22 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 8192d8e..b7d2f03 100644
> --- a/block.c
> +++ b/block.c
> @@ -3351,6 +3351,29 @@ int bdrv_snapshot_load_tmp(BlockDriverState *bs,
>      return -ENOTSUP;
>  }
>  
> +int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
> +                       const char *name)
> +{
> +    QEMUSnapshotInfo *sn_tab, *sn;
> +    int nb_sns, i, ret;
> +
> +    ret = -ENOENT;
> +    nb_sns = bdrv_snapshot_list(bs, &sn_tab);
> +    if (nb_sns < 0) {
> +        return ret;
> +    }
> +    for (i = 0; i < nb_sns; i++) {
> +        sn = &sn_tab[i];
> +        if (!strcmp(sn->id_str, name) || !strcmp(sn->name, name)) {

It is possible (albeit probably stupid) to create a qcow2 file where
snapshot names are merely numeric strings.  In fact, just to see what
would happen, I once[1] created a file where:

snapshot id '1' was named '2'
snapshot id '2' was named 'foo'

This code comparison favors ids over names; so if I request to delvm 2,
I end up removing the second snapshot, not the first.  This is okay, but
probably worth documenting, and probably worth making sure that all code
that looks up a snapshot by name or id goes through this function so
that we get the same behavior everywhere.  My experiment was done
several months ago, but my recollection was that at the time, there was
an inconsistency where 'qemu-img snapshot' picked a different snapshot
for the request of '2' than the online 'delvm' monitor command of qemu;
making it unsafe to rely on either behavior in that version of qemu
source code.

[1]https://bugzilla.redhat.com/show_bug.cgi?id=733143

-- 
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: 621 bytes --]

Re: [Qemu-devel] [PATCH V3 11/11] hmp: show snapshot on single block device

[Wenchao Xia]

 > On Mon, 2013-01-14 at 15:09 +0800, Wenchao Xia wrote:
>>    This patch use block layer API to qmp snapshot info on a block
>> device, then use the same code dumping vm snapshot info, to print
>> in monitor.
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>> Note:
>>    This patch need previous hmp extention patch which enable
>> info sub command take qdict * as paramter.
>
>> diff --git a/savevm.c b/savevm.c
>> index cabdcb6..cd474e9 100644
>> --- a/savevm.c
>> +++ b/savevm.c
>> @@ -2354,9 +2354,50 @@ static void do_info_snapshots_vm(Monitor *mon)
>>       return;
>>   }
>>
>> +static void do_info_snapshots_blk(Monitor *mon, const char *device)
>> +{
>> +    Error *err = NULL;
>> +    SnapshotInfoList *list;
>> +    BlockDriverState *bs;
>> +
>> +    /* find the target bs */
>> +    bs = bdrv_find(device);
>> +    if (!bs) {
>> +        monitor_printf(mon, "Device '%s' not found.\n", device);
>> +        return ;
>> +    }
>> +
>> +    if (!bdrv_can_snapshot(bs)) {
>> +        monitor_printf(mon, "Device '%s' can't have snapshot.\n", device);
>> +        return ;
>> +    }
>> +
>> +    list = bdrv_query_snapshot_infolist(bs, NULL, NULL, &err);
>> +    if (error_is_set(&err)) {
>> +        hmp_handle_error(mon, &err);
>> +        return;
>> +    }
>> +
>> +    if (list == NULL) {
>> +        monitor_printf(mon, "There is no snapshot available.\n");
>> +        return;
>> +    }
>> +
>> +    monitor_printf(mon, "Device '%s':\n", device);
>> +    monitor_dump_snapshotinfolist(mon, list);
>> +    qapi_free_SnapshotInfoList(list);
>> +    return;
>> +}
>> +
>>   void do_info_snapshots(Monitor *mon, const QDict *qdict)
>>   {
>> -    do_info_snapshots_vm(mon);
>> +    const char *device = qdict_get_try_str(qdict, "device");
>> +    if (!device) {
>> +        do_info_snapshots_vm(mon);
>> +    } else {
>> +        do_info_snapshots_blk(mon, device);
>> +    }
>> +    return;
>>   }
>>
>
> I think that you should move these functions into hmp.c file. This also
> applies to previous patch and according to this changes you don't have
> to export hmp_handle_error() function which should be used only in hmp.c
>
> Pavel
>
   It seems there are other "do_info_**" function in other .c files,
so I suggest a different serial later which move those functions, if
necessary.

-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH V3 04/11] qemu-img: switch image retrieving function

[Wenchao Xia]

于 2013-1-14 19:25, Pavel Hrdina 写道:
> On Mon, 2013-01-14 at 15:09 +0800, Wenchao Xia wrote:
>>    Now qemu-img call block layer function to get image info.
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> ---
>>   qemu-img.c |   86 +----------------------------------------------------------
>>   1 files changed, 2 insertions(+), 84 deletions(-)
>>
>> diff --git a/qemu-img.c b/qemu-img.c
>> index 9dab48f..e20551a 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -1338,6 +1257,7 @@ static ImageInfoList *collect_image_info_list(const char *filename,
>>       ImageInfoList *head = NULL;
>>       ImageInfoList **last = &head;
>>       GHashTable *filenames;
>> +    Error *err = NULL;
>>
>>       filenames = g_hash_table_new_full(g_str_hash, str_equal_func, NULL, NULL);
>>
>> @@ -1359,9 +1279,7 @@ static ImageInfoList *collect_image_info_list(const char *filename,
>>               goto err;
>>           }
>>
>> -        info = g_new0(ImageInfo, 1);
>> -        collect_image_info(bs, info, filename);
>> -        collect_snapshots(bs, info);
>> +        info = bdrv_query_image_info(bs, &err);
>
> You are not using the 'err' variable so you should pass 'NULL' instead.
>
> 	info = bdrv_query_image_info(bs, NULL);
>
OK.

>>
>>           elem = g_new0(ImageInfoList, 1);
>>           elem->value = info;
>
>


-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH V3 01/11] qemu-img: remove unused parameter in collect_image_info()

[Wenchao Xia]

于 2013-1-15 1:08, Luiz Capitulino 写道:
> On Mon, 14 Jan 2013 15:09:37 +0800
> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>
>>    Parameter *fmt was not used, so remove it.
>>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>>   qemu-img.c |    5 ++---
>>   1 files changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/qemu-img.c b/qemu-img.c
>> index 85d3740..9dab48f 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -1186,8 +1186,7 @@ static void dump_json_image_info(ImageInfo *info)
>>
>>   static void collect_image_info(BlockDriverState *bs,
>>                      ImageInfo *info,
>> -                   const char *filename,
>> -                   const char *fmt)
>> +                   const char *filename)
>
> collect_image_info_list() doc reads:
>
>   @fmt: topmost image format (may be NULL to autodetect)
>
> However, right now only fmt=NULL is supported, as collect_image_info()
> ignores fmt altogether.
>
> So, if this patch is correct we better update the comment. Otherwise,
> we should improve collect_image_info() to actually obey fmt != NULL.
>
   @fmt was ignored in the function and I can't see a reason to have
it while *bs contains the info, will change the comments.

>>   {
>>       uint64_t total_sectors;
>>       char backing_filename[1024];
>> @@ -1361,7 +1360,7 @@ static ImageInfoList *collect_image_info_list(const char *filename,
>>           }
>>
>>           info = g_new0(ImageInfo, 1);
>> -        collect_image_info(bs, info, filename, fmt);
>> +        collect_image_info(bs, info, filename);
>>           collect_snapshots(bs, info);
>>
>>           elem = g_new0(ImageInfoList, 1);
>


-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH V3 02/11] block: add bdrv_get_filename() function

[Wenchao Xia]

于 2013-1-15 1:08, Luiz Capitulino 写道:
> On Mon, 14 Jan 2013 15:09:38 +0800
> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>
>>    This function will simply return the uri or filename used
>> to open the image.
>>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>>   block.c               |    5 +++++
>>   include/block/block.h |    1 +
>>   2 files changed, 6 insertions(+), 0 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 4e28c55..5f95da5 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -2967,6 +2967,11 @@ BlockStatsList *qmp_query_blockstats(Error **errp)
>>       return head;
>>   }
>>
>> +const char *bdrv_get_filename(BlockDriverState *bs)
>> +{
>
> bs can be const.
>
OK.


-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH V3 01/11] qemu-img: remove unused parameter in collect_image_info()

[Wenchao Xia]

于 2013-1-15 15:27, Wenchao Xia 写道:
> 于 2013-1-15 1:08, Luiz Capitulino 写道:
>> On Mon, 14 Jan 2013 15:09:37 +0800
>> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>>
>>>    Parameter *fmt was not used, so remove it.
>>>
>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>>> ---
>>>   qemu-img.c |    5 ++---
>>>   1 files changed, 2 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/qemu-img.c b/qemu-img.c
>>> index 85d3740..9dab48f 100644
>>> --- a/qemu-img.c
>>> +++ b/qemu-img.c
>>> @@ -1186,8 +1186,7 @@ static void dump_json_image_info(ImageInfo *info)
>>>
>>>   static void collect_image_info(BlockDriverState *bs,
>>>                      ImageInfo *info,
>>> -                   const char *filename,
>>> -                   const char *fmt)
>>> +                   const char *filename)
>>
>> collect_image_info_list() doc reads:
>>
>>   @fmt: topmost image format (may be NULL to autodetect)
>>
>> However, right now only fmt=NULL is supported, as collect_image_info()
>> ignores fmt altogether.
>>
>> So, if this patch is correct we better update the comment. Otherwise,
>> we should improve collect_image_info() to actually obey fmt != NULL.
>>
>    @fmt was ignored in the function and I can't see a reason to have
> it while *bs contains the info, will change the comments.
>
   Hi, *fmt was used only in collect_image_info_list() when it tries to
open the image, and it is not useful any more in collect_image_info,
so nothing need change in comments.

>>>   {
>>>       uint64_t total_sectors;
>>>       char backing_filename[1024];
>>> @@ -1361,7 +1360,7 @@ static ImageInfoList
>>> *collect_image_info_list(const char *filename,
>>>           }
>>>
>>>           info = g_new0(ImageInfo, 1);
>>> -        collect_image_info(bs, info, filename, fmt);
>>> +        collect_image_info(bs, info, filename);
>>>           collect_snapshots(bs, info);
>>>
>>>           elem = g_new0(ImageInfoList, 1);
>>
>
>


-- 
Best Regards

Wenchao Xia

[Qemu-devel] IS_USER(s) in target-arm

[Shijesta Victor]

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

I am trying to use QEMU to print the virtual and physical addresses of all memory accesses for target-arm. I can see the addresses being computed, but they pass IS_USER(s) as an index to routines like gen_ld8s etc, Can you please explain what this does?
Any help is appreciated; thanks in advance!

regards,

shijesta

--- On Tue, 15/1/13, Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:

From: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
Subject: Re: [Qemu-devel] [PATCH V3 02/11] block: add bdrv_get_filename() function
To: "Luiz Capitulino" <lcapitulino@redhat.com>
Cc: aliguori@us.ibm.com, phrdina@redhat.com, stefanha@gmail.com, qemu-devel@nongnu.org, armbru@redhat.com, pbonzini@redhat.com
Date: Tuesday, 15 January, 2013, 13:00

于 2013-1-15 1:08, Luiz Capitulino 写道:
> On Mon, 14 Jan 2013 15:09:38 +0800
> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>
>>    This function will simply return the uri or filename used
>> to open the image.
>>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>>   block.c               |    5 +++++
>>   include/block/block.h |    1 +
>>   2 files changed, 6 insertions(+), 0 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 4e28c55..5f95da5 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -2967,6 +2967,11 @@ BlockStatsList *qmp_query_blockstats(Error **errp)
>>       return head;
>>   }
>>
>> +const char *bdrv_get_filename(BlockDriverState *bs)
>> +{
>
> bs can be const.
>
OK.


-- 
Best Regards

Wenchao Xia



[-- Attachment #2: Type: text/html, Size: 2594 bytes --]

Re: [Qemu-devel] [PATCH V3 07/11] block: export function bdrv_find_snapshot()

[Wenchao Xia]

于 2013-1-15 7:39, Eric Blake 写道:
> On 01/14/2013 12:09 AM, Wenchao Xia wrote:
>>    This patch move it from savevm.c to block.c and export it.
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>>   block.c               |   23 +++++++++++++++++++++++
>>   include/block/block.h |    2 ++
>>   savevm.c              |   22 ----------------------
>>   3 files changed, 25 insertions(+), 22 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 8192d8e..b7d2f03 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -3351,6 +3351,29 @@ int bdrv_snapshot_load_tmp(BlockDriverState *bs,
>>       return -ENOTSUP;
>>   }
>>
>> +int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
>> +                       const char *name)
>> +{
>> +    QEMUSnapshotInfo *sn_tab, *sn;
>> +    int nb_sns, i, ret;
>> +
>> +    ret = -ENOENT;
>> +    nb_sns = bdrv_snapshot_list(bs, &sn_tab);
>> +    if (nb_sns < 0) {
>> +        return ret;
>> +    }
>> +    for (i = 0; i < nb_sns; i++) {
>> +        sn = &sn_tab[i];
>> +        if (!strcmp(sn->id_str, name) || !strcmp(sn->name, name)) {
>
> It is possible (albeit probably stupid) to create a qcow2 file where
> snapshot names are merely numeric strings.  In fact, just to see what
> would happen, I once[1] created a file where:
>
> snapshot id '1' was named '2'
> snapshot id '2' was named 'foo'
>
> This code comparison favors ids over names; so if I request to delvm 2,
> I end up removing the second snapshot, not the first.  This is okay, but
> probably worth documenting, and probably worth making sure that all code
> that looks up a snapshot by name or id goes through this function so
> that we get the same behavior everywhere.  My experiment was done
> several months ago, but my recollection was that at the time, there was
> an inconsistency where 'qemu-img snapshot' picked a different snapshot
> for the request of '2' than the online 'delvm' monitor command of qemu;
> making it unsafe to rely on either behavior in that version of qemu
> source code.
>
> [1]https://bugzilla.redhat.com/show_bug.cgi?id=733143
>

how about:
/*  if id is not NULL, try find it with id, if not exist, return NULL
  *  if id is NULL and name is not NULL, try find it with name.
  */ if id and name is NULL, direct return fail.
int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
                        const char *id, const char *name)

-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH V3 06/11] qmp: add interface query-images.

[Wenchao Xia]

>
> When the device doesn't have an image, this returns:
>
>          {
>              "device": "ide1-cd0",
>              "info": {
>                  "virtual-size": 0,
>                  "filename": "",
>                  "format": ""
>              }
>          },
>
> But it would be better to return an empty dict or even omit 'info'
> completely. One more comment below.
>

   Will make info as optional, thanks.
>> +
>> +    return head;
>> +}
>> +
>>   BlockInfo *bdrv_query_block_info(BlockDriverState *bs)
>>   {
>>       BlockInfo *info = g_malloc0(sizeof(*info));
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index 5dfa052..565737c 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -245,6 +245,22 @@
>>              '*backing-filename-format': 'str', '*snapshots': ['SnapshotInfo'] } }
>>
>>   ##
>> +# @DeviceImageInfo:
>> +#
>> +# Information about an image used by QEMU block device
>> +#
>> +# @device: name of the block device
>> +#
>> +# @info: info of the image used.
>
> Info is too generic, please call it 'image'.
>

  OK.

Re: [Qemu-devel] [PATCH V3 11/11] hmp: show snapshot on single block device

[Luiz Capitulino]

On Tue, 15 Jan 2013 10:36:22 +0800
Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:

>  > On Mon, 2013-01-14 at 15:09 +0800, Wenchao Xia wrote:
> >>    This patch use block layer API to qmp snapshot info on a block
> >> device, then use the same code dumping vm snapshot info, to print
> >> in monitor.
> >>
> >> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> >> ---
> >> Note:
> >>    This patch need previous hmp extention patch which enable
> >> info sub command take qdict * as paramter.
> >
> >> diff --git a/savevm.c b/savevm.c
> >> index cabdcb6..cd474e9 100644
> >> --- a/savevm.c
> >> +++ b/savevm.c
> >> @@ -2354,9 +2354,50 @@ static void do_info_snapshots_vm(Monitor *mon)
> >>       return;
> >>   }
> >>
> >> +static void do_info_snapshots_blk(Monitor *mon, const char *device)
> >> +{
> >> +    Error *err = NULL;
> >> +    SnapshotInfoList *list;
> >> +    BlockDriverState *bs;
> >> +
> >> +    /* find the target bs */
> >> +    bs = bdrv_find(device);
> >> +    if (!bs) {
> >> +        monitor_printf(mon, "Device '%s' not found.\n", device);
> >> +        return ;
> >> +    }
> >> +
> >> +    if (!bdrv_can_snapshot(bs)) {
> >> +        monitor_printf(mon, "Device '%s' can't have snapshot.\n", device);
> >> +        return ;
> >> +    }
> >> +
> >> +    list = bdrv_query_snapshot_infolist(bs, NULL, NULL, &err);
> >> +    if (error_is_set(&err)) {
> >> +        hmp_handle_error(mon, &err);
> >> +        return;
> >> +    }
> >> +
> >> +    if (list == NULL) {
> >> +        monitor_printf(mon, "There is no snapshot available.\n");
> >> +        return;
> >> +    }
> >> +
> >> +    monitor_printf(mon, "Device '%s':\n", device);
> >> +    monitor_dump_snapshotinfolist(mon, list);
> >> +    qapi_free_SnapshotInfoList(list);
> >> +    return;
> >> +}
> >> +
> >>   void do_info_snapshots(Monitor *mon, const QDict *qdict)
> >>   {
> >> -    do_info_snapshots_vm(mon);
> >> +    const char *device = qdict_get_try_str(qdict, "device");
> >> +    if (!device) {
> >> +        do_info_snapshots_vm(mon);
> >> +    } else {
> >> +        do_info_snapshots_blk(mon, device);
> >> +    }
> >> +    return;
> >>   }
> >>
> >
> > I think that you should move these functions into hmp.c file. This also
> > applies to previous patch and according to this changes you don't have
> > to export hmp_handle_error() function which should be used only in hmp.c
> >
> > Pavel
> >
>    It seems there are other "do_info_**" function in other .c files,
> so I suggest a different serial later which move those functions, if
> necessary.

The other functions are probably old hmp code. That is, code that is not
converted to make QMP calls. Generally, new hmp code goes into hmp.c unless
there's a good reason to put them in a different file.

So, please move it to hmp.c.

Re: [Qemu-devel] [PATCH V3 01/11] qemu-img: remove unused parameter in collect_image_info()

[Luiz Capitulino]

On Tue, 15 Jan 2013 15:58:34 +0800
Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:

> 于 2013-1-15 15:27, Wenchao Xia 写道:
> > 于 2013-1-15 1:08, Luiz Capitulino 写道:
> >> On Mon, 14 Jan 2013 15:09:37 +0800
> >> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
> >>
> >>>    Parameter *fmt was not used, so remove it.
> >>>
> >>> Reviewed-by: Eric Blake <eblake@redhat.com>
> >>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> >>> ---
> >>>   qemu-img.c |    5 ++---
> >>>   1 files changed, 2 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/qemu-img.c b/qemu-img.c
> >>> index 85d3740..9dab48f 100644
> >>> --- a/qemu-img.c
> >>> +++ b/qemu-img.c
> >>> @@ -1186,8 +1186,7 @@ static void dump_json_image_info(ImageInfo *info)
> >>>
> >>>   static void collect_image_info(BlockDriverState *bs,
> >>>                      ImageInfo *info,
> >>> -                   const char *filename,
> >>> -                   const char *fmt)
> >>> +                   const char *filename)
> >>
> >> collect_image_info_list() doc reads:
> >>
> >>   @fmt: topmost image format (may be NULL to autodetect)
> >>
> >> However, right now only fmt=NULL is supported, as collect_image_info()
> >> ignores fmt altogether.
> >>
> >> So, if this patch is correct we better update the comment. Otherwise,
> >> we should improve collect_image_info() to actually obey fmt != NULL.
> >>
> >    @fmt was ignored in the function and I can't see a reason to have
> > it while *bs contains the info, will change the comments.
> >
>    Hi, *fmt was used only in collect_image_info_list() when it tries to
> open the image, and it is not useful any more in collect_image_info,
> so nothing need change in comments.

This really doesn't answer my comment above. The code comment implies that
fmt may be NULL or non-NULL and they have different behavior.

If you choose to touch fmt (as this patch does) then please, do the
right thing. Otherwise it's better to let it untouched.

Re: [Qemu-devel] [PATCH V3 07/11] block: export function bdrv_find_snapshot()

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 01/14/2013 12:09 AM, Wenchao Xia wrote:
>>   This patch move it from savevm.c to block.c and export it.
>> 
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>>  block.c               |   23 +++++++++++++++++++++++
>>  include/block/block.h |    2 ++
>>  savevm.c              |   22 ----------------------
>>  3 files changed, 25 insertions(+), 22 deletions(-)
>> 
>> diff --git a/block.c b/block.c
>> index 8192d8e..b7d2f03 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -3351,6 +3351,29 @@ int bdrv_snapshot_load_tmp(BlockDriverState *bs,
>>      return -ENOTSUP;
>>  }
>>  
>> +int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
>> +                       const char *name)
>> +{
>> +    QEMUSnapshotInfo *sn_tab, *sn;
>> +    int nb_sns, i, ret;
>> +
>> +    ret = -ENOENT;
>> +    nb_sns = bdrv_snapshot_list(bs, &sn_tab);
>> +    if (nb_sns < 0) {
>> +        return ret;
>> +    }
>> +    for (i = 0; i < nb_sns; i++) {
>> +        sn = &sn_tab[i];
>> +        if (!strcmp(sn->id_str, name) || !strcmp(sn->name, name)) {
>
> It is possible (albeit probably stupid) to create a qcow2 file where
> snapshot names are merely numeric strings.  In fact, just to see what
> would happen, I once[1] created a file where:
>
> snapshot id '1' was named '2'
> snapshot id '2' was named 'foo'
>
> This code comparison favors ids over names; so if I request to delvm 2,
> I end up removing the second snapshot, not the first.  This is okay, but
> probably worth documenting, and probably worth making sure that all code
> that looks up a snapshot by name or id goes through this function so
> that we get the same behavior everywhere.  My experiment was done
> several months ago, but my recollection was that at the time, there was
> an inconsistency where 'qemu-img snapshot' picked a different snapshot
> for the request of '2' than the online 'delvm' monitor command of qemu;
> making it unsafe to rely on either behavior in that version of qemu
> source code.
>
> [1]https://bugzilla.redhat.com/show_bug.cgi?id=733143

In QemuOpts, we restricted IDs to [[:alpha:]][[:alnum:]-._]*, see
id_wellformed().  I'd recommend the same for new interfaces.  But this
is an old one, and we shouldn't make existing snapshots inaccessible
just because their names were chosen unwisely.

Re: [Qemu-devel] [PATCH V3 07/11] block: export function bdrv_find_snapshot()

[Pavel Hrdina]

On 01/15/2013 01:01 PM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
>
>> On 01/14/2013 12:09 AM, Wenchao Xia wrote:
>>>    This patch move it from savevm.c to block.c and export it.
>>>
>>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>>> ---
>>>   block.c               |   23 +++++++++++++++++++++++
>>>   include/block/block.h |    2 ++
>>>   savevm.c              |   22 ----------------------
>>>   3 files changed, 25 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/block.c b/block.c
>>> index 8192d8e..b7d2f03 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -3351,6 +3351,29 @@ int bdrv_snapshot_load_tmp(BlockDriverState *bs,
>>>       return -ENOTSUP;
>>>   }
>>>
>>> +int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
>>> +                       const char *name)
>>> +{
>>> +    QEMUSnapshotInfo *sn_tab, *sn;
>>> +    int nb_sns, i, ret;
>>> +
>>> +    ret = -ENOENT;
>>> +    nb_sns = bdrv_snapshot_list(bs, &sn_tab);
>>> +    if (nb_sns < 0) {
>>> +        return ret;
>>> +    }
>>> +    for (i = 0; i < nb_sns; i++) {
>>> +        sn = &sn_tab[i];
>>> +        if (!strcmp(sn->id_str, name) || !strcmp(sn->name, name)) {
>>
>> It is possible (albeit probably stupid) to create a qcow2 file where
>> snapshot names are merely numeric strings.  In fact, just to see what
>> would happen, I once[1] created a file where:
>>
>> snapshot id '1' was named '2'
>> snapshot id '2' was named 'foo'
>>
>> This code comparison favors ids over names; so if I request to delvm 2,
>> I end up removing the second snapshot, not the first.  This is okay, but
>> probably worth documenting, and probably worth making sure that all code
>> that looks up a snapshot by name or id goes through this function so
>> that we get the same behavior everywhere.  My experiment was done
>> several months ago, but my recollection was that at the time, there was
>> an inconsistency where 'qemu-img snapshot' picked a different snapshot
>> for the request of '2' than the online 'delvm' monitor command of qemu;
>> making it unsafe to rely on either behavior in that version of qemu
>> source code.
>>
>> [1]https://bugzilla.redhat.com/show_bug.cgi?id=733143
>
> In QemuOpts, we restricted IDs to [[:alpha:]][[:alnum:]-._]*, see
> id_wellformed().  I'd recommend the same for new interfaces.  But this
> is an old one, and we shouldn't make existing snapshots inaccessible
> just because their names were chosen unwisely.
>

There is my proposal for handling snapshots id and name.

http://lists.gnu.org/archive/html/qemu-devel/2013-01/msg01551.html

In general, we will have two options for operation with snapshots, an 
'id' option and a 'name' option. You could choose one of them or both to 
specify which snapshot you want to load/delete/create/modify.

This behaviour should be the same for online and offline snapshots even 
for vm-snapshots or block-snapshots.

With these modifications old snapshots should also works.

Pavel

Re: [Qemu-devel] [PATCH V3 07/11] block: export function bdrv_find_snapshot()

[Eric Blake]

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

On 01/15/2013 03:24 AM, Wenchao Xia wrote:

>>>
>>> +int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
>>> +                       const char *name)

>>> +        if (!strcmp(sn->id_str, name) || !strcmp(sn->name, name)) {
>>

>>
>> This code comparison favors ids over names; so if I request to delvm 2,
>> I end up removing the second snapshot, not the first.  This is okay, but
>> probably worth documenting,

> how about:
> /*  if id is not NULL, try find it with id, if not exist, return NULL
>  *  if id is NULL and name is not NULL, try find it with name.
>  */ if id and name is NULL, direct return fail.
> int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
>                        const char *id, const char *name)

That would be pushing the burden onto the callers to decide whether they
are doing an id lookup, a name lookup, or both.  In QMP terms, that
means that your QMP command to delete a snapshot would now need an
additional optional argument to decide whether the associated name is
only an id, only a name, or can match either.  But I'm not sure you want
that.

What I was trying to get at is that given a single string "2", it does
seem nicer to do both an id and a name lookup, and return the first hit;
you just need to document that ids take preference over names (and thus,
naming a snapshot "2" may make the snapshot become invisible by name,
but not by id, if a later snapshot creation causes id 2 to be used).
Then your QMP command for deleting a snapshot no longer needs to care
whether "2" is an id or a name, just whether it matches.

Hmm, while typing this, I thought of another snag.  Suppose you have a
VM with two disks, but where only the first disk previously had a
snapshot with id 1.  If I create a new snapshot across both disks, does
that mean disk 1 gets id 2 while disk 2 gets id 1, or do both disks get
id 2, even though that means disk 2 skips over id 1?  As long as the
snapshot is named, you can refer to the name to get the same snapshot
across both disks, regardless of what id it has.  But if the name is
numeric, and id takes preference over name when doing a lookup, we could
get ourselves into the situation where a snapshot created with name "2"
can eventually never be restored in one piece, because the individual
disks have different ids for the same snapshot name.  So maybe we DO
need a way after all for QMP to specify whether a name lookup is for id,
name, or both.

-- 
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: 621 bytes --]

Re: [Qemu-devel] [PATCH V3 11/11] hmp: show snapshot on single block device

[Wenchao Xia]

于 2013-1-15 19:05, Luiz Capitulino 写道:
> On Tue, 15 Jan 2013 10:36:22 +0800
> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>
>>   > On Mon, 2013-01-14 at 15:09 +0800, Wenchao Xia wrote:
>>>>     This patch use block layer API to qmp snapshot info on a block
>>>> device, then use the same code dumping vm snapshot info, to print
>>>> in monitor.
>>>>
>>>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>>>> ---
>>>> Note:
>>>>     This patch need previous hmp extention patch which enable
>>>> info sub command take qdict * as paramter.
>>>
>>>> diff --git a/savevm.c b/savevm.c
>>>> index cabdcb6..cd474e9 100644
>>>> --- a/savevm.c
>>>> +++ b/savevm.c
>>>> @@ -2354,9 +2354,50 @@ static void do_info_snapshots_vm(Monitor *mon)
>>>>        return;
>>>>    }
>>>>
>>>> +static void do_info_snapshots_blk(Monitor *mon, const char *device)
>>>> +{
>>>> +    Error *err = NULL;
>>>> +    SnapshotInfoList *list;
>>>> +    BlockDriverState *bs;
>>>> +
>>>> +    /* find the target bs */
>>>> +    bs = bdrv_find(device);
>>>> +    if (!bs) {
>>>> +        monitor_printf(mon, "Device '%s' not found.\n", device);
>>>> +        return ;
>>>> +    }
>>>> +
>>>> +    if (!bdrv_can_snapshot(bs)) {
>>>> +        monitor_printf(mon, "Device '%s' can't have snapshot.\n", device);
>>>> +        return ;
>>>> +    }
>>>> +
>>>> +    list = bdrv_query_snapshot_infolist(bs, NULL, NULL, &err);
>>>> +    if (error_is_set(&err)) {
>>>> +        hmp_handle_error(mon, &err);
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    if (list == NULL) {
>>>> +        monitor_printf(mon, "There is no snapshot available.\n");
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    monitor_printf(mon, "Device '%s':\n", device);
>>>> +    monitor_dump_snapshotinfolist(mon, list);
>>>> +    qapi_free_SnapshotInfoList(list);
>>>> +    return;
>>>> +}
>>>> +
>>>>    void do_info_snapshots(Monitor *mon, const QDict *qdict)
>>>>    {
>>>> -    do_info_snapshots_vm(mon);
>>>> +    const char *device = qdict_get_try_str(qdict, "device");
>>>> +    if (!device) {
>>>> +        do_info_snapshots_vm(mon);
>>>> +    } else {
>>>> +        do_info_snapshots_blk(mon, device);
>>>> +    }
>>>> +    return;
>>>>    }
>>>>
>>>
>>> I think that you should move these functions into hmp.c file. This also
>>> applies to previous patch and according to this changes you don't have
>>> to export hmp_handle_error() function which should be used only in hmp.c
>>>
>>> Pavel
>>>
>>     It seems there are other "do_info_**" function in other .c files,
>> so I suggest a different serial later which move those functions, if
>> necessary.
>
> The other functions are probably old hmp code. That is, code that is not
> converted to make QMP calls. Generally, new hmp code goes into hmp.c unless
> there's a good reason to put them in a different file.
>
> So, please move it to hmp.c.
>

OK, new hmp function will goto hmp.c

-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH V3 01/11] qemu-img: remove unused parameter in collect_image_info()

[Wenchao Xia]

于 2013-1-15 19:11, Luiz Capitulino 写道:
> On Tue, 15 Jan 2013 15:58:34 +0800
> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>
>> 于 2013-1-15 15:27, Wenchao Xia 写道:
>>> 于 2013-1-15 1:08, Luiz Capitulino 写道:
>>>> On Mon, 14 Jan 2013 15:09:37 +0800
>>>> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>>>>
>>>>>     Parameter *fmt was not used, so remove it.
>>>>>
>>>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>>>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>>>>> ---
>>>>>    qemu-img.c |    5 ++---
>>>>>    1 files changed, 2 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/qemu-img.c b/qemu-img.c
>>>>> index 85d3740..9dab48f 100644
>>>>> --- a/qemu-img.c
>>>>> +++ b/qemu-img.c
>>>>> @@ -1186,8 +1186,7 @@ static void dump_json_image_info(ImageInfo *info)
>>>>>
>>>>>    static void collect_image_info(BlockDriverState *bs,
>>>>>                       ImageInfo *info,
>>>>> -                   const char *filename,
>>>>> -                   const char *fmt)
>>>>> +                   const char *filename)
>>>>
>>>> collect_image_info_list() doc reads:
>>>>
>>>>    @fmt: topmost image format (may be NULL to autodetect)
>>>>
>>>> However, right now only fmt=NULL is supported, as collect_image_info()
>>>> ignores fmt altogether.
>>>>
>>>> So, if this patch is correct we better update the comment. Otherwise,
>>>> we should improve collect_image_info() to actually obey fmt != NULL.
>>>>
>>>     @fmt was ignored in the function and I can't see a reason to have
>>> it while *bs contains the info, will change the comments.
>>>
>>     Hi, *fmt was used only in collect_image_info_list() when it tries to
>> open the image, and it is not useful any more in collect_image_info,
>> so nothing need change in comments.
>
> This really doesn't answer my comment above. The code comment implies that
> fmt may be NULL or non-NULL and they have different behavior.
>
> If you choose to touch fmt (as this patch does) then please, do the
> right thing. Otherwise it's better to let it untouched.
>
   I think the "fmt may be NULL or non-NULL" indeed have different
behavior for that later following is called:
bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS | BDRV_O_NO_BACKING,
                            false);
but it is not related to collect_image_info(), it is more like a
slip in coding having add *fmt in above funtion. :)
-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH V3 07/11] block: export function bdrv_find_snapshot()

[Wenchao Xia]

于 2013-1-16 1:57, Eric Blake 写道:
> On 01/15/2013 03:24 AM, Wenchao Xia wrote:
>
>>>>
>>>> +int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
>>>> +                       const char *name)
>
>>>> +        if (!strcmp(sn->id_str, name) || !strcmp(sn->name, name)) {
>>>
>
>>>
>>> This code comparison favors ids over names; so if I request to delvm 2,
>>> I end up removing the second snapshot, not the first.  This is okay, but
>>> probably worth documenting,
>
>> how about:
>> /*  if id is not NULL, try find it with id, if not exist, return NULL
>>   *  if id is NULL and name is not NULL, try find it with name.
>>   */ if id and name is NULL, direct return fail.
>> int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
>>                         const char *id, const char *name)
>
> That would be pushing the burden onto the callers to decide whether they
> are doing an id lookup, a name lookup, or both.  In QMP terms, that
> means that your QMP command to delete a snapshot would now need an
> additional optional argument to decide whether the associated name is
> only an id, only a name, or can match either.  But I'm not sure you want
> that.
>
> What I was trying to get at is that given a single string "2", it does
> seem nicer to do both an id and a name lookup, and return the first hit;
> you just need to document that ids take preference over names (and thus,
> naming a snapshot "2" may make the snapshot become invisible by name,
> but not by id, if a later snapshot creation causes id 2 to be used).
> Then your QMP command for deleting a snapshot no longer needs to care
> whether "2" is an id or a name, just whether it matches.
>
   OK, I guess following is better and clear:

  /* Try to find the snapshot with @id and @name, @id have higher
   * priority in searching. Both @id and @name can be NULL.
   */
int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
                         const char *id, const char *name)

Then caller at hmp/qmp layer can decide how to use it. For info of
snapshot retrieving in this serial, things are simple. for create/delete
of snapshot in future, I think a check with id_wellformed() and an
interface like the proposal from Pavel is needed.
http://lists.gnu.org/archive/html/qemu-devel/2013-01/msg01551.html

> Hmm, while typing this, I thought of another snag.  Suppose you have a
> VM with two disks, but where only the first disk previously had a
> snapshot with id 1.  If I create a new snapshot across both disks, does
> that mean disk 1 gets id 2 while disk 2 gets id 1, or do both disks get
> id 2, even though that means disk 2 skips over id 1?  As long as the
> snapshot is named, you can refer to the name to get the same snapshot
> across both disks, regardless of what id it has.  But if the name is
> numeric, and id takes preference over name when doing a lookup, we could
> get ourselves into the situation where a snapshot created with name "2"
> can eventually never be restored in one piece, because the individual
> disks have different ids for the same snapshot name.  So maybe we DO
> need a way after all for QMP to specify whether a name lookup is for id,
> name, or both.
>


-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH V3 01/11] qemu-img: remove unused parameter in collect_image_info()

[Luiz Capitulino]

On Wed, 16 Jan 2013 11:10:14 +0800
Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:

> 于 2013-1-15 19:11, Luiz Capitulino 写道:
> > On Tue, 15 Jan 2013 15:58:34 +0800
> > Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
> >
> >> 于 2013-1-15 15:27, Wenchao Xia 写道:
> >>> 于 2013-1-15 1:08, Luiz Capitulino 写道:
> >>>> On Mon, 14 Jan 2013 15:09:37 +0800
> >>>> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
> >>>>
> >>>>>     Parameter *fmt was not used, so remove it.
> >>>>>
> >>>>> Reviewed-by: Eric Blake <eblake@redhat.com>
> >>>>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> >>>>> ---
> >>>>>    qemu-img.c |    5 ++---
> >>>>>    1 files changed, 2 insertions(+), 3 deletions(-)
> >>>>>
> >>>>> diff --git a/qemu-img.c b/qemu-img.c
> >>>>> index 85d3740..9dab48f 100644
> >>>>> --- a/qemu-img.c
> >>>>> +++ b/qemu-img.c
> >>>>> @@ -1186,8 +1186,7 @@ static void dump_json_image_info(ImageInfo *info)
> >>>>>
> >>>>>    static void collect_image_info(BlockDriverState *bs,
> >>>>>                       ImageInfo *info,
> >>>>> -                   const char *filename,
> >>>>> -                   const char *fmt)
> >>>>> +                   const char *filename)
> >>>>
> >>>> collect_image_info_list() doc reads:
> >>>>
> >>>>    @fmt: topmost image format (may be NULL to autodetect)
> >>>>
> >>>> However, right now only fmt=NULL is supported, as collect_image_info()
> >>>> ignores fmt altogether.
> >>>>
> >>>> So, if this patch is correct we better update the comment. Otherwise,
> >>>> we should improve collect_image_info() to actually obey fmt != NULL.
> >>>>
> >>>     @fmt was ignored in the function and I can't see a reason to have
> >>> it while *bs contains the info, will change the comments.
> >>>
> >>     Hi, *fmt was used only in collect_image_info_list() when it tries to
> >> open the image, and it is not useful any more in collect_image_info,
> >> so nothing need change in comments.
> >
> > This really doesn't answer my comment above. The code comment implies that
> > fmt may be NULL or non-NULL and they have different behavior.
> >
> > If you choose to touch fmt (as this patch does) then please, do the
> > right thing. Otherwise it's better to let it untouched.
> >
>    I think the "fmt may be NULL or non-NULL" indeed have different
> behavior for that later following is called:
> bs = bdrv_new_open(filename, fmt, BDRV_O_FLAGS | BDRV_O_NO_BACKING,
>                             false);
> but it is not related to collect_image_info(), it is more like a
> slip in coding having add *fmt in above funtion. :)

Oh, you seem to be right. Sorry for the noise.