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

[Wenchao Xia]

于 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