于 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.
> Stefan > -- Best Regards Wenchao Xia