Re: [Qemu-devel] [PATCH V2 5/5] block: make all steps in qmp_transaction() as callback

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1584 bytes --]

On 04/13/2013 05:11 AM, Wenchao Xia wrote:
>   Now qmp_transaction() can be extended with other operation,
> external snapshot or backing chain creation, is just one case it.

This read a bit awkwardly.  Might I suggest:

block: use callbacks in qmp_transaction()

Make it easier to add other operations to qmp_transaction() by using
callbacks, with external snapshots serving as an example implementation
of the callbacks.

> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  blockdev.c |   68 +++++++++++++++++++++++++++++++++++++++++++++++++++--------
>  1 files changed, 58 insertions(+), 10 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 3e69569..9f0acfb 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -779,14 +779,35 @@ void qmp_blockdev_snapshot_sync(const char *device, const char *snapshot_file,
>  
>  
>  /* New and old BlockDriverState structs for group snapshots */
> +
> +/* Only in prepare() it may fail, so roll back just need to take care
> +   what is done in prepare(). */

Better might be:

/* Only prepare() may fail.  In a single transaction, only one of
commit() or rollback() will be called.  */

> +typedef struct BdrvActionOps {
> +    /* Prepare the work, must NOT be NULL. */
> +    int (*prepare)(BlockdevAction *action, void **p_opaque, Error **errp);

Based on the comments on 1/5, should prepare have a void signature and
just let errp serve to indicate failure?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]