Re: [Qemu-devel] [PATCH V2 07/10] snapshot: qmp use new internal API for external snapshot transaction

[Wenchao Xia <xiawenc@linux.vnet.ibm.com>]

于 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 <xiawenc@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.

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().

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.

-- 
Best Regards

Wenchao Xia