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-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

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

>> 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.
>
> BlkTransactionStates is only relevant to external snapshots because they
> change the BlkDriverState chain and must be able to undo that.
>
> For internal snapshots you simply need to store the snapshot name so you
> can delete it if there is an error partway through.  (BTW it's not
> possible to completely restore state if you allow overwriting existing
> internal snapshots unless you do something like taking a backup snapshot
> of the existing snapshot first!)
>
   Still I need BlkTransactionStates to record BlkDriverState because
the granularity is block device. And yes it is not possible to
completely restore state in overwrite case, which can be solved later
and mark it in comments now.

> Stefan
>


-- 
Best Regards

Wenchao Xia