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 <xiaw...@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
