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

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

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 |  263 ++++++++++++++++++++++++++++++++++++++---------------------
 1 files changed, 169 insertions(+), 94 deletions(-)

[Qemu-devel] [PATCH V4 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 6e293e9..14784eb 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

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

[Eric Blake]

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

On 05/01/2013 08:26 PM, Wenchao Xia wrote:
> The code before really committing is moved into a function. Most
> code are simply moved from qmp_transaction(), except that on fail it

s/are/is/

> just return now. Other code such as input parsing is not touched,

s/return/returns/

> 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(-)
> 

Assuming the maintainer can correct commit messages without needing a v5.

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 --]

[Qemu-devel] [PATCH V4 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 14784eb..06100d7 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 V4 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 06100d7..26bc78e 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

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

[Eric Blake]

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

On 05/01/2013 08:26 PM, Wenchao Xia wrote:
> 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(-)

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 --]

[Qemu-devel] [PATCH V4 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 26bc78e..77adec8 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

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

[Eric Blake]

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

On 05/01/2013 08:26 PM, Wenchao Xia wrote:
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  blockdev.c |   11 ++++++++---
>  1 files changed, 8 insertions(+), 3 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 --]

[Qemu-devel] [PATCH V4 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 |   92 ++++++++++++++++++++++++++++++++++++++++++++++-------------
 1 files changed, 71 insertions(+), 21 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 77adec8..87ed99e 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
+   rollback() 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);
+    /* 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 +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_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);
     }
 }
 
+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,
+    },
+};
+
 /*
  * '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,31 @@ 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 +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 V4 5/5] block: make all steps in qmp_transaction() as callback

[Eric Blake]

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

On 05/01/2013 08:26 PM, Wenchao Xia wrote:
> 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 |   92 ++++++++++++++++++++++++++++++++++++++++++++++-------------
>  1 files changed, 71 insertions(+), 21 deletions(-)
> 

> +/*
> + * This structure must be arranged as first member in parent type, assuming

To me, "parent type" seems like the wrong relationship - in C++, the
parent type is the smaller type, and the child type is the larger type
that includes the parent type as its first member.  That is,
BlkTransationStates IS the parent type, and the comment would read
better as "first member in child type".

> + * 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;
> +};
>  

>  
>      /* Now we are going to do the actual pivot.  Everything up to this point
>       * is reversible, but we are committed at this point */

Is this comment still correct, now that things are extensible, or should
you s/pivot/commit/?

If you do respin, I agree that s/rollback/abort/ might be a nicer name
for that particular callback.  But I'm also quite okay with using the
patch as-is without a respin (while I pointed out two possible comment
wording changes, they don't make or break the patch for me).

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

[Wenchao Xia]

于 2013-5-3 22:46, Eric Blake 写道:
> On 05/01/2013 08:26 PM, Wenchao Xia wrote:
>> 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 |   92 ++++++++++++++++++++++++++++++++++++++++++++++-------------
>>   1 files changed, 71 insertions(+), 21 deletions(-)
>>
>
>> +/*
>> + * This structure must be arranged as first member in parent type, assuming
>
> To me, "parent type" seems like the wrong relationship - in C++, the
> parent type is the smaller type, and the child type is the larger type
> that includes the parent type as its first member.  That is,
> BlkTransationStates IS the parent type, and the comment would read
> better as "first member in child type".
>
>> + * 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;
>> +};
>>
>
>>
>>       /* Now we are going to do the actual pivot.  Everything up to this point
>>        * is reversible, but we are committed at this point */
>
> Is this comment still correct, now that things are extensible, or should
> you s/pivot/commit/?
>
> If you do respin, I agree that s/rollback/abort/ might be a nicer name
> for that particular callback.  But I'm also quite okay with using the
> patch as-is without a respin (while I pointed out two possible comment
> wording changes, they don't make or break the patch for me).
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
   I'll respin to fix comments and use "abort" as v5, thanks for your
reviewing!

-- 
Best Regards

Wenchao Xia

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

[Kevin Wolf]

Am 02.05.2013 um 04:26 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.
> 
> 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 |  263 ++++++++++++++++++++++++++++++++++++++---------------------
>  1 files changed, 169 insertions(+), 94 deletions(-)

I would have used s/rollback/abort/ for consistency with bdrv_reopen_*(),
but this doesn't make a difference for the correctness, so:

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

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

[Stefan Hajnoczi]

On Thu, May 02, 2013 at 10:26:41AM +0800, Wenchao Xia wrote:
>   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.
> 
> 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 |  263 ++++++++++++++++++++++++++++++++++++++---------------------
>  1 files changed, 169 insertions(+), 94 deletions(-)

Good for QEMU 1.6.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

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

[Luiz Capitulino]

On Fri, 3 May 2013 13:17:59 +0200
Stefan Hajnoczi <stefanha@gmail.com> wrote:

> On Thu, May 02, 2013 at 10:26:41AM +0800, Wenchao Xia wrote:
> >   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.
> > 
> > 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 |  263 ++++++++++++++++++++++++++++++++++++++---------------------
> >  1 files changed, 169 insertions(+), 94 deletions(-)
> 
> Good for QEMU 1.6.
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Looks good to me too. Are you going to apply v5 to the block branch?