[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH V2 07/10] snapshot: qmp use new internal API for
From: |
Wenchao Xia |
Subject: |
Re: [Qemu-devel] [PATCH V2 07/10] snapshot: qmp use new internal API for external snapshot transaction |
Date: |
Wed, 13 Mar 2013 09:36:06 +0800 |
User-agent: |
Mozilla/5.0 (Windows NT 5.1; rv:17.0) Gecko/20130215 Thunderbird/17.0.3 |
于 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 <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.
>>>>>>>>>>
>>>>>>>>>> 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