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