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

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

于 2013-5-3 22:46, Eric Blake 写道:
> On 05/01/2013 08:26 PM, Wenchao Xia wrote:
>> Make it easier to add other operations to qmp_transaction() by using
>> callbacks, with external snapshots serving as an example implementation
>> of the callbacks.
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>>   blockdev.c |   92 ++++++++++++++++++++++++++++++++++++++++++++++-------------
>>   1 files changed, 71 insertions(+), 21 deletions(-)
>>
>
>> +/*
>> + * This structure must be arranged as first member in parent type, assuming
>
> To me, "parent type" seems like the wrong relationship - in C++, the
> parent type is the smaller type, and the child type is the larger type
> that includes the parent type as its first member.  That is,
> BlkTransationStates IS the parent type, and the comment would read
> better as "first member in child type".
>
>> + * that compiler will also arrange it to the same address with parent instance.
>> + * Later it will be used in free().
>> + */
>> +struct BlkTransactionStates {
>> +    BlockdevAction *action;
>> +    const BdrvActionOps *ops;
>> +    QSIMPLEQ_ENTRY(BlkTransactionStates) entry;
>> +};
>>
>
>>
>>       /* Now we are going to do the actual pivot.  Everything up to this point
>>        * is reversible, but we are committed at this point */
>
> Is this comment still correct, now that things are extensible, or should
> you s/pivot/commit/?
>
> If you do respin, I agree that s/rollback/abort/ might be a nicer name
> for that particular callback.  But I'm also quite okay with using the
> patch as-is without a respin (while I pointed out two possible comment
> wording changes, they don't make or break the patch for me).
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
   I'll respin to fix comments and use "abort" as v5, thanks for your
reviewing!

-- 
Best Regards

Wenchao Xia