Re: [Qemu-devel] [PATCH 2/3] block: adjust qmp_transaction to be extendable

[Wenchao Xia]

于 2013-4-17 22:42, Stefan Hajnoczi 写道:
> On Mon, Apr 01, 2013 at 06:01:30PM +0800, Wenchao Xia wrote:
> >   /* New and old BlockDriverState structs for group snapshots */
> > -typedef struct BlkTransactionStates {
> > +typedef struct BdrvActionOps {
> > +    int (*commit)(BlockdevAction *action, void **p_opaque, Error **errp);
> > +    void (*rollback)(BlockdevAction *action, void *opaque);
> > +    void (*clean)(BlockdevAction *action, void *opaque);
>
> Please document these functions.
  OK.

> > +const BdrvActionOps external_snapshot_ops = {
> > +    .commit = external_snapshot_commit,
> > +    .rollback = external_snapshot_rollback,
> > +    .clean = external_snapshot_clean,
> > +};
> > +
> > +typedef struct BlkTransactionStates {
> > +    BlockdevAction *action;
> > +    void *opaque;
> > +    const BdrvActionOps *ops;
> >       QSIMPLEQ_ENTRY(BlkTransactionStates) entry;
> >   } BlkTransactionStates;
>
> The relationship between BlkTransactionStates and ExternalSnapshotState
> can be simplified:
>
> typedef struct {
>      int (*commit)(BlkTransactionState *state, Error **errp);
>      void (*rollback)(BlkTransactionState *state);
>      void (*clean)(BlkTransactionState *state);
>      size_t instance_size;
> } BdrvActionOps;
>
> typedef struct BlkTransactionState {
>      BlockDevAction *action;
>      const BdrvActionOps *ops;
>      QSIMPLEQ_ENTRY(BlkTransactionState) entry;
> } BlkTransactionState;
>
> typedef struct {
>      BlkTransactionState common;
>      BlockDriverState *old_bs;
>      BlockDriverState *new_bs;
> } ExternalSnapshotState;
>
> const BdrvActionOps external_snapshot_ops = {
>      .commit = external_snapshot_commit,
>      .rollback = external_snapshot_rollback,
>      .clean = external_snapshot_clean,
>      .instance_size = sizeof(ExternalSnapshotState);
> };
>
> This eliminates the opaque pointer and g_free(state) can be handled by
> qmp_transaction().  This way action types no longer need to free opaque.
  Where should be ExternalSnapshotState* placed, insert it into
BlkTransactionState, or BdrvActionOps?

--
Best Regards

Wenchao Xia