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

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

Wenchao Xia (5):
  block: package preparation code in qmp_transaction()
  block: move input parsing code in qmp_transaction()
  block: package committing code in qmp_transaction()
  block: package rolling back code in qmp_transaction()
  block: make all steps in qmp_transaction() as callback

 blockdev.c |  252 +++++++++++++++++++++++++++++++++++++++---------------------
 1 files changed, 165 insertions(+), 87 deletions(-)

[Qemu-devel] [PATCH V2 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()i, except fail handling
label is changed from "delete_and_fail" to "fail". 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 |  146 +++++++++++++++++++++++++++++++++--------------------------
 1 files changed, 82 insertions(+), 64 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 8a1652b..e01906c 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -785,6 +785,84 @@ typedef struct BlkTransactionStates {
     QSIMPLEQ_ENTRY(BlkTransactionStates) entry;
 } BlkTransactionStates;
 
+static int 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);
+        goto fail;
+    }
+
+    states->old_bs = bdrv_find(device);
+    if (!states->old_bs) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+        goto fail;
+    }
+
+    if (!bdrv_is_inserted(states->old_bs)) {
+        error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
+        goto fail;
+    }
+
+    if (bdrv_in_use(states->old_bs)) {
+        error_set(errp, QERR_DEVICE_IN_USE, device);
+        goto fail;
+    }
+
+    if (!bdrv_is_read_only(states->old_bs)) {
+        if (bdrv_flush(states->old_bs)) {
+            error_set(errp, QERR_IO_ERROR);
+            goto 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 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);
+        if (error_is_set(&local_err)) {
+            error_propagate(errp, local_err);
+            goto fail;
+        }
+    }
+
+    /* 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 fail;
+    }
+
+    return 0;
+
+ fail:
+    return -1;
+}
+
 /*
  * '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,10 +870,8 @@ 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;
 
     QSIMPLEQ_HEAD(snap_bdrv_states, BlkTransactionStates) snap_bdrv_states;
     QSIMPLEQ_INIT(&snap_bdrv_states);
@@ -806,9 +882,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 +904,15 @@ void qmp_transaction(BlockdevActionList *dev_list, Error **errp)
                 format = dev_info->blockdev_snapshot_sync->format;
             }
             mode = dev_info->blockdev_snapshot_sync->mode;
+            if (external_snapshot_prepare(device, format, new_image_file,
+                                          mode, states, errp)) {
+                goto delete_and_fail;
+            }
             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);
-            if (error_is_set(&local_err)) {
-                error_propagate(errp, local_err);
-                goto delete_and_fail;
-            }
-        }
-
-        /* 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 V2 2/5] block: move input parsing code in qmp_transaction()

[Wenchao Xia]

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

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

diff --git a/blockdev.c b/blockdev.c
index e01906c..8d1faf8 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -785,10 +785,7 @@ typedef struct BlkTransactionStates {
     QSIMPLEQ_ENTRY(BlkTransactionStates) entry;
 } BlkTransactionStates;
 
-static int external_snapshot_prepare(const char *device,
-                                     const char *format,
-                                     const char *new_image_file,
-                                     enum NewImageMode mode,
+static int external_snapshot_prepare(BlockdevAction *action,
                                      BlkTransactionStates *states,
                                      Error **errp)
 {
@@ -796,7 +793,24 @@ static int 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);
@@ -882,10 +896,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;
@@ -895,17 +905,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;
-            if (external_snapshot_prepare(device, format, new_image_file,
-                                          mode, states, errp)) {
+            if (external_snapshot_prepare(dev_info, states, errp)) {
                 goto delete_and_fail;
             }
             break;
-- 
1.7.1

[Qemu-devel] [PATCH V2 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 |   20 +++++++++++++-------
 1 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 8d1faf8..38cb7d5 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -877,6 +877,18 @@ static int external_snapshot_prepare(BlockdevAction *action,
     return -1;
 }
 
+static void external_snapshot_commit(BlockdevAction *action,
+                                     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
@@ -919,13 +931,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(NULL, states);
     }
 
     /* success */
-- 
1.7.1

[Qemu-devel] [PATCH V2 4/5] block: package rolling back code in qmp_transaction()

[Wenchao Xia]

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

diff --git a/blockdev.c b/blockdev.c
index 38cb7d5..3e69569 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -889,6 +889,14 @@ static void external_snapshot_commit(BlockdevAction *action,
                 NULL);
 }
 
+static void external_snapshot_rollback(BlockdevAction *action,
+                                       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
@@ -943,9 +951,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(NULL, states);
     }
 exit:
     QSIMPLEQ_FOREACH_SAFE(states, &snap_bdrv_states, entry, next) {
-- 
1.7.1

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

[Wenchao Xia]

  Now qmp_transaction() can be extended with other operation,
external snapshot or backing chain creation, is just one case it.

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

diff --git a/blockdev.c b/blockdev.c
index 3e69569..9f0acfb 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -779,14 +779,35 @@ void qmp_blockdev_snapshot_sync(const char *device, const char *snapshot_file,
 
 
 /* New and old BlockDriverState structs for group snapshots */
+
+/* Only in prepare() it may fail, so roll back just need to take care
+   what is done in prepare(). */
+typedef struct BdrvActionOps {
+    /* Prepare the work, must NOT be NULL. */
+    int (*prepare)(BlockdevAction *action, void **p_opaque, Error **errp);
+    /* Commit the changes, can be NULL. */
+    void (*commit)(BlockdevAction *action, void *opaque);
+    /* Rollback the changes on fail, mut NOT be NULL. */
+    void (*rollback)(BlockdevAction *action, void *opaque);
+    /* Clean up resource in the end, can be NULL. */
+    void (*clean)(BlockdevAction *action, void *opaque);
+} BdrvActionOps;
+
 typedef struct BlkTransactionStates {
-    BlockDriverState *old_bs;
-    BlockDriverState *new_bs;
+    BlockdevAction *action;
+    void *opaque;
+    const BdrvActionOps *ops;
     QSIMPLEQ_ENTRY(BlkTransactionStates) entry;
 } BlkTransactionStates;
 
+/* external snapshot private data */
+typedef struct ExternalSnapshotState {
+    BlockDriverState *old_bs;
+    BlockDriverState *new_bs;
+} ExternalSnapshotState;
+
 static int external_snapshot_prepare(BlockdevAction *action,
-                                     BlkTransactionStates *states,
+                                     void **p_opaque,
                                      Error **errp)
 {
     BlockDriver *proto_drv;
@@ -797,6 +818,7 @@ static int external_snapshot_prepare(BlockdevAction *action,
     const char *new_image_file;
     const char *format = "qcow2";
     enum NewImageMode mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;
+    ExternalSnapshotState *states;
 
     /* get parameters */
     g_assert(action->kind == BLOCKDEV_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC);
@@ -817,6 +839,9 @@ static int external_snapshot_prepare(BlockdevAction *action,
         goto fail;
     }
 
+    *p_opaque = g_malloc0(sizeof(ExternalSnapshotState));
+    states = *p_opaque;
+
     states->old_bs = bdrv_find(device);
     if (!states->old_bs) {
         error_set(errp, QERR_DEVICE_NOT_FOUND, device);
@@ -878,8 +903,10 @@ static int external_snapshot_prepare(BlockdevAction *action,
 }
 
 static void external_snapshot_commit(BlockdevAction *action,
-                                     BlkTransactionStates *states)
+                                     void *opaque)
 {
+    ExternalSnapshotState *states = opaque;
+
     /* 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
@@ -890,13 +917,27 @@ static void external_snapshot_commit(BlockdevAction *action,
 }
 
 static void external_snapshot_rollback(BlockdevAction *action,
-                                       BlkTransactionStates *states)
+                                       void *opaque)
 {
+    ExternalSnapshotState *states = opaque;
     if (states->new_bs) {
         bdrv_delete(states->new_bs);
     }
 }
 
+static void external_snapshot_clean(BlockdevAction *action,
+                                    void *opaque)
+{
+    g_free(opaque);
+}
+
+const BdrvActionOps external_snapshot_ops = {
+    .prepare = external_snapshot_prepare,
+    .commit = external_snapshot_commit,
+    .rollback = external_snapshot_rollback,
+    .clean = external_snapshot_clean,
+};
+
 /*
  * '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
@@ -923,23 +964,27 @@ void qmp_transaction(BlockdevActionList *dev_list, Error **errp)
         states = g_malloc0(sizeof(BlkTransactionStates));
         QSIMPLEQ_INSERT_TAIL(&snap_bdrv_states, states, entry);
 
+        states->action = dev_info;
         switch (dev_info->kind) {
         case BLOCKDEV_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC:
-            if (external_snapshot_prepare(dev_info, states, errp)) {
-                goto delete_and_fail;
-            }
+            states->ops = &external_snapshot_ops;
             break;
         default:
             abort();
         }
 
+        if (states->ops->prepare(states->action, &states->opaque, errp)) {
+            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(NULL, states);
+        if (states->ops->commit) {
+            states->ops->commit(states->action, states->opaque);
+        }
     }
 
     /* success */
@@ -951,10 +996,13 @@ delete_and_fail:
     * the original bs for all images
     */
     QSIMPLEQ_FOREACH(states, &snap_bdrv_states, entry) {
-        external_snapshot_rollback(NULL, states);
+        states->ops->rollback(states->action, states->opaque);
     }
 exit:
     QSIMPLEQ_FOREACH_SAFE(states, &snap_bdrv_states, entry, next) {
+        if (states->ops->clean) {
+            states->ops->clean(states->action, states->opaque);
+        }
         g_free(states);
     }
 }
-- 
1.7.1

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

[Kevin Wolf]

Am 13.04.2013 um 13:11 hat Wenchao Xia geschrieben:
>   The code before really committing is moved into a function. Most

This line seems to be indented accidentally?

> code are simply moved from qmp_transaction()i, except fail handling
> label is changed from "delete_and_fail" to "fail". 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 |  146 +++++++++++++++++++++++++++++++++--------------------------
>  1 files changed, 82 insertions(+), 64 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 8a1652b..e01906c 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -785,6 +785,84 @@ typedef struct BlkTransactionStates {
>      QSIMPLEQ_ENTRY(BlkTransactionStates) entry;
>  } BlkTransactionStates;
>  
> +static int external_snapshot_prepare(const char *device,
> +                                     const char *format,
> +                                     const char *new_image_file,
> +                                     enum NewImageMode mode,
> +                                     BlkTransactionStates *states,
> +                                     Error **errp)

One error reporting mechanism is enough, so this function should return
void.

Kevin

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

[Kevin Wolf]

Am 13.04.2013 um 13:11 hat Wenchao Xia geschrieben:
>   Now qmp_transaction() can be extended with other operation,
> external snapshot or backing chain creation, is just one case it.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  blockdev.c |   68 +++++++++++++++++++++++++++++++++++++++++++++++++++--------
>  1 files changed, 58 insertions(+), 10 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 3e69569..9f0acfb 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -779,14 +779,35 @@ void qmp_blockdev_snapshot_sync(const char *device, const char *snapshot_file,
>  
>  
>  /* New and old BlockDriverState structs for group snapshots */
> +
> +/* Only in prepare() it may fail, so roll back just need to take care
> +   what is done in prepare(). */
> +typedef struct BdrvActionOps {
> +    /* Prepare the work, must NOT be NULL. */
> +    int (*prepare)(BlockdevAction *action, void **p_opaque, Error **errp);
> +    /* Commit the changes, can be NULL. */
> +    void (*commit)(BlockdevAction *action, void *opaque);
> +    /* Rollback the changes on fail, mut NOT be NULL. */

Typo: s/mut/must/

> +    void (*rollback)(BlockdevAction *action, void *opaque);
> +    /* Clean up resource in the end, can be NULL. */
> +    void (*clean)(BlockdevAction *action, void *opaque);
> +} BdrvActionOps;

What's the reason that commit can be NULL, but rollback can't? I can't
imagine a case where a commit isn't needed, but one without a rollback
action sounds possible.

> +
>  typedef struct BlkTransactionStates {
> -    BlockDriverState *old_bs;
> -    BlockDriverState *new_bs;
> +    BlockdevAction *action;
> +    void *opaque;
> +    const BdrvActionOps *ops;
>      QSIMPLEQ_ENTRY(BlkTransactionStates) entry;
>  } BlkTransactionStates;
>  
> +/* external snapshot private data */
> +typedef struct ExternalSnapshotState {
> +    BlockDriverState *old_bs;
> +    BlockDriverState *new_bs;
> +} ExternalSnapshotState;
> +
>  static int external_snapshot_prepare(BlockdevAction *action,
> -                                     BlkTransactionStates *states,
> +                                     void **p_opaque,

Call it just opaque.

>                                       Error **errp)
>  {
>      BlockDriver *proto_drv;
> @@ -797,6 +818,7 @@ static int external_snapshot_prepare(BlockdevAction *action,
>      const char *new_image_file;
>      const char *format = "qcow2";
>      enum NewImageMode mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;
> +    ExternalSnapshotState *states;
>  
>      /* get parameters */
>      g_assert(action->kind == BLOCKDEV_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC);
> @@ -817,6 +839,9 @@ static int external_snapshot_prepare(BlockdevAction *action,
>          goto fail;
>      }
>  
> +    *p_opaque = g_malloc0(sizeof(ExternalSnapshotState));
> +    states = *p_opaque;
> +
>      states->old_bs = bdrv_find(device);
>      if (!states->old_bs) {
>          error_set(errp, QERR_DEVICE_NOT_FOUND, device);
> @@ -878,8 +903,10 @@ static int external_snapshot_prepare(BlockdevAction *action,
>  }
>  
>  static void external_snapshot_commit(BlockdevAction *action,
> -                                     BlkTransactionStates *states)
> +                                     void *opaque)
>  {
> +    ExternalSnapshotState *states = opaque;
> +
>      /* 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
> @@ -890,13 +917,27 @@ static void external_snapshot_commit(BlockdevAction *action,
>  }
>  
>  static void external_snapshot_rollback(BlockdevAction *action,
> -                                       BlkTransactionStates *states)
> +                                       void *opaque)
>  {
> +    ExternalSnapshotState *states = opaque;
>      if (states->new_bs) {
>          bdrv_delete(states->new_bs);
>      }
>  }
>  
> +static void external_snapshot_clean(BlockdevAction *action,
> +                                    void *opaque)

This fits in a single line.

> +{
> +    g_free(opaque);
> +}
> +
> +const BdrvActionOps external_snapshot_ops = {
> +    .prepare = external_snapshot_prepare,
> +    .commit = external_snapshot_commit,
> +    .rollback = external_snapshot_rollback,
> +    .clean = external_snapshot_clean,
> +};

Please align all = to the same column.

Kevin

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

[Eric Blake]

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

On 04/13/2013 05:11 AM, Wenchao Xia wrote:
>   The code before really committing is moved into a function. Most
> code are simply moved from qmp_transaction()i, except fail handling

s/i,/,/

> label is changed from "delete_and_fail" to "fail". 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 |  146 +++++++++++++++++++++++++++++++++--------------------------
>  1 files changed, 82 insertions(+), 64 deletions(-)
> 

-- 
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 2/5] block: move input parsing code in qmp_transaction()

[Eric Blake]

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

On 04/13/2013 05:11 AM, Wenchao Xia wrote:
>   The code is moved into preparation function, and is changed
> a bit to tip more clearly what it is doing.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  blockdev.c |   38 +++++++++++++++++++-------------------
>  1 files changed, 19 insertions(+), 19 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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 3/5] block: package committing code in qmp_transaction()

[Eric Blake]

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

On 04/13/2013 05:11 AM, Wenchao Xia wrote:
>   The code is simply moved into a separate function.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  blockdev.c |   20 +++++++++++++-------
>  1 files changed, 13 insertions(+), 7 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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 4/5] block: package rolling back code in qmp_transaction()

[Eric Blake]

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

On 04/13/2013 05:11 AM, Wenchao Xia wrote:

In the subject: s/rolling back/rollback/

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

With the subject fix, feel free to add:

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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 5/5] block: make all steps in qmp_transaction() as callback

[Eric Blake]

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

On 04/13/2013 05:11 AM, Wenchao Xia wrote:
>   Now qmp_transaction() can be extended with other operation,
> external snapshot or backing chain creation, is just one case it.

This read a bit awkwardly.  Might I suggest:

block: use callbacks in qmp_transaction()

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 |   68 +++++++++++++++++++++++++++++++++++++++++++++++++++--------
>  1 files changed, 58 insertions(+), 10 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 3e69569..9f0acfb 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -779,14 +779,35 @@ void qmp_blockdev_snapshot_sync(const char *device, const char *snapshot_file,
>  
>  
>  /* New and old BlockDriverState structs for group snapshots */
> +
> +/* Only in prepare() it may fail, so roll back just need to take care
> +   what is done in prepare(). */

Better might be:

/* 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. */
> +    int (*prepare)(BlockdevAction *action, void **p_opaque, Error **errp);

Based on the comments on 1/5, should prepare have a void signature and
just let errp serve to indicate failure?

-- 
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 5/5] block: make all steps in qmp_transaction() as callback

[Wenchao Xia]

于 2013-4-16 21:52, Kevin Wolf 写道:
> Am 13.04.2013 um 13:11 hat Wenchao Xia geschrieben:
>>    Now qmp_transaction() can be extended with other operation,
>> external snapshot or backing chain creation, is just one case it.
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>>   blockdev.c |   68 +++++++++++++++++++++++++++++++++++++++++++++++++++--------
>>   1 files changed, 58 insertions(+), 10 deletions(-)
>>
>> diff --git a/blockdev.c b/blockdev.c
>> index 3e69569..9f0acfb 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -779,14 +779,35 @@ void qmp_blockdev_snapshot_sync(const char *device, const char *snapshot_file,
>>
>>
>>   /* New and old BlockDriverState structs for group snapshots */
>> +
>> +/* Only in prepare() it may fail, so roll back just need to take care
>> +   what is done in prepare(). */
>> +typedef struct BdrvActionOps {
>> +    /* Prepare the work, must NOT be NULL. */
>> +    int (*prepare)(BlockdevAction *action, void **p_opaque, Error **errp);
>> +    /* Commit the changes, can be NULL. */
>> +    void (*commit)(BlockdevAction *action, void *opaque);
>> +    /* Rollback the changes on fail, mut NOT be NULL. */
>
> Typo: s/mut/must/
>
>> +    void (*rollback)(BlockdevAction *action, void *opaque);
>> +    /* Clean up resource in the end, can be NULL. */
>> +    void (*clean)(BlockdevAction *action, void *opaque);
>> +} BdrvActionOps;
>
> What's the reason that commit can be NULL, but rollback can't? I can't
> imagine a case where a commit isn't needed, but one without a rollback
> action sounds possible.
>
   Commit should no be NULL, will fix it together with other comments,
thanks.

>> +
>>   typedef struct BlkTransactionStates {
>> -    BlockDriverState *old_bs;
>> -    BlockDriverState *new_bs;
>> +    BlockdevAction *action;
>> +    void *opaque;
>> +    const BdrvActionOps *ops;
>>       QSIMPLEQ_ENTRY(BlkTransactionStates) entry;
>>   } BlkTransactionStates;
>>
>> +/* external snapshot private data */
>> +typedef struct ExternalSnapshotState {
>> +    BlockDriverState *old_bs;
>> +    BlockDriverState *new_bs;
>> +} ExternalSnapshotState;
>> +
>>   static int external_snapshot_prepare(BlockdevAction *action,
>> -                                     BlkTransactionStates *states,
>> +                                     void **p_opaque,
>
> Call it just opaque.
>
>>                                        Error **errp)
>>   {
>>       BlockDriver *proto_drv;
>> @@ -797,6 +818,7 @@ static int external_snapshot_prepare(BlockdevAction *action,
>>       const char *new_image_file;
>>       const char *format = "qcow2";
>>       enum NewImageMode mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;
>> +    ExternalSnapshotState *states;
>>
>>       /* get parameters */
>>       g_assert(action->kind == BLOCKDEV_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC);
>> @@ -817,6 +839,9 @@ static int external_snapshot_prepare(BlockdevAction *action,
>>           goto fail;
>>       }
>>
>> +    *p_opaque = g_malloc0(sizeof(ExternalSnapshotState));
>> +    states = *p_opaque;
>> +
>>       states->old_bs = bdrv_find(device);
>>       if (!states->old_bs) {
>>           error_set(errp, QERR_DEVICE_NOT_FOUND, device);
>> @@ -878,8 +903,10 @@ static int external_snapshot_prepare(BlockdevAction *action,
>>   }
>>
>>   static void external_snapshot_commit(BlockdevAction *action,
>> -                                     BlkTransactionStates *states)
>> +                                     void *opaque)
>>   {
>> +    ExternalSnapshotState *states = opaque;
>> +
>>       /* 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
>> @@ -890,13 +917,27 @@ static void external_snapshot_commit(BlockdevAction *action,
>>   }
>>
>>   static void external_snapshot_rollback(BlockdevAction *action,
>> -                                       BlkTransactionStates *states)
>> +                                       void *opaque)
>>   {
>> +    ExternalSnapshotState *states = opaque;
>>       if (states->new_bs) {
>>           bdrv_delete(states->new_bs);
>>       }
>>   }
>>
>> +static void external_snapshot_clean(BlockdevAction *action,
>> +                                    void *opaque)
>
> This fits in a single line.
>
>> +{
>> +    g_free(opaque);
>> +}
>> +
>> +const BdrvActionOps external_snapshot_ops = {
>> +    .prepare = external_snapshot_prepare,
>> +    .commit = external_snapshot_commit,
>> +    .rollback = external_snapshot_rollback,
>> +    .clean = external_snapshot_clean,
>> +};
>
> Please align all = to the same column.
>
> Kevin
>


-- 
Best Regards

Wenchao Xia