[Qemu-devel] [PATCH V2 00/12] add internal snapshot support at block device level

[Qemu-devel] [PATCH V2 00/12] add internal snapshot support at block device level

[Wenchao Xia]

  This series brings internal snapshot support at block devices level, now we
have two main methods to do snapshot: 1) backing chain and 2) internal one, and
3) Stefan's unmerged backing up approach. Compared with 1), internal snapshot
is faster in R/W/Delete. Compared with 3), it complete faster, take less space
in total, but take more space in local image. As a summary, When user want to
take a local snapshot quickly, it is encouraged to use internal snapshot when
possible for performance reason. If user want to duplicate or backup vm to
another place, it is encouraged to use backing up approach.

  A full view may be: it can take snapshot by 1), 2), and the snapshot can
be exported to 3rd place. it can also directly export/backup data by 3).
export means transfering the data out of qemu/libvirt's scope, let 3rd party
take over.

host A                       backup server B
 vm         --------->
  |           backup
 \|/                     Data that can distinguish base/delta, disk/memory 
snapshot v0   export
snapshot v1 --------->

Next steps to better full VM snapshot:
  Add support to export internal snapshot data lively, possible use qemu-img
or qemu-nbd.
  Better vmstate saving.

Note:
  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. We can
have "internal snapshot (or backing chain) + standing alone vmstate file" as
replacement.  


Some details:
  To avoid trouble, this serial have hide ID in 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 4/11,
others are not quite related. Patch 4/11 will not set errp when no snapshot
find, since patch 7/11 need to distinguish real error case.

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

V2:
  Add test case for it.
  Address Kevin's comments:
  04/12: use assert(id || name), instead of abort().
  05/12: better snapshot_name_wellformed().
  06/12: rename parameter *name to *id_or_name in
find_snapshot_by_id_or_name(), add {} for old code, add a macro in
qemu-common.h to avoid print NULL char*.


  Address Fam Zheng's comments:
  06/12: better error message.
  08/12: better document and spell fix.
  10/12, 11/12: rename the command and related function to
snapshot_blkdev_internal.

Stefan Hajnoczi (3):
  1 blockdev: drop redundant proto_drv check
  2 blockdev: rename BlkTransactionStates to singular
  3 blockdev: allow BdrvActionOps->commit() to be NULL

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

 block/qcow2-snapshot.c     |   67 ++++++++---
 block/qcow2.h              |    5 +-
 block/rbd.c                |   19 +++-
 block/sheepdog.c           |    5 +-
 block/snapshot.c           |  130 ++++++++++++++++++++-
 blockdev.c                 |  278 +++++++++++++++++++++++++++++++++-----------
 hmp-commands.hx            |   43 +++++++-
 hmp.c                      |   20 +++
 hmp.h                      |    2 +
 include/block/block_int.h  |    5 +-
 include/block/snapshot.h   |   14 ++-
 include/qemu-common.h      |    3 +
 qapi-schema.json           |   58 +++++++++
 qemu-img.c                 |    5 +-
 qmp-commands.hx            |   93 ++++++++++++++-
 savevm.c                   |   10 ++-
 tests/qemu-iotests/055     |  157 +++++++++++++++++++++++++
 tests/qemu-iotests/055.out |    5 +
 tests/qemu-iotests/group   |    1 +
 19 files changed, 815 insertions(+), 105 deletions(-)
 create mode 100755 tests/qemu-iotests/055
 create mode 100644 tests/qemu-iotests/055.out

[Qemu-devel] [PATCH V2 01/12] blockdev: drop redundant proto_drv check

[Wenchao Xia]

From: Stefan Hajnoczi <stefanha@redhat.com>

It is not necessary to check that we can find a protocol block driver
since we create or open the image file.  This produces the error that we
need anyway.

Besides, the QERR_INVALID_BLOCK_FORMAT is inappropriate since the
protocol is incorrect rather than the format.

Also drop an empty line between bdrv_open() and checking its return
value.  This may be due to copy-pasting from earlier code that performed
other operations before handling errors.

Reported-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 blockdev.c |   15 ---------------
 1 files changed, 0 insertions(+), 15 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 9937311..e70225b 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -818,7 +818,6 @@ typedef struct ExternalSnapshotStates {
 static void external_snapshot_prepare(BlkTransactionStates *common,
                                       Error **errp)
 {
-    BlockDriver *proto_drv;
     BlockDriver *drv;
     int flags, ret;
     Error *local_err = NULL;
@@ -874,12 +873,6 @@ static void external_snapshot_prepare(BlkTransactionStates *common,
 
     flags = states->old_bs->open_flags;
 
-    proto_drv = bdrv_find_protocol(new_image_file);
-    if (!proto_drv) {
-        error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
-        return;
-    }
-
     /* create new image w/backing file */
     if (mode != NEW_IMAGE_MODE_EXISTING) {
         bdrv_img_create(new_image_file, format,
@@ -1372,7 +1365,6 @@ void qmp_drive_mirror(const char *device, const char *target,
 {
     BlockDriverState *bs;
     BlockDriverState *source, *target_bs;
-    BlockDriver *proto_drv;
     BlockDriver *drv = NULL;
     Error *local_err = NULL;
     int flags;
@@ -1440,12 +1432,6 @@ void qmp_drive_mirror(const char *device, const char *target,
         sync = MIRROR_SYNC_MODE_FULL;
     }
 
-    proto_drv = bdrv_find_protocol(target);
-    if (!proto_drv) {
-        error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
-        return;
-    }
-
     bdrv_get_geometry(bs, &size);
     size *= 512;
     if (sync == MIRROR_SYNC_MODE_FULL && mode != NEW_IMAGE_MODE_EXISTING) {
@@ -1480,7 +1466,6 @@ void qmp_drive_mirror(const char *device, const char *target,
      */
     target_bs = bdrv_new("");
     ret = bdrv_open(target_bs, target, NULL, flags | BDRV_O_NO_BACKING, drv);
-
     if (ret < 0) {
         bdrv_delete(target_bs);
         error_set(errp, QERR_OPEN_FILE_FAILED, target);
-- 
1.7.1

[Qemu-devel] [PATCH V2 02/12] blockdev: rename BlkTransactionStates to singular

[Wenchao Xia]

From: Stefan Hajnoczi <stefanha@redhat.com>

The QMP 'transaction' command keeps a list of in-flight transactions.
The transaction state structure is called BlkTransactionStates even
though it only deals with a single transaction.  The only plural thing
is the linked list of transaction states.

I find it confusing to call the single structure "States".  This patch
renames it to "State", just like BlockDriverState is singular.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 blockdev.c |  104 ++++++++++++++++++++++++++++++------------------------------
 1 files changed, 52 insertions(+), 52 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index e70225b..46a43b0 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -780,7 +780,7 @@ void qmp_blockdev_snapshot_sync(const char *device, const char *snapshot_file,
 
 /* New and old BlockDriverState structs for group snapshots */
 
-typedef struct BlkTransactionStates BlkTransactionStates;
+typedef struct BlkTransactionState BlkTransactionState;
 
 /* Only prepare() may fail. In a single transaction, only one of commit() or
    abort() will be called, clean() will always be called if it present. */
@@ -788,13 +788,13 @@ typedef struct BdrvActionOps {
     /* Size of state struct, in bytes. */
     size_t instance_size;
     /* Prepare the work, must NOT be NULL. */
-    void (*prepare)(BlkTransactionStates *common, Error **errp);
+    void (*prepare)(BlkTransactionState *common, Error **errp);
     /* Commit the changes, must NOT be NULL. */
-    void (*commit)(BlkTransactionStates *common);
+    void (*commit)(BlkTransactionState *common);
     /* Abort the changes on fail, can be NULL. */
-    void (*abort)(BlkTransactionStates *common);
+    void (*abort)(BlkTransactionState *common);
     /* Clean up resource in the end, can be NULL. */
-    void (*clean)(BlkTransactionStates *common);
+    void (*clean)(BlkTransactionState *common);
 } BdrvActionOps;
 
 /*
@@ -802,20 +802,20 @@ typedef struct BdrvActionOps {
  * that compiler will also arrange it to the same address with parent instance.
  * Later it will be used in free().
  */
-struct BlkTransactionStates {
+struct BlkTransactionState {
     TransactionAction *action;
     const BdrvActionOps *ops;
-    QSIMPLEQ_ENTRY(BlkTransactionStates) entry;
+    QSIMPLEQ_ENTRY(BlkTransactionState) entry;
 };
 
 /* external snapshot private data */
-typedef struct ExternalSnapshotStates {
-    BlkTransactionStates common;
+typedef struct ExternalSnapshotState {
+    BlkTransactionState common;
     BlockDriverState *old_bs;
     BlockDriverState *new_bs;
-} ExternalSnapshotStates;
+} ExternalSnapshotState;
 
-static void external_snapshot_prepare(BlkTransactionStates *common,
+static void external_snapshot_prepare(BlkTransactionState *common,
                                       Error **errp)
 {
     BlockDriver *drv;
@@ -825,8 +825,8 @@ static void external_snapshot_prepare(BlkTransactionStates *common,
     const char *new_image_file;
     const char *format = "qcow2";
     enum NewImageMode mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;
-    ExternalSnapshotStates *states =
-                             DO_UPCAST(ExternalSnapshotStates, common, common);
+    ExternalSnapshotState *state =
+                             DO_UPCAST(ExternalSnapshotState, common, common);
     TransactionAction *action = common->action;
 
     /* get parameters */
@@ -848,36 +848,36 @@ static void external_snapshot_prepare(BlkTransactionStates *common,
         return;
     }
 
-    states->old_bs = bdrv_find(device);
-    if (!states->old_bs) {
+    state->old_bs = bdrv_find(device);
+    if (!state->old_bs) {
         error_set(errp, QERR_DEVICE_NOT_FOUND, device);
         return;
     }
 
-    if (!bdrv_is_inserted(states->old_bs)) {
+    if (!bdrv_is_inserted(state->old_bs)) {
         error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
         return;
     }
 
-    if (bdrv_in_use(states->old_bs)) {
+    if (bdrv_in_use(state->old_bs)) {
         error_set(errp, QERR_DEVICE_IN_USE, device);
         return;
     }
 
-    if (!bdrv_is_read_only(states->old_bs)) {
-        if (bdrv_flush(states->old_bs)) {
+    if (!bdrv_is_read_only(state->old_bs)) {
+        if (bdrv_flush(state->old_bs)) {
             error_set(errp, QERR_IO_ERROR);
             return;
         }
     }
 
-    flags = states->old_bs->open_flags;
+    flags = state->old_bs->open_flags;
 
     /* create new image w/backing file */
     if (mode != NEW_IMAGE_MODE_EXISTING) {
         bdrv_img_create(new_image_file, format,
-                        states->old_bs->filename,
-                        states->old_bs->drv->format_name,
+                        state->old_bs->filename,
+                        state->old_bs->drv->format_name,
                         NULL, -1, flags, &local_err, false);
         if (error_is_set(&local_err)) {
             error_propagate(errp, local_err);
@@ -886,42 +886,42 @@ static void external_snapshot_prepare(BlkTransactionStates *common,
     }
 
     /* We will manually add the backing_hd field to the bs later */
-    states->new_bs = bdrv_new("");
+    state->new_bs = bdrv_new("");
     /* TODO Inherit bs->options or only take explicit options with an
      * extended QMP command? */
-    ret = bdrv_open(states->new_bs, new_image_file, NULL,
+    ret = bdrv_open(state->new_bs, new_image_file, NULL,
                     flags | BDRV_O_NO_BACKING, drv);
     if (ret != 0) {
         error_set(errp, QERR_OPEN_FILE_FAILED, new_image_file);
     }
 }
 
-static void external_snapshot_commit(BlkTransactionStates *common)
+static void external_snapshot_commit(BlkTransactionState *common)
 {
-    ExternalSnapshotStates *states =
-                             DO_UPCAST(ExternalSnapshotStates, common, common);
+    ExternalSnapshotState *state =
+                             DO_UPCAST(ExternalSnapshotState, common, common);
 
-    /* This removes our old bs from the bdrv_states, and adds the new bs */
-    bdrv_append(states->new_bs, states->old_bs);
+    /* This removes our old bs and adds the new bs */
+    bdrv_append(state->new_bs, state->old_bs);
     /* We don't need (or want) to use the transactional
      * bdrv_reopen_multiple() across all the entries at once, because we
      * don't want to abort all of them if one of them fails the reopen */
-    bdrv_reopen(states->new_bs, states->new_bs->open_flags & ~BDRV_O_RDWR,
+    bdrv_reopen(state->new_bs, state->new_bs->open_flags & ~BDRV_O_RDWR,
                 NULL);
 }
 
-static void external_snapshot_abort(BlkTransactionStates *common)
+static void external_snapshot_abort(BlkTransactionState *common)
 {
-    ExternalSnapshotStates *states =
-                             DO_UPCAST(ExternalSnapshotStates, common, common);
-    if (states->new_bs) {
-        bdrv_delete(states->new_bs);
+    ExternalSnapshotState *state =
+                             DO_UPCAST(ExternalSnapshotState, common, common);
+    if (state->new_bs) {
+        bdrv_delete(state->new_bs);
     }
 }
 
 static const BdrvActionOps actions[] = {
     [TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC] = {
-        .instance_size = sizeof(ExternalSnapshotStates),
+        .instance_size = sizeof(ExternalSnapshotState),
         .prepare  = external_snapshot_prepare,
         .commit   = external_snapshot_commit,
         .abort = external_snapshot_abort,
@@ -936,10 +936,10 @@ static const BdrvActionOps actions[] = {
 void qmp_transaction(TransactionActionList *dev_list, Error **errp)
 {
     TransactionActionList *dev_entry = dev_list;
-    BlkTransactionStates *states, *next;
+    BlkTransactionState *state, *next;
     Error *local_err = NULL;
 
-    QSIMPLEQ_HEAD(snap_bdrv_states, BlkTransactionStates) snap_bdrv_states;
+    QSIMPLEQ_HEAD(snap_bdrv_states, BlkTransactionState) snap_bdrv_states;
     QSIMPLEQ_INIT(&snap_bdrv_states);
 
     /* drain all i/o before any snapshots */
@@ -956,20 +956,20 @@ void qmp_transaction(TransactionActionList *dev_list, Error **errp)
         assert(dev_info->kind < ARRAY_SIZE(actions));
 
         ops = &actions[dev_info->kind];
-        states = g_malloc0(ops->instance_size);
-        states->ops = ops;
-        states->action = dev_info;
-        QSIMPLEQ_INSERT_TAIL(&snap_bdrv_states, states, entry);
+        state = g_malloc0(ops->instance_size);
+        state->ops = ops;
+        state->action = dev_info;
+        QSIMPLEQ_INSERT_TAIL(&snap_bdrv_states, state, entry);
 
-        states->ops->prepare(states, &local_err);
+        state->ops->prepare(state, &local_err);
         if (error_is_set(&local_err)) {
             error_propagate(errp, local_err);
             goto delete_and_fail;
         }
     }
 
-    QSIMPLEQ_FOREACH(states, &snap_bdrv_states, entry) {
-        states->ops->commit(states);
+    QSIMPLEQ_FOREACH(state, &snap_bdrv_states, entry) {
+        state->ops->commit(state);
     }
 
     /* success */
@@ -980,17 +980,17 @@ delete_and_fail:
     * failure, and it is all-or-none; abandon each new bs, and keep using
     * the original bs for all images
     */
-    QSIMPLEQ_FOREACH(states, &snap_bdrv_states, entry) {
-        if (states->ops->abort) {
-            states->ops->abort(states);
+    QSIMPLEQ_FOREACH(state, &snap_bdrv_states, entry) {
+        if (state->ops->abort) {
+            state->ops->abort(state);
         }
     }
 exit:
-    QSIMPLEQ_FOREACH_SAFE(states, &snap_bdrv_states, entry, next) {
-        if (states->ops->clean) {
-            states->ops->clean(states);
+    QSIMPLEQ_FOREACH_SAFE(state, &snap_bdrv_states, entry, next) {
+        if (state->ops->clean) {
+            state->ops->clean(state);
         }
-        g_free(states);
+        g_free(state);
     }
 }
 
-- 
1.7.1

[Qemu-devel] [PATCH V2 03/12] blockdev: allow BdrvActionOps->commit() to be NULL

[Wenchao Xia]

From: Stefan Hajnoczi <stefanha@redhat.com>

Some QMP 'transaction' types don't need to do anything on .commit().
Make .commit() optional just like .abort().

The "drive-backup" action will take advantage of this, it only needs to
cancel the block job on .abort().  Other block job actions will probably
follow the same pattern, so allow .commit() to be NULL.

Suggested-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 blockdev.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 46a43b0..4bd6cbc 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -789,7 +789,7 @@ typedef struct BdrvActionOps {
     size_t instance_size;
     /* Prepare the work, must NOT be NULL. */
     void (*prepare)(BlkTransactionState *common, Error **errp);
-    /* Commit the changes, must NOT be NULL. */
+    /* Commit the changes, can be NULL. */
     void (*commit)(BlkTransactionState *common);
     /* Abort the changes on fail, can be NULL. */
     void (*abort)(BlkTransactionState *common);
@@ -969,7 +969,9 @@ void qmp_transaction(TransactionActionList *dev_list, Error **errp)
     }
 
     QSIMPLEQ_FOREACH(state, &snap_bdrv_states, entry) {
-        state->ops->commit(state);
+        if (state->ops->commit) {
+            state->ops->commit(state);
+        }
     }
 
     /* success */
-- 
1.7.1

[Qemu-devel] [PATCH V2 04/12] 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 V2 05/12] 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 mess them up. 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 mess up. 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 mess up 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 992a5c8..7108d46 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -284,7 +284,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 V2 06/12] 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    |   65 ++++++++++++++++++++++++++++++++++-----------
 block/qcow2.h             |    5 +++-
 block/rbd.c               |   19 ++++++++++++-
 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, 131 insertions(+), 27 deletions(-)

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 7108d46..4aeb625 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -287,31 +287,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 */
@@ -333,7 +349,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;
     }
 
@@ -530,15 +546,22 @@ 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];
@@ -550,6 +573,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;
     }
 
@@ -567,6 +594,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));
@@ -574,6 +604,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 6959c6a..5e39f78 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -385,7 +385,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 0f2608b..c0ac1b0 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -903,12 +903,29 @@ 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;
 
+    if (snapshot_id && snapshot_id[0] != '\0') {
+        error_setg(errp, "rbd do not support snapshot id");
+        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");
+        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, bdrv_get_device_name(bs));
+    }
     return r;
 }
 
diff --git a/block/sheepdog.c b/block/sheepdog.c
index 21a4edf..972d59b 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -2092,7 +2092,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 ba52247..532acef 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -157,7 +157,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 ed8b6e2..a20fdc4 100644
--- a/include/qemu-common.h
+++ b/include/qemu-common.h
@@ -204,6 +204,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 809b4f1..b660b39 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 2ce439f..91c36df 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2289,7 +2289,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",
@@ -2519,7 +2522,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 V2 07/12] 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 mess up
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       |  118 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qapi-schema.json |   16 +++++++
 qmp-commands.hx  |   32 +++++++++++---
 3 files changed, 159 insertions(+), 7 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 4bd6cbc..aaeb0e8 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -808,6 +808,119 @@ 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;
+
+    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 mess up 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 exist 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;
+    /* not good to use vm_clock in block layer, but that is what we can do now,
+       or drop it later, since it is an emulater concept. */
+    sn1->vm_clock_nsec = qemu_get_clock_ns(vm_clock);
+
+    if (bdrv_snapshot_create(bs, sn1) < 0) {
+        error_setg(errp, "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;
@@ -926,6 +1039,11 @@ static const BdrvActionOps actions[] = {
         .commit   = external_snapshot_commit,
         .abort = external_snapshot_abort,
     },
+    [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 5ad6894..c53cb31 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1613,6 +1613,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 format support it, for example, qcow2, rbd, and sheepdog.
+#
+# Since: 1.6
+##
+{ 'type': 'BlockdevSnapshotInternal',
+  'data': { 'device': 'str', 'name': 'str' } }
 
 ##
 # @TransactionAction
@@ -1623,6 +1638,7 @@
 { 'union': 'TransactionAction',
   'data': {
        'blockdev-snapshot-sync': 'BlockdevSnapshot'
+       'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal'
    } }
 
 ##
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 8cea5e5..db5d4e3 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -948,13 +948,13 @@ 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.  The
+sequence of the requests will not affect the result.  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 snapshot, The dictionary is 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
@@ -963,6 +963,18 @@ 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 fail the original disks pre-snapshot attempt will be used.
+
+For internal snapshot, The dictionary is the device and the snapshot's name.
+If name is a numeric string which will mess up with ID, the request will be
+rejected.  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 format support it, for example, qcow2, rbd, and sheepdog.
+
+On fail, qemu will try delete new created internal snapshot in the
+transaction.  When I/O error make delete fail, user need to fix it later
+with qemu-img or other command.
+
 Arguments:
 
 actions array:
@@ -975,6 +987,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:
 
@@ -986,7 +1001,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

Re: [Qemu-devel] [PATCH V2 07/12] qmp: add internal snapshot support in qmp_transaction

[Eric Blake]

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

On 06/14/2013 12:39 PM, Wenchao Xia wrote:
> 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 mess up
> 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       |  118 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qapi-schema.json |   16 +++++++
>  qmp-commands.hx  |   32 +++++++++++---
>  3 files changed, 159 insertions(+), 7 deletions(-)

> +++ b/qapi-schema.json
> @@ -1613,6 +1613,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 format support it, for example, qcow2, rbd, and sheepdog.

s/format/formats/

> +#
> +# Since: 1.6
> +##
> +{ 'type': 'BlockdevSnapshotInternal',
> +  'data': { 'device': 'str', 'name': 'str' } }
>  
>  ##
>  # @TransactionAction
> @@ -1623,6 +1638,7 @@
>  { 'union': 'TransactionAction',
>    'data': {
>         'blockdev-snapshot-sync': 'BlockdevSnapshot'
> +       'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal'

Invalid JSON - you need a comma between name-value pairs. (I _wish_ JSON
had allowed trailing commas, the way C99 does, because it reduces the
size of diffs when adding an element at the end...)

Yet another instance of the introspection problem.  Is there some other
patch in this series that introduces a standalone command that witnesses
whether this transaction is available?  If so, I'm okay; if not, then
libvirt won't know if this transaction is available without
introspection, or by trying it first and detecting the error when it is
not available.  For the purposes of transactions, since any failure
rolls back the entire transaction (including the failure of an unknown
action within the transaction), the try-and-fail approach may be
sufficient, even if it is not the prettiest.

> +++ b/qmp-commands.hx
> @@ -948,13 +948,13 @@ 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.  The
> +sequence of the requests will not affect the result.

Not true.  Taking an internal and then an external snapshot has a
different effect than taking an external and then internal snapshot of
the same disk.  The sequence of requests is significant, because it
controls which of two files involved contains the internal snapshot.

>  
> -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 snapshot, The dictionary is the device, the file to use for the

s/snapshot, The/snapshots, the/
s/dictionary is/dictionary contains/

> +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
> @@ -963,6 +963,18 @@ 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 fail the original disks pre-snapshot attempt will be used.
> +
> +For internal snapshot, The dictionary is the device and the snapshot's name.

s/snapshot, The/snapsohts, the/
s/dictionary is/dictionary contains/

> +If name is a numeric string which will mess up with ID, the request will be
> +rejected.  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 format support it, for example, qcow2, rbd, and sheepdog.
> +
> +On fail, qemu will try delete new created internal snapshot in the

s/fail/failure/

why "try"?  The point of a transaction is that it either completely
happens or is completely aborted.  If you are leaving behind garbage
after a failure later in the transaction, then you didn't design the
transaction correctly.  (And yes, we have an existing bug where external
snapshots that get aborted after creating an empty destination file are
leaving behind garbage, but that's pre-existing and unrelated to your
feature addition.)

> +transaction.  When I/O error make delete fail, user need to fix it later

s/make delete fail/causes deletion failure/
s/user need/the user needs/

-- 
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 V2 07/12] qmp: add internal snapshot support in qmp_transaction

[Wenchao Xia]

   Will fix the grammar, thanks for checking.


> On 06/14/2013 12:39 PM, Wenchao Xia wrote:
>> 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 mess up
>> 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       |  118 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   qapi-schema.json |   16 +++++++
>>   qmp-commands.hx  |   32 +++++++++++---
>>   3 files changed, 159 insertions(+), 7 deletions(-)
>
>> +++ b/qapi-schema.json
>> @@ -1613,6 +1613,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 format support it, for example, qcow2, rbd, and sheepdog.
>
> s/format/formats/
>
>> +#
>> +# Since: 1.6
>> +##
>> +{ 'type': 'BlockdevSnapshotInternal',
>> +  'data': { 'device': 'str', 'name': 'str' } }
>>
>>   ##
>>   # @TransactionAction
>> @@ -1623,6 +1638,7 @@
>>   { 'union': 'TransactionAction',
>>     'data': {
>>          'blockdev-snapshot-sync': 'BlockdevSnapshot'
>> +       'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal'
>
> Invalid JSON - you need a comma between name-value pairs. (I _wish_ JSON
> had allowed trailing commas, the way C99 does, because it reduces the
> size of diffs when adding an element at the end...)
   Will fix it. By the way, why the build/run can succeed without comma
in my test....

>
> Yet another instance of the introspection problem.  Is there some other
> patch in this series that introduces a standalone command that witnesses
> whether this transaction is available?  If so, I'm okay; if not, then
   You mean a command detecting whether
'blockdev-snapshot-internal-sync' is available? No I haven't introduce
it, a standalone command for 'TransactionAction' type detection seems
not so wisely, maybe integration it in qapi capability query.

> libvirt won't know if this transaction is available without
> introspection, or by trying it first and detecting the error when it is
> not available.  For the purposes of transactions, since any failure
> rolls back the entire transaction (including the failure of an unknown
> action within the transaction), the try-and-fail approach may be
> sufficient, even if it is not the prettiest.
>
>> +++ b/qmp-commands.hx
>> @@ -948,13 +948,13 @@ 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.  The
>> +sequence of the requests will not affect the result.
>
> Not true.  Taking an internal and then an external snapshot has a
> different effect than taking an external and then internal snapshot of
> the same disk.  The sequence of requests is significant, because it
   In external case, the device changes its active image in commit()
stage, so it will not affect internal snapshot. For example:

Taking an internal snapshot sn0 on ide-hd0 with image0.qcow2, then
an external snapshot to create image1.qcow2, result in:
image0.qcow2(sn0)-->image1.qcow2.

Taking an external snapshot to create image1.qcow2 on ide-hd0 with 
image0.qcow2, then an external an internal snapshot sn0 on ide-hd0:
image0.qcow2(sn0)-->image1.qcow2.

   However, the comments reminds me that it maybe different to
take two snapshots sn0, sn1 on same device with different sequence:
the name looks the same, but the actually L1 table contents may
change according to the sequence, although invisible to user.
Maybe I should remove this line in document.

> controls which of two files involved contains the internal snapshot.
>
>>
>> -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 snapshot, The dictionary is the device, the file to use for the
>
> s/snapshot, The/snapshots, the/
> s/dictionary is/dictionary contains/
>
>> +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
>> @@ -963,6 +963,18 @@ 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 fail the original disks pre-snapshot attempt will be used.
>> +
>> +For internal snapshot, The dictionary is the device and the snapshot's name.
>
> s/snapshot, The/snapsohts, the/
> s/dictionary is/dictionary contains/
>
>> +If name is a numeric string which will mess up with ID, the request will be
>> +rejected.  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 format support it, for example, qcow2, rbd, and sheepdog.
>> +
>> +On fail, qemu will try delete new created internal snapshot in the
>
> s/fail/failure/
>
> why "try"?  The point of a transaction is that it either completely
> happens or is completely aborted.  If you are leaving behind garbage
> after a failure later in the transaction, then you didn't design the
> transaction correctly.  (And yes, we have an existing bug where external
> snapshots that get aborted after creating an empty destination file are
> leaving behind garbage, but that's pre-existing and unrelated to your
> feature addition.)
>
   I can't find a perfect way to rollback disks, since it may I/O error.
The same thing happens to external case, try delete seems the best way.
Consider breaking qcow_snapshot_create() into 3 stage, there would not
be much improvement for same reason. The best thing I can see is make
sure it is fixable by qemu-img.

>> +transaction.  When I/O error make delete fail, user need to fix it later
>
> s/make delete fail/causes deletion failure/
> s/user need/the user needs/
>


-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH V2 07/12] qmp: add internal snapshot support in qmp_transaction

[Stefan Hajnoczi]

On Fri, Jun 14, 2013 at 07:39:54PM +0800, Wenchao Xia wrote:
> +    /* 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",

Please use '%s' instead of just %s for arguments in error messages.
This will make the message easier to understand, especially when name is
empty.  It's also consistent with the rest of blockdev.c.

> +                   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;
> +    /* not good to use vm_clock in block layer, but that is what we can do now,
> +       or drop it later, since it is an emulater concept. */

It's appropriate here and blockdev.c is emulator code, so you can drop
the comment.

[Qemu-devel] [PATCH V2 08/12] qmp: add interface blockdev-snapshot-internal-sync

[Wenchao Xia]

This interface can generate snapshot name automatically if it is not
specified, since it is a single opertion.

Snapshot ID can't be specified in this interface.

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

diff --git a/blockdev.c b/blockdev.c
index aaeb0e8..6a952cd 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -777,6 +777,31 @@ void qmp_blockdev_snapshot_sync(const char *device, const char *snapshot_file,
                        &snapshot, errp);
 }
 
+void qmp_blockdev_snapshot_internal_sync(const char *device,
+                                         bool has_name, const char *name,
+                                         Error **errp)
+{
+    qemu_timeval tv;
+    struct tm tm;
+    char name1[128];
+
+    BlockdevSnapshotInternal snapshot = {
+        .device = (char *) device,
+    };
+
+    if (has_name) {
+        snapshot.name = (char *) name;
+    } else {
+        qemu_gettimeofday(&tv);
+        localtime_r((const time_t *)&tv.tv_sec, &tm);
+        strftime(name1, sizeof(name1), "vm-%Y%m%d%H%M%S", &tm);
+        snapshot.name = name1;
+    }
+
+    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 c53cb31..749a349 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1689,6 +1689,29 @@
             '*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: #optional the new snapshot name. If not specified, a name will be
+# generated according to time by qemu
+#
+# Returns: nothing on success
+#          If @device is not a valid block device, DeviceNotFound
+#          If any snapshot matching @name exists, or the name is a numeric
+#          which may mess up with snapshot ID, generic error will be returned
+#          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 db5d4e3..3d5f996 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1044,6 +1044,37 @@ 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 name is not specified, it will be automatically
+generated by qemu according to host time.  If the name is a numeric string
+which may mess up with ID, such as "19", 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, optional)
+
+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

Re: [Qemu-devel] [PATCH V2 08/12] qmp: add interface blockdev-snapshot-internal-sync

[Eric Blake]

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

On 06/14/2013 12:39 PM, Wenchao Xia wrote:
> This interface can generate snapshot name automatically if it is not
> specified, since it is a single opertion.

s/opertion/operation/

> 
> Snapshot ID can't be specified in this interface.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  blockdev.c       |   25 +++++++++++++++++++++++++
>  qapi-schema.json |   23 +++++++++++++++++++++++
>  qmp-commands.hx  |   31 +++++++++++++++++++++++++++++++
>  3 files changed, 79 insertions(+), 0 deletions(-)
> 

> +++ b/qapi-schema.json
> @@ -1689,6 +1689,29 @@
>              '*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: #optional the new snapshot name. If not specified, a name will be
> +# generated according to time by qemu

So why is name optional here but mandatory within a transaction?  If
qemu is able to generate names, then it should be able to generate names
in both cases.  Otherwise, make the name mandatory in both places.

Should this patch be folded in to 7/12?  Compare with Stefan's series on
adding block-snapshot as a transaction (for that matter, the two series
have a [trivial] merge conflict since both add a transaction), and make
sure you are using the same approach between the two series at
introducing things.

> +#
> +# Returns: nothing on success
> +#          If @device is not a valid block device, DeviceNotFound
> +#          If any snapshot matching @name exists, or the name is a numeric
> +#          which may mess up with snapshot ID, generic error will be returned
> +#          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'} }

Ah, so this answers my question in 7/12 about a witness that libvirt can
use for knowing when transaction supports the new action, without
needing introspection.


> +
> +SQMP
> +blockdev-snapshot-internal-sync
> +-------------------------------
> +
> +Synchronously take an internal snapshot of a block device when the format of
> +image used supports it.  If name is not specified, it will be automatically
> +generated by qemu according to host time.  If the name is a numeric string
> +which may mess up with ID, such as "19", the operation will fail.

Wait a second.  If we DON'T pass a name, then the generated name is all
numeric.  But if we DO pass an all-numeric name, it gets rejected.
That's awkward to explain.  Maybe you want to instead have a cutoff,
where a number < 64k (is that the right threshold? I just picked a
number out of the air) is rejected, but a number >= 64k is treated as
valid because it might represent a timestamp.  Or allow all possible
numbers, and only reject the creation of a name that collides with an
existing id.  It may be too hard to predict if a name will collide with
a future id of a later snapshot operation.


> +Example:
> +
> +-> { "execute": "blockdev-snapshot-internal-sync",
> +                "arguments": { "device": "ide-hd0",
> +                               "name": "snapshot0" }
> +   }
> +<- { "return": {} }

Evil.  If I don't pass a name, then I NEED to know what name got
generated on my behalf.  So that argues you need to return something,
rather than nothing.  I can see why you can't return a string via
'transaction', but maybe this is an argument that 'name' should be
mandatory in this QMP command (and any generation of a timestamp id must
be higher up in the stack, at the HMP level, so that HMP can still treat
name as optional).  But then you are back to solving the problem of
allowing an all-numeric generated timestamp as a valid name.

-- 
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 V2 08/12] qmp: add interface blockdev-snapshot-internal-sync

[Wenchao Xia]

于 2013-6-15 17:51, Eric Blake 写道:
> On 06/14/2013 12:39 PM, Wenchao Xia wrote:
>> This interface can generate snapshot name automatically if it is not
>> specified, since it is a single opertion.
>
> s/opertion/operation/
>
>>
>> Snapshot ID can't be specified in this interface.
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>>   blockdev.c       |   25 +++++++++++++++++++++++++
>>   qapi-schema.json |   23 +++++++++++++++++++++++
>>   qmp-commands.hx  |   31 +++++++++++++++++++++++++++++++
>>   3 files changed, 79 insertions(+), 0 deletions(-)
>>
>
>> +++ b/qapi-schema.json
>> @@ -1689,6 +1689,29 @@
>>               '*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: #optional the new snapshot name. If not specified, a name will be
>> +# generated according to time by qemu
>
> So why is name optional here but mandatory within a transaction?  If
> qemu is able to generate names, then it should be able to generate names
> in both cases.  Otherwise, make the name mandatory in both places.
   A bit different: transaction take multiple requests, if some thing
is generated it should return the info, but it didn't, so forbid it.
But for this action, it is single, make caller possible to get accurate
info by a info query. I'll make name mandatory in both case, it seems
better.


>
> Should this patch be folded in to 7/12?  Compare with Stefan's series on
   I think the two patch can distinguish each other: one is doing the job
in "batch" mode, one is doing in "single" mode, and make each patch
smaller. After review, I am OK to squash them.

> adding block-snapshot as a transaction (for that matter, the two series
> have a [trivial] merge conflict since both add a transaction), and make
> sure you are using the same approach between the two series at
> introducing things.
   I am OK to rebase if Stefan's patch upstream first.

>
>> +#
>> +# Returns: nothing on success
>> +#          If @device is not a valid block device, DeviceNotFound
>> +#          If any snapshot matching @name exists, or the name is a numeric
>> +#          which may mess up with snapshot ID, generic error will be returned
>> +#          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'} }
>
> Ah, so this answers my question in 7/12 about a witness that libvirt can
> use for knowing when transaction supports the new action, without
> needing introspection.
>
>
>> +
>> +SQMP
>> +blockdev-snapshot-internal-sync
>> +-------------------------------
>> +
>> +Synchronously take an internal snapshot of a block device when the format of
>> +image used supports it.  If name is not specified, it will be automatically
>> +generated by qemu according to host time.  If the name is a numeric string
>> +which may mess up with ID, such as "19", the operation will fail.
>
> Wait a second.  If we DON'T pass a name, then the generated name is all
   Nop, it starts with "vm", like "vm-20130608141726". But I'll make
parameter name mandatory in next version.

> numeric.  But if we DO pass an all-numeric name, it gets rejected.
> That's awkward to explain.  Maybe you want to instead have a cutoff,
> where a number < 64k (is that the right threshold? I just picked a
> number out of the air) is rejected, but a number >= 64k is treated as
> valid because it might represent a timestamp.  Or allow all possible
> numbers, and only reject the creation of a name that collides with an
> existing id.  It may be too hard to predict if a name will collide with
> a future id of a later snapshot operation.
>
>
>> +Example:
>> +
>> +-> { "execute": "blockdev-snapshot-internal-sync",
>> +                "arguments": { "device": "ide-hd0",
>> +                               "name": "snapshot0" }
>> +   }
>> +<- { "return": {} }
>
> Evil.  If I don't pass a name, then I NEED to know what name got
> generated on my behalf.  So that argues you need to return something,
> rather than nothing.  I can see why you can't return a string via
> 'transaction', but maybe this is an argument that 'name' should be
> mandatory in this QMP command (and any generation of a timestamp id must
> be higher up in the stack, at the HMP level, so that HMP can still treat
> name as optional).  But then you are back to solving the problem of
> allowing an all-numeric generated timestamp as a valid name.
>


-- 
Best Regards

Wenchao Xia

[Qemu-devel] [PATCH V2 09/12] qmp: add interface blockdev-snapshot-delete-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       |   12 ++++++++++++
 qapi-schema.json |   19 +++++++++++++++++++
 qmp-commands.hx  |   30 ++++++++++++++++++++++++++++++
 3 files changed, 61 insertions(+), 0 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 6a952cd..0277482 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -802,6 +802,18 @@ void qmp_blockdev_snapshot_internal_sync(const char *device,
                        &snapshot, errp);
 }
 
+void qmp_blockdev_snapshot_delete_internal_sync(const char *device,
+                                                const char *name,
+                                                Error **errp)
+{
+    BlockDriverState *bs = bdrv_find(device);
+    if (!bs) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+        return;
+    };
+
+    bdrv_snapshot_delete(bs, NULL, name, errp);
+}
 
 /* New and old BlockDriverState structs for group snapshots */
 
diff --git a/qapi-schema.json b/qapi-schema.json
index 749a349..b5e4f87 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1712,6 +1712,25 @@
   '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.
+#
+# @device: the name of the device to delete the snapshot from
+#
+# @name: the snapshot's name to be deleted
+#
+# Returns: nothing on success
+#          If @device is not a valid block device, DeviceNotFound
+#          If snapshot not found, generic error will be returned
+#
+# Since 1.6
+##
+{ 'command': 'blockdev-snapshot-delete-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 3d5f996..904950f 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1075,6 +1075,36 @@ Example:
 EQMP
 
     {
+        .name       = "blockdev-snapshot-delete-internal-sync",
+        .args_type  = "device:B,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 support it.  If the snapshot matching name is not found, operation
+will fail.
+
+Arguments:
+
+- "device": device name (json-string)
+- "name": name of the snapshot (json-string)
+
+Example:
+
+-> { "execute": "blockdev-snapshot-delete-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

Re: [Qemu-devel] [PATCH V2 09/12] qmp: add interface blockdev-snapshot-delete-internal-sync

[Eric Blake]

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

On 06/14/2013 12:39 PM, Wenchao Xia wrote:
> Snapshot ID can't be specified in this interface.

Why not?  If it is possible to look up by id in isolation, then it
should be possible to delete by id in isolation.  Also, if it is
possible to create a snapshot that has an id but no name (and I think
qcow2 allows such snapshots, even if we don't want to expose that corner
case of creation via normal QMP), then it is mandatory to have a way to
delete by id, since there is no name to delete by.

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

> +++ b/qapi-schema.json
> @@ -1712,6 +1712,25 @@
>    '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.

Should this command be made available via 'transaction'?  That is, if I
have a two-disk VM, and use 'transaction' to take a snapshot of both
disks at once, shouldn't I also have a way to delete the snapshots of
both at once, or gracefully fail without data loss if the second one has
problems?

> +#
> +# @device: the name of the device to delete the snapshot from
> +#
> +# @name: the snapshot's name to be deleted
> +#
> +# Returns: nothing on success
> +#          If @device is not a valid block device, DeviceNotFound
> +#          If snapshot not found, generic error will be returned
> +#
> +# Since 1.6
> +##
> +{ 'command': 'blockdev-snapshot-delete-internal-sync',
> +  'data': { 'device': 'str', 'name': 'str'} }

I'm not convinced that this interface is powerful enough.

-- 
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 V2 09/12] qmp: add interface blockdev-snapshot-delete-internal-sync

[Wenchao Xia]

于 2013-6-15 17:55, Eric Blake 写道:
> On 06/14/2013 12:39 PM, Wenchao Xia wrote:
>> Snapshot ID can't be specified in this interface.
>
> Why not?  If it is possible to look up by id in isolation, then it
> should be possible to delete by id in isolation.  Also, if it is
> possible to create a snapshot that has an id but no name (and I think
> qcow2 allows such snapshots, even if we don't want to expose that corner
> case of creation via normal QMP), then it is mandatory to have a way to
> delete by id, since there is no name to delete by.
>
   OK, qemu-img allows create snapshot with name="" before, so we need
take care for that corener case in this interface. Here is the redesign:

##
# @blockdev-snapshot-delete-internal-sync
#
# Synchronously delete an internal snapshot of a block device, when the 
ormat
# of the image used support it.
#
# @device: the name of the device to delete the snapshot from
#
# @name: the snapshot's name to be deleted
#
# @id: optional the snapshot's ID to be deleted. If specified, qemu will
#      try delete the snapshot matching both @name and @id.
#
# Returns: nothing on success
#          If @device is not a valid block device, DeviceNotFound
#          If snapshot not found, generic error will be returned
#
# Since 1.6
##
{ 'command': 'blockdev-snapshot-delete-internal-sync',
   'data': { 'device': 'str', 'name': 'str', '*id': 'str' } }

   Then caller specify name='' and id=[ID] to handle the corner case.


>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>>   blockdev.c       |   12 ++++++++++++
>>   qapi-schema.json |   19 +++++++++++++++++++
>>   qmp-commands.hx  |   30 ++++++++++++++++++++++++++++++
>>   3 files changed, 61 insertions(+), 0 deletions(-)
>>
>
>> +++ b/qapi-schema.json
>> @@ -1712,6 +1712,25 @@
>>     '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.
>
> Should this command be made available via 'transaction'?  That is, if I
> have a two-disk VM, and use 'transaction' to take a snapshot of both
> disks at once, shouldn't I also have a way to delete the snapshots of
> both at once, or gracefully fail without data loss if the second one has
> problems?

   I think adding it in transaction is not very useful but brings more
complexity. Transcation is used to guareentee all operations are taken
in one time point, for example, snapshot creation use it to make sure
all are consistent to VM. But for deletion, this requirement do not
exist.


>
>> +#
>> +# @device: the name of the device to delete the snapshot from
>> +#
>> +# @name: the snapshot's name to be deleted
>> +#
>> +# Returns: nothing on success
>> +#          If @device is not a valid block device, DeviceNotFound
>> +#          If snapshot not found, generic error will be returned
>> +#
>> +# Since 1.6
>> +##
>> +{ 'command': 'blockdev-snapshot-delete-internal-sync',
>> +  'data': { 'device': 'str', 'name': 'str'} }
>
> I'm not convinced that this interface is powerful enough.
>


-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH V2 09/12] qmp: add interface blockdev-snapshot-delete-internal-sync

[Stefan Hajnoczi]

On Mon, Jun 17, 2013 at 11:25:26AM +0800, Wenchao Xia wrote:
> 于 2013-6-15 17:55, Eric Blake 写道:
> >Should this command be made available via 'transaction'?  That is, if I
> >have a two-disk VM, and use 'transaction' to take a snapshot of both
> >disks at once, shouldn't I also have a way to delete the snapshots of
> >both at once, or gracefully fail without data loss if the second one has
> >problems?
> 
>   I think adding it in transaction is not very useful but brings more
> complexity. Transcation is used to guareentee all operations are taken
> in one time point, for example, snapshot creation use it to make sure
> all are consistent to VM. But for deletion, this requirement do not
> exist.

I guess the problem is: can we make internal snapshot deletion
transactional?  It's hard to do rollback for snapshot deletion.

But batching is definitely useful for doing 'delvm' in QMP.  I just
don't think transactions help.  We just need a 'delvm' equivalent in
QMP.

Stefan

Re: [Qemu-devel] [PATCH V2 09/12] qmp: add interface blockdev-snapshot-delete-internal-sync

[Wenchao Xia]

于 2013-6-18 22:20, Stefan Hajnoczi 写道:
> On Mon, Jun 17, 2013 at 11:25:26AM +0800, Wenchao Xia wrote:
>> 于 2013-6-15 17:55, Eric Blake 写道:
>>> Should this command be made available via 'transaction'?  That is, if I
>>> have a two-disk VM, and use 'transaction' to take a snapshot of both
>>> disks at once, shouldn't I also have a way to delete the snapshots of
>>> both at once, or gracefully fail without data loss if the second one has
>>> problems?
>>
>>    I think adding it in transaction is not very useful but brings more
>> complexity. Transcation is used to guareentee all operations are taken
>> in one time point, for example, snapshot creation use it to make sure
>> all are consistent to VM. But for deletion, this requirement do not
>> exist.
>
> I guess the problem is: can we make internal snapshot deletion
> transactional?  It's hard to do rollback for snapshot deletion.
>
   Several deletion in transaction equals to several calls of
'blockdev-snapshot-delete-internal-sync', unlike creation, so I hope
not add it which have rollback issue.


> But batching is definitely useful for doing 'delvm' in QMP.  I just
> don't think transactions help.  We just need a 'delvm' equivalent in
> QMP.
>
   Maybe the caller can encapsulate a batch interface at its level.

> Stefan
>


-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH V2 09/12] qmp: add interface blockdev-snapshot-delete-internal-sync

[Stefan Hajnoczi]

On Wed, Jun 19, 2013 at 02:18:48PM +0800, Wenchao Xia wrote:
> 于 2013-6-18 22:20, Stefan Hajnoczi 写道:
> >On Mon, Jun 17, 2013 at 11:25:26AM +0800, Wenchao Xia wrote:
> >>于 2013-6-15 17:55, Eric Blake 写道:
> >>>Should this command be made available via 'transaction'?  That is, if I
> >>>have a two-disk VM, and use 'transaction' to take a snapshot of both
> >>>disks at once, shouldn't I also have a way to delete the snapshots of
> >>>both at once, or gracefully fail without data loss if the second one has
> >>>problems?
> >>
> >>   I think adding it in transaction is not very useful but brings more
> >>complexity. Transcation is used to guareentee all operations are taken
> >>in one time point, for example, snapshot creation use it to make sure
> >>all are consistent to VM. But for deletion, this requirement do not
> >>exist.
> >
> >I guess the problem is: can we make internal snapshot deletion
> >transactional?  It's hard to do rollback for snapshot deletion.
> >
>   Several deletion in transaction equals to several calls of
> 'blockdev-snapshot-delete-internal-sync', unlike creation, so I hope
> not add it which have rollback issue.
> 
> 
> >But batching is definitely useful for doing 'delvm' in QMP.  I just
> >don't think transactions help.  We just need a 'delvm' equivalent in
> >QMP.
> >
>   Maybe the caller can encapsulate a batch interface at its level.

'delvm' is a batch interface - it deletes internal snapshots that have
the same name across multiple devices.

It's not as flexible as:
blkdev-internal-snapshot-delete drive0 drive2 drive4

Because that would allow you to specify specific drives.

Stefan

Re: [Qemu-devel] [PATCH V2 09/12] qmp: add interface blockdev-snapshot-delete-internal-sync

[Wenchao Xia]

于 2013-6-19 15:46, Stefan Hajnoczi 写道:
> On Wed, Jun 19, 2013 at 02:18:48PM +0800, Wenchao Xia wrote:
>> 于 2013-6-18 22:20, Stefan Hajnoczi 写道:
>>> On Mon, Jun 17, 2013 at 11:25:26AM +0800, Wenchao Xia wrote:
>>>> 于 2013-6-15 17:55, Eric Blake 写道:
>>>>> Should this command be made available via 'transaction'?  That is, if I
>>>>> have a two-disk VM, and use 'transaction' to take a snapshot of both
>>>>> disks at once, shouldn't I also have a way to delete the snapshots of
>>>>> both at once, or gracefully fail without data loss if the second one has
>>>>> problems?
>>>>
>>>>    I think adding it in transaction is not very useful but brings more
>>>> complexity. Transcation is used to guareentee all operations are taken
>>>> in one time point, for example, snapshot creation use it to make sure
>>>> all are consistent to VM. But for deletion, this requirement do not
>>>> exist.
>>>
>>> I guess the problem is: can we make internal snapshot deletion
>>> transactional?  It's hard to do rollback for snapshot deletion.
>>>
>>    Several deletion in transaction equals to several calls of
>> 'blockdev-snapshot-delete-internal-sync', unlike creation, so I hope
>> not add it which have rollback issue.
>>
>>
>>> But batching is definitely useful for doing 'delvm' in QMP.  I just
>>> don't think transactions help.  We just need a 'delvm' equivalent in
>>> QMP.
>>>
>>    Maybe the caller can encapsulate a batch interface at its level.
>
> 'delvm' is a batch interface - it deletes internal snapshots that have
> the same name across multiple devices.
>
> It's not as flexible as:
> blkdev-internal-snapshot-delete drive0 drive2 drive4
>
> Because that would allow you to specify specific drives.
>
   Do you mean this interface should be changed as
blkdev-internal-snapshot-delete devices_array name *id?

   It seems not much difference with following methods:

for device in device_list:
   blkdev-internal-snapshot-delete device name


> Stefan
>


-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH V2 09/12] qmp: add interface blockdev-snapshot-delete-internal-sync

[Stefan Hajnoczi]

On Wed, Jun 19, 2013 at 04:53:38PM +0800, Wenchao Xia wrote:
> 于 2013-6-19 15:46, Stefan Hajnoczi 写道:
> >On Wed, Jun 19, 2013 at 02:18:48PM +0800, Wenchao Xia wrote:
> >>于 2013-6-18 22:20, Stefan Hajnoczi 写道:
> >>>On Mon, Jun 17, 2013 at 11:25:26AM +0800, Wenchao Xia wrote:
> >>>>于 2013-6-15 17:55, Eric Blake 写道:
> >>>>>Should this command be made available via 'transaction'?  That is, if I
> >>>>>have a two-disk VM, and use 'transaction' to take a snapshot of both
> >>>>>disks at once, shouldn't I also have a way to delete the snapshots of
> >>>>>both at once, or gracefully fail without data loss if the second one has
> >>>>>problems?
> >>>>
> >>>>   I think adding it in transaction is not very useful but brings more
> >>>>complexity. Transcation is used to guareentee all operations are taken
> >>>>in one time point, for example, snapshot creation use it to make sure
> >>>>all are consistent to VM. But for deletion, this requirement do not
> >>>>exist.
> >>>
> >>>I guess the problem is: can we make internal snapshot deletion
> >>>transactional?  It's hard to do rollback for snapshot deletion.
> >>>
> >>   Several deletion in transaction equals to several calls of
> >>'blockdev-snapshot-delete-internal-sync', unlike creation, so I hope
> >>not add it which have rollback issue.
> >>
> >>
> >>>But batching is definitely useful for doing 'delvm' in QMP.  I just
> >>>don't think transactions help.  We just need a 'delvm' equivalent in
> >>>QMP.
> >>>
> >>   Maybe the caller can encapsulate a batch interface at its level.
> >
> >'delvm' is a batch interface - it deletes internal snapshots that have
> >the same name across multiple devices.
> >
> >It's not as flexible as:
> >blkdev-internal-snapshot-delete drive0 drive2 drive4
> >
> >Because that would allow you to specify specific drives.
> >
>   Do you mean this interface should be changed as
> blkdev-internal-snapshot-delete devices_array name *id?
> 
>   It seems not much difference with following methods:
> 
> for device in device_list:
>   blkdev-internal-snapshot-delete device name

The ability to batch in a single QMP command feels a little nicer.
There are less opportunities for the operation to stop half-way through.

It would be usable as a QMP 'delvm' and in a more flexible way to delete
multiple internal snapshots.

Would be interesting to see what Eric Blake thinks from a libvirt
perspective.

Stefan

Re: [Qemu-devel] [PATCH V2 09/12] qmp: add interface blockdev-snapshot-delete-internal-sync

[Wenchao Xia]

于 2013-6-19 22:24, Stefan Hajnoczi 写道:
> On Wed, Jun 19, 2013 at 04:53:38PM +0800, Wenchao Xia wrote:
>> 于 2013-6-19 15:46, Stefan Hajnoczi 写道:
>>> On Wed, Jun 19, 2013 at 02:18:48PM +0800, Wenchao Xia wrote:
>>>> 于 2013-6-18 22:20, Stefan Hajnoczi 写道:
>>>>> On Mon, Jun 17, 2013 at 11:25:26AM +0800, Wenchao Xia wrote:
>>>>>> 于 2013-6-15 17:55, Eric Blake 写道:
>>>>>>> Should this command be made available via 'transaction'?  That is, if I
>>>>>>> have a two-disk VM, and use 'transaction' to take a snapshot of both
>>>>>>> disks at once, shouldn't I also have a way to delete the snapshots of
>>>>>>> both at once, or gracefully fail without data loss if the second one has
>>>>>>> problems?
>>>>>>
>>>>>>    I think adding it in transaction is not very useful but brings more
>>>>>> complexity. Transcation is used to guareentee all operations are taken
>>>>>> in one time point, for example, snapshot creation use it to make sure
>>>>>> all are consistent to VM. But for deletion, this requirement do not
>>>>>> exist.
>>>>>
>>>>> I guess the problem is: can we make internal snapshot deletion
>>>>> transactional?  It's hard to do rollback for snapshot deletion.
>>>>>
>>>>    Several deletion in transaction equals to several calls of
>>>> 'blockdev-snapshot-delete-internal-sync', unlike creation, so I hope
>>>> not add it which have rollback issue.
>>>>
>>>>
>>>>> But batching is definitely useful for doing 'delvm' in QMP.  I just
>>>>> don't think transactions help.  We just need a 'delvm' equivalent in
>>>>> QMP.
>>>>>
>>>>    Maybe the caller can encapsulate a batch interface at its level.
>>>
>>> 'delvm' is a batch interface - it deletes internal snapshots that have
>>> the same name across multiple devices.
>>>
>>> It's not as flexible as:
>>> blkdev-internal-snapshot-delete drive0 drive2 drive4
>>>
>>> Because that would allow you to specify specific drives.
>>>
>>    Do you mean this interface should be changed as
>> blkdev-internal-snapshot-delete devices_array name *id?
>>
>>    It seems not much difference with following methods:
>>
>> for device in device_list:
>>    blkdev-internal-snapshot-delete device name
>
> The ability to batch in a single QMP command feels a little nicer.
> There are less opportunities for the operation to stop half-way through.
>
> It would be usable as a QMP 'delvm' and in a more flexible way to delete
> multiple internal snapshots.
>
> Would be interesting to see what Eric Blake thinks from a libvirt
> perspective.
>
> Stefan
>
   Just want to mention: other interfaces for block device take single
device as parameter now, use array here would be a little strange, and
increase the complexity in qmp and hmp interface. My opinion is
let caller do:

for device in device_list:
     blkdev-internal-snapshot-delete device name

Still, hope to know Eric's opinion.

-- 
Best Regards

Wenchao Xia

[Qemu-devel] [PATCH V2 10/12] hmp: add interface hmp_snapshot_blkdev_internal

[Wenchao Xia]

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

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 396691a..312a0c7 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1025,8 +1025,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.",
@@ -1040,6 +1039,27 @@ 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"
+                      "Name is the snapshot's name, if not specified\n\t\t\t"
+                      "qemu will generate one according to time.\n\t\t\t"
+                      "If any internal snapshot matching name already\n\t\t\t"
+                      "exist, or the name is an numeric string,\n\t\t\t"
+                      "the operation will fail.\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 494a9aa..2ddcafc 100644
--- a/hmp.c
+++ b/hmp.c
@@ -912,6 +912,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_try_str(qdict, "name");
+    Error *errp = NULL;
+
+    qmp_blockdev_snapshot_internal_sync(device, !!name, 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 V2 11/12] hmp: add interface hmp_snapshot_delete_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, 30 insertions(+), 0 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 312a0c7..5a66515 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1060,6 +1060,25 @@ Take an internal snapshot on device if it support
 ETEXI
 
     {
+        .name       = "snapshot_delete_blkdev_internal",
+        .args_type  = "device:B,name:s",
+        .params     = "device name",
+        .help       = "delete an internal snapshot of device.\n\t\t\t"
+                      "Name is the snapshot's name.\n\t\t\t"
+                      "If no snapshot matching name found,\n\t\t\t"
+                      "the operation will fail.\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 2ddcafc..1e90c7f 100644
--- a/hmp.c
+++ b/hmp.c
@@ -922,6 +922,16 @@ 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_try_str(qdict, "name");
+    Error *errp = NULL;
+
+    qmp_blockdev_snapshot_delete_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 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 V2 12/12] qemu-iotests: add 055 internal snapshot for block device test case

[Wenchao Xia]

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

diff --git a/tests/qemu-iotests/055 b/tests/qemu-iotests/055
new file mode 100755
index 0000000..7658f22
--- /dev/null
+++ b/tests/qemu-iotests/055
@@ -0,0 +1,157 @@
+#!/usr/bin/env python
+#
+# Tests for internal snapshot.
+#
+# Copyright (C) 2012 Red Hat, Inc.
+#
+# 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 i in range(0, len(self.expect)):
+            os.remove(self.expect[i]['image'])
+
+    def createSnapshotInTransaction(self, snapshot_num):
+        actions = []
+        for i in range(0, len(self.expect)):
+            num = self.expect[i]['snapshots_name_counter']
+            for j in range(0, snapshot_num):
+                name = '%s_sn%d' % (self.expect[i]['device'], num)
+                num = num + 1
+                self.expect[i]['snapshots'].append(name)
+                actions.append({
+                    'type': 'blockdev-snapshot-internal-sync',
+                    'data': { 'device': self.expect[i]['device'],
+                              'name': name },
+                })
+            self.expect[i]['snapshots_name_counter'] = num
+        result = self.vm.qmp('transaction', actions = actions)
+        self.assert_qmp(result, 'return', {})
+
+    def verifySnapshotInfo(self):
+        expect = self.expect
+        result = self.vm.qmp('query-block')
+        for i in range(0, len(expect)):
+            image = None
+            for device in result['return']:
+                if device['device'] == expect[i]['device']:
+                    image = device['inserted']['image']
+                    break
+            self.assertTrue(image != None)
+            self.assertTrue(len(image['snapshots']) ==
+                            len(expect[i]['snapshots']))
+            for j in range(0, len(image['snapshots'])):
+                try:
+                    expect[i]['snapshots'].index(image['snapshots'][j]['name'])
+                except:
+                    print 'not full match, image info:\n%s\n, expect:%s\n' % \
+                          (str(image), str(expect))
+                    self.assertTrue(False)
+
+    def deleteSnapshot(self, device, name):
+        result = self.vm.qmp('blockdev-snapshot-delete-internal-sync',
+                              device = device,
+                              name = name)
+        self.assert_qmp(result, 'return', {})
+        expect = self.expect
+        sn_list_expect = None
+        for i in range(0, len(expect)):
+            if expect[i]['device'] == device :
+                sn_list_expect = expect[i]['snapshots']
+                break
+        self.assertTrue(sn_list_expect != None)
+        try:
+            sn_list_expect.remove(name)
+        except:
+            print 'snapshot %s not found in expect:\n%s\n' % \
+                  (name, str(expect))
+            self.assertTrue(False)
+
+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(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_device(self):
+        actions = [{'type': 'blockdev-snapshot-internal-sync',
+                    'data': { 'device': 'driveerror',
+                              '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_delete(self):
+        self.createSnapshotInTransaction(3)
+        self.verifySnapshotInfo()
+        self.deleteSnapshot(self.expect[0]['device'], self.expect[0]['snapshots'][1])
+        self.verifySnapshotInfo()
+
+if __name__ == '__main__':
+    iotests.main(supported_fmts=['qcow2'])
diff --git a/tests/qemu-iotests/055.out b/tests/qemu-iotests/055.out
new file mode 100644
index 0000000..3f8a935
--- /dev/null
+++ b/tests/qemu-iotests/055.out
@@ -0,0 +1,5 @@
+......
+----------------------------------------------------------------------
+Ran 6 tests
+
+OK
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 387b050..e5762f9 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -61,3 +61,4 @@
 052 rw auto backing
 053 rw auto
 054 rw auto
+055 rw auto
-- 
1.7.1

Re: [Qemu-devel] [PATCH V2 12/12] qemu-iotests: add 055 internal snapshot for block device test case

[Stefan Hajnoczi]

On Fri, Jun 14, 2013 at 07:39:59PM +0800, Wenchao Xia wrote:
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  tests/qemu-iotests/055     |  157 ++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/055.out |    5 ++

drive-backup already uses 055, please name it 056 to avoid conflicts.

>  tests/qemu-iotests/group   |    1 +
>  3 files changed, 163 insertions(+), 0 deletions(-)
>  create mode 100755 tests/qemu-iotests/055
>  create mode 100644 tests/qemu-iotests/055.out
> 
> diff --git a/tests/qemu-iotests/055 b/tests/qemu-iotests/055
> new file mode 100755
> index 0000000..7658f22
> --- /dev/null
> +++ b/tests/qemu-iotests/055
> @@ -0,0 +1,157 @@
> +#!/usr/bin/env python
> +#
> +# Tests for internal snapshot.
> +#

Copyright (C) 2013 IBM ...

Based on 055:

> +# Copyright (C) 2012 Red Hat, Inc.
> +#
> +# 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 i in range(0, len(self.expect)):
> +            os.remove(self.expect[i]['image'])
> +
> +    def createSnapshotInTransaction(self, snapshot_num):
> +        actions = []
> +        for i in range(0, len(self.expect)):
> +            num = self.expect[i]['snapshots_name_counter']
> +            for j in range(0, snapshot_num):
> +                name = '%s_sn%d' % (self.expect[i]['device'], num)
> +                num = num + 1
> +                self.expect[i]['snapshots'].append(name)
> +                actions.append({
> +                    'type': 'blockdev-snapshot-internal-sync',
> +                    'data': { 'device': self.expect[i]['device'],
> +                              'name': name },
> +                })
> +            self.expect[i]['snapshots_name_counter'] = num
> +        result = self.vm.qmp('transaction', actions = actions)
> +        self.assert_qmp(result, 'return', {})

This tests the success case.  Please also use the 'Abort' action to test
the error case.

> +    def verifySnapshotInfo(self):
> +        expect = self.expect
> +        result = self.vm.qmp('query-block')
> +        for i in range(0, len(expect)):
> +            image = None
> +            for device in result['return']:
> +                if device['device'] == expect[i]['device']:
> +                    image = device['inserted']['image']
> +                    break
> +            self.assertTrue(image != None)
> +            self.assertTrue(len(image['snapshots']) ==
> +                            len(expect[i]['snapshots']))
> +            for j in range(0, len(image['snapshots'])):
> +                try:
> +                    expect[i]['snapshots'].index(image['snapshots'][j]['name'])
> +                except:
> +                    print 'not full match, image info:\n%s\n, expect:%s\n' % \
> +                          (str(image), str(expect))
> +                    self.assertTrue(False)
> +
> +    def deleteSnapshot(self, device, name):
> +        result = self.vm.qmp('blockdev-snapshot-delete-internal-sync',
> +                              device = device,
> +                              name = name)
> +        self.assert_qmp(result, 'return', {})
> +        expect = self.expect
> +        sn_list_expect = None
> +        for i in range(0, len(expect)):
> +            if expect[i]['device'] == device :
> +                sn_list_expect = expect[i]['snapshots']
> +                break
> +        self.assertTrue(sn_list_expect != None)
> +        try:
> +            sn_list_expect.remove(name)
> +        except:
> +            print 'snapshot %s not found in expect:\n%s\n' % \
> +                  (name, str(expect))
> +            self.assertTrue(False)
> +
> +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(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')

Please check that empty names error too.

Re: [Qemu-devel] [PATCH V2 12/12] qemu-iotests: add 055 internal snapshot for block device test case

[Wenchao Xia]

于 2013-6-18 22:32, Stefan Hajnoczi 写道:
> On Fri, Jun 14, 2013 at 07:39:59PM +0800, Wenchao Xia wrote:
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>>   tests/qemu-iotests/055     |  157 ++++++++++++++++++++++++++++++++++++++++++++
>>   tests/qemu-iotests/055.out |    5 ++
>
> drive-backup already uses 055, please name it 056 to avoid conflicts.
>
   OK.

>>   tests/qemu-iotests/group   |    1 +
>>   3 files changed, 163 insertions(+), 0 deletions(-)
>>   create mode 100755 tests/qemu-iotests/055
>>   create mode 100644 tests/qemu-iotests/055.out
>>
>> diff --git a/tests/qemu-iotests/055 b/tests/qemu-iotests/055
>> new file mode 100755
>> index 0000000..7658f22
>> --- /dev/null
>> +++ b/tests/qemu-iotests/055
>> @@ -0,0 +1,157 @@
>> +#!/usr/bin/env python
>> +#
>> +# Tests for internal snapshot.
>> +#
>
> Copyright (C) 2013 IBM ...
>
> Based on 055:
>
   oops....

>> +# Copyright (C) 2012 Red Hat, Inc.
>> +#
>> +# 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 i in range(0, len(self.expect)):
>> +            os.remove(self.expect[i]['image'])
>> +
>> +    def createSnapshotInTransaction(self, snapshot_num):
>> +        actions = []
>> +        for i in range(0, len(self.expect)):
>> +            num = self.expect[i]['snapshots_name_counter']
>> +            for j in range(0, snapshot_num):
>> +                name = '%s_sn%d' % (self.expect[i]['device'], num)
>> +                num = num + 1
>> +                self.expect[i]['snapshots'].append(name)
>> +                actions.append({
>> +                    'type': 'blockdev-snapshot-internal-sync',
>> +                    'data': { 'device': self.expect[i]['device'],
>> +                              'name': name },
>> +                })
>> +            self.expect[i]['snapshots_name_counter'] = num
>> +        result = self.vm.qmp('transaction', actions = actions)
>> +        self.assert_qmp(result, 'return', {})
>
> This tests the success case.  Please also use the 'Abort' action to test
> the error case.
   Is "Abort" upstreamed? If yes I will add that part.


>
>> +    def verifySnapshotInfo(self):
>> +        expect = self.expect
>> +        result = self.vm.qmp('query-block')
>> +        for i in range(0, len(expect)):
>> +            image = None
>> +            for device in result['return']:
>> +                if device['device'] == expect[i]['device']:
>> +                    image = device['inserted']['image']
>> +                    break
>> +            self.assertTrue(image != None)
>> +            self.assertTrue(len(image['snapshots']) ==
>> +                            len(expect[i]['snapshots']))
>> +            for j in range(0, len(image['snapshots'])):
>> +                try:
>> +                    expect[i]['snapshots'].index(image['snapshots'][j]['name'])
>> +                except:
>> +                    print 'not full match, image info:\n%s\n, expect:%s\n' % \
>> +                          (str(image), str(expect))
>> +                    self.assertTrue(False)
>> +
>> +    def deleteSnapshot(self, device, name):
>> +        result = self.vm.qmp('blockdev-snapshot-delete-internal-sync',
>> +                              device = device,
>> +                              name = name)
>> +        self.assert_qmp(result, 'return', {})
>> +        expect = self.expect
>> +        sn_list_expect = None
>> +        for i in range(0, len(expect)):
>> +            if expect[i]['device'] == device :
>> +                sn_list_expect = expect[i]['snapshots']
>> +                break
>> +        self.assertTrue(sn_list_expect != None)
>> +        try:
>> +            sn_list_expect.remove(name)
>> +        except:
>> +            print 'snapshot %s not found in expect:\n%s\n' % \
>> +                  (name, str(expect))
>> +            self.assertTrue(False)
>> +
>> +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(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')
>
> Please check that empty names error too.
>
   OK.

-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH V2 12/12] qemu-iotests: add 055 internal snapshot for block device test case

[Stefan Hajnoczi]

On Wed, Jun 19, 2013 at 02:21:31PM +0800, Wenchao Xia wrote:
> 于 2013-6-18 22:32, Stefan Hajnoczi 写道:
> >On Fri, Jun 14, 2013 at 07:39:59PM +0800, Wenchao Xia wrote:
> >>+    def createSnapshotInTransaction(self, snapshot_num):
> >>+        actions = []
> >>+        for i in range(0, len(self.expect)):
> >>+            num = self.expect[i]['snapshots_name_counter']
> >>+            for j in range(0, snapshot_num):
> >>+                name = '%s_sn%d' % (self.expect[i]['device'], num)
> >>+                num = num + 1
> >>+                self.expect[i]['snapshots'].append(name)
> >>+                actions.append({
> >>+                    'type': 'blockdev-snapshot-internal-sync',
> >>+                    'data': { 'device': self.expect[i]['device'],
> >>+                              'name': name },
> >>+                })
> >>+            self.expect[i]['snapshots_name_counter'] = num
> >>+        result = self.vm.qmp('transaction', actions = actions)
> >>+        self.assert_qmp(result, 'return', {})
> >
> >This tests the success case.  Please also use the 'Abort' action to test
> >the error case.
>   Is "Abort" upstreamed? If yes I will add that part.

It's part of the drive-backup series which you have already
cherry-picked from.

Re: [Qemu-devel] [PATCH V2 12/12] qemu-iotests: add 055 internal snapshot for block device test case

[Wenchao Xia]

 > On Wed, Jun 19, 2013 at 02:21:31PM +0800, Wenchao Xia wrote:
>> 于 2013-6-18 22:32, Stefan Hajnoczi 写道:
>>> On Fri, Jun 14, 2013 at 07:39:59PM +0800, Wenchao Xia wrote:
>>>> +    def createSnapshotInTransaction(self, snapshot_num):
>>>> +        actions = []
>>>> +        for i in range(0, len(self.expect)):
>>>> +            num = self.expect[i]['snapshots_name_counter']
>>>> +            for j in range(0, snapshot_num):
>>>> +                name = '%s_sn%d' % (self.expect[i]['device'], num)
>>>> +                num = num + 1
>>>> +                self.expect[i]['snapshots'].append(name)
>>>> +                actions.append({
>>>> +                    'type': 'blockdev-snapshot-internal-sync',
>>>> +                    'data': { 'device': self.expect[i]['device'],
>>>> +                              'name': name },
>>>> +                })
>>>> +            self.expect[i]['snapshots_name_counter'] = num
>>>> +        result = self.vm.qmp('transaction', actions = actions)
>>>> +        self.assert_qmp(result, 'return', {})
>>>
>>> This tests the success case.  Please also use the 'Abort' action to test
>>> the error case.
>>    Is "Abort" upstreamed? If yes I will add that part.
>
> It's part of the drive-backup series which you have already
> cherry-picked from.
>
   Will you continue work on drive-backup? I'd like to rebase this
serial after your v6, to reduce conflict.

-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH V2 12/12] qemu-iotests: add 055 internal snapshot for block device test case

[Stefan Hajnoczi]

On Wed, Jun 19, 2013 at 05:01:35PM +0800, Wenchao Xia wrote:
> > On Wed, Jun 19, 2013 at 02:21:31PM +0800, Wenchao Xia wrote:
> >>于 2013-6-18 22:32, Stefan Hajnoczi 写道:
> >>>On Fri, Jun 14, 2013 at 07:39:59PM +0800, Wenchao Xia wrote:
> >>>>+    def createSnapshotInTransaction(self, snapshot_num):
> >>>>+        actions = []
> >>>>+        for i in range(0, len(self.expect)):
> >>>>+            num = self.expect[i]['snapshots_name_counter']
> >>>>+            for j in range(0, snapshot_num):
> >>>>+                name = '%s_sn%d' % (self.expect[i]['device'], num)
> >>>>+                num = num + 1
> >>>>+                self.expect[i]['snapshots'].append(name)
> >>>>+                actions.append({
> >>>>+                    'type': 'blockdev-snapshot-internal-sync',
> >>>>+                    'data': { 'device': self.expect[i]['device'],
> >>>>+                              'name': name },
> >>>>+                })
> >>>>+            self.expect[i]['snapshots_name_counter'] = num
> >>>>+        result = self.vm.qmp('transaction', actions = actions)
> >>>>+        self.assert_qmp(result, 'return', {})
> >>>
> >>>This tests the success case.  Please also use the 'Abort' action to test
> >>>the error case.
> >>   Is "Abort" upstreamed? If yes I will add that part.
> >
> >It's part of the drive-backup series which you have already
> >cherry-picked from.
> >
>   Will you continue work on drive-backup? I'd like to rebase this
> serial after your v6, to reduce conflict.

Yep, I'll send another revision soon.  Kevin has been reviewing it
today.

Stefan