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

[Wenchao Xia]

于 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