|
| From: | Wenchao Xia |
| Subject: | Re: [Qemu-devel] [PATCH V2 07/10] snapshot: qmp use new internal API for external snapshot transaction |
| Date: | Tue, 12 Mar 2013 16:30:41 +0800 |
| User-agent: | Mozilla/5.0 (Windows NT 5.1; rv:17.0) Gecko/20130215 Thunderbird/17.0.3 |
于 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 <address@hidden> --- 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. StefanAs 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->bdrvUse 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 layerSaving 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'sthe right direction. But please don't duplicate the qmp_transaction() and BlockdevAction/BlockdevActionList APIs. In other words, change the engine, not the whole car. StefanIf 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?
--
Best Regards
Wenchao Xia
| [Prev in Thread] | Current Thread | [Next in Thread] |