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

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

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

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