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.