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

[Wenchao Xia]

于 2013-4-16 21:52, Kevin Wolf 写道:
> 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 <address@hidden>
> > ---
> >   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.
  Commit should no be NULL, will fix it together with other comments,
thanks.

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


--
Best Regards

Wenchao Xia