On Wed, Mar 13, 2013 at 09:36:06AM +0800, Wenchao Xia wrote: > 于 2013-3-12 23:43, Stefan Hajnoczi 写道: > > On Tue, Mar 12, 2013 at 04:30:41PM +0800, Wenchao Xia wrote: > >> 于 2013-1-15 15:03, Wenchao Xia 写道: > >>> 于 2013-1-14 18:06, 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 <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. > >>>> > >>> Seems good, I will package callback functions into *Ops, thanks. > >>> > >>>> 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.) > >>>> > >>> OK, *BlockdevAction plus *BlockDriverState and some other > >>> data used internal will be added in states. > >>> > >>>>> 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. > >>>> > >>> Basic process is almost the same in two approaches, I'd like to > >>> adjust the code to avoid data duplication as much as possible, and > >>> see if some function can be removed when code keeps clear, in next > >>> version. > >>> > >>>> Just embrace QAPI, the point of it was to eliminate these external <-> > >>>> internal translations we were doing all the time. > >>>> > >>>> Stefan > >>>> > >>> > >>> > >> Hi, Stefan > >> I redesigned the structure, Following is the fake code: > >> > >> typedef struct BdrvActionOps { > >> /* check the request's validation, allocate p_opaque if needed */ > >> int (*check)(BlockdevAction *action, void **p_opaque, Error **errp); > >> /* take the action */ > >> int (*submit)(BlockdevAction *action, void *opaque, Error **errp); > >> /* update emulator */ > >> int (*commit)(BlockdevAction *action, void *opaque, Error **errp); > >> /* cancel the action */ > >> int (*rollback)(BlockdevAction *action, void *opaque, Error **errp); > >> } BdrvActionOps; > >> > >> typedef struct BlkTransactionStates { > >> BlockdevAction *action; > >> void *opaque; > >> BdrvActionOps *ops; > >> QSIMPLEQ_ENTRY(BlkTransactionStates) entry; > >> } BlkTransactionStates; > >> > >> /* call ops->check and return state* to be enqueued */ > >> static BlkTransactionStates *transaction_create(BlockdevAction *action, > >> Error **errp); > >> > >> void qmp_transaction(BlockdevActionList *dev_list, Error **errp) > >> { > >> BlockdevActionList *dev_entry = dev_list; > >> BlkTransactionStates *state; > >> > >> while (NULL != dev_entry) { > >> state = transaction_create(dev_entry->value, errp); > >> /* enqueue */ > >> dev_entry = dev_entry->next; > >> } > >> > >> /* use queue with submit, commit, rollback callback */ > >> } > >> > >> > >> In this way, parameter duplication is saved, but one problem remains: > >> parameter can't be hidden to user such as vm_state_size, but this would > >> not be a problem if hmp "savevm" use his own code about block snapshot > >> later, I mean not use qmp_transaction(). What do you think about the > >> design? Do you have a better way to solve this problem? > > > > Can you explain the vm_state_size problem again? Sorry I forgot - I > > think it had something to do with having an internal parameter in the > > action that should not be exposed via QMP/HMP? > > > Yep, this parameter will be used by qemu "live savevm" code later, > but should not expose it to user in qmp interface.
The simplest solution is: void bdrv_transaction(...) { /* Common code here */ } void qmp_transaction(...) { /* Reject optional internal fields here */ if (opts->has_vm_state_size) { error_setg(errp, "internal field vm_state_size must not be set"); return; } bdrv_transaction(...); } If transaction is used from inside QEMU with vm_state_size, then call bdrv_transaction() instead of qmp_transaction() to skip the public field checks. My second idea is to add QAPI syntax to make a field as "private" or "internal". The marshalling code will skip or return an error if the field is set. This is a little nicer since all callers use qmp_transaction() but we're guaranteed that external JSON cannot make use of the field. Stefan