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

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

> 2) all capability are forced to be exposed.

Is there something you cannot expose?

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

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

Stefan