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

[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.

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.)

> 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.

Just embrace QAPI, the point of it was to eliminate these external <->
internal translations we were doing all the time.

Stefan