[Qemu-devel] [PATCH V5 0/5] block: make qmp_transaction extendable

[Qemu-devel] [PATCH V5 0/5] block: make qmp_transaction extendable

[Wenchao Xia]

  This serial will package backing chain snapshot code as one case, to make it
possible adding more operations later.

v2:
  Address Kevin's comments:
  Use the same prototype prepare, commit, rollback model in original code,
commit should never fail.

v3:
  Address Stefan's comments:
  3/5, 4/5: remove *action parameter since later only BlkTransactionStates* is
needed.
  5/5: embbed BlkTransactionStates in ExternalSnapshotStates, *opaque is
removed, related call back function format change for external snapshot.
  Address Kevin's comments:
  removed all indention in commit message.
  1/5: return void for prepare() function, *errp plays the role as error
checker.
  5/5: mark *commit callback must exist, *rollback callback can be NULL. Align
"callback =" in "const BdrvActionOps external_snapshot_ops" to the same colum.
  Address Eric's comments:
  1/5: better commit message.
  5/5: better commit message and comments in code that only one of rollback()
or commit() will be called.

v4:
  5/5: document clean() callback will always be called if it present, declare
static for global variable "actions", use array plus .instance_size to remove
"switch" checking code according to caller input.

v5:
  better commit message and comments, switch callback function name "rollback"
to "abort".

Wenchao Xia (5):
  1 block: package preparation code in qmp_transaction()
  2 block: move input parsing code in qmp_transaction()
  3 block: package committing code in qmp_transaction()
  4 block: package rollback code in qmp_transaction()
  5 block: make all steps in qmp_transaction() as callback

 blockdev.c |  266 ++++++++++++++++++++++++++++++++++++++----------------------
 1 files changed, 169 insertions(+), 97 deletions(-)

[Qemu-devel] [PATCH V5 1/5] block: package preparation code in qmp_transaction()

[Wenchao Xia]

The code before really committing is moved into a function. Most
code is simply moved from qmp_transaction(), except that on fail it
just returns now. Other code such as input parsing is not touched,
to make it easier in review.

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

diff --git a/blockdev.c b/blockdev.c
index 7c9d8dd..1cb9640 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -785,6 +785,78 @@ typedef struct BlkTransactionStates {
     QSIMPLEQ_ENTRY(BlkTransactionStates) entry;
 } BlkTransactionStates;
 
+static void external_snapshot_prepare(const char *device,
+                                      const char *format,
+                                      const char *new_image_file,
+                                      enum NewImageMode mode,
+                                      BlkTransactionStates *states,
+                                      Error **errp)
+{
+    BlockDriver *proto_drv;
+    BlockDriver *drv;
+    int flags, ret;
+    Error *local_err = NULL;
+
+    drv = bdrv_find_format(format);
+    if (!drv) {
+        error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
+        return;
+    }
+
+    states->old_bs = bdrv_find(device);
+    if (!states->old_bs) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+        return;
+    }
+
+    if (!bdrv_is_inserted(states->old_bs)) {
+        error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
+        return;
+    }
+
+    if (bdrv_in_use(states->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)) {
+            error_set(errp, QERR_IO_ERROR);
+            return;
+        }
+    }
+
+    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,
+                        states->old_bs->filename,
+                        states->old_bs->drv->format_name,
+                        NULL, -1, flags, &local_err, false);
+        if (error_is_set(&local_err)) {
+            error_propagate(errp, local_err);
+            return;
+        }
+    }
+
+    /* We will manually add the backing_hd field to the bs later */
+    states->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,
+                    flags | BDRV_O_NO_BACKING, drv);
+    if (ret != 0) {
+        error_set(errp, QERR_OPEN_FILE_FAILED, new_image_file);
+    }
+}
+
 /*
  * 'Atomic' group snapshots.  The snapshots are taken as a set, and if any fail
  *  then we do not pivot any of the devices in the group, and abandon the
@@ -792,7 +864,6 @@ typedef struct BlkTransactionStates {
  */
 void qmp_transaction(BlockdevActionList *dev_list, Error **errp)
 {
-    int ret = 0;
     BlockdevActionList *dev_entry = dev_list;
     BlkTransactionStates *states, *next;
     Error *local_err = NULL;
@@ -806,9 +877,6 @@ void qmp_transaction(BlockdevActionList *dev_list, Error **errp)
     /* We don't do anything in this loop that commits us to the snapshot */
     while (NULL != dev_entry) {
         BlockdevAction *dev_info = NULL;
-        BlockDriver *proto_drv;
-        BlockDriver *drv;
-        int flags;
         enum NewImageMode mode;
         const char *new_image_file;
         const char *device;
@@ -831,70 +899,17 @@ void qmp_transaction(BlockdevActionList *dev_list, Error **errp)
                 format = dev_info->blockdev_snapshot_sync->format;
             }
             mode = dev_info->blockdev_snapshot_sync->mode;
-            break;
-        default:
-            abort();
-        }
-
-        drv = bdrv_find_format(format);
-        if (!drv) {
-            error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
-            goto delete_and_fail;
-        }
-
-        states->old_bs = bdrv_find(device);
-        if (!states->old_bs) {
-            error_set(errp, QERR_DEVICE_NOT_FOUND, device);
-            goto delete_and_fail;
-        }
-
-        if (!bdrv_is_inserted(states->old_bs)) {
-            error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
-            goto delete_and_fail;
-        }
-
-        if (bdrv_in_use(states->old_bs)) {
-            error_set(errp, QERR_DEVICE_IN_USE, device);
-            goto delete_and_fail;
-        }
-
-        if (!bdrv_is_read_only(states->old_bs)) {
-            if (bdrv_flush(states->old_bs)) {
-                error_set(errp, QERR_IO_ERROR);
-                goto delete_and_fail;
-            }
-        }
-
-        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);
-            goto delete_and_fail;
-        }
-
-        /* 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,
-                            NULL, -1, flags, &local_err, false);
+            external_snapshot_prepare(device, format, new_image_file,
+                                      mode, states, &local_err);
             if (error_is_set(&local_err)) {
                 error_propagate(errp, local_err);
                 goto delete_and_fail;
             }
+            break;
+        default:
+            abort();
         }
 
-        /* We will manually add the backing_hd field to the bs later */
-        states->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,
-                        flags | BDRV_O_NO_BACKING, drv);
-        if (ret != 0) {
-            error_set(errp, QERR_OPEN_FILE_FAILED, new_image_file);
-            goto delete_and_fail;
-        }
     }
 
 
-- 
1.7.1

[Qemu-devel] [PATCH V5 2/5] block: move input parsing code in qmp_transaction()

[Wenchao Xia]

The code is moved into preparation function, and changed
a bit to tip more clearly what it is doing.

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

diff --git a/blockdev.c b/blockdev.c
index 1cb9640..0115f46 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -785,10 +785,7 @@ typedef struct BlkTransactionStates {
     QSIMPLEQ_ENTRY(BlkTransactionStates) entry;
 } BlkTransactionStates;
 
-static void external_snapshot_prepare(const char *device,
-                                      const char *format,
-                                      const char *new_image_file,
-                                      enum NewImageMode mode,
+static void external_snapshot_prepare(BlockdevAction *action,
                                       BlkTransactionStates *states,
                                       Error **errp)
 {
@@ -796,7 +793,24 @@ static void external_snapshot_prepare(const char *device,
     BlockDriver *drv;
     int flags, ret;
     Error *local_err = NULL;
+    const char *device;
+    const char *new_image_file;
+    const char *format = "qcow2";
+    enum NewImageMode mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;
 
+    /* get parameters */
+    g_assert(action->kind == BLOCKDEV_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC);
+
+    device = action->blockdev_snapshot_sync->device;
+    new_image_file = action->blockdev_snapshot_sync->snapshot_file;
+    if (action->blockdev_snapshot_sync->has_format) {
+        format = action->blockdev_snapshot_sync->format;
+    }
+    if (action->blockdev_snapshot_sync->has_mode) {
+        mode = action->blockdev_snapshot_sync->mode;
+    }
+
+    /* start processing */
     drv = bdrv_find_format(format);
     if (!drv) {
         error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
@@ -877,10 +891,6 @@ void qmp_transaction(BlockdevActionList *dev_list, Error **errp)
     /* We don't do anything in this loop that commits us to the snapshot */
     while (NULL != dev_entry) {
         BlockdevAction *dev_info = NULL;
-        enum NewImageMode mode;
-        const char *new_image_file;
-        const char *device;
-        const char *format = "qcow2";
 
         dev_info = dev_entry->value;
         dev_entry = dev_entry->next;
@@ -890,17 +900,7 @@ void qmp_transaction(BlockdevActionList *dev_list, Error **errp)
 
         switch (dev_info->kind) {
         case BLOCKDEV_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC:
-            device = dev_info->blockdev_snapshot_sync->device;
-            if (!dev_info->blockdev_snapshot_sync->has_mode) {
-                dev_info->blockdev_snapshot_sync->mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;
-            }
-            new_image_file = dev_info->blockdev_snapshot_sync->snapshot_file;
-            if (dev_info->blockdev_snapshot_sync->has_format) {
-                format = dev_info->blockdev_snapshot_sync->format;
-            }
-            mode = dev_info->blockdev_snapshot_sync->mode;
-            external_snapshot_prepare(device, format, new_image_file,
-                                      mode, states, &local_err);
+            external_snapshot_prepare(dev_info, states, errp);
             if (error_is_set(&local_err)) {
                 error_propagate(errp, local_err);
                 goto delete_and_fail;
-- 
1.7.1

[Qemu-devel] [PATCH V5 3/5] block: package committing code in qmp_transaction()

[Wenchao Xia]

The code is simply moved into a separate function.

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

diff --git a/blockdev.c b/blockdev.c
index 0115f46..76f0532 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -871,6 +871,17 @@ static void external_snapshot_prepare(BlockdevAction *action,
     }
 }
 
+static void external_snapshot_commit(BlkTransactionStates *states)
+{
+    /* This removes our old bs from the bdrv_states, and adds the new bs */
+    bdrv_append(states->new_bs, states->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,
+                NULL);
+}
+
 /*
  * 'Atomic' group snapshots.  The snapshots are taken as a set, and if any fail
  *  then we do not pivot any of the devices in the group, and abandon the
@@ -916,13 +927,7 @@ void qmp_transaction(BlockdevActionList *dev_list, Error **errp)
     /* Now we are going to do the actual pivot.  Everything up to this point
      * is reversible, but we are committed at this point */
     QSIMPLEQ_FOREACH(states, &snap_bdrv_states, entry) {
-        /* This removes our old bs from the bdrv_states, and adds the new bs */
-        bdrv_append(states->new_bs, states->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,
-                    NULL);
+        external_snapshot_commit(states);
     }
 
     /* success */
-- 
1.7.1

[Qemu-devel] [PATCH V5 4/5] block: package rollback code in qmp_transaction()

[Wenchao Xia]

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

diff --git a/blockdev.c b/blockdev.c
index 76f0532..2131e13 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -882,6 +882,13 @@ static void external_snapshot_commit(BlkTransactionStates *states)
                 NULL);
 }
 
+static void external_snapshot_abort(BlkTransactionStates *states)
+{
+    if (states->new_bs) {
+        bdrv_delete(states->new_bs);
+    }
+}
+
 /*
  * 'Atomic' group snapshots.  The snapshots are taken as a set, and if any fail
  *  then we do not pivot any of the devices in the group, and abandon the
@@ -939,9 +946,7 @@ delete_and_fail:
     * the original bs for all images
     */
     QSIMPLEQ_FOREACH(states, &snap_bdrv_states, entry) {
-        if (states->new_bs) {
-             bdrv_delete(states->new_bs);
-        }
+        external_snapshot_abort(states);
     }
 exit:
     QSIMPLEQ_FOREACH_SAFE(states, &snap_bdrv_states, entry, next) {
-- 
1.7.1

[Qemu-devel] [PATCH V5 5/5] block: make all steps in qmp_transaction() as callback

[Wenchao Xia]

Make it easier to add other operations to qmp_transaction() by using
callbacks, with external snapshots serving as an example implementation
of the callbacks.

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

diff --git a/blockdev.c b/blockdev.c
index 2131e13..617501c 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -779,14 +779,43 @@ void qmp_blockdev_snapshot_sync(const char *device, const char *snapshot_file,
 
 
 /* New and old BlockDriverState structs for group snapshots */
-typedef struct BlkTransactionStates {
+
+typedef struct BlkTransactionStates BlkTransactionStates;
+
+/* 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. */
+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);
+    /* Commit the changes, must NOT be NULL. */
+    void (*commit)(BlkTransactionStates *common);
+    /* Abort the changes on fail, can be NULL. */
+    void (*abort)(BlkTransactionStates *common);
+    /* Clean up resource in the end, can be NULL. */
+    void (*clean)(BlkTransactionStates *common);
+} BdrvActionOps;
+
+/*
+ * This structure must be arranged as first member in child type, assuming
+ * that compiler will also arrange it to the same address with parent instance.
+ * Later it will be used in free().
+ */
+struct BlkTransactionStates {
+    BlockdevAction *action;
+    const BdrvActionOps *ops;
+    QSIMPLEQ_ENTRY(BlkTransactionStates) entry;
+};
+
+/* external snapshot private data */
+typedef struct ExternalSnapshotStates {
+    BlkTransactionStates common;
     BlockDriverState *old_bs;
     BlockDriverState *new_bs;
-    QSIMPLEQ_ENTRY(BlkTransactionStates) entry;
-} BlkTransactionStates;
+} ExternalSnapshotStates;
 
-static void external_snapshot_prepare(BlockdevAction *action,
-                                      BlkTransactionStates *states,
+static void external_snapshot_prepare(BlkTransactionStates *common,
                                       Error **errp)
 {
     BlockDriver *proto_drv;
@@ -797,6 +826,9 @@ static void external_snapshot_prepare(BlockdevAction *action,
     const char *new_image_file;
     const char *format = "qcow2";
     enum NewImageMode mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;
+    ExternalSnapshotStates *states =
+                             DO_UPCAST(ExternalSnapshotStates, common, common);
+    BlockdevAction *action = common->action;
 
     /* get parameters */
     g_assert(action->kind == BLOCKDEV_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC);
@@ -871,8 +903,11 @@ static void external_snapshot_prepare(BlockdevAction *action,
     }
 }
 
-static void external_snapshot_commit(BlkTransactionStates *states)
+static void external_snapshot_commit(BlkTransactionStates *common)
 {
+    ExternalSnapshotStates *states =
+                             DO_UPCAST(ExternalSnapshotStates, common, common);
+
     /* This removes our old bs from the bdrv_states, and adds the new bs */
     bdrv_append(states->new_bs, states->old_bs);
     /* We don't need (or want) to use the transactional
@@ -882,13 +917,24 @@ static void external_snapshot_commit(BlkTransactionStates *states)
                 NULL);
 }
 
-static void external_snapshot_abort(BlkTransactionStates *states)
+static void external_snapshot_abort(BlkTransactionStates *common)
 {
+    ExternalSnapshotStates *states =
+                             DO_UPCAST(ExternalSnapshotStates, common, common);
     if (states->new_bs) {
         bdrv_delete(states->new_bs);
     }
 }
 
+static const BdrvActionOps actions[] = {
+    [BLOCKDEV_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC] = {
+        .instance_size = sizeof(ExternalSnapshotStates),
+        .prepare  = external_snapshot_prepare,
+        .commit   = external_snapshot_commit,
+        .abort = external_snapshot_abort,
+    },
+};
+
 /*
  * 'Atomic' group snapshots.  The snapshots are taken as a set, and if any fail
  *  then we do not pivot any of the devices in the group, and abandon the
@@ -909,32 +955,28 @@ void qmp_transaction(BlockdevActionList *dev_list, Error **errp)
     /* We don't do anything in this loop that commits us to the snapshot */
     while (NULL != dev_entry) {
         BlockdevAction *dev_info = NULL;
+        const BdrvActionOps *ops;
 
         dev_info = dev_entry->value;
         dev_entry = dev_entry->next;
 
-        states = g_malloc0(sizeof(BlkTransactionStates));
+        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);
 
-        switch (dev_info->kind) {
-        case BLOCKDEV_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC:
-            external_snapshot_prepare(dev_info, states, errp);
-            if (error_is_set(&local_err)) {
-                error_propagate(errp, local_err);
-                goto delete_and_fail;
-            }
-            break;
-        default:
-            abort();
+        states->ops->prepare(states, &local_err);
+        if (error_is_set(&local_err)) {
+            error_propagate(errp, local_err);
+            goto delete_and_fail;
         }
-
     }
 
-
-    /* Now we are going to do the actual pivot.  Everything up to this point
-     * is reversible, but we are committed at this point */
     QSIMPLEQ_FOREACH(states, &snap_bdrv_states, entry) {
-        external_snapshot_commit(states);
+        states->ops->commit(states);
     }
 
     /* success */
@@ -946,10 +988,15 @@ delete_and_fail:
     * the original bs for all images
     */
     QSIMPLEQ_FOREACH(states, &snap_bdrv_states, entry) {
-        external_snapshot_abort(states);
+        if (states->ops->abort) {
+            states->ops->abort(states);
+        }
     }
 exit:
     QSIMPLEQ_FOREACH_SAFE(states, &snap_bdrv_states, entry, next) {
+        if (states->ops->clean) {
+            states->ops->clean(states);
+        }
         g_free(states);
     }
 }
-- 
1.7.1

Re: [Qemu-devel] [PATCH V5 0/5] block: make qmp_transaction extendable

[Kevin Wolf]

Am 08.05.2013 um 12:25 hat Wenchao Xia geschrieben:
>   This serial will package backing chain snapshot code as one case, to make it
> possible adding more operations later.
> 
> v2:
>   Address Kevin's comments:
>   Use the same prototype prepare, commit, rollback model in original code,
> commit should never fail.
> 
> v3:
>   Address Stefan's comments:
>   3/5, 4/5: remove *action parameter since later only BlkTransactionStates* is
> needed.
>   5/5: embbed BlkTransactionStates in ExternalSnapshotStates, *opaque is
> removed, related call back function format change for external snapshot.
>   Address Kevin's comments:
>   removed all indention in commit message.
>   1/5: return void for prepare() function, *errp plays the role as error
> checker.
>   5/5: mark *commit callback must exist, *rollback callback can be NULL. Align
> "callback =" in "const BdrvActionOps external_snapshot_ops" to the same colum.
>   Address Eric's comments:
>   1/5: better commit message.
>   5/5: better commit message and comments in code that only one of rollback()
> or commit() will be called.
> 
> v4:
>   5/5: document clean() callback will always be called if it present, declare
> static for global variable "actions", use array plus .instance_size to remove
> "switch" checking code according to caller input.
> 
> v5:
>   better commit message and comments, switch callback function name "rollback"
> to "abort".
> 
> Wenchao Xia (5):
>   1 block: package preparation code in qmp_transaction()
>   2 block: move input parsing code in qmp_transaction()
>   3 block: package committing code in qmp_transaction()
>   4 block: package rollback code in qmp_transaction()
>   5 block: make all steps in qmp_transaction() as callback
> 
>  blockdev.c |  266 ++++++++++++++++++++++++++++++++++++++----------------------
>  1 files changed, 169 insertions(+), 97 deletions(-)

Thanks, applied all to block-next for 1.6.

Kevin

Re: [Qemu-devel] [PATCH V5 0/5] block: make qmp_transaction extendable

[Wenchao Xia]

于 2013-5-8 19:21, Kevin Wolf 写道:
> Am 08.05.2013 um 12:25 hat Wenchao Xia geschrieben:
>>    This serial will package backing chain snapshot code as one case, to make it
>> possible adding more operations later.
>>
>> v2:
>>    Address Kevin's comments:
>>    Use the same prototype prepare, commit, rollback model in original code,
>> commit should never fail.
>>
>> v3:
>>    Address Stefan's comments:
>>    3/5, 4/5: remove *action parameter since later only BlkTransactionStates* is
>> needed.
>>    5/5: embbed BlkTransactionStates in ExternalSnapshotStates, *opaque is
>> removed, related call back function format change for external snapshot.
>>    Address Kevin's comments:
>>    removed all indention in commit message.
>>    1/5: return void for prepare() function, *errp plays the role as error
>> checker.
>>    5/5: mark *commit callback must exist, *rollback callback can be NULL. Align
>> "callback =" in "const BdrvActionOps external_snapshot_ops" to the same colum.
>>    Address Eric's comments:
>>    1/5: better commit message.
>>    5/5: better commit message and comments in code that only one of rollback()
>> or commit() will be called.
>>
>> v4:
>>    5/5: document clean() callback will always be called if it present, declare
>> static for global variable "actions", use array plus .instance_size to remove
>> "switch" checking code according to caller input.
>>
>> v5:
>>    better commit message and comments, switch callback function name "rollback"
>> to "abort".
>>
>> Wenchao Xia (5):
>>    1 block: package preparation code in qmp_transaction()
>>    2 block: move input parsing code in qmp_transaction()
>>    3 block: package committing code in qmp_transaction()
>>    4 block: package rollback code in qmp_transaction()
>>    5 block: make all steps in qmp_transaction() as callback
>>
>>   blockdev.c |  266 ++++++++++++++++++++++++++++++++++++++----------------------
>>   1 files changed, 169 insertions(+), 97 deletions(-)
>
> Thanks, applied all to block-next for 1.6.
>
> Kevin
>
Hi, Kevin
   I am looking at adding internal snapshot into qmp_transaction().
Although it have been discussed before, but there are possible two
ways to do it, want to know you opinion.

Methods:
1) in block.c, break bdrv_snapshot_create() into
bdrv_snapshot_create_prepare() and bdrv_snapshot_create_commit(), add
bdrv_snapshot_create_abort(). In block/qcow2-snapshot.c, do related
changing, adding qcow2_snapshot_create_abort(). Currently the abort()
action can't fail, so just deallocate the cluster, but can't correct
the refs, maybe put a dirty flag to let qemu-img correct it?

2) like external backing chain, on fail qemu don't delete the new
created one, instead it just guarentee qemu works normal, that
means nothing need to be done now. In creation, if it found a snapshot
existing have same name, just fail. User should delete the existing
one himself, and clean up the created one in fail.

1) deployed the commit/abort() conception in the block level,
2) deployed the conception in qemu level similar to external
snapshot chain, I am not sure which is better.

-- 
Best Regards

Wenchao Xia