Re: [Qemu-devel] [PATCH V2 07/10] snapshot: qmp use new internal API for external snapshot transaction

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

于 2013-3-14 16:22, Kevin Wolf 写道:
> Am 14.03.2013 um 06:08 hat Wenchao Xia geschrieben:
>> 于 2013-3-13 18:18, Kevin Wolf 写道:
>>> Am 12.03.2013 um 09:30 hat Wenchao Xia geschrieben:
>>>>    I redesigned the structure, Following is the fake code:
>>>>
>>>> typedef struct BdrvActionOps {
>>>>      /* check the request's validation, allocate p_opaque if needed */
>>>>      int (*check)(BlockdevAction *action, void **p_opaque, Error **errp);
>>>>      /* take the action */
>>>>      int (*submit)(BlockdevAction *action, void *opaque, Error **errp);
>>>>      /* update emulator */
>>>>      int (*commit)(BlockdevAction *action, void *opaque, Error **errp);
>>>>      /* cancel the action */
>>>>      int (*rollback)(BlockdevAction *action, void *opaque, Error **errp);
>>>> } BdrvActionOps;
>>>
>>> Why do you need the split of prepare into check/submit?
>>>
>>> If you have prepare/commit/abort, everybody will recognise this as the
>>> standard transaction pattern because this is just how it's done.
>>> Deviating from it needs a good justification in my opinion.
>>>
>>> Kevin
>>>
>>
>>    My thought is rejecting the request in *check if parameter invalid
>> before take any action, while submit do the real action, to reduce
>> the chance to of rolling back when some request not valid in the batch.
>
> Okay, so it's not strictly needed, but an optimisation of the error
> case?
>
> Does it work well when the transaction includes an operation that
> depends on the previous one, like create a snapshot and then do
> something with this snapshot?
>
   This seems to complex, since prepare of all actions are executed
in first round, they may interrupt each other later. So I am
thinking make it more clear as complete one job one time, which
may change the old qmp_transcation() logic a little.

> Anyway, even if we think it works and is worth the effort to optimise
> such error cases, please use names that are consistent with the
> transactions used for reopening: (check/)prepare/commit/abort.
>
   In above way check/prepare can be merged, how do you think of it?

> Kevin
>


-- 
Best Regards

Wenchao Xia