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

[Stefan Hajnoczi]

On Tue, Mar 12, 2013 at 04:30:41PM +0800, Wenchao Xia wrote:
> 于 2013-1-15 15:03, Wenchao Xia 写道:
> >于 2013-1-14 18:06, Stefan Hajnoczi 写道:
> >>On Mon, Jan 14, 2013 at 10:56:30AM +0800, Wenchao Xia wrote:
> >>>于 2013-1-11 17:12, Stefan Hajnoczi 写道:
> >>>>On Fri, Jan 11, 2013 at 02:22:28PM +0800, Wenchao Xia wrote:
> >>>>>于 2013-1-10 20:41, Stefan Hajnoczi 写道:
> >>>>>>On Thu, Jan 10, 2013 at 11:21:22AM +0800, Wenchao Xia wrote:
> >>>>>>>于 2013-1-9 20:44, Stefan Hajnoczi 写道:
> >>>>>>>>On Mon, Jan 07, 2013 at 03:28:06PM +0800, Wenchao Xia wrote:
> >>>>>>>>>   This patch switch to internal common API to take group external
> >>>>>>>>>snapshots from qmp_transaction interface. qmp layer simply does
> >>>>>>>>>a translation from user input.
> >>>>>>>>>
> >>>>>>>>>Signed-off-by: Wenchao Xia <address@hidden>
> >>>>>>>>>---
> >>>>>>>>>  blockdev.c |  215
> >>>>>>>>>++++++++++++++++++++++++------------------------------------
> >>>>>>>>>  1 files changed, 87 insertions(+), 128 deletions(-)
> >>>>>>>>
> >>>>>>>>An internal API for snapshots is not necessary.
> >>>>>>>>qmp_transaction() is
> >>>>>>>>already usable both from the monitor and C code.
> >>>>>>>>
> >>>>>>>>The QAPI code generator creates structs that can be accessed
> >>>>>>>>directly
> >>>>>>>>from C.  qmp_transaction(), BlockdevAction, and
> >>>>>>>BlockdevActionList *is*
> >>>>>>>>the snapshot API.  It just doesn't support internal snapshots
> >>>>>>>>yet, which
> >>>>>>>>is what you are trying to add.
> >>>>>>>>
> >>>>>>>>To add internal snapshot support, define a
> >>>>>>>>BlockdevInternalSnapshot type
> >>>>>>>>in qapi-schema.json and add internal snapshot support in
> >>>>>>>>qmp_transaction().
> >>>>>>>>
> >>>>>>>>qmp_transaction() was designed with this in mind from the
> >>>>>>>>beginning and
> >>>>>>>>dispatches based on BlockdevAction->kind.
> >>>>>>>>
> >>>>>>>>The patch series will become much smaller while still adding
> >>>>>>>>internal
> >>>>>>>>snapshot support.
> >>>>>>>>
> >>>>>>>>Stefan
> >>>>>>>>
> >>>>>>>
> >>>>>>>   As API, qmp_transaction have following disadvantages:
> >>>>>>>1) interface is based on string not data type inside qemu, that
> >>>>>>>means
> >>>>>>>other function calling it result in: bdrv->string->bdrv
> >>>>>>
> >>>>>>Use bdrv_get_device_name().  You already need to fill in filename or
> >>>>>>snapshot name strings.  This is not a big disadvantage.
> >>>>>>
> >>>>>   Yes, not a big disadvantage, but why not save string operation but
> >>>>>use (bdrv*) as much as possible?
> >>>>>
> >>>>>what happens will be:
> >>>>>
> >>>>>hmp-snapshot
> >>>>>     |
> >>>>>qmp-snapshot
> >>>>>     |---------
> >>>>>              |
> >>>>>         qmp-transaction            savevm(may be other..)
> >>>>>              |----------------------|
> >>>>>                             |
> >>>>>               internal transaction layer
> >>>>
> >>>>Saving the string operation is not worth duplicating the API.
> >>>>
> >>>   I agree with you for this line:), but,  it is a weight on the balance
> >>>of choice, pls consider it together with issues below.
> >>>
> >>>>>>>2) all capability are forced to be exposed.
> >>>>>>
> >>>>>>Is there something you cannot expose?
> >>>>>>
> >>>>>   As other component in qemu can use it, some option may
> >>>>>be used only in qemu not to user. For eg, vm-state-size.
> >>>>
> >>>>When we hit a limitation of QAPI then it needs to be extended.  I'm
> >>>>sure
> >>>>there's a solution for splitting or hiding parts of the QAPI generated
> >>>>API.
> >>>>
> >>>   I can't think it out now, it seems to be a bit tricky.
> >>>
> >>>>>>>3) need structure to record each transaction state, such as
> >>>>>>>BlkTransactionStates. Extending it is equal to add an internal
> >>>>>>>layer.
> >>>>>>
> >>>>>>I agree that extending it is equal coding effort to adding an
> >>>>>>internal
> >>>>>>layer because you'll need to refactor qmp_transaction() a bit to
> >>>>>>really
> >>>>>>support additional action types.
> >>>>>>
> >>>>>>But it's the right thing to do.  Don't add unnecessary layers just
> >>>>>>because writing new code is more fun than extending existing code.
> >>>>>>
> >>>>>  If this layer is not added but depending only qmp_transaction, there
> >>>>>will be many "if else" fragment. I have tried that and the code
> >>>>>is awkful, this layer did not bring extra burden only make what
> >>>>>happens inside qmp_transaction clearer, I did not add this layer just
> >>>>>for fun.
> >>>>>
> >>>>>
> >>>>>>>   Actually I started up by use qmp_transaction as API, but soon
> >>>>>>>found that work is almost done around BlkTransactionStates, so
> >>>>>>>added a layer around it clearly.
> >>>>
> >>>>The qmp_transaction() implementation can be changed, I'm not saying you
> >>>>have to hack in more if statements.  It's cleanest to introduce a
> >>>>BdrvActionOps abstraction:
> >>>>
> >>>>typedef struct BdrvActionOps BdrvActionOps;
> >>>>typedef struct BdrvTransactionState {
> >>>>     const BdrvActionOps *ops;
> >>>>     QLIST_ENTRY(BdrvTransactionState);
> >>>>} BdrvTransactionState;
> >>>>
> >>>>struct BdrvActionOps {
> >>>>     int (*prepare)(BdrvTransactionState *s, ...);
> >>>>     int (*commit)(BdrvTransactionState *s, ...);
> >>>>     int (*rollback)(BdrvTransactionState *s, ...);
> >>>>};
> >>>>
> >>>>BdrvTransactionState *bdrv_transaction_create(BlockdevAction *action);
> >>>>
> >>>>Then qmp_transaction() can be generic code that steps through the
> >>>>transactions.
> >>>   With internal API, qmp_transaction can still be generic code with
> >>>a translate from bdrv* to char* at caller level.
> >>>
> >>>   This is similar to what your series does and I think it's
> >>>>the right direction.
> >>>>
> >>>>But please don't duplicate the qmp_transaction() and
> >>>>BlockdevAction/BlockdevActionList APIs.  In other words, change the
> >>>>engine, not the whole car.
> >>>>
> >>>>Stefan
> >>>>
> >>>
> >>>   If my understanding is correct, the BdrvActionOps need to be extended
> >>>as following:
> >>>struct BdrvActionOps {
> >>>      /* need following for callback functions */
> >>>      const char *sn_name;
> >>>      BlockDriverState *bs;
> >>>      ...
> >>>      int (*prepare)(BdrvTransactionState *s, ...);
> >>>      int (*commit)(BdrvTransactionState *s, ...);
> >>>      int (*rollback)(BdrvTransactionState *s, ...);
> >>>};
> >>>Or an opaque* should used for every BdrvActionOps.
> >>
> >>It is nice to keep *Ops structs read-only so they can be static const.
> >>This way the ops are shared between all instances of the same action
> >>type.  Also the function pointers can be in read-only memory pages,
> >>which is a slight security win since it prevents memory corruption
> >>exploits from taking advantage of function pointers to execute arbitrary
> >>code.
> >>
> >   Seems good, I will package callback functions into *Ops, thanks.
> >
> >>In the pseudo-code I posted the sn_name or bs fields go into an
> >>action-specific state struct:
> >>
> >>typedef struct {
> >>     BdrvTransactionState common;
> >>     char *backup_sn_name;
> >>} InternalSnapshotTransactionState;
> >>
> >>typedef struct {
> >>     BdrvTransactionState common;
> >>     BlockDriverState *old_bs;
> >>     BlockDriverState *new_bs;
> >>} ExternalSnapshotTransactionState;
> >>
> >>>Comparation:
> >>>The way above:
> >>>  1) translate from BlockdevAction to BdrvTransactionState by
> >>>     bdrv_transaction_create().
> >>>  2) enqueue BdrvTransactionState by
> >>>     some code.
> >>>  3) execute them by
> >>>     a new function, name it as BdrvActionOpsRun().
> >>
> >>If you include .prepare() in the transaction creation, then it becomes
> >>simpler:
> >>
> >>states = []
> >>for action in actions:
> >>     result = bdrv_transaction_create(action)  # invokes .prepare()
> >>     if result is error:
> >>         for state in states:
> >>        state.rollback()
> >>    return
> >>     states.append(result)
> >>for state in states:
> >>     state.commit()
> >>
> >>Because we don't wait until BdrvActionOpsRun() before processing the
> >>transaction, there's no need to translate from BlockdevAction to
> >>BdrvTransactionState.  The BdrvTransactionState struct really only has
> >>state required to commit/rollback the transaction.
> >>
> >>(Even if it becomes necessary to keep information from BlockdevAction
> >>after .prepare() returns, just keep a pointer to BlockdevAction.  Don't
> >>duplicate it.)
> >>
> >   OK, *BlockdevAction plus *BlockDriverState and some other
> >data used internal will be added in states.
> >
> >>>Internal API way:
> >>>  1) translate BlockdevAction to BlkTransStates by
> >>>     fill_blk_trs().
> >>>  2) enqueue BlkTransStates to BlkTransStates by
> >>>     add_transaction().
> >>>  3) execute them by
> >>>     submit_transaction().
> >>>
> >>>   It seems the way above will end as something like an internal
> >>>layer, but without clear APIs tips what it is doing. Please reconsider
> >>>the advantages about a clear internal API layer.
> >>
> >>I'm not convinced by the internal API approach.  It took me a while when
> >>reviewing the code before I understood what was actually going on
> >>because of the qmp_transaction() and BlockdevAction duplication code.
> >>
> >>I see the internal API approach as an unnecessary layer of indirection.
> >>It makes the code more complicated to understand and maintain.  Next
> >>time we add something to qmp_transaction() it would also be necessary to
> >>duplicate that change for the internal API.  It creates unnecessary
> >>work.
> >>
> >   Basic process is almost the same in two approaches, I'd like to
> >adjust the code to avoid data duplication as much as possible, and
> >see if some function can be removed when code keeps clear, in next
> >version.
> >
> >>Just embrace QAPI, the point of it was to eliminate these external <->
> >>internal translations we were doing all the time.
> >>
> >>Stefan
> >>
> >
> >
> Hi, Stefan
>   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;
> 
> typedef struct BlkTransactionStates {
>     BlockdevAction *action;
>     void *opaque;
>     BdrvActionOps *ops;
>     QSIMPLEQ_ENTRY(BlkTransactionStates) entry;
> } BlkTransactionStates;
> 
> /* call ops->check and return state* to be enqueued */
> static BlkTransactionStates *transaction_create(BlockdevAction *action,
>                                                 Error **errp);
> 
> void qmp_transaction(BlockdevActionList *dev_list, Error **errp)
> {
>     BlockdevActionList *dev_entry = dev_list;
>     BlkTransactionStates *state;
> 
>     while (NULL != dev_entry) {
>         state = transaction_create(dev_entry->value, errp);
>         /* enqueue */
>         dev_entry = dev_entry->next;
>     }
> 
>     /* use queue with submit, commit, rollback callback */
> }
> 
> 
>   In this way, parameter duplication is saved, but one problem remains:
> parameter can't be hidden to user such as vm_state_size, but this would
> not be a problem if hmp "savevm" use his own code about block snapshot
> later, I mean not use qmp_transaction(). What do you think about the
> design? Do you have a better way to solve this problem?

Can you explain the vm_state_size problem again?  Sorry I forgot - I
think it had something to do with having an internal parameter in the
action that should not be exposed via QMP/HMP?

Stefan