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

[Wenchao Xia <xiawenc@linux.vnet.ibm.com>]

于 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