[Qemu-devel] [PATCH V4 0/9] add internal snapshot support at block device level

[Qemu-devel] [PATCH V4 0/9] add internal snapshot support at block device level

[Wenchao Xia]

  This series brings internal snapshot support at block devices level, now we
have two three methods to do block snapshot lively: 1) backing chain,
2) internal one and 3) drive-back up approach.

Comparation:
             Advantages:                            Disadvantages:
1)    delta data, taken fast, export, size        performance, delete slow.
2)  taken fast, delete fast, performance, size       delta data, format
3)      performance, export, format               taken slow, delta data, size

  I think in most case, saving vmstate in an standalone file is better than
saving it inside qcow2, So suggest treat internal snapshot as block level
methods and not encourage user to savevm in qcow2 any more.

Implemention details:
  To avoid trouble, this serial have hide ID in create interfaces, this make
sure no chaos of ID and name will be introduced by these interfaces.
  There is one patch may be common to Pavel's savvm transaction, patch 1/11,
others are not quite related. Patch 1/11 will not set errp when no snapshot
find, since patch 3/11 need to distinguish real error case.

Next steps to better full VM snapshot:
  Improve internal snapshot's export capability.
  Better vmstate saving.

  Thanks Kevin to give advisement about how add it in qmp_transaction, oldest
version comes drom Dietmar Maurer.

v3:
  General:
  Rebased after Stenfan's driver-backup patch V6.

  Address Eric's comments:
  4/9: grammar fix and better doc.
  5/9: parameter name is mandatory now. grammar fix.
  6/9: redesiged interface: take both id and name as optional parameter, return
the deleted snapshot's info.

  Address Stefan's comments:
  4/9: add '' around %s in message. drop code comments about vm_clock.
  9/9: better doc, refined the code and add more test case.

v4:
  Address Stefan's comments:
  4/9: use error_setg_errno() to show error reason for bdrv_snapshot_create(),
spell fix and better doc.
  5/9: better doc.
  6/9: remove spurious ';' in code, spell fix and better doc.

Wenchao Xia (9):
  1 snapshot: new function bdrv_snapshot_find_by_id_and_name()
  2 snapshot: add paired functions for internal snapshot id and name
  3 snapshot: distinguish id and name in snapshot delete
  4 qmp: add internal snapshot support in qmp_transaction
  5 qmp: add interface blockdev-snapshot-internal-sync
  6 qmp: add interface blockdev-snapshot-delete-internal-sync
  7 hmp: add interface hmp_snapshot_blkdev_internal
  8 hmp: add interface hmp_snapshot_delete_blkdev_internal
  9 qemu-iotests: add 056 internal snapshot for block device test case

 block/qcow2-snapshot.c     |   68 +++++++++---
 block/qcow2.h              |    5 +-
 block/rbd.c                |   23 ++++-
 block/sheepdog.c           |    5 +-
 block/snapshot.c           |  130 +++++++++++++++++++++-
 blockdev.c                 |  194 ++++++++++++++++++++++++++++++++
 hmp-commands.hx            |   37 ++++++-
 hmp.c                      |   22 ++++
 hmp.h                      |    2 +
 include/block/block_int.h  |    5 +-
 include/block/snapshot.h   |   14 ++-
 include/qemu-common.h      |    3 +
 qapi-schema.json           |   67 +++++++++++-
 qemu-img.c                 |    5 +-
 qmp-commands.hx            |  105 ++++++++++++++++-
 savevm.c                   |   10 ++-
 tests/qemu-iotests/056     |  267 ++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/056.out |    5 +
 tests/qemu-iotests/group   |    1 +
 19 files changed, 930 insertions(+), 38 deletions(-)
 create mode 100755 tests/qemu-iotests/056
 create mode 100644 tests/qemu-iotests/056.out

[Qemu-devel] [PATCH V4 1/9] snapshot: new function bdrv_snapshot_find_by_id_and_name()

[Wenchao Xia]

To make it clear about id and name in searching, add this API
to distinguish them. Caller can choose to search by id or name,
*errp will be set only for exception.

Some code are modified based on Pavel's patch.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 block/snapshot.c         |   73 ++++++++++++++++++++++++++++++++++++++++++++++
 include/block/snapshot.h |    6 ++++
 2 files changed, 79 insertions(+), 0 deletions(-)

diff --git a/block/snapshot.c b/block/snapshot.c
index 6c6d9de..481a3ee 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -48,6 +48,79 @@ int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
     return ret;
 }
 
+/**
+ * Look up an internal snapshot by @id and @name.
+ * @bs: block device to search
+ * @id: unique snapshot ID, or NULL
+ * @name: snapshot name, or NULL
+ * @sn_info: location to store information on the snapshot found
+ * @errp: location to store error, will be set only for exception
+ *
+ * This function will traverse snapshot list in @bs to search the matching
+ * one, @id and @name are the matching condition:
+ * If both @id and @name are specified, find the first one with id @id and
+ * name @name.
+ * If only @id is specified, find the first one with id @id.
+ * If only @name is specified, find the first one with name @name.
+ * if none is specified, abort().
+ *
+ * Returns: true when a snapshot is found and @sn_info will be filled, false
+ * when error or not found. If all operation succeed but no matching one is
+ * found, @errp will NOT be set.
+ */
+bool bdrv_snapshot_find_by_id_and_name(BlockDriverState *bs,
+                                       const char *id,
+                                       const char *name,
+                                       QEMUSnapshotInfo *sn_info,
+                                       Error **errp)
+{
+    QEMUSnapshotInfo *sn_tab, *sn;
+    int nb_sns, i;
+    bool ret = false;
+
+    assert(id || name);
+
+    nb_sns = bdrv_snapshot_list(bs, &sn_tab);
+    if (nb_sns < 0) {
+        error_setg_errno(errp, -nb_sns, "Failed to get a snapshot list");
+        return false;
+    } else if (nb_sns == 0) {
+        return false;
+    }
+
+    if (id && name) {
+        for (i = 0; i < nb_sns; i++) {
+            sn = &sn_tab[i];
+            if (!strcmp(sn->id_str, id) && !strcmp(sn->name, name)) {
+                *sn_info = *sn;
+                ret = true;
+                break;
+            }
+        }
+    } else if (id) {
+        for (i = 0; i < nb_sns; i++) {
+            sn = &sn_tab[i];
+            if (!strcmp(sn->id_str, id)) {
+                *sn_info = *sn;
+                ret = true;
+                break;
+            }
+        }
+    } else if (name) {
+        for (i = 0; i < nb_sns; i++) {
+            sn = &sn_tab[i];
+            if (!strcmp(sn->name, name)) {
+                *sn_info = *sn;
+                ret = true;
+                break;
+            }
+        }
+    }
+
+    g_free(sn_tab);
+    return ret;
+}
+
 int bdrv_can_snapshot(BlockDriverState *bs)
 {
     BlockDriver *drv = bs->drv;
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index eaf61f0..9d06dc1 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -26,6 +26,7 @@
 #define SNAPSHOT_H
 
 #include "qemu-common.h"
+#include "qapi/error.h"
 
 typedef struct QEMUSnapshotInfo {
     char id_str[128]; /* unique snapshot id */
@@ -40,6 +41,11 @@ typedef struct QEMUSnapshotInfo {
 
 int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
                        const char *name);
+bool bdrv_snapshot_find_by_id_and_name(BlockDriverState *bs,
+                                       const char *id,
+                                       const char *name,
+                                       QEMUSnapshotInfo *sn_info,
+                                       Error **errp);
 int bdrv_can_snapshot(BlockDriverState *bs);
 int bdrv_snapshot_create(BlockDriverState *bs,
                          QEMUSnapshotInfo *sn_info);
-- 
1.7.1

[Qemu-devel] [PATCH V4 2/9] snapshot: add paired functions for internal snapshot id and name

[Wenchao Xia]

Internal snapshot's ID and name concept are both visible in general
block level, they are observed by user in "info snapshots", so it is
possible to have conflict. Although we can separate the two concept in
programming, but if they can be distinguished in string itself, things
will be simple and clear, so introduce two functions to do it.

The implemention, qcow2 snapshot calls snapshot_id_string_generate() to
make sure it follows the rule in driver. If caller or user give a check
with snapshot_name_wellformed() before create snapshot, then the ID
and name will never conflict. The check can be also taken in
qcow2_snapshot_create(), but require it to return error reason.

For rbd, it have no ID, so have no impact.
For sheepdog, ID can't be determined in qemu, so still can't guarantee
that no more conflict for ID and name.

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

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 0caac90..4f9c524 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -285,7 +285,7 @@ static void find_new_snapshot_id(BlockDriverState *bs,
         if (id > id_max)
             id_max = id;
     }
-    snprintf(id_str, id_str_size, "%d", id_max + 1);
+    snapshot_id_string_generate(id_max + 1, id_str, id_str_size);
 }
 
 static int find_snapshot_by_id(BlockDriverState *bs, const char *id_str)
diff --git a/block/snapshot.c b/block/snapshot.c
index 481a3ee..40e0cc7 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -228,3 +228,24 @@ int bdrv_snapshot_load_tmp(BlockDriverState *bs,
     }
     return -ENOTSUP;
 }
+
+/*
+ * Return true if the given internal snapshot name is valid, false
+ * otherwise.
+ *
+ * To prevent clashes with internal snapshot IDs, names consisting only
+ * of digits are rejected.  Empty strings are also rejected.
+ */
+bool snapshot_name_wellformed(const char *name)
+{
+    return strspn(name, "0123456789") != strlen(name);
+}
+
+/*
+ * Following function generate id string, used by block driver, such as qcow2.
+ * Since no better place to go, place the funtion here for now.
+ */
+void snapshot_id_string_generate(int id, char *id_str, int id_str_size)
+{
+    snprintf(id_str, id_str_size, "%d", id);
+}
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index 9d06dc1..3d93719 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -56,4 +56,7 @@ int bdrv_snapshot_list(BlockDriverState *bs,
                        QEMUSnapshotInfo **psn_info);
 int bdrv_snapshot_load_tmp(BlockDriverState *bs,
                            const char *snapshot_name);
+
+bool snapshot_name_wellformed(const char *name);
+void snapshot_id_string_generate(int id, char *id_str, int id_str_size);
 #endif
-- 
1.7.1

[Qemu-devel] [PATCH V4 3/9] snapshot: distinguish id and name in snapshot delete

[Wenchao Xia]

Snapshot creation actually already distinguish id and name since it take
a structured parameter *sn, but delete can't. Later an accurate delete
is needed in qmp_transaction abort and blockdev-snapshot-delete-sync,
so change its prototype. Also *errp is added to tip error, but return
value is kepted to let caller check what kind of error happens. Existing
caller for it are savevm, delvm and qemu-img, they are not impacted by
check the return value and do the operations again.

Before this patch:
  For qcow2, it search id first then name to find the one to delete.
  For rbd, it search name.
  For sheepdog, it does nothing.

After this patch:
  For qcow2, logic is the same by call it twice in caller.
  For rbd, it always fails in delete with id, but still search for name
in second try, no change for user.

Some code for *errp is based on Pavel's patch.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 block/qcow2-snapshot.c    |   66 ++++++++++++++++++++++++++++++++++-----------
 block/qcow2.h             |    5 +++-
 block/rbd.c               |   23 +++++++++++++++-
 block/sheepdog.c          |    5 +++-
 block/snapshot.c          |   36 ++++++++++++++++++++++--
 include/block/block_int.h |    5 +++-
 include/block/snapshot.h  |    5 +++-
 include/qemu-common.h     |    3 ++
 qemu-img.c                |    5 +++-
 savevm.c                  |   10 +++++-
 10 files changed, 136 insertions(+), 27 deletions(-)

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 4f9c524..e8a8e15 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -288,31 +288,47 @@ static void find_new_snapshot_id(BlockDriverState *bs,
     snapshot_id_string_generate(id_max + 1, id_str, id_str_size);
 }
 
-static int find_snapshot_by_id(BlockDriverState *bs, const char *id_str)
+static int find_snapshot_by_id_and_name(BlockDriverState *bs,
+                                        const char *id,
+                                        const char *name)
 {
     BDRVQcowState *s = bs->opaque;
     int i;
 
-    for(i = 0; i < s->nb_snapshots; i++) {
-        if (!strcmp(s->snapshots[i].id_str, id_str))
-            return i;
+    if (id && name) {
+        for (i = 0; i < s->nb_snapshots; i++) {
+            if (!strcmp(s->snapshots[i].id_str, id) &&
+                !strcmp(s->snapshots[i].name, name)) {
+                return i;
+            }
+        }
+    } else if (id) {
+        for (i = 0; i < s->nb_snapshots; i++) {
+            if (!strcmp(s->snapshots[i].id_str, id)) {
+                return i;
+            }
+        }
+    } else if (name) {
+        for (i = 0; i < s->nb_snapshots; i++) {
+            if (!strcmp(s->snapshots[i].name, name)) {
+                return i;
+            }
+        }
     }
+
     return -1;
 }
 
-static int find_snapshot_by_id_or_name(BlockDriverState *bs, const char *name)
+static int find_snapshot_by_id_or_name(BlockDriverState *bs,
+                                       const char *id_or_name)
 {
-    BDRVQcowState *s = bs->opaque;
-    int i, ret;
+    int ret;
 
-    ret = find_snapshot_by_id(bs, name);
-    if (ret >= 0)
+    ret = find_snapshot_by_id_and_name(bs, id_or_name, NULL);
+    if (ret >= 0) {
         return ret;
-    for(i = 0; i < s->nb_snapshots; i++) {
-        if (!strcmp(s->snapshots[i].name, name))
-            return i;
     }
-    return -1;
+    return find_snapshot_by_id_and_name(bs, NULL, id_or_name);
 }
 
 /* if no id is provided, a new one is constructed */
@@ -334,7 +350,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
     }
 
     /* Check that the ID is unique */
-    if (find_snapshot_by_id(bs, sn_info->id_str) >= 0) {
+    if (find_snapshot_by_id_and_name(bs, sn_info->id_str, NULL) >= 0) {
         return -EEXIST;
     }
 
@@ -531,15 +547,23 @@ fail:
     return ret;
 }
 
-int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
+int qcow2_snapshot_delete(BlockDriverState *bs,
+                          const char *snapshot_id,
+                          const char *name,
+                          Error **errp)
 {
     BDRVQcowState *s = bs->opaque;
     QCowSnapshot sn;
     int snapshot_index, ret;
+    const char *device = bdrv_get_device_name(bs);
 
     /* Search the snapshot */
-    snapshot_index = find_snapshot_by_id_or_name(bs, snapshot_id);
+    snapshot_index = find_snapshot_by_id_and_name(bs, snapshot_id, name);
     if (snapshot_index < 0) {
+        error_setg(errp,
+                   "Can't find a snapshot with ID '%s' and name '%s' "
+                   "on device '%s'",
+                   STR_PRINT_CHAR(snapshot_id), STR_PRINT_CHAR(name), device);
         return -ENOENT;
     }
     sn = s->snapshots[snapshot_index];
@@ -551,6 +575,10 @@ int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
     s->nb_snapshots--;
     ret = qcow2_write_snapshots(bs);
     if (ret < 0) {
+        error_setg(errp,
+                   "Failed to remove snapshot with ID '%s' and name '%s' from "
+                   "the snapshot list on device '%s'",
+                   STR_PRINT_CHAR(snapshot_id), STR_PRINT_CHAR(name), device);
         return ret;
     }
 
@@ -568,6 +596,9 @@ int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
     ret = qcow2_update_snapshot_refcount(bs, sn.l1_table_offset,
                                          sn.l1_size, -1);
     if (ret < 0) {
+        error_setg(errp,
+                   "Failed to free the cluster and L1 table on device '%s'",
+                   device);
         return ret;
     }
     qcow2_free_clusters(bs, sn.l1_table_offset, sn.l1_size * sizeof(uint64_t),
@@ -576,6 +607,9 @@ int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
     /* must update the copied flag on the current cluster offsets */
     ret = qcow2_update_snapshot_refcount(bs, s->l1_table_offset, s->l1_size, 0);
     if (ret < 0) {
+        error_setg(errp,
+                   "Failed to update snapshot status in disk on device '%s'",
+                   device);
         return ret;
     }
 
diff --git a/block/qcow2.h b/block/qcow2.h
index 3b2d5cd..2e6c471 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -411,7 +411,10 @@ int qcow2_zero_clusters(BlockDriverState *bs, uint64_t offset, int nb_sectors);
 /* qcow2-snapshot.c functions */
 int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info);
 int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id);
-int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id);
+int qcow2_snapshot_delete(BlockDriverState *bs,
+                          const char *snapshot_id,
+                          const char *name,
+                          Error **errp);
 int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab);
 int qcow2_snapshot_load_tmp(BlockDriverState *bs, const char *snapshot_name);
 
diff --git a/block/rbd.c b/block/rbd.c
index cb71751..688b571 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -903,12 +903,33 @@ static int qemu_rbd_snap_create(BlockDriverState *bs,
 }
 
 static int qemu_rbd_snap_remove(BlockDriverState *bs,
-                                const char *snapshot_name)
+                                const char *snapshot_id,
+                                const char *snapshot_name,
+                                Error **errp)
 {
     BDRVRBDState *s = bs->opaque;
     int r;
+    const char *device = bdrv_get_device_name(bs);
+
+    if (snapshot_id && snapshot_id[0] != '\0') {
+        error_setg(errp,
+                   "rbd do not support snapshot id on device '%s'", device);
+        return -EINVAL;
+    }
+    /* then snapshot_id == NULL or it contain an empty line, ignore it */
+
+    if (!snapshot_name) {
+        error_setg(errp,
+                   "rbd need a valid snapshot name on device '%s'", device);
+        return -EINVAL;
+    }
 
     r = rbd_snap_remove(s->image, snapshot_name);
+    if (r < 0) {
+        error_setg_errno(errp, -r,
+                         "Failed to remove snapshot '%s' on device '%s'",
+                         snapshot_name, device);
+    }
     return r;
 }
 
diff --git a/block/sheepdog.c b/block/sheepdog.c
index b397b5b..7fdae71 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -2089,7 +2089,10 @@ out:
     return ret;
 }
 
-static int sd_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
+static int sd_snapshot_delete(BlockDriverState *bs,
+                              const char *snapshot_id,
+                              const char *name,
+                              Error **errp)
 {
     /* FIXME: Delete specified snapshot id.  */
     return 0;
diff --git a/block/snapshot.c b/block/snapshot.c
index 40e0cc7..707b42d 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -182,18 +182,48 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
     return -ENOTSUP;
 }
 
-int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
+/**
+ * Delete an internal snapshot by @snapshot_id and @name.
+ * @bs: block device used in the operation
+ * @snapshot_id: unique snapshot ID, or NULL
+ * @name: snapshot name, or NULL
+ * @errp: location to store error
+ *
+ * If both @snapshot_id and @name are specified, delete the first one with
+ * id @snapshot_id and name @name.
+ * If only @snapshot_id is specified, delete the first one with id
+ * @snapshot_id.
+ * If only @name is specified, delete the first one with name @name.
+ * if none is specified, return -ENINVAL.
+ *
+ * Returns: 0 on success, -errno on fail. If @bs is not inserted, return
+ * -ENOMEDIUM. If can't find one matching @id and @name, return -ENOENT.
+ * If @bs did not support internal snapshot, return -ENOTSUP. If @bs do not
+ * support parameter @snapshot_id or @name, return -EINVAL.
+ */
+int bdrv_snapshot_delete(BlockDriverState *bs,
+                         const char *snapshot_id,
+                         const char *name,
+                         Error **errp)
 {
     BlockDriver *drv = bs->drv;
     if (!drv) {
+        error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, bdrv_get_device_name(bs));
         return -ENOMEDIUM;
     }
+    if (!snapshot_id && !name) {
+        error_setg(errp, "snapshot_id and name are both NULL");
+        return -EINVAL;
+    }
     if (drv->bdrv_snapshot_delete) {
-        return drv->bdrv_snapshot_delete(bs, snapshot_id);
+        return drv->bdrv_snapshot_delete(bs, snapshot_id, name, errp);
     }
     if (bs->file) {
-        return bdrv_snapshot_delete(bs->file, snapshot_id);
+        return bdrv_snapshot_delete(bs->file, snapshot_id, name, errp);
     }
+    error_set(errp, QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED,
+              drv->format_name, bdrv_get_device_name(bs),
+              "internal snapshot");
     return -ENOTSUP;
 }
 
diff --git a/include/block/block_int.h b/include/block/block_int.h
index c6ac871..4499b8b 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -166,7 +166,10 @@ struct BlockDriver {
                                 QEMUSnapshotInfo *sn_info);
     int (*bdrv_snapshot_goto)(BlockDriverState *bs,
                               const char *snapshot_id);
-    int (*bdrv_snapshot_delete)(BlockDriverState *bs, const char *snapshot_id);
+    int (*bdrv_snapshot_delete)(BlockDriverState *bs,
+                                const char *snapshot_id,
+                                const char *name,
+                                Error **errp);
     int (*bdrv_snapshot_list)(BlockDriverState *bs,
                               QEMUSnapshotInfo **psn_info);
     int (*bdrv_snapshot_load_tmp)(BlockDriverState *bs,
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index 3d93719..fbc03c0 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -51,7 +51,10 @@ int bdrv_snapshot_create(BlockDriverState *bs,
                          QEMUSnapshotInfo *sn_info);
 int bdrv_snapshot_goto(BlockDriverState *bs,
                        const char *snapshot_id);
-int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id);
+int bdrv_snapshot_delete(BlockDriverState *bs,
+                         const char *snapshot_id,
+                         const char *name,
+                         Error **errp);
 int bdrv_snapshot_list(BlockDriverState *bs,
                        QEMUSnapshotInfo **psn_info);
 int bdrv_snapshot_load_tmp(BlockDriverState *bs,
diff --git a/include/qemu-common.h b/include/qemu-common.h
index f439738..06c777f 100644
--- a/include/qemu-common.h
+++ b/include/qemu-common.h
@@ -191,6 +191,9 @@ int64_t strtosz_suffix(const char *nptr, char **end, const char default_suffix);
 int64_t strtosz_suffix_unit(const char *nptr, char **end,
                             const char default_suffix, int64_t unit);
 
+/* used to print char* safely */
+#define STR_PRINT_CHAR(str) ((str) ? (str) : "null")
+
 /* path.c */
 void init_paths(const char *prefix);
 const char *path(const char *pathname);
diff --git a/qemu-img.c b/qemu-img.c
index f8c97d3..05564d3 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1883,7 +1883,10 @@ static int img_snapshot(int argc, char **argv)
         break;
 
     case SNAPSHOT_DELETE:
-        ret = bdrv_snapshot_delete(bs, snapshot_name);
+        ret = bdrv_snapshot_delete(bs, snapshot_name, NULL, NULL);
+        if (ret == -ENOENT || ret == -EINVAL) {
+            ret = bdrv_snapshot_delete(bs, NULL, snapshot_name, NULL);
+        }
         if (ret) {
             error_report("Could not delete snapshot '%s': %d (%s)",
                 snapshot_name, ret, strerror(-ret));
diff --git a/savevm.c b/savevm.c
index e0491e7..56afebb 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2332,7 +2332,10 @@ static int del_existing_snapshots(Monitor *mon, const char *name)
         if (bdrv_can_snapshot(bs) &&
             bdrv_snapshot_find(bs, snapshot, name) >= 0)
         {
-            ret = bdrv_snapshot_delete(bs, name);
+            ret = bdrv_snapshot_delete(bs, name, NULL, NULL);
+            if (ret == -ENOENT || ret == -EINVAL) {
+                ret = bdrv_snapshot_delete(bs, NULL, name, NULL);
+            }
             if (ret < 0) {
                 monitor_printf(mon,
                                "Error while deleting snapshot on '%s'\n",
@@ -2562,7 +2565,10 @@ void do_delvm(Monitor *mon, const QDict *qdict)
     bs1 = NULL;
     while ((bs1 = bdrv_next(bs1))) {
         if (bdrv_can_snapshot(bs1)) {
-            ret = bdrv_snapshot_delete(bs1, name);
+            ret = bdrv_snapshot_delete(bs1, name, NULL, NULL);
+            if (ret == -ENOENT || ret == -EINVAL) {
+                ret = bdrv_snapshot_delete(bs, NULL, name, NULL);
+            }
             if (ret < 0) {
                 if (ret == -ENOTSUP)
                     monitor_printf(mon,
-- 
1.7.1

[Qemu-devel] [PATCH V4 4/9] qmp: add internal snapshot support in qmp_transaction

[Wenchao Xia]

Unlike savevm, the qmp_transaction interface will not generate
snapshot name automatically, saving trouble to return information
of the new created snapshot. The snapshot name should not collide
with snapshot ID, there is a check for it.

Although qcow2 support storing multiple snapshots with same name
but different ID, here it will fail when an snapshot with that name
already exist before the operation. Format such as rbd do not support
ID at all, and in most case, it means trouble to user when he faces
multiple snapshots with same name, so ban that case.

Snapshot ID can't be specified in this interface.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 blockdev.c       |  120 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qapi-schema.json |   18 ++++++++-
 qmp-commands.hx  |   34 ++++++++++++---
 3 files changed, 164 insertions(+), 8 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index b3a57e0..2fda33b 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -808,6 +808,121 @@ struct BlkTransactionState {
     QSIMPLEQ_ENTRY(BlkTransactionState) entry;
 };
 
+/* internal snapshot private data */
+typedef struct InternalSnapshotState {
+    BlkTransactionState common;
+    BlockDriverState *bs;
+    QEMUSnapshotInfo sn;
+} InternalSnapshotState;
+
+static void internal_snapshot_prepare(BlkTransactionState *common,
+                                      Error **errp)
+{
+    const char *device;
+    const char *name;
+    BlockDriverState *bs;
+    QEMUSnapshotInfo sn, *sn1;
+    bool ret;
+    qemu_timeval tv;
+    BlockdevSnapshotInternal *internal;
+    InternalSnapshotState *state;
+    int ret1;
+
+    g_assert(common->action->kind ==
+             TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_INTERNAL_SYNC);
+    internal = common->action->blockdev_snapshot_internal_sync;
+    state = DO_UPCAST(InternalSnapshotState, common, common);
+
+    /* 1. parse input */
+    device = internal->device;
+    name = internal->name;
+
+    /* 2. check for validation */
+    bs = bdrv_find(device);
+    if (!bs) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+        return;
+    }
+
+    if (!bdrv_is_inserted(bs)) {
+        error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
+        return;
+    }
+
+    if (bdrv_is_read_only(bs)) {
+        error_set(errp, QERR_DEVICE_IS_READ_ONLY, device);
+        return;
+    }
+
+    if (!bdrv_can_snapshot(bs)) {
+        error_set(errp, QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED,
+                  bs->drv->format_name, device, "internal snapshot");
+        return;
+    }
+
+    /* check whether a snapshot with name exist, no need to check id, since
+       name will be checked later to make sure it does not conflict with id. */
+    ret = bdrv_snapshot_find_by_id_and_name(bs, NULL, name, &sn, errp);
+    if (error_is_set(errp)) {
+        return;
+    }
+    if (ret) {
+        error_setg(errp,
+                   "Snapshot with name '%s' already exists on device '%s'",
+                   name, device);
+        return;
+    }
+
+    /* Forbid having a name similar to id, empty name is also forbidden. */
+    if (!snapshot_name_wellformed(name)) {
+        error_setg(errp, "Name '%s' on device '%s' is not a valid one",
+                   name, device);
+        return;
+    }
+
+    /* 3. take the snapshot */
+    sn1 = &state->sn;
+    pstrcpy(sn1->name, sizeof(sn1->name), name);
+    qemu_gettimeofday(&tv);
+    sn1->date_sec = tv.tv_sec;
+    sn1->date_nsec = tv.tv_usec * 1000;
+    sn1->vm_clock_nsec = qemu_get_clock_ns(vm_clock);
+
+    ret1 = bdrv_snapshot_create(bs, sn1);
+    if (ret1 < 0) {
+        error_setg_errno(errp, -ret1,
+                         "Failed to create snapshot '%s' on device '%s'",
+                         name, device);
+        return;
+    }
+
+    /* 4. succeed, mark a snapshot is created */
+    state->bs = bs;
+}
+
+static void internal_snapshot_abort(BlkTransactionState *common)
+{
+    InternalSnapshotState *state =
+                             DO_UPCAST(InternalSnapshotState, common, common);
+    BlockDriverState *bs = state->bs;
+    QEMUSnapshotInfo *sn = &state->sn;
+    Error *local_error = NULL;
+
+    if (!bs) {
+        return;
+    }
+
+    if (bdrv_snapshot_delete(bs, sn->id_str, sn->name, &local_error) < 0) {
+        error_report("Failed to delete snapshot with id '%s' and name '%s' on "
+                     "device '%s' in abort, reason is: '%s'",
+                     sn->id_str,
+                     sn->name,
+                     bdrv_get_device_name(bs),
+                     error_get_pretty(local_error));
+        error_free(local_error);
+    }
+}
+
 /* external snapshot private data */
 typedef struct ExternalSnapshotState {
     BlkTransactionState common;
@@ -990,6 +1105,11 @@ static const BdrvActionOps actions[] = {
         .prepare = abort_prepare,
         .commit = abort_commit,
     },
+    [TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_INTERNAL_SYNC] = {
+        .instance_size = sizeof(InternalSnapshotState),
+        .prepare  = internal_snapshot_prepare,
+        .abort = internal_snapshot_abort,
+    },
 };
 
 /*
diff --git a/qapi-schema.json b/qapi-schema.json
index 5c32528..a2d27c8 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1621,6 +1621,21 @@
 { 'type': 'BlockdevSnapshot',
   'data': { 'device': 'str', 'snapshot-file': 'str', '*format': 'str',
             '*mode': 'NewImageMode' } }
+##
+# @BlockdevSnapshotInternal
+#
+# @device: the name of the device to generate the snapshot from
+#
+# @name: the name of the internal snapshot to be created
+#
+# Notes: In transaction, if any snapshot matching @name exists, the operation
+#        will fail. If the name is a numeric string, it will also fail. Only
+#        some image formats support it, for example, qcow2, rbd, and sheepdog.
+#
+# Since: 1.6
+##
+{ 'type': 'BlockdevSnapshotInternal',
+  'data': { 'device': 'str', 'name': 'str' } }
 
 ##
 # @DriveBackup
@@ -1679,7 +1694,8 @@
   'data': {
        'blockdev-snapshot-sync': 'BlockdevSnapshot',
        'drive-backup': 'DriveBackup',
-       'abort': 'Abort'
+       'abort': 'Abort',
+       'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal'
    } }
 
 ##
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 362f0e1..d154506 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -994,13 +994,14 @@ transaction
 -----------
 
 Atomically operate on one or more block devices.  The only supported
-operation for now is snapshotting.  If there is any failure performing
-any of the operations, all snapshots for the group are abandoned, and
-the original disks pre-snapshot attempt are used.
+operations for now are internal and external snapshotting.  A list of
+dictionaries is accepted, that contains the actions to be performed.
+If there is any failure performing any of the operations, all operations
+for the group are abandoned.
 
-A list of dictionaries is accepted, that contains the actions to be performed.
-For snapshots this is the device, the file to use for the new snapshot,
-and the format.  The default format, if not specified, is qcow2.
+For external snapshots, the dictionary contains the device, the file to use for
+the new snapshot, and the format.  The default format, if not specified, is
+qcow2.
 
 Each new snapshot defaults to being created by QEMU (wiping any
 contents if the file already exists), but it is also possible to reuse
@@ -1009,6 +1010,19 @@ the new image file has the same contents as the current one; QEMU cannot
 perform any meaningful check.  Typically this is achieved by using the
 current image file as the backing file for the new image.
 
+On failure, the original disks pre-snapshot attempt will be used.
+
+For internal snapshots, the dictionary contains the device and the snapshot's
+name.  The name must not be a numeric string since this collides with snapshot
+IDs and an error will be returned.  For example, name "99" is not a valid name.
+If an internal snapshot matching name already exists, the request will be also
+rejected.  Only some image formats support it, for example, qcow2, rbd, and
+sheepdog.
+
+On failure, qemu will try delete the newly created internal snapshot in the
+transaction.  When an I/O error occurs during deletion, the user needs to fix
+it later with qemu-img or other command.
+
 Arguments:
 
 actions array:
@@ -1021,6 +1035,9 @@ actions array:
       - "format": format of new image (json-string, optional)
       - "mode": whether and how QEMU should create the snapshot file
         (NewImageMode, optional, default "absolute-paths")
+      When "type" is "blockdev-snapshot-internal-sync":
+      - "device": device name to snapshot (json-string)
+      - "name": name of the new snapshot (json-string)
 
 Example:
 
@@ -1032,7 +1049,10 @@ Example:
          { 'type': 'blockdev-snapshot-sync', 'data' : { "device": "ide-hd1",
                                          "snapshot-file": "/some/place/my-image2",
                                          "mode": "existing",
-                                         "format": "qcow2" } } ] } }
+                                         "format": "qcow2" } },
+         { 'type': 'blockdev-snapshot-internal-sync', 'data' : {
+                                         "device": "ide-hd2",
+                                         "name": "snapshot0" } } ] } }
 <- { "return": {} }
 
 EQMP
-- 
1.7.1

[Qemu-devel] [PATCH V4 5/9] qmp: add interface blockdev-snapshot-internal-sync

[Wenchao Xia]

Snapshot ID can't be specified in this interface.

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

diff --git a/blockdev.c b/blockdev.c
index 2fda33b..d2cca6c 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -777,6 +777,19 @@ void qmp_blockdev_snapshot_sync(const char *device, const char *snapshot_file,
                        &snapshot, errp);
 }
 
+void qmp_blockdev_snapshot_internal_sync(const char *device,
+                                         const char *name,
+                                         Error **errp)
+{
+    BlockdevSnapshotInternal snapshot = {
+        .device = (char *) device,
+        .name = (char *) name
+    };
+
+    blockdev_do_action(TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_INTERNAL_SYNC,
+                       &snapshot, errp);
+}
+
 
 /* New and old BlockDriverState structs for group snapshots */
 
diff --git a/qapi-schema.json b/qapi-schema.json
index a2d27c8..40f9c8e 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1746,6 +1746,28 @@
             '*mode': 'NewImageMode'} }
 
 ##
+# @blockdev-snapshot-internal-sync
+#
+# Synchronously take an internal snapshot of a block device, when the format
+# of the image used supports it.
+#
+# @device: the name of the device to generate the snapshot from
+#
+# @name: the name of new snapshot name
+#
+# Returns: nothing on success
+#          If @device is not a valid block device, DeviceNotFound
+#          If any snapshot matching @name exists, or the name string is invalid
+#          which may collide with snapshot IDs, or name is empty, GenericError
+#          If the format of the image used does not support it,
+#          BlockFormatFeatureNotSupported
+#
+# Since 1.6
+##
+{ 'command': 'blockdev-snapshot-internal-sync',
+  'data': { 'device': 'str', 'name': 'str'} }
+
+##
 # @human-monitor-command:
 #
 # Execute a command on the human monitor and return the output.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index d154506..a2675cf 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1092,6 +1092,36 @@ Example:
 EQMP
 
     {
+        .name       = "blockdev-snapshot-internal-sync",
+        .args_type  = "device:B,name:s",
+        .mhandler.cmd_new = qmp_marshal_input_blockdev_snapshot_internal_sync,
+    },
+
+SQMP
+blockdev-snapshot-internal-sync
+-------------------------------
+
+Synchronously take an internal snapshot of a block device when the format of
+image used supports it.  If the name is a numeric string which may collide with
+snapshot IDs, such as "19", or an empty string, the operation will fail.  If a
+snapshot with name already exists, the operation will fail.
+
+Arguments:
+
+- "device": device name to snapshot (json-string)
+- "name": name of the new snapshot (json-string)
+
+Example:
+
+-> { "execute": "blockdev-snapshot-internal-sync",
+                "arguments": { "device": "ide-hd0",
+                               "name": "snapshot0" }
+   }
+<- { "return": {} }
+
+EQMP
+
+    {
         .name       = "drive-mirror",
         .args_type  = "sync:s,device:B,target:s,speed:i?,mode:s?,format:s?,"
                       "on-source-error:s?,on-target-error:s?,"
-- 
1.7.1

[Qemu-devel] [PATCH V4 6/9] qmp: add interface blockdev-snapshot-delete-internal-sync

[Wenchao Xia]

This interface use id and name as optional parameters, to handle the
case that one image contain multiple snapshots with same name which
may be '', but with different id.

Adding parameter id is for historical compatiability reason, and
that case is not possible in qemu's new interface for internal
snapshot at block device level, but still possible in qemu-img.

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

diff --git a/blockdev.c b/blockdev.c
index d2cca6c..ccc1d49 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -790,6 +790,67 @@ void qmp_blockdev_snapshot_internal_sync(const char *device,
                        &snapshot, errp);
 }
 
+SnapshotInfo *qmp_blockdev_snapshot_delete_internal_sync(const char *device,
+                                                         bool has_id,
+                                                         const char *id,
+                                                         bool has_name,
+                                                         const char *name,
+                                                         Error **errp)
+{
+    BlockDriverState *bs = bdrv_find(device);
+    QEMUSnapshotInfo sn;
+    Error *local_err = NULL;
+    SnapshotInfo *info = NULL;
+    int ret;
+
+    if (!bs) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+        return NULL;
+    }
+
+    if (!has_id) {
+        id = NULL;
+    }
+
+    if (!has_name) {
+        name = NULL;
+    }
+
+    if (!id && !name) {
+        error_setg(errp, "Name or id must be provided");
+        return NULL;
+    }
+
+    ret = bdrv_snapshot_find_by_id_and_name(bs, id, name, &sn, &local_err);
+    if (error_is_set(&local_err)) {
+        error_propagate(errp, local_err);
+        return NULL;
+    }
+    if (!ret) {
+        error_setg(errp,
+                   "Snapshot with id '%s' and name '%s' does not exist on "
+                   "device '%s'",
+                   STR_PRINT_CHAR(id), STR_PRINT_CHAR(name), device);
+        return NULL;
+    }
+
+    bdrv_snapshot_delete(bs, id, name, &local_err);
+    if (error_is_set(&local_err)) {
+        error_propagate(errp, local_err);
+        return NULL;
+    }
+
+    info = g_malloc0(sizeof(SnapshotInfo));
+    info->id = g_strdup(sn.id_str);
+    info->name = g_strdup(sn.name);
+    info->date_nsec = sn.date_nsec;
+    info->date_sec = sn.date_sec;
+    info->vm_state_size = sn.vm_state_size;
+    info->vm_clock_nsec = sn.vm_clock_nsec % 1000000000;
+    info->vm_clock_sec = sn.vm_clock_nsec / 1000000000;
+
+    return info;
+}
 
 /* New and old BlockDriverState structs for group snapshots */
 
diff --git a/qapi-schema.json b/qapi-schema.json
index 40f9c8e..9908f86 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1768,6 +1768,33 @@
   'data': { 'device': 'str', 'name': 'str'} }
 
 ##
+# @blockdev-snapshot-delete-internal-sync
+#
+# Synchronously delete an internal snapshot of a block device, when the format
+# of the image used support it. The snapshot is identified by name or id or
+# both. One of the name or id is required. Return SnapshotInfo for the
+# successfully deleted snapshot.
+#
+# @device: the name of the device to delete the snapshot from
+#
+# @id: optional the snapshot's ID to be deleted
+#
+# @name: optional the snapshot's name to be deleted
+#
+# Returns: SnapshotInfo on success
+#          If @device is not a valid block device, DeviceNotFound
+#          If snapshot not found, GenericError
+#          If the format of the image used does not support it,
+#          BlockFormatFeatureNotSupported
+#          If @id and @name are both not specified, GenericError
+#
+# Since 1.6
+##
+{ 'command': 'blockdev-snapshot-delete-internal-sync',
+  'data': { 'device': 'str', '*id': 'str', '*name': 'str'},
+  'returns': 'SnapshotInfo' }
+
+##
 # @human-monitor-command:
 #
 # Execute a command on the human monitor and return the output.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index a2675cf..6a4401f 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1122,6 +1122,47 @@ Example:
 EQMP
 
     {
+        .name       = "blockdev-snapshot-delete-internal-sync",
+        .args_type  = "device:B,id:s?,name:s?",
+        .mhandler.cmd_new =
+                      qmp_marshal_input_blockdev_snapshot_delete_internal_sync,
+    },
+
+SQMP
+blockdev-snapshot-delete-internal-sync
+--------------------------------------
+
+Synchronously delete an internal snapshot of a block device when the format of
+image used supports it.  The snapshot is identified by name or id or both.  One
+of name or id is required.  If the snapshot is not found, the operation will
+fail.
+
+Arguments:
+
+- "device": device name (json-string)
+- "id": ID of the snapshot (json-string, optional)
+- "name": name of the snapshot (json-string, optional)
+
+Example:
+
+-> { "execute": "blockdev-snapshot-delete-internal-sync",
+                "arguments": { "device": "ide-hd0",
+                               "name": "snapshot0" }
+   }
+<- { "return": {
+                   "id": "1",
+                   "name": "snapshot0",
+                   "vm-state-size": 0,
+                   "date-sec": 1000012,
+                   "date-nsec": 10,
+                   "vm-clock-sec": 100,
+                   "vm-clock-nsec": 20
+     }
+   }
+
+EQMP
+
+    {
         .name       = "drive-mirror",
         .args_type  = "sync:s,device:B,target:s,speed:i?,mode:s?,format:s?,"
                       "on-source-error:s?,on-target-error:s?,"
-- 
1.7.1

[Qemu-devel] [PATCH V4 7/9] hmp: add interface hmp_snapshot_blkdev_internal

[Wenchao Xia]

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

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 915b0d1..53e3ef3 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1023,8 +1023,7 @@ ETEXI
                       "of device. If a new image file is specified, the\n\t\t\t"
                       "new image file will become the new root image.\n\t\t\t"
                       "If format is specified, the snapshot file will\n\t\t\t"
-                      "be created in that format. Otherwise the\n\t\t\t"
-                      "snapshot will be internal! (currently unsupported).\n\t\t\t"
+                      "be created in that format.\n\t\t\t"
                       "The default format is qcow2.  The -n flag requests QEMU\n\t\t\t"
                       "to reuse the image found in new-image-file, instead of\n\t\t\t"
                       "recreating it from scratch.",
@@ -1038,6 +1037,22 @@ Snapshot device, using snapshot file as target if provided
 ETEXI
 
     {
+        .name       = "snapshot_blkdev_internal",
+        .args_type  = "device:B,name:s",
+        .params     = "device name",
+        .help       = "take an internal snapshot of device.\n\t\t\t"
+                      "The format of the image used by device must\n\t\t\t"
+                      "support it, such as qcow2.\n\t\t\t",
+        .mhandler.cmd = hmp_snapshot_blkdev_internal,
+    },
+
+STEXI
+@item snapshot_blkdev_internal
+@findex snapshot_blkdev_internal
+Take an internal snapshot on device if it support
+ETEXI
+
+    {
         .name       = "drive_mirror",
         .args_type  = "reuse:-n,full:-f,device:B,target:s,format:s?",
         .params     = "[-n] [-f] device target [format]",
diff --git a/hmp.c b/hmp.c
index 2daed43..d300366 100644
--- a/hmp.c
+++ b/hmp.c
@@ -930,6 +930,16 @@ void hmp_snapshot_blkdev(Monitor *mon, const QDict *qdict)
     hmp_handle_error(mon, &errp);
 }
 
+void hmp_snapshot_blkdev_internal(Monitor *mon, const QDict *qdict)
+{
+    const char *device = qdict_get_str(qdict, "device");
+    const char *name = qdict_get_str(qdict, "name");
+    Error *errp = NULL;
+
+    qmp_blockdev_snapshot_internal_sync(device, name, &errp);
+    hmp_handle_error(mon, &errp);
+}
+
 void hmp_migrate_cancel(Monitor *mon, const QDict *qdict)
 {
     qmp_migrate_cancel(NULL);
diff --git a/hmp.h b/hmp.h
index 56d2e92..fddf3a9 100644
--- a/hmp.h
+++ b/hmp.h
@@ -54,6 +54,7 @@ void hmp_block_passwd(Monitor *mon, const QDict *qdict);
 void hmp_balloon(Monitor *mon, const QDict *qdict);
 void hmp_block_resize(Monitor *mon, const QDict *qdict);
 void hmp_snapshot_blkdev(Monitor *mon, const QDict *qdict);
+void hmp_snapshot_blkdev_internal(Monitor *mon, const QDict *qdict);
 void hmp_drive_mirror(Monitor *mon, const QDict *qdict);
 void hmp_migrate_cancel(Monitor *mon, const QDict *qdict);
 void hmp_migrate_set_downtime(Monitor *mon, const QDict *qdict);
-- 
1.7.1

[Qemu-devel] [PATCH V4 8/9] hmp: add interface hmp_snapshot_delete_blkdev_internal

[Wenchao Xia]

It is hard to make both id and name optional in hmp console as qmp
interface, so this interface require user to specify name.

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

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 53e3ef3..b8d17dc 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1053,6 +1053,24 @@ Take an internal snapshot on device if it support
 ETEXI
 
     {
+        .name       = "snapshot_delete_blkdev_internal",
+        .args_type  = "device:B,name:s,id:s?",
+        .params     = "device name [id]",
+        .help       = "delete an internal snapshot of device.\n\t\t\t"
+                      "If id is specified, qemu will try delete\n\t\t\t"
+                      "the snapshot matching both id and name.\n\t\t\t"
+                      "The format of the image used by device must\n\t\t\t"
+                      "support it, such as qcow2.\n\t\t\t",
+        .mhandler.cmd = hmp_snapshot_delete_blkdev_internal,
+    },
+
+STEXI
+@item snapshot_delete_blkdev_internal
+@findex snapshot_delete_blkdev_internal
+Delete an internal snapshot on device if it support
+ETEXI
+
+    {
         .name       = "drive_mirror",
         .args_type  = "reuse:-n,full:-f,device:B,target:s,format:s?",
         .params     = "[-n] [-f] device target [format]",
diff --git a/hmp.c b/hmp.c
index d300366..9f9f908 100644
--- a/hmp.c
+++ b/hmp.c
@@ -940,6 +940,18 @@ void hmp_snapshot_blkdev_internal(Monitor *mon, const QDict *qdict)
     hmp_handle_error(mon, &errp);
 }
 
+void hmp_snapshot_delete_blkdev_internal(Monitor *mon, const QDict *qdict)
+{
+    const char *device = qdict_get_str(qdict, "device");
+    const char *name = qdict_get_str(qdict, "name");
+    const char *id = qdict_get_try_str(qdict, "id");
+    Error *errp = NULL;
+
+    qmp_blockdev_snapshot_delete_internal_sync(device, !!id, id,
+                                               true, name, &errp);
+    hmp_handle_error(mon, &errp);
+}
+
 void hmp_migrate_cancel(Monitor *mon, const QDict *qdict)
 {
     qmp_migrate_cancel(NULL);
diff --git a/hmp.h b/hmp.h
index fddf3a9..fa0e5d9 100644
--- a/hmp.h
+++ b/hmp.h
@@ -55,6 +55,7 @@ void hmp_balloon(Monitor *mon, const QDict *qdict);
 void hmp_block_resize(Monitor *mon, const QDict *qdict);
 void hmp_snapshot_blkdev(Monitor *mon, const QDict *qdict);
 void hmp_snapshot_blkdev_internal(Monitor *mon, const QDict *qdict);
+void hmp_snapshot_delete_blkdev_internal(Monitor *mon, const QDict *qdict);
 void hmp_drive_mirror(Monitor *mon, const QDict *qdict);
 void hmp_migrate_cancel(Monitor *mon, const QDict *qdict);
 void hmp_migrate_set_downtime(Monitor *mon, const QDict *qdict);
-- 
1.7.1

[Qemu-devel] [PATCH V4 9/9] qemu-iotests: add 056 internal snapshot for block device test case

[Wenchao Xia]

Create in transaction and deletion in single command will be tested.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 tests/qemu-iotests/056     |  267 ++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/056.out |    5 +
 tests/qemu-iotests/group   |    1 +
 3 files changed, 273 insertions(+), 0 deletions(-)
 create mode 100755 tests/qemu-iotests/056
 create mode 100644 tests/qemu-iotests/056.out

diff --git a/tests/qemu-iotests/056 b/tests/qemu-iotests/056
new file mode 100755
index 0000000..1a8a02c
--- /dev/null
+++ b/tests/qemu-iotests/056
@@ -0,0 +1,267 @@
+#!/usr/bin/env python
+#
+# Tests for internal snapshot.
+#
+# Copyright (C) 2013 IBM, Inc.
+#
+# Based on 055.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+import time
+import os
+import iotests
+from iotests import qemu_img, qemu_io
+
+test_drv_base_name = 'drive'
+
+class ImageSnapshotTestCase(iotests.QMPTestCase):
+    image_len = 120 * 1024 * 1024 # MB
+
+    def __init__(self, *args):
+        self.expect = []
+        super(ImageSnapshotTestCase, self).__init__(*args)
+
+    def _setUp(self, test_img_base_name, image_num):
+        self.vm = iotests.VM()
+        for i in range(0, image_num):
+            filename = '%s%d' % (test_img_base_name, i)
+            img = os.path.join(iotests.test_dir, filename)
+            device = '%s%d' % (test_drv_base_name, i)
+            qemu_img('create', '-f', iotests.imgfmt, img, str(self.image_len))
+            self.vm.add_drive(img)
+            self.expect.append({'image': img, 'device': device,
+                                'snapshots': [],
+                                'snapshots_name_counter': 0})
+        self.vm.launch()
+
+    def tearDown(self):
+        self.vm.shutdown()
+        for dev_expect in self.expect:
+            os.remove(dev_expect['image'])
+
+    def createSnapshotInTransaction(self, snapshot_num, abort = False):
+        actions = []
+        for dev_expect in self.expect:
+            num = dev_expect['snapshots_name_counter']
+            for j in range(0, snapshot_num):
+                name = '%s_sn%d' % (dev_expect['device'], num)
+                num = num + 1
+                if abort == False:
+                    dev_expect['snapshots'].append({'name': name})
+                    dev_expect['snapshots_name_counter'] = num
+                actions.append({
+                    'type': 'blockdev-snapshot-internal-sync',
+                    'data': { 'device': dev_expect['device'],
+                              'name': name },
+                })
+
+        if abort == True:
+            actions.append({
+                'type': 'abort',
+                'data': {},
+            })
+
+        result = self.vm.qmp('transaction', actions = actions)
+
+        if abort == True:
+            self.assert_qmp(result, 'error/class', 'GenericError')
+        else:
+            self.assert_qmp(result, 'return', {})
+
+    def verifySnapshotInfo(self):
+        result = self.vm.qmp('query-block')
+
+        # Verify each expected result
+        for dev_expect in self.expect:
+            # 1. Find the returned image value and snapshot info
+            image_result = None
+            for device in result['return']:
+                if device['device'] == dev_expect['device']:
+                    image_result = device['inserted']['image']
+                    break
+            self.assertTrue(image_result != None)
+            # Do not consider zero snapshot case now
+            sn_list_result = image_result['snapshots']
+            sn_list_expect = dev_expect['snapshots']
+
+            # 2. Verify it with expect
+            self.assertTrue(len(sn_list_result) == len(sn_list_expect))
+
+            for sn_expect in sn_list_expect:
+                sn_result = None
+                for sn in sn_list_result:
+                    if sn_expect['name'] == sn['name']:
+                        sn_result = sn
+                        break
+                self.assertTrue(sn_result != None)
+                # Fill in the detail info
+                sn_expect.update(sn_result)
+
+    def deleteSnapshot(self, device, id = None, name = None):
+        sn_list_expect = None
+        sn_expect = None
+
+        self.assertTrue(id != None or name != None)
+
+        # Fill in the detail info include ID
+        self.verifySnapshotInfo()
+
+        #find the expected snapshot list
+        for dev_expect in self.expect:
+            if dev_expect['device'] == device:
+                sn_list_expect = dev_expect['snapshots']
+                break
+        self.assertTrue(sn_list_expect != None)
+
+        if id != None and name != None:
+            for sn in sn_list_expect:
+                if sn['id'] == id and sn['name'] == name:
+                    sn_expect = sn
+                    result = \
+                          self.vm.qmp('blockdev-snapshot-delete-internal-sync',
+                                      device = device,
+                                      id = id,
+                                      name = name)
+                    break
+        elif id != None:
+            for sn in sn_list_expect:
+                if sn['id'] == id:
+                    sn_expect = sn
+                    result = \
+                          self.vm.qmp('blockdev-snapshot-delete-internal-sync',
+                                      device = device,
+                                      id = id)
+                    break
+        else:
+            for sn in sn_list_expect:
+                if sn['name'] == name:
+                    sn_expect = sn
+                    result = \
+                          self.vm.qmp('blockdev-snapshot-delete-internal-sync',
+                                      device = device,
+                                      name = name)
+                    break
+
+        self.assertTrue(sn_expect != None)
+
+        self.assert_qmp(result, 'return', sn_expect)
+        sn_list_expect.remove(sn_expect)
+
+class TestSingleTransaction(ImageSnapshotTestCase):
+    def setUp(self):
+        self._setUp('test_a.img', 1)
+
+    def test_create(self):
+        self.createSnapshotInTransaction(1)
+        self.verifySnapshotInfo()
+
+    def test_error_name_numeric(self):
+        actions = [{'type': 'blockdev-snapshot-internal-sync',
+                    'data': { 'device': self.expect[0]['device'],
+                              'name': '1234' },
+                  }]
+        result = self.vm.qmp('transaction', actions = actions)
+        self.assert_qmp(result, 'error/class', 'GenericError')
+
+    def test_error_name_empty(self):
+        actions = [{'type': 'blockdev-snapshot-internal-sync',
+                    'data': { 'device': self.expect[0]['device'],
+                              'name': '' },
+                  }]
+        result = self.vm.qmp('transaction', actions = actions)
+        self.assert_qmp(result, 'error/class', 'GenericError')
+
+    def test_error_device(self):
+        actions = [{'type': 'blockdev-snapshot-internal-sync',
+                    'data': { 'device': 'drive_error',
+                              'name': 'a' },
+                  }]
+        result = self.vm.qmp('transaction', actions = actions)
+        self.assert_qmp(result, 'error/class', 'DeviceNotFound')
+
+    def test_error_exist(self):
+        self.createSnapshotInTransaction(1)
+        self.verifySnapshotInfo()
+        actions = [{'type': 'blockdev-snapshot-internal-sync',
+                    'data': { 'device': self.expect[0]['device'],
+                              'name': self.expect[0]['snapshots'][0] },
+                  }]
+        result = self.vm.qmp('transaction', actions = actions)
+        self.assert_qmp(result, 'error/class', 'GenericError')
+
+class TestMultipleTransaction(ImageSnapshotTestCase):
+    def setUp(self):
+        self._setUp('test_b.img', 2)
+
+    def test_create(self):
+        self.createSnapshotInTransaction(3)
+        self.verifySnapshotInfo()
+
+    def test_abort(self):
+        self.createSnapshotInTransaction(2)
+        self.verifySnapshotInfo()
+        self.createSnapshotInTransaction(3, abort = True)
+        self.verifySnapshotInfo()
+
+class TestSnapshotDelete(ImageSnapshotTestCase):
+    def setUp(self):
+        self._setUp('test_c.img', 1)
+
+    def test_delete_with_id(self):
+        self.createSnapshotInTransaction(2)
+        self.verifySnapshotInfo()
+        self.deleteSnapshot(self.expect[0]['device'],
+                            id = self.expect[0]['snapshots'][0]['id'])
+        self.verifySnapshotInfo()
+
+    def test_delete_with_name(self):
+        self.createSnapshotInTransaction(3)
+        self.verifySnapshotInfo()
+        self.deleteSnapshot(self.expect[0]['device'],
+                            name = self.expect[0]['snapshots'][1]['name'])
+        self.verifySnapshotInfo()
+
+    def test_delete_with_id_and_name(self):
+        self.createSnapshotInTransaction(4)
+        self.verifySnapshotInfo()
+        self.deleteSnapshot(self.expect[0]['device'],
+                            id = self.expect[0]['snapshots'][2]['id'],
+                            name = self.expect[0]['snapshots'][2]['name'])
+        self.verifySnapshotInfo()
+
+
+    def test_error_device(self):
+        result = self.vm.qmp('blockdev-snapshot-delete-internal-sync',
+                              device = 'drive_error',
+                              id = '0')
+        self.assert_qmp(result, 'error/class', 'DeviceNotFound')
+
+    def test_error_no_id_and_name(self):
+        result = self.vm.qmp('blockdev-snapshot-delete-internal-sync',
+                              device = self.expect[0]['device'])
+        self.assert_qmp(result, 'error/class', 'GenericError')
+
+    def test_error_snapshot_not_exist(self):
+        self.createSnapshotInTransaction(2)
+        self.verifySnapshotInfo()
+        result = self.vm.qmp('blockdev-snapshot-delete-internal-sync',
+                              device = self.expect[0]['device'],
+                              id = self.expect[0]['snapshots'][0]['id'],
+                              name = self.expect[0]['snapshots'][1]['name'])
+        self.assert_qmp(result, 'error/class', 'GenericError')
+
+if __name__ == '__main__':
+    iotests.main(supported_fmts=['qcow2'])
diff --git a/tests/qemu-iotests/056.out b/tests/qemu-iotests/056.out
new file mode 100644
index 0000000..fa16b5c
--- /dev/null
+++ b/tests/qemu-iotests/056.out
@@ -0,0 +1,5 @@
+.............
+----------------------------------------------------------------------
+Ran 13 tests
+
+OK
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index fdc6ed1..5f5009b 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -62,3 +62,4 @@
 053 rw auto
 054 rw auto
 055 rw auto
+056 rw auto
-- 
1.7.1

Re: [Qemu-devel] [PATCH V4 2/9] snapshot: add paired functions for internal snapshot id and name

[Kevin Wolf]

Am 08.07.2013 um 10:00 hat Wenchao Xia geschrieben:
> Internal snapshot's ID and name concept are both visible in general
> block level, they are observed by user in "info snapshots", so it is
> possible to have conflict. Although we can separate the two concept in
> programming, but if they can be distinguished in string itself, things
> will be simple and clear, so introduce two functions to do it.
> 
> The implemention, qcow2 snapshot calls snapshot_id_string_generate() to
> make sure it follows the rule in driver. If caller or user give a check
> with snapshot_name_wellformed() before create snapshot, then the ID
> and name will never conflict. The check can be also taken in
> qcow2_snapshot_create(), but require it to return error reason.

I'm not sure how useful this is. While we can restrict what IDs we allow
for creating new snapshots, we cannot take any advantage from it because
existing snapshots could already be named with only digits (they could
also use a non-numeric ID). At the end of the day we're limiting the
choice of IDs that can be generated with the QMP command without a real
reason.

Kevin

Re: [Qemu-devel] [PATCH V4 2/9] snapshot: add paired functions for internal snapshot id and name

[Wenchao Xia]

于 2013-7-10 21:10, Kevin Wolf 写道:
> Am 08.07.2013 um 10:00 hat Wenchao Xia geschrieben:
>> Internal snapshot's ID and name concept are both visible in general
>> block level, they are observed by user in "info snapshots", so it is
>> possible to have conflict. Although we can separate the two concept in
>> programming, but if they can be distinguished in string itself, things
>> will be simple and clear, so introduce two functions to do it.
>>
>> The implemention, qcow2 snapshot calls snapshot_id_string_generate() to
>> make sure it follows the rule in driver. If caller or user give a check
>> with snapshot_name_wellformed() before create snapshot, then the ID
>> and name will never conflict. The check can be also taken in
>> qcow2_snapshot_create(), but require it to return error reason.
>
> I'm not sure how useful this is. While we can restrict what IDs we allow
> for creating new snapshots, we cannot take any advantage from it because
> existing snapshots could already be named with only digits (they could
> also use a non-numeric ID). At the end of the day we're limiting the
   Qcow2's ID seems always numeric, do you mean sheepdog may have
non-numeric ID?

> choice of IDs that can be generated with the QMP command without a real
> reason.
>
   This patch may limit the choice of snapshot name. Maybe we can't
benefit from it now because existing code, but shouldn't it be improved
in any new interface to alleviate the problem? I guess you idea
is drop this check in the new interface, isn't it?

> Kevin
>


-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH V4 2/9] snapshot: add paired functions for internal snapshot id and name

[Kevin Wolf]

Am 10.07.2013 um 15:54 hat Wenchao Xia geschrieben:
> 于 2013-7-10 21:10, Kevin Wolf 写道:
> >Am 08.07.2013 um 10:00 hat Wenchao Xia geschrieben:
> >>Internal snapshot's ID and name concept are both visible in general
> >>block level, they are observed by user in "info snapshots", so it is
> >>possible to have conflict. Although we can separate the two concept in
> >>programming, but if they can be distinguished in string itself, things
> >>will be simple and clear, so introduce two functions to do it.
> >>
> >>The implemention, qcow2 snapshot calls snapshot_id_string_generate() to
> >>make sure it follows the rule in driver. If caller or user give a check
> >>with snapshot_name_wellformed() before create snapshot, then the ID
> >>and name will never conflict. The check can be also taken in
> >>qcow2_snapshot_create(), but require it to return error reason.
> >
> >I'm not sure how useful this is. While we can restrict what IDs we allow
> >for creating new snapshots, we cannot take any advantage from it because
> >existing snapshots could already be named with only digits (they could
> >also use a non-numeric ID). At the end of the day we're limiting the
>   Qcow2's ID seems always numeric, do you mean sheepdog may have
> non-numeric ID?

That's not true, qcow2 has an ID string rather than a numeric ID.

> >choice of IDs that can be generated with the QMP command without a real
> >reason.
> >
>   This patch may limit the choice of snapshot name. Maybe we can't
> benefit from it now because existing code, but shouldn't it be improved
> in any new interface to alleviate the problem? I guess you idea
> is drop this check in the new interface, isn't it?

Well, the thought is that limiting it in QMP is of no use if there is no
corresponding limitation in the backend. So yes, I think, as long as
block drivers can deal with numeric snapshots names, QMP should allow
dealing with them.

Kevin

Re: [Qemu-devel] [PATCH V4 2/9] snapshot: add paired functions for internal snapshot id and name

[Wenchao Xia]

于 2013-7-10 22:22, Kevin Wolf 写道:
> Am 10.07.2013 um 15:54 hat Wenchao Xia geschrieben:
>> 于 2013-7-10 21:10, Kevin Wolf 写道:
>>> Am 08.07.2013 um 10:00 hat Wenchao Xia geschrieben:
>>>> Internal snapshot's ID and name concept are both visible in general
>>>> block level, they are observed by user in "info snapshots", so it is
>>>> possible to have conflict. Although we can separate the two concept in
>>>> programming, but if they can be distinguished in string itself, things
>>>> will be simple and clear, so introduce two functions to do it.
>>>>
>>>> The implemention, qcow2 snapshot calls snapshot_id_string_generate() to
>>>> make sure it follows the rule in driver. If caller or user give a check
>>>> with snapshot_name_wellformed() before create snapshot, then the ID
>>>> and name will never conflict. The check can be also taken in
>>>> qcow2_snapshot_create(), but require it to return error reason.
>>>
>>> I'm not sure how useful this is. While we can restrict what IDs we allow
>>> for creating new snapshots, we cannot take any advantage from it because
>>> existing snapshots could already be named with only digits (they could
>>> also use a non-numeric ID). At the end of the day we're limiting the
>>    Qcow2's ID seems always numeric, do you mean sheepdog may have
>> non-numeric ID?
>
> That's not true, qcow2 has an ID string rather than a numeric ID.
>
   But there is no way to specify non-numeric ID in public interface,
such as savevm, qemu-img, and in qcow2_snapshot_create(),
find_new_snapshot_id() will always fill in a numeric ID string, so
result is that, qcow2's snapshot ID string is always a numeric string.

>>> choice of IDs that can be generated with the QMP command without a real
>>> reason.
>>>
>>    This patch may limit the choice of snapshot name. Maybe we can't
>> benefit from it now because existing code, but shouldn't it be improved
>> in any new interface to alleviate the problem? I guess you idea
>> is drop this check in the new interface, isn't it?
>
> Well, the thought is that limiting it in QMP is of no use if there is no
> corresponding limitation in the backend. So yes, I think, as long as
> block drivers can deal with numeric snapshots names, QMP should allow
> dealing with them.
>
   OK, I'll drop this limit.

> Kevin
>


-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH V4 2/9] snapshot: add paired functions for internal snapshot id and name

[Kevin Wolf]

Am 10.07.2013 um 16:43 hat Wenchao Xia geschrieben:
> 于 2013-7-10 22:22, Kevin Wolf 写道:
> >Am 10.07.2013 um 15:54 hat Wenchao Xia geschrieben:
> >>于 2013-7-10 21:10, Kevin Wolf 写道:
> >>>Am 08.07.2013 um 10:00 hat Wenchao Xia geschrieben:
> >>>>Internal snapshot's ID and name concept are both visible in general
> >>>>block level, they are observed by user in "info snapshots", so it is
> >>>>possible to have conflict. Although we can separate the two concept in
> >>>>programming, but if they can be distinguished in string itself, things
> >>>>will be simple and clear, so introduce two functions to do it.
> >>>>
> >>>>The implemention, qcow2 snapshot calls snapshot_id_string_generate() to
> >>>>make sure it follows the rule in driver. If caller or user give a check
> >>>>with snapshot_name_wellformed() before create snapshot, then the ID
> >>>>and name will never conflict. The check can be also taken in
> >>>>qcow2_snapshot_create(), but require it to return error reason.
> >>>
> >>>I'm not sure how useful this is. While we can restrict what IDs we allow
> >>>for creating new snapshots, we cannot take any advantage from it because
> >>>existing snapshots could already be named with only digits (they could
> >>>also use a non-numeric ID). At the end of the day we're limiting the
> >>   Qcow2's ID seems always numeric, do you mean sheepdog may have
> >>non-numeric ID?
> >
> >That's not true, qcow2 has an ID string rather than a numeric ID.
> >
>   But there is no way to specify non-numeric ID in public interface,
> such as savevm, qemu-img, and in qcow2_snapshot_create(),
> find_new_snapshot_id() will always fill in a numeric ID string, so
> result is that, qcow2's snapshot ID string is always a numeric string.

Who said that qemu is the only program on the world that can create
qcow2 snapshots? It might be, I don't know of any other, but qcow2 is
defined by its spec, not by qemu's source code. And the spec talks about
an ID string (just like the qemu code does, even though it happens to
only write numbers into this string).

Kevin

Re: [Qemu-devel] [PATCH V4 2/9] snapshot: add paired functions for internal snapshot id and name

[Wenchao Xia]

于 2013-7-10 22:49, Kevin Wolf 写道:
> Am 10.07.2013 um 16:43 hat Wenchao Xia geschrieben:
>> 于 2013-7-10 22:22, Kevin Wolf 写道:
>>> Am 10.07.2013 um 15:54 hat Wenchao Xia geschrieben:
>>>> 于 2013-7-10 21:10, Kevin Wolf 写道:
>>>>> Am 08.07.2013 um 10:00 hat Wenchao Xia geschrieben:
>>>>>> Internal snapshot's ID and name concept are both visible in general
>>>>>> block level, they are observed by user in "info snapshots", so it is
>>>>>> possible to have conflict. Although we can separate the two concept in
>>>>>> programming, but if they can be distinguished in string itself, things
>>>>>> will be simple and clear, so introduce two functions to do it.
>>>>>>
>>>>>> The implemention, qcow2 snapshot calls snapshot_id_string_generate() to
>>>>>> make sure it follows the rule in driver. If caller or user give a check
>>>>>> with snapshot_name_wellformed() before create snapshot, then the ID
>>>>>> and name will never conflict. The check can be also taken in
>>>>>> qcow2_snapshot_create(), but require it to return error reason.
>>>>>
>>>>> I'm not sure how useful this is. While we can restrict what IDs we allow
>>>>> for creating new snapshots, we cannot take any advantage from it because
>>>>> existing snapshots could already be named with only digits (they could
>>>>> also use a non-numeric ID). At the end of the day we're limiting the
>>>>    Qcow2's ID seems always numeric, do you mean sheepdog may have
>>>> non-numeric ID?
>>>
>>> That's not true, qcow2 has an ID string rather than a numeric ID.
>>>
>>    But there is no way to specify non-numeric ID in public interface,
>> such as savevm, qemu-img, and in qcow2_snapshot_create(),
>> find_new_snapshot_id() will always fill in a numeric ID string, so
>> result is that, qcow2's snapshot ID string is always a numeric string.
>
> Who said that qemu is the only program on the world that can create
> qcow2 snapshots? It might be, I don't know of any other, but qcow2 is
> defined by its spec, not by qemu's source code. And the spec talks about
> an ID string (just like the qemu code does, even though it happens to
> only write numbers into this string).
>
> Kevin
>
   OK, it make sense.


-- 
Best Regards

Wenchao Xia