[V2,07/10] snapshot: qmp use new internal API for external snapshot transaction

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

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

于 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
2) all capability are forced to be exposed.
3) need structure to record each transaction state, such as
BlkTransactionStates. Extending it is equal to add an internal layer.

   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.

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.

> 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

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

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.

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

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

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

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

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.

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

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

Just embrace QAPI, the point of it was to eliminate these external <->
internal translations we were doing all the time.

Stefan

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

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

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

Stefan

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

On Wed, Mar 13, 2013 at 09:36:06AM +0800, Wenchao Xia wrote:
> 于 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 <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.
> >>>>
> >>>> 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.

The simplest solution is:

void bdrv_transaction(...)
{
    /* Common code here */
}

void qmp_transaction(...)
{
    /* Reject optional internal fields here */
    if (opts->has_vm_state_size) {
        error_setg(errp, "internal field vm_state_size must not be set");
	return;
    }

    bdrv_transaction(...);
}

If transaction is used from inside QEMU with vm_state_size, then call
bdrv_transaction() instead of qmp_transaction() to skip the public field
checks.

My second idea is to add QAPI syntax to make a field as "private" or
"internal".  The marshalling code will skip or return an error if the
field is set.

This is a little nicer since all callers use qmp_transaction() but we're
guaranteed that external JSON cannot make use of the field.

Stefan

Am 12.03.2013 um 09:30 hat Wenchao Xia geschrieben:
>   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;

Why do you need the split of prepare into check/submit?

If you have prepare/commit/abort, everybody will recognise this as the
standard transaction pattern because this is just how it's done.
Deviating from it needs a good justification in my opinion.

Kevin

于 2013-3-13 18:18, Kevin Wolf 写道:
> Am 12.03.2013 um 09:30 hat Wenchao Xia geschrieben:
>>    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;
>
> Why do you need the split of prepare into check/submit?
>
> If you have prepare/commit/abort, everybody will recognise this as the
> standard transaction pattern because this is just how it's done.
> Deviating from it needs a good justification in my opinion.
>
> Kevin
>

   My thought is rejecting the request in *check if parameter invalid
before take any action, while submit do the real action, to reduce
the chance to of rolling back when some request not valid in the batch.

Am 14.03.2013 um 06:08 hat Wenchao Xia geschrieben:
> 于 2013-3-13 18:18, Kevin Wolf 写道:
> >Am 12.03.2013 um 09:30 hat Wenchao Xia geschrieben:
> >>   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;
> >
> >Why do you need the split of prepare into check/submit?
> >
> >If you have prepare/commit/abort, everybody will recognise this as the
> >standard transaction pattern because this is just how it's done.
> >Deviating from it needs a good justification in my opinion.
> >
> >Kevin
> >
> 
>   My thought is rejecting the request in *check if parameter invalid
> before take any action, while submit do the real action, to reduce
> the chance to of rolling back when some request not valid in the batch.

Okay, so it's not strictly needed, but an optimisation of the error
case?

Does it work well when the transaction includes an operation that
depends on the previous one, like create a snapshot and then do
something with this snapshot?

Anyway, even if we think it works and is worth the effort to optimise
such error cases, please use names that are consistent with the
transactions used for reopening: (check/)prepare/commit/abort.

Kevin

于 2013-3-14 16:22, Kevin Wolf 写道:
> Am 14.03.2013 um 06:08 hat Wenchao Xia geschrieben:
>> 于 2013-3-13 18:18, Kevin Wolf 写道:
>>> Am 12.03.2013 um 09:30 hat Wenchao Xia geschrieben:
>>>>    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;
>>>
>>> Why do you need the split of prepare into check/submit?
>>>
>>> If you have prepare/commit/abort, everybody will recognise this as the
>>> standard transaction pattern because this is just how it's done.
>>> Deviating from it needs a good justification in my opinion.
>>>
>>> Kevin
>>>
>>
>>    My thought is rejecting the request in *check if parameter invalid
>> before take any action, while submit do the real action, to reduce
>> the chance to of rolling back when some request not valid in the batch.
>
> Okay, so it's not strictly needed, but an optimisation of the error
> case?
>
> Does it work well when the transaction includes an operation that
> depends on the previous one, like create a snapshot and then do
> something with this snapshot?
>
   This seems to complex, since prepare of all actions are executed
in first round, they may interrupt each other later. So I am
thinking make it more clear as complete one job one time, which
may change the old qmp_transcation() logic a little.

> Anyway, even if we think it works and is worth the effort to optimise
> such error cases, please use names that are consistent with the
> transactions used for reopening: (check/)prepare/commit/abort.
>
   In above way check/prepare can be merged, how do you think of it?

> Kevin
>