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