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

[Stefan Hajnoczi]

On Thu, Apr 18, 2013 at 5:00 AM, Wenchao Xia <address@hidden> wrote:
> 于 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?

No explicit pointer is needed since ExternalSnapshotState embeds
BlkTransactionState.  See container_of() or DO_UPCAST().

Stefan