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

[Stefan Hajnoczi]

On Wed, Mar 13, 2013 at 09:36:06AM +0800, Wenchao Xia wrote:
> 于 2013-3-12 23:43, 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?
> > 
>   Yep, this parameter will be used by qemu "live savevm" code later,
> but should not expose it to user in qmp interface.

The simplest solution is:

void bdrv_transaction(...)
{
    /* Common code here */
}

void qmp_transaction(...)
{
    /* Reject optional internal fields here */
    if (opts->has_vm_state_size) {
        error_setg(errp, "internal field vm_state_size must not be set");
        return;
    }

    bdrv_transaction(...);
}

If transaction is used from inside QEMU with vm_state_size, then call
bdrv_transaction() instead of qmp_transaction() to skip the public field
checks.

My second idea is to add QAPI syntax to make a field as "private" or
"internal".  The marshalling code will skip or return an error if the
field is set.

This is a little nicer since all callers use qmp_transaction() but we're
guaranteed that external JSON cannot make use of the field.

Stefan