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 <xiaw...@linux.vnet.ibm.com>
> >>>>---
> >>>> 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
