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

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

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 |  251 +++++++++++++++++++++++++++++++++++++++---------------------
 1 files changed, 163 insertions(+), 88 deletions(-)

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

[Wenchao Xia]

The code before really committing is moved into a function. Most
code are simply moved from qmp_transaction(), except that on fail it
just return 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>
---
 blockdev.c |  139 +++++++++++++++++++++++++++++++++---------------------------
 1 files changed, 77 insertions(+), 62 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 8a1652b..6cbf4e5 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 V3 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: Eric Blake <eblake@redhat.com>
---
 blockdev.c |   38 +++++++++++++++++++-------------------
 1 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 6cbf4e5..89521bb 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 V3 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>
---
 blockdev.c |   19 ++++++++++++-------
 1 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 89521bb..275c6f9 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 V3 4/5] block: package rollback code in qmp_transaction()

[Wenchao Xia]

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

diff --git a/blockdev.c b/blockdev.c
index 275c6f9..051be98 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -882,6 +882,13 @@ static void external_snapshot_commit(BlkTransactionStates *states)
                 NULL);
 }
 
+static void external_snapshot_rollback(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_rollback(states);
     }
 exit:
     QSIMPLEQ_FOREACH_SAFE(states, &snap_bdrv_states, entry, next) {
-- 
1.7.1

[Qemu-devel] [PATCH V3 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>
---
 blockdev.c |   84 +++++++++++++++++++++++++++++++++++++++++++++++------------
 1 files changed, 67 insertions(+), 17 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 051be98..b336794 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -779,14 +779,41 @@ 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
+   rollback() will be called. */
+typedef struct BdrvActionOps {
+    /* Prepare the work, must NOT be NULL. */
+    void (*prepare)(BlkTransactionStates *common, Error **errp);
+    /* Commit the changes, must NOT be NULL. */
+    void (*commit)(BlkTransactionStates *common);
+    /* Rollback the changes on fail, can be NULL. */
+    void (*rollback)(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 parent 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 +824,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 +901,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 +915,21 @@ static void external_snapshot_commit(BlkTransactionStates *states)
                 NULL);
 }
 
-static void external_snapshot_rollback(BlkTransactionStates *states)
+static void external_snapshot_rollback(BlkTransactionStates *common)
 {
+    ExternalSnapshotStates *states =
+                             DO_UPCAST(ExternalSnapshotStates, common, common);
     if (states->new_bs) {
         bdrv_delete(states->new_bs);
     }
 }
 
+const BdrvActionOps external_snapshot_ops = {
+    .prepare  = external_snapshot_prepare,
+    .commit   = external_snapshot_commit,
+    .rollback = external_snapshot_rollback,
+};
+
 /*
  * '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 +950,36 @@ 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;
+        ExternalSnapshotStates *ext;
 
         dev_info = dev_entry->value;
         dev_entry = dev_entry->next;
 
-        states = g_malloc0(sizeof(BlkTransactionStates));
-        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;
-            }
+            ext = g_malloc0(sizeof(ExternalSnapshotStates));
+            states = &ext->common;
+            states->ops = &external_snapshot_ops;
             break;
         default:
             abort();
         }
 
+        states->action = dev_info;
+        QSIMPLEQ_INSERT_TAIL(&snap_bdrv_states, states, entry);
+
+        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 +991,15 @@ delete_and_fail:
     * the original bs for all images
     */
     QSIMPLEQ_FOREACH(states, &snap_bdrv_states, entry) {
-        external_snapshot_rollback(states);
+        if (states->ops->rollback) {
+            states->ops->rollback(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 V3 5/5] block: make all steps in qmp_transaction() as callback

[Stefan Hajnoczi]

On Fri, Apr 19, 2013 at 08:57:10AM +0800, Wenchao Xia wrote:
> diff --git a/blockdev.c b/blockdev.c
> index 051be98..b336794 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -779,14 +779,41 @@ 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
> +   rollback() will be called. */

Please document that clean() is always called - after either commit() or
rollback().

> +const BdrvActionOps external_snapshot_ops = {

static

> @@ -909,32 +950,36 @@ 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;
> +        ExternalSnapshotStates *ext;
>  
>          dev_info = dev_entry->value;
>          dev_entry = dev_entry->next;
>  
> -        states = g_malloc0(sizeof(BlkTransactionStates));
> -        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;
> -            }
> +            ext = g_malloc0(sizeof(ExternalSnapshotStates));
> +            states = &ext->common;
> +            states->ops = &external_snapshot_ops;
>              break;
>          default:
>              abort();
>          }

Code duplication can be avoided like this:

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);
    /* Rollback the changes on fail, can be NULL. */
    void (*rollback)(BlkTransactionStates *common);
    /* Clean up resource in the end, can be NULL. */
    void (*clean)(BlkTransactionStates *common);
} BdrvActionOps;

static const BdrvActionOps actions[] = {
    [BLOCKDEV_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC] = {
        .instance_size = sizeof(ExternalSnapshotStates),
        .prepare  = external_snapshot_prepare,
        .commit   = external_snapshot_commit,
        .rollback = external_snapshot_rollback,
    },
};

Then the state struct is allocated as follows:

assert(dev_info->kind < ARRAY_SIZE(actions));
const BdrvActionOps *ops = &actions[dev_info->kind];
states = g_malloc0(ops->instance_size);
states->ops = ops;

No switch statement is necessary and the states setup doesn't need to be
duplicated when new actions are added.

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

[Wenchao Xia]

于 2013-4-24 17:25, Stefan Hajnoczi 写道:
> On Fri, Apr 19, 2013 at 08:57:10AM +0800, Wenchao Xia wrote:
>> diff --git a/blockdev.c b/blockdev.c
>> index 051be98..b336794 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -779,14 +779,41 @@ 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
>> +   rollback() will be called. */
>
> Please document that clean() is always called - after either commit() or
> rollback().
>
>> +const BdrvActionOps external_snapshot_ops = {
>
> static
>
>> @@ -909,32 +950,36 @@ 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;
>> +        ExternalSnapshotStates *ext;
>>
>>           dev_info = dev_entry->value;
>>           dev_entry = dev_entry->next;
>>
>> -        states = g_malloc0(sizeof(BlkTransactionStates));
>> -        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;
>> -            }
>> +            ext = g_malloc0(sizeof(ExternalSnapshotStates));
>> +            states = &ext->common;
>> +            states->ops = &external_snapshot_ops;
>>               break;
>>           default:
>>               abort();
>>           }
>
> Code duplication can be avoided like this:
>
> 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);
>      /* Rollback the changes on fail, can be NULL. */
>      void (*rollback)(BlkTransactionStates *common);
>      /* Clean up resource in the end, can be NULL. */
>      void (*clean)(BlkTransactionStates *common);
> } BdrvActionOps;
>
> static const BdrvActionOps actions[] = {
>      [BLOCKDEV_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC] = {
>          .instance_size = sizeof(ExternalSnapshotStates),
>          .prepare  = external_snapshot_prepare,
>          .commit   = external_snapshot_commit,
>          .rollback = external_snapshot_rollback,
>      },
> };
>
> Then the state struct is allocated as follows:
>
> assert(dev_info->kind < ARRAY_SIZE(actions));
> const BdrvActionOps *ops = &actions[dev_info->kind];
> states = g_malloc0(ops->instance_size);
> states->ops = ops;
>
> No switch statement is necessary and the states setup doesn't need to be
> duplicated when new actions are added.
>
   It is better, will use it, thanks.

-- 
Best Regards

Wenchao Xia