δΊ 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