[Qemu-devel] [PATCH 0/6] snapshot: take snapshots in unified way

[Qemu-devel] [PATCH 0/6] snapshot: take snapshots in unified way

[Wenchao Xia]

  These patch added a seperated layer to take internal or external snapshots
in a unified way, the granularity is block device, so other functions can
just combine the request and submit, such as group snapshot, savevm.

Total goal are:
  Live back up vm in external or internal image, which need three function:
1 live snapshot block device internal/external.
2 live save vmstate internal/external.
3 combination of the function unit.

  This patch basically provide function one in unified style with granularity
of device.

Wenchao Xia (6):
  snapshot: export function in block.c
  snapshot: add error set function
  snapshot: design of common API to take snapshots
  snapshot: implemention of common API to take snapshots
  snapshot: qmp interface
  snapshot: human monitor interface

 block.c          |   30 ++
 block.h          |    3 +
 blockdev.c       |  833 +++++++++++++++++++++++++++++++++++++++++++++---------
 blockdev.h       |  129 +++++++++
 error.c          |   23 ++
 error.h          |    9 +
 hmp-commands.hx  |   50 +++-
 hmp.c            |   30 ++-
 hmp.h            |    1 +
 monitor.c        |   21 +-
 qapi-schema.json |  102 ++++++-
 savevm.c         |   77 ++++--
 sysemu.h         |    2 +-
 13 files changed, 1117 insertions(+), 193 deletions(-)

[Qemu-devel] [PATCH 1/6] snapshot: export function in block.c

[Wenchao Xia]

  This patch moves bdrv_snapshotfind from savevm.c to block.c and export
it, also added bdrv_deappend in block.c.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 block.c  |   30 ++++++++++++++++++++++++++++++
 block.h  |    3 +++
 savevm.c |   22 ----------------------
 3 files changed, 33 insertions(+), 22 deletions(-)

diff --git a/block.c b/block.c
index 0668c4b..61c7c6a 100644
--- a/block.c
+++ b/block.c
@@ -1376,6 +1376,13 @@ void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top)
             bs_new->drv ? bs_new->drv->format_name : "");
 }
 
+/* revert the action */
+void bdrv_deappend(BlockDriverState *bs_new, BlockDriverState *bs_top)
+{
+    bdrv_swap(bs_new, bs_top);
+    /* this is enough? */
+}
+
 void bdrv_delete(BlockDriverState *bs)
 {
     assert(!bs->dev);
@@ -3210,6 +3217,29 @@ int bdrv_snapshot_load_tmp(BlockDriverState *bs,
     return -ENOTSUP;
 }
 
+int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
+                       const char *name)
+{
+    QEMUSnapshotInfo *sn_tab, *sn;
+    int nb_sns, i, ret;
+
+    ret = -ENOENT;
+    nb_sns = bdrv_snapshot_list(bs, &sn_tab);
+    if (nb_sns < 0) {
+        return ret;
+    }
+    for (i = 0; i < nb_sns; i++) {
+        sn = &sn_tab[i];
+        if (!strcmp(sn->id_str, name) || !strcmp(sn->name, name)) {
+            *sn_info = *sn;
+            ret = 0;
+            break;
+        }
+    }
+    g_free(sn_tab);
+    return ret;
+}
+
 /* backing_file can either be relative, or absolute, or a protocol.  If it is
  * relative, it must be relative to the chain.  So, passing in bs->filename
  * from a BDS as backing_file should not be done, as that may be relative to
diff --git a/block.h b/block.h
index 893448a..2322b13 100644
--- a/block.h
+++ b/block.h
@@ -130,6 +130,7 @@ BlockDriverState *bdrv_new(const char *device_name);
 void bdrv_make_anon(BlockDriverState *bs);
 void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old);
 void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top);
+void bdrv_deappend(BlockDriverState *bs_new, BlockDriverState *bs_top);
 void bdrv_delete(BlockDriverState *bs);
 int bdrv_parse_cache_flags(const char *mode, int *flags);
 int bdrv_file_open(BlockDriverState **pbs, const char *filename, int flags);
@@ -330,6 +331,8 @@ int bdrv_snapshot_list(BlockDriverState *bs,
 int bdrv_snapshot_load_tmp(BlockDriverState *bs,
                            const char *snapshot_name);
 char *bdrv_snapshot_dump(char *buf, int buf_size, QEMUSnapshotInfo *sn);
+int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
+                       const char *name);
 
 char *get_human_readable_size(char *buf, int buf_size, int64_t size);
 int path_is_absolute(const char *path);
diff --git a/savevm.c b/savevm.c
index 5d04d59..c027529 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2061,28 +2061,6 @@ out:
     return ret;
 }
 
-static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
-                              const char *name)
-{
-    QEMUSnapshotInfo *sn_tab, *sn;
-    int nb_sns, i, ret;
-
-    ret = -ENOENT;
-    nb_sns = bdrv_snapshot_list(bs, &sn_tab);
-    if (nb_sns < 0)
-        return ret;
-    for(i = 0; i < nb_sns; i++) {
-        sn = &sn_tab[i];
-        if (!strcmp(sn->id_str, name) || !strcmp(sn->name, name)) {
-            *sn_info = *sn;
-            ret = 0;
-            break;
-        }
-    }
-    g_free(sn_tab);
-    return ret;
-}
-
 /*
  * Deletes snapshots of a given name in all opened images.
  */
-- 
1.7.1

Re: [Qemu-devel] [PATCH 1/6] snapshot: export function in block.c

[Juan Quintela]

Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>   This patch moves bdrv_snapshotfind from savevm.c to block.c and export
> it, also added bdrv_deappend in block.c.

I think this patch can be splitted in two:
- new bdv_deappend
- move bdrv_snapshot_find

Re: [Qemu-devel] [PATCH 1/6] snapshot: export function in block.c

[Wenchao Xia]

于 2012-12-22 2:13, Juan Quintela 写道:
> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>>    This patch moves bdrv_snapshotfind from savevm.c to block.c and export
>> it, also added bdrv_deappend in block.c.
> 
> I think this patch can be splitted in two:
> - new bdv_deappend
> - move bdrv_snapshot_find
> 
  OK.

-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH 1/6] snapshot: export function in block.c

[Stefan Hajnoczi]

On Mon, Dec 17, 2012 at 02:25:04PM +0800, Wenchao Xia wrote:
>   This patch moves bdrv_snapshotfind from savevm.c to block.c and export
> it, also added bdrv_deappend in block.c.

I suggest naming the patch "block: export bdrv_snapshot_find()" because
it is more specific than "snapshot: export function in block.c".

> +/* revert the action */
> +void bdrv_deappend(BlockDriverState *bs_new, BlockDriverState *bs_top)
> +{
> +    bdrv_swap(bs_new, bs_top);
> +    /* this is enough? */
> +}

Please document this function in the same level of detail as
bdrv_append().  These functions are tricky and you must explain the
semantics so anyone else reading the code can safely use or modify this
function.

We also need to resolve the "this is enough?" comment.  I'll think it
through when I see how bdrv_deappend() is used later.

Stefan

Re: [Qemu-devel] [PATCH 1/6] snapshot: export function in block.c

[Wenchao Xia]

   Thanks for reviewing carefully, this patch would be split to two
patches.

> On Mon, Dec 17, 2012 at 02:25:04PM +0800, Wenchao Xia wrote:
>>    This patch moves bdrv_snapshotfind from savevm.c to block.c and export
>> it, also added bdrv_deappend in block.c.
>
> I suggest naming the patch "block: export bdrv_snapshot_find()" because
> it is more specific than "snapshot: export function in block.c".
>
>> +/* revert the action */
>> +void bdrv_deappend(BlockDriverState *bs_new, BlockDriverState *bs_top)
>> +{
>> +    bdrv_swap(bs_new, bs_top);
>> +    /* this is enough? */
>> +}
>
> Please document this function in the same level of detail as
> bdrv_append().  These functions are tricky and you must explain the
> semantics so anyone else reading the code can safely use or modify this
> function.
>
> We also need to resolve the "this is enough?" comment.  I'll think it
> through when I see how bdrv_deappend() is used later.
>
> Stefan
>


-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH 1/6] snapshot: export function in block.c

[Kevin Wolf]

Am 17.12.2012 07:25, schrieb Wenchao Xia:
>   This patch moves bdrv_snapshotfind from savevm.c to block.c and export
> it, also added bdrv_deappend in block.c.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>

Should be two separate patches.

> ---
>  block.c  |   30 ++++++++++++++++++++++++++++++
>  block.h  |    3 +++
>  savevm.c |   22 ----------------------
>  3 files changed, 33 insertions(+), 22 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 0668c4b..61c7c6a 100644
> --- a/block.c
> +++ b/block.c
> @@ -1376,6 +1376,13 @@ void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top)
>              bs_new->drv ? bs_new->drv->format_name : "");
>  }
>  
> +/* revert the action */
> +void bdrv_deappend(BlockDriverState *bs_new, BlockDriverState *bs_top)
> +{
> +    bdrv_swap(bs_new, bs_top);
> +    /* this is enough? */
> +}

What will this be used for? Maybe it's better to introduce a function
simple as this only when it's actually used.

Kevin

Re: [Qemu-devel] [PATCH 1/6] snapshot: export function in block.c

[Wenchao Xia]

于 2013-1-8 0:43, Kevin Wolf 写道:
> Am 17.12.2012 07:25, schrieb Wenchao Xia:
>>    This patch moves bdrv_snapshotfind from savevm.c to block.c and export
>> it, also added bdrv_deappend in block.c.
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>
> Should be two separate patches.
>
   OK, it have been split into two patches in V2.

>> ---
>>   block.c  |   30 ++++++++++++++++++++++++++++++
>>   block.h  |    3 +++
>>   savevm.c |   22 ----------------------
>>   3 files changed, 33 insertions(+), 22 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 0668c4b..61c7c6a 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -1376,6 +1376,13 @@ void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top)
>>               bs_new->drv ? bs_new->drv->format_name : "");
>>   }
>>
>> +/* revert the action */
>> +void bdrv_deappend(BlockDriverState *bs_new, BlockDriverState *bs_top)
>> +{
>> +    bdrv_swap(bs_new, bs_top);
>> +    /* this is enough? */
>> +}
>
> What will this be used for? Maybe it's better to introduce a function
> simple as this only when it's actually used.
>
   If bdrv_append is called before, this function should revert it, so
new file created at the top of the backing file chain is dropped.

> Kevin
>


-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH 1/6] snapshot: export function in block.c

[Kevin Wolf]

Am 08.01.2013 03:25, schrieb Wenchao Xia:
> 于 2013-1-8 0:43, Kevin Wolf 写道:
>> Am 17.12.2012 07:25, schrieb Wenchao Xia:
>>>    This patch moves bdrv_snapshotfind from savevm.c to block.c and export
>>> it, also added bdrv_deappend in block.c.
>>>
>>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>>
>> Should be two separate patches.
>>
>    OK, it have been split into two patches in V2.
> 
>>> ---
>>>   block.c  |   30 ++++++++++++++++++++++++++++++
>>>   block.h  |    3 +++
>>>   savevm.c |   22 ----------------------
>>>   3 files changed, 33 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/block.c b/block.c
>>> index 0668c4b..61c7c6a 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -1376,6 +1376,13 @@ void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top)
>>>               bs_new->drv ? bs_new->drv->format_name : "");
>>>   }
>>>
>>> +/* revert the action */
>>> +void bdrv_deappend(BlockDriverState *bs_new, BlockDriverState *bs_top)
>>> +{
>>> +    bdrv_swap(bs_new, bs_top);
>>> +    /* this is enough? */
>>> +}
>>
>> What will this be used for? Maybe it's better to introduce a function
>> simple as this only when it's actually used.
>>
>    If bdrv_append is called before, this function should revert it, so
> new file created at the top of the backing file chain is dropped.

Yes, I understand that. I meant which functionality/feature will make
use of the function?

Kevin

Re: [Qemu-devel] [PATCH 1/6] snapshot: export function in block.c

[Wenchao Xia]

于 2013-1-8 18:37, Kevin Wolf 写道:
> Am 08.01.2013 03:25, schrieb Wenchao Xia:
>> 于 2013-1-8 0:43, Kevin Wolf 写道:
>>> Am 17.12.2012 07:25, schrieb Wenchao Xia:
>>>>     This patch moves bdrv_snapshotfind from savevm.c to block.c and export
>>>> it, also added bdrv_deappend in block.c.
>>>>
>>>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>>>
>>> Should be two separate patches.
>>>
>>     OK, it have been split into two patches in V2.
>>
>>>> ---
>>>>    block.c  |   30 ++++++++++++++++++++++++++++++
>>>>    block.h  |    3 +++
>>>>    savevm.c |   22 ----------------------
>>>>    3 files changed, 33 insertions(+), 22 deletions(-)
>>>>
>>>> diff --git a/block.c b/block.c
>>>> index 0668c4b..61c7c6a 100644
>>>> --- a/block.c
>>>> +++ b/block.c
>>>> @@ -1376,6 +1376,13 @@ void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top)
>>>>                bs_new->drv ? bs_new->drv->format_name : "");
>>>>    }
>>>>
>>>> +/* revert the action */
>>>> +void bdrv_deappend(BlockDriverState *bs_new, BlockDriverState *bs_top)
>>>> +{
>>>> +    bdrv_swap(bs_new, bs_top);
>>>> +    /* this is enough? */
>>>> +}
>>>
>>> What will this be used for? Maybe it's better to introduce a function
>>> simple as this only when it's actually used.
>>>
>>     If bdrv_append is called before, this function should revert it, so
>> new file created at the top of the backing file chain is dropped.
> 
> Yes, I understand that. I meant which functionality/feature will make
> use of the function?
> 
> Kevin
> 
  I have used it in group snapshot transaction canceling callback, but
currently it will never be executed because updating step never
fail, so it can be removed but leaves a comments that need add in
if an updating function which may fail....



-- 
Best Regards

Wenchao Xia

[Qemu-devel] [PATCH 2/6] snapshot: add error set function

[Wenchao Xia]

  Added two function which will try replace the error if it is
already set, so only last error is reported.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 error.c |   23 +++++++++++++++++++++++
 error.h |    9 +++++++++
 2 files changed, 32 insertions(+), 0 deletions(-)

diff --git a/error.c b/error.c
index 128d88c..5a82f8e 100644
--- a/error.c
+++ b/error.c
@@ -43,6 +43,29 @@ void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...)
     *errp = err;
 }
 
+void error_set_replace(Error **errp, ErrorClass err_class, const char *fmt, ...)
+{
+    Error *err;
+    va_list ap;
+
+    if (errp == NULL) {
+        return;
+    }
+    if (*errp != NULL) {
+        error_free(*errp);
+        *errp = NULL;
+    }
+
+    err = g_malloc0(sizeof(*err));
+
+    va_start(ap, fmt);
+    err->msg = g_strdup_vprintf(fmt, ap);
+    va_end(ap);
+    err->err_class = err_class;
+
+    *errp = err;
+}
+
 void error_set_errno(Error **errp, int os_errno, ErrorClass err_class,
                      const char *fmt, ...)
 {
diff --git a/error.h b/error.h
index 4d52e73..8039408 100644
--- a/error.h
+++ b/error.h
@@ -29,6 +29,9 @@ typedef struct Error Error;
  */
 void error_set(Error **err, ErrorClass err_class, const char *fmt, ...) GCC_FMT_ATTR(3, 4);
 
+void error_set_replace(Error **err, ErrorClass err_class, const char *fmt, ...)
+GCC_FMT_ATTR(3, 4);
+
 /**
  * Set an indirect pointer to an error given a ErrorClass value and a
  * printf-style human message, followed by a strerror() string if
@@ -43,6 +46,12 @@ void error_set_errno(Error **err, int os_error, ErrorClass err_class, const char
     error_set(err, ERROR_CLASS_GENERIC_ERROR, fmt, ## __VA_ARGS__)
 #define error_setg_errno(err, os_error, fmt, ...) \
     error_set_errno(err, os_error, ERROR_CLASS_GENERIC_ERROR, fmt, ## __VA_ARGS__)
+#define error_setg_replace(err, fmt, ...) do {                     \
+    if (*err != NULL) { \
+        error_free(*err); \
+     } \
+    error_set(err, ERROR_CLASS_GENERIC_ERROR, fmt, ## __VA_ARGS__); \
+} while (/*CONSTCOND*/0)
 
 /**
  * Returns true if an indirect pointer to an error is pointing to a valid
-- 
1.7.1

Re: [Qemu-devel] [PATCH 2/6] snapshot: add error set function

[Eric Blake]

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

On 12/16/2012 11:25 PM, Wenchao Xia wrote:
>   Added two function which will try replace the error if it is
> already set, so only last error is reported.
> 

> +#define error_setg_replace(err, fmt, ...) do {                     \
> +    if (*err != NULL) { \
> +        error_free(*err); \
> +     } \
> +    error_set(err, ERROR_CLASS_GENERIC_ERROR, fmt, ## __VA_ARGS__); \
> +} while (/*CONSTCOND*/0)

qemu-queue.h is the only other file in qemu.git that uses /*CONSTCOND*/
notation, and that's because of the file origins; do we really need it here?

-- 
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: 619 bytes --]

Re: [Qemu-devel] [PATCH 2/6] snapshot: add error set function

[Wenchao Xia]

于 2012-12-21 5:36, Eric Blake 写道:
> On 12/16/2012 11:25 PM, Wenchao Xia wrote:
>>    Added two function which will try replace the error if it is
>> already set, so only last error is reported.
>>
>
>> +#define error_setg_replace(err, fmt, ...) do {                     \
>> +    if (*err != NULL) { \
>> +        error_free(*err); \
>> +     } \
>> +    error_set(err, ERROR_CLASS_GENERIC_ERROR, fmt, ## __VA_ARGS__); \
>> +} while (/*CONSTCOND*/0)
>
> qemu-queue.h is the only other file in qemu.git that uses /*CONSTCOND*/
> notation, and that's because of the file origins; do we really need it here?
>
  OK, /*CONSTCOND*/ will be removed.

-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH 2/6] snapshot: add error set function

[Stefan Hajnoczi]

On Mon, Dec 17, 2012 at 02:25:05PM +0800, Wenchao Xia wrote:

This patch has nothing to do with snapshots, so "snapshot: add error set
function" is not a useful commit message.  "error: add
error_set_replace()" would be okay.  Please use git log <filename> on
the file you are modifying to find good component names (e.g. "block:",
"error:").

> @@ -29,6 +29,9 @@ typedef struct Error Error;
>   */
>  void error_set(Error **err, ErrorClass err_class, const char *fmt, ...) GCC_FMT_ATTR(3, 4);
>  
> +void error_set_replace(Error **err, ErrorClass err_class, const char *fmt, ...)
> +GCC_FMT_ATTR(3, 4);
> +
>  /**
>   * Set an indirect pointer to an error given a ErrorClass value and a
>   * printf-style human message, followed by a strerror() string if
> @@ -43,6 +46,12 @@ void error_set_errno(Error **err, int os_error, ErrorClass err_class, const char
>      error_set(err, ERROR_CLASS_GENERIC_ERROR, fmt, ## __VA_ARGS__)
>  #define error_setg_errno(err, os_error, fmt, ...) \
>      error_set_errno(err, os_error, ERROR_CLASS_GENERIC_ERROR, fmt, ## __VA_ARGS__)
> +#define error_setg_replace(err, fmt, ...) do {                     \
> +    if (*err != NULL) { \
> +        error_free(*err); \
> +     } \
> +    error_set(err, ERROR_CLASS_GENERIC_ERROR, fmt, ## __VA_ARGS__); \
> +} while (/*CONSTCOND*/0)

Why not use error_set_replace() to get rid of the error_free() check?

Stefan

Re: [Qemu-devel] [PATCH 2/6] snapshot: add error set function

[Wenchao Xia]

于 2013-1-4 22:55, Stefan Hajnoczi 写道:
> On Mon, Dec 17, 2012 at 02:25:05PM +0800, Wenchao Xia wrote:
>
> This patch has nothing to do with snapshots, so "snapshot: add error set
> function" is not a useful commit message.  "error: add
> error_set_replace()" would be okay.  Please use git log <filename> on
> the file you are modifying to find good component names (e.g. "block:",
> "error:").
>
>> @@ -29,6 +29,9 @@ typedef struct Error Error;
>>    */
>>   void error_set(Error **err, ErrorClass err_class, const char *fmt, ...) GCC_FMT_ATTR(3, 4);
>>
>> +void error_set_replace(Error **err, ErrorClass err_class, const char *fmt, ...)
>> +GCC_FMT_ATTR(3, 4);
>> +
>>   /**
>>    * Set an indirect pointer to an error given a ErrorClass value and a
>>    * printf-style human message, followed by a strerror() string if
>> @@ -43,6 +46,12 @@ void error_set_errno(Error **err, int os_error, ErrorClass err_class, const char
>>       error_set(err, ERROR_CLASS_GENERIC_ERROR, fmt, ## __VA_ARGS__)
>>   #define error_setg_errno(err, os_error, fmt, ...) \
>>       error_set_errno(err, os_error, ERROR_CLASS_GENERIC_ERROR, fmt, ## __VA_ARGS__)
>> +#define error_setg_replace(err, fmt, ...) do {                     \
>> +    if (*err != NULL) { \
>> +        error_free(*err); \
>> +     } \
>> +    error_set(err, ERROR_CLASS_GENERIC_ERROR, fmt, ## __VA_ARGS__); \
>> +} while (/*CONSTCOND*/0)
>
> Why not use error_set_replace() to get rid of the error_free() check?
>
> Stefan
>

   It will be changed to error_set_check() which only set error when
*err is not set, to keep first error not over written.

-- 
Best Regards

Wenchao Xia

[Qemu-devel] [PATCH 3/6] snapshot: design of common API to take snapshots

[Wenchao Xia]

  This patch added API to take snapshots in unified style for
both internal or external type. The core structure is based
on transaction, for that there is a qmp interface need to support
, qmp_transaction, so all operations are packed as requests.
  In this way a sperate internal layer for snapshot is splitted
out from qmp layer, and now qmp can just translate the user request
and fill in internal API. Internal API use params defined inside
qemu, so other component inside qemu can use it without considering
the qmp's parameter format.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 blockdev.h |  129 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 129 insertions(+), 0 deletions(-)

diff --git a/blockdev.h b/blockdev.h
index d73d552..4a1b508 100644
--- a/blockdev.h
+++ b/blockdev.h
@@ -66,4 +66,133 @@ void qmp_change_blockdev(const char *device, const char *filename,
                          bool has_format, const char *format, Error **errp);
 void do_commit(Monitor *mon, const QDict *qdict);
 int do_drive_del(Monitor *mon, const QDict *qdict, QObject **ret_data);
+
+
+/*   snapshot transaction API.
+ *   Split out a layer around core struct BlkTransactionStates, so other
+ *  component in qemu can fill the request and simply use the API to submit,
+ *  QMP may just use part of the API's function, no need to expose all internal
+ *  function to user.
+ */
+
+/* sync snapshot */
+
+typedef enum BlkTransactionOperationSync {
+    BLK_SN_SYNC_CREATE = 0,
+    BLK_SN_SYNC_DELETE,
+} BlkTransactionOperationSync;
+
+/* internal snapshot */
+
+typedef struct SNTime {
+    uint32_t date_sec; /* UTC date of the snapshot */
+    uint32_t date_nsec;
+    uint64_t vm_clock_nsec; /* VM clock relative to boot */
+} SNTime;
+
+typedef enum BlkSnapshotIntStep {
+    BLK_SNAPSHOT_INT_START = 0,
+    BLK_SNAPSHOT_INT_CREATED,
+    BLK_SNAPSHOT_INT_CANCELED,
+} BlkSnapshotIntStep;
+
+typedef struct BlkSnapshotInternal {
+    /* caller input */
+    const char *sn_name; /* must be set in create/delete. */
+    BlockDriverState *bs; /* must be set in create/delete */
+    SNTime time; /* must be set in create. */
+    uint64_t vm_state_size; /* optional, default is 0, only valid in create. */
+    /* following were used internal */
+    QEMUSnapshotInfo sn;
+    QEMUSnapshotInfo old_sn;
+    bool old_sn_exist;
+    BlkSnapshotIntStep step;
+} BlkSnapshotInternal;
+
+/* external snapshot */
+
+typedef enum BlkSnapshotExtStep {
+    BLK_SNAPSHOT_EXT_START = 0,
+    BLK_SNAPSHOT_EXT_CREATED,
+    BLK_SNAPSHOT_EXT_INVALIDATED,
+    BLK_SNAPSHOT_EXT_CANCELED,
+} BlkSnapshotExtStep;
+
+typedef struct BlkSnapshotExternal {
+    /* caller input */
+    const char *new_image_file; /* must be set in create/delete. */
+    BlockDriverState *old_bs; /* must be set in create/delete. */
+    const char *format; /* must be set in create. */
+    /* following were used internal */
+    BlockDriverState *new_bs;
+    BlockDriver *format_drv;
+    BlkSnapshotExtStep step;
+} BlkSnapshotExternal;
+
+typedef enum BlkSnapshotType {
+    BLK_SNAPSHOT_INTERNAL = 0,
+    BLK_SNAPSHOT_EXTERNAL,
+    BLK_SNAPSHOT_NOSUPPORT,
+} BlkSnapshotType;
+
+/* for simple sync type params were all put here ignoring the difference of
+   different operation type as create/delete. */
+typedef struct BlkTransactionStatesSync {
+    /* caller input */
+    BlkSnapshotType type;
+    union {
+        BlkSnapshotInternal internal;
+        BlkSnapshotExternal external;
+    };
+    bool use_existing;
+    BlkTransactionOperationSync op;
+} BlkTransactionStatesSync;
+
+/* async snapshot, not supported now */
+typedef struct BlkTransactionStatesAsync {
+    int reserved;
+} BlkTransactionStatesAsync;
+
+/* Core structure for group snapshots, fill in it and then call the API. */
+typedef struct BlkTransactionStates BlkTransactionStates;
+
+struct BlkTransactionStates {
+    /* caller input */
+    bool async;
+    union {
+        BlkTransactionStatesSync st_sync;
+        BlkTransactionStatesSync st_async;
+    };
+    /* following were used internal */
+    Error *err;
+    int (*blk_trans_do)(BlkTransactionStates *states, Error **errp);
+    int (*blk_trans_invalid)(BlkTransactionStates *states, Error **errp);
+    int (*blk_trans_cancel)(BlkTransactionStates *states, Error **errp);
+    QSIMPLEQ_ENTRY(BlkTransactionStates) entry;
+};
+
+typedef QSIMPLEQ_HEAD(snap_bdrv_states, BlkTransactionStates) \
+                      BlkTransactionStatesList;
+
+/* API */
+BlkTransactionStates *blk_trans_st_new(void);
+void blk_trans_st_delete(BlkTransactionStates **p_st);
+BlkTransactionStatesList *blk_trans_st_list_new(void);
+void blk_trans_st_list_delete(BlkTransactionStatesList **p_list);
+
+/* add a request to list, request would be checked to see if it is valid,
+   return -1 when met error and states would not be queued. */
+int add_transaction(BlkTransactionStatesList *list,
+                    BlkTransactionStates *states,
+                    Error **errp);
+
+/*    'Atomic' submit the request. In snapshot creation case, if any
+ *  fail then we do not pivot any of the devices in the group, and abandon the
+ *  snapshots
+ */
+int submit_transaction(BlkTransactionStatesList *list, Error **errp);
+
+/* helper */
+SNTime get_sn_time(void);
+void generate_sn_name_from_time(SNTime *time, char *time_str, int size);
 #endif
-- 
1.7.1

Re: [Qemu-devel] [PATCH 3/6] snapshot: design of common API to take snapshots

[Eric Blake]

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

On 12/16/2012 11:25 PM, Wenchao Xia wrote:
>   This patch added API to take snapshots in unified style for
> both internal or external type. The core structure is based
> on transaction, for that there is a qmp interface need to support
> , qmp_transaction, so all operations are packed as requests.
>   In this way a sperate internal layer for snapshot is splitted
> out from qmp layer, and now qmp can just translate the user request
> and fill in internal API. Internal API use params defined inside
> qemu, so other component inside qemu can use it without considering
> the qmp's parameter format.
> 

> +typedef struct SNTime {
> +    uint32_t date_sec; /* UTC date of the snapshot */

Relative to what?  Seconds since Epoch?  Shouldn't this be 64-bits, to
avoid wraparound problems in 2038?

> +typedef struct BlkSnapshotInternal {
> +    /* caller input */
> +    const char *sn_name; /* must be set in create/delete. */
> +    BlockDriverState *bs; /* must be set in create/delete */
> +    SNTime time; /* must be set in create. */
> +    uint64_t vm_state_size; /* optional, default is 0, only valid in create. */
> +    /* following were used internal */

Prefer present tense: The following are for internal use

> +
> +/* for simple sync type params were all put here ignoring the difference of
> +   different operation type as create/delete. */
> +typedef struct BlkTransactionStatesSync {

Again, prefer present tense (avoid 'were' in comments).

> +/* async snapshot, not supported now */
> +typedef struct BlkTransactionStatesAsync {
> +    int reserved;
> +} BlkTransactionStatesAsync;

Why declare a struct if we aren't supporting it yet?

-- 
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: 619 bytes --]

Re: [Qemu-devel] [PATCH 3/6] snapshot: design of common API to take snapshots

[Wenchao Xia]

 > On 12/16/2012 11:25 PM, Wenchao Xia wrote:
>>    This patch added API to take snapshots in unified style for
>> both internal or external type. The core structure is based
>> on transaction, for that there is a qmp interface need to support
>> , qmp_transaction, so all operations are packed as requests.
>>    In this way a sperate internal layer for snapshot is splitted
>> out from qmp layer, and now qmp can just translate the user request
>> and fill in internal API. Internal API use params defined inside
>> qemu, so other component inside qemu can use it without considering
>> the qmp's parameter format.
>>
>
>> +typedef struct SNTime {
>> +    uint32_t date_sec; /* UTC date of the snapshot */
>
> Relative to what?  Seconds since Epoch?  Shouldn't this be 64-bits, to
> avoid wraparound problems in 2038?
>
   original code for snapshot time is uint32_t, but I think 64bits is
better.

>> +typedef struct BlkSnapshotInternal {
>> +    /* caller input */
>> +    const char *sn_name; /* must be set in create/delete. */
>> +    BlockDriverState *bs; /* must be set in create/delete */
>> +    SNTime time; /* must be set in create. */
>> +    uint64_t vm_state_size; /* optional, default is 0, only valid in create. */
>> +    /* following were used internal */
>
> Prefer present tense: The following are for internal use
>
   OK.

>> +
>> +/* for simple sync type params were all put here ignoring the difference of
>> +   different operation type as create/delete. */
>> +typedef struct BlkTransactionStatesSync {
>
> Again, prefer present tense (avoid 'were' in comments).
>
>> +/* async snapshot, not supported now */
>> +typedef struct BlkTransactionStatesAsync {
>> +    int reserved;
>> +} BlkTransactionStatesAsync;
>
> Why declare a struct if we aren't supporting it yet?
>
   Just a reserve value for type completion


-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH 3/6] snapshot: design of common API to take snapshots

[Juan Quintela]

Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
> +
> +typedef struct SNTime {
> +    uint32_t date_sec; /* UTC date of the snapshot */
> +    uint32_t date_nsec;

This two fields are just struct timespec, does it makes sense to use it?

> +    uint64_t vm_clock_nsec; /* VM clock relative to boot */
> +} SNTime;
> +
> +typedef enum BlkSnapshotIntStep {
> +    BLK_SNAPSHOT_INT_START = 0,
> +    BLK_SNAPSHOT_INT_CREATED,
> +    BLK_SNAPSHOT_INT_CANCELED,
> +} BlkSnapshotIntStep;
> +
> +typedef struct BlkSnapshotInternal {
> +    /* caller input */
> +    const char *sn_name; /* must be set in create/delete. */
> +    BlockDriverState *bs; /* must be set in create/delete */
> +    SNTime time; /* must be set in create. */
> +    uint64_t vm_state_size; /* optional, default is 0, only valid in create. */
> +    /* following were used internal */
> +    QEMUSnapshotInfo sn;
> +    QEMUSnapshotInfo old_sn;
> +    bool old_sn_exist;
> +    BlkSnapshotIntStep step;
> +} BlkSnapshotInternal;
> +
> +/* external snapshot */
> +
> +typedef enum BlkSnapshotExtStep {
> +    BLK_SNAPSHOT_EXT_START = 0,
> +    BLK_SNAPSHOT_EXT_CREATED,
> +    BLK_SNAPSHOT_EXT_INVALIDATED,
> +    BLK_SNAPSHOT_EXT_CANCELED,
> +} BlkSnapshotExtStep;
> +
> +typedef struct BlkSnapshotExternal {
> +    /* caller input */
> +    const char *new_image_file; /* must be set in create/delete. */
> +    BlockDriverState *old_bs; /* must be set in create/delete. */
> +    const char *format; /* must be set in create. */
> +    /* following were used internal */
> +    BlockDriverState *new_bs;
> +    BlockDriver *format_drv;
> +    BlkSnapshotExtStep step;
> +} BlkSnapshotExternal;
> +
> +typedef enum BlkSnapshotType {
> +    BLK_SNAPSHOT_INTERNAL = 0,
> +    BLK_SNAPSHOT_EXTERNAL,
> +    BLK_SNAPSHOT_NOSUPPORT,
> +} BlkSnapshotType;
> +
> +/* for simple sync type params were all put here ignoring the difference of
> +   different operation type as create/delete. */
> +typedef struct BlkTransactionStatesSync {
> +    /* caller input */
> +    BlkSnapshotType type;
> +    union {
> +        BlkSnapshotInternal internal;
> +        BlkSnapshotExternal external;
> +    };
> +    bool use_existing;
> +    BlkTransactionOperationSync op;
> +} BlkTransactionStatesSync;
> +
> +/* async snapshot, not supported now */
> +typedef struct BlkTransactionStatesAsync {
> +    int reserved;
> +} BlkTransactionStatesAsync;
> +
> +/* Core structure for group snapshots, fill in it and then call the API. */
> +typedef struct BlkTransactionStates BlkTransactionStates;
> +
> +struct BlkTransactionStates {
> +    /* caller input */
> +    bool async;

Why do we have this variable?  As far as I can see, we only test its
value, and set it to false.  Are we missing any patch?

> +    union {
> +        BlkTransactionStatesSync st_sync;
> +        BlkTransactionStatesSync st_async;
> +    };
> +    /* following were used internal */
> +    Error *err;
> +    int (*blk_trans_do)(BlkTransactionStates *states, Error **errp);
> +    int (*blk_trans_invalid)(BlkTransactionStates *states, Error **errp);
> +    int (*blk_trans_cancel)(BlkTransactionStates *states, Error **errp);
> +    QSIMPLEQ_ENTRY(BlkTransactionStates) entry;
> +};
> +
> +typedef QSIMPLEQ_HEAD(snap_bdrv_states, BlkTransactionStates) \
> +                      BlkTransactionStatesList;
> +
> +/* API */
> +BlkTransactionStates *blk_trans_st_new(void);
> +void blk_trans_st_delete(BlkTransactionStates **p_st);
> +BlkTransactionStatesList *blk_trans_st_list_new(void);
> +void blk_trans_st_list_delete(BlkTransactionStatesList **p_list);
> +
> +/* add a request to list, request would be checked to see if it is valid,
> +   return -1 when met error and states would not be queued. */
> +int add_transaction(BlkTransactionStatesList *list,
> +                    BlkTransactionStates *states,
> +                    Error **errp);
> +
> +/*    'Atomic' submit the request. In snapshot creation case, if any
> + *  fail then we do not pivot any of the devices in the group, and abandon the
> + *  snapshots
> + */
> +int submit_transaction(BlkTransactionStatesList *list, Error **errp);
> +
> +/* helper */
> +SNTime get_sn_time(void);

> +void generate_sn_name_from_time(SNTime *time, char *time_str, int size);
>  #endif

Re: [Qemu-devel] [PATCH 3/6] snapshot: design of common API to take snapshots

[Wenchao Xia]

于 2012-12-22 2:49, Juan Quintela 写道:
> Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>> +
>> +typedef struct SNTime {
>> +    uint32_t date_sec; /* UTC date of the snapshot */
>> +    uint32_t date_nsec;
> 
> This two fields are just struct timespec, does it makes sense to use it?
> 
  make sense, I did not notice timespec before, will use it if windows
support timespec too.


>> +
>> +/* Core structure for group snapshots, fill in it and then call the API. */
>> +typedef struct BlkTransactionStates BlkTransactionStates;
>> +
>> +struct BlkTransactionStates {
>> +    /* caller input */
>> +    bool async;
> 
> Why do we have this variable?  As far as I can see, we only test its
> value, and set it to false.  Are we missing any patch?
> 
  No, I just put it here as a reserved option. Maybe live block commit
can be considered as async deletion of snapshot, but nevermind I'll
delete it for that it is not supported now.



-- 
Best Regards

Wenchao Xia

[Qemu-devel] [PATCH 4/6] snapshot: implemention of common API to take snapshots

[Wenchao Xia]

  This patch is the implemention of transaction based snapshot
internal API. Internal snapshot for specified block device
is added, which can be used as part of functionality in one way
to live full back up vm seperating internal block snapshot and
vmstate(vmstate goes to another file, not implemented in this
patch).
  Every request's handler need to have three function:
execution, updating, cancelling. Also another check function
could be provided to check if request is valid before execition.
  internal snapshot implemention was based on the code from
dietmar@proxmox.com.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
Signed-off-by: Dietmar Maurer <dietmar@proxmox.com>
---
 blockdev.c |  515 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 515 insertions(+), 0 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 9a05e57..1c38c67 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -660,6 +660,521 @@ void do_commit(Monitor *mon, const QDict *qdict)
     }
 }
 
+/*   snapshot functions.
+ *   following are implemention around core struct BlkTransactionStates.
+ * to start, invalidate, cancel the action.
+ */
+
+/* Block internal snapshot(qcow2, sheepdog image...) support.
+   checking and execution was splitted to enable a check for every device
+before execution in group case. */
+
+SNTime get_sn_time(void)
+{
+    SNTime time;
+    /* is uint32_t big enough to get time_t value on other machine ? */
+#ifdef _WIN32
+    struct _timeb tb;
+    _ftime(&tb);
+    time.date_sec = tb.time;
+    time.date_nsec = tb.millitm * 1000000;
+#else
+    struct timeval tv;
+    gettimeofday(&tv, NULL);
+    time.date_sec = tv.tv_sec;
+    time.date_nsec = tv.tv_usec * 1000;
+#endif
+    time.vm_clock_nsec = qemu_get_clock_ns(vm_clock);
+    return time;
+}
+
+void generate_sn_name_from_time(SNTime *time, char *time_str, int size)
+{
+#ifdef _WIN32
+    time_t t = time->date_sec;
+    struct tm *ptm = localtime(&t);
+    strftime(time_str, size, "vm-%Y%m%d%H%M%S", ptm);
+#else
+    /* cast below needed for OpenBSD where tv_sec is still 'long' */
+    time_t second = time->date_sec;
+    struct tm tm;
+    localtime_r((const time_t *)&second, &tm);
+    strftime(time_str, size, "vm-%Y%m%d%H%M%S", &tm);
+#endif
+}
+
+static int blk_sn_check_create_internal_sync(BlkTransactionStates *states,
+                                             Error **errp)
+{
+    BlkTransactionStatesSync *st_sync = &states->st_sync;
+    BlockDriverState *bs = st_sync->internal.bs;
+    const char *device = bdrv_get_device_name(bs);
+    QEMUSnapshotInfo *old_sn = &st_sync->internal.old_sn;
+    const char *name = st_sync->internal.sn_name;
+    bool use_existing = st_sync->use_existing;
+    int ret;
+
+    if (!bdrv_is_inserted(bs)) {
+        error_set_replace(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
+        return -1;
+    }
+
+    if (bdrv_is_read_only(bs)) {
+        error_set_replace(errp, QERR_DEVICE_IS_READ_ONLY, device);
+        return -1;
+    }
+
+    if (!bdrv_can_snapshot(bs)) {
+        error_set_replace(errp, QERR_NOT_SUPPORTED);
+        return -1;
+    }
+
+    ret = bdrv_snapshot_find(bs, old_sn, name);
+    if (ret >= 0) {
+        st_sync->internal.old_sn_exist = TRUE;
+    } else {
+        if (use_existing) {
+            /* caller require use existing one */
+            error_set_replace(errp, ERROR_CLASS_GENERIC_ERROR,
+                              "snapshot '%s' not exists on '%s' but caller "
+                              "want to use it.",
+                              name, device);
+            return -1;
+        }
+    }
+
+    st_sync->internal.step = BLK_SNAPSHOT_INT_START;
+    return 0;
+}
+
+static int blk_sn_create_internal_sync(BlkTransactionStates *states,
+                                       Error **errp)
+{
+    BlkTransactionStatesSync *st_sync = &states->st_sync;
+    BlockDriverState *bs = st_sync->internal.bs;
+    const char *name = st_sync->internal.sn_name;
+    QEMUSnapshotInfo *sn = &st_sync->internal.sn;
+    int ret = 0;
+    const char *device = bdrv_get_device_name(bs);
+    QEMUSnapshotInfo *old_sn = &st_sync->internal.old_sn;
+
+    SNTime *time = &st_sync->internal.time;
+    if (time->vm_clock_nsec == 0) {
+        /* not preset, it need to be set */
+        error_setg_replace(errp,
+                           "Invalid timestamp was set on for '%s' on '%s'\n",
+                           name, device);
+        return -1;
+    }
+
+    sn->date_sec = time->date_sec;
+    sn->date_nsec = time->date_nsec;
+    sn->vm_clock_nsec = time->vm_clock_nsec;
+    sn->vm_state_size = st_sync->internal.vm_state_size;
+
+    if (st_sync->internal.old_sn_exist) {
+        ret = bdrv_snapshot_delete(bs, old_sn->id_str);
+        if (ret < 0) {
+            error_setg_replace(errp,
+                       "Error in deleting existing snapshot %s on '%s'\n",
+                       name, device);
+            return -1;
+        }
+        pstrcpy(sn->name, sizeof(sn->name), old_sn->name);
+        pstrcpy(sn->id_str, sizeof(sn->id_str), old_sn->id_str);
+    } else {
+        pstrcpy(sn->name, sizeof(sn->name), name);
+    }
+
+    ret = bdrv_snapshot_create(bs, sn);
+    if (ret < 0) {
+        error_set_replace(errp, ERROR_CLASS_GENERIC_ERROR,
+                  "Error while creating snapshot on '%s'\n", device);
+        return -1;
+    }
+
+    st_sync->internal.step = BLK_SNAPSHOT_INT_CREATED;
+    return 0;
+}
+
+static int blk_sn_cancel_internal_sync(BlkTransactionStates *states,
+                                       Error **errp)
+{
+    BlkTransactionStatesSync *st_sync = &states->st_sync;
+    const char *name = st_sync->internal.sn_name;
+    BlockDriverState *bs = st_sync->internal.bs;
+    const char *device = bdrv_get_device_name(bs);
+    int ret;
+
+    if (st_sync->internal.step == BLK_SNAPSHOT_INT_CREATED) {
+        if (st_sync->internal.old_sn_exist) {
+            error_setg_replace(errp,
+                       "sn '%s' on '%s' exist originally at it have been "
+                       "overwritten, can't cancel the action.\n",
+                       name, device);
+            return -1;
+        }
+        ret = bdrv_snapshot_delete(bs, name);
+        if (ret < 0) {
+            error_setg_replace(errp,
+                       "Error while deleting snapshot '%s' on '%s' to "
+                       "cancel the operation.\n", name, device);
+            return -1;
+        }
+        st_sync->internal.step = BLK_SNAPSHOT_INT_CANCELED;
+    }
+    return 0;
+}
+
+static int blk_sn_check_delete_internal_sync(BlkTransactionStates *states,
+                                             Error **errp)
+{
+    BlkTransactionStatesSync *st_sync = &states->st_sync;
+    BlockDriverState *bs = st_sync->internal.bs;
+    QEMUSnapshotInfo *old_sn = &st_sync->internal.old_sn;
+    const char *device = bdrv_get_device_name(bs);
+    const char *name = st_sync->internal.sn_name;
+    int ret;
+
+    if (bdrv_is_read_only(bs)) {
+        error_set_replace(errp, QERR_DEVICE_IS_READ_ONLY, device);
+        return -1;
+    }
+
+    if (!bdrv_can_snapshot(bs)) {
+        error_set_replace(errp, QERR_NOT_SUPPORTED);
+        return -1;
+    }
+
+    ret = bdrv_snapshot_find(bs, old_sn, name);
+    if (ret < 0) {
+        /* caller require use existing one */
+        error_set_replace(errp, ERROR_CLASS_GENERIC_ERROR,
+                          "snapshot '%s' not exists on '%s'.",
+                          name, device);
+        return -1;
+    }
+    return 0;
+}
+
+static int blk_sn_delete_internal_sync(BlkTransactionStates *states,
+                                       Error **errp)
+{
+    BlkTransactionStatesSync *st_sync = &states->st_sync;
+    int ret = 0;
+    const char *name = st_sync->internal.sn_name;
+    BlockDriverState *bs = st_sync->internal.bs;
+    const char *device = bdrv_get_device_name(bs);
+
+    /* no need to check snapshot existence? */
+    ret = bdrv_snapshot_delete(bs, name);
+    if (ret < 0) {
+        error_setg_replace(errp,
+                  "Error while deleting snapshot on '%s'\n", device);
+        return -1;
+    }
+    return 0;
+}
+
+
+/* Block external snapshot(backing file chain) support. */
+
+static int blk_sn_check_create_external_sync(BlkTransactionStates *states,
+                                             Error **errp)
+{
+    BlkTransactionStatesSync *st_sync = &states->st_sync;
+    BlockDriverState *old_bs = st_sync->external.old_bs;
+    const char *device = bdrv_get_device_name(old_bs);
+    const char *format = st_sync->external.format;
+    const char *new_image_file = st_sync->external.new_image_file;
+    BlockDriver *proto_drv;
+    BlockDriver *drv;
+
+    if (!bdrv_is_inserted(old_bs)) {
+        error_set_replace(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
+        return -1;
+    }
+
+    if (bdrv_in_use(old_bs)) {
+        error_set_replace(errp, QERR_DEVICE_IN_USE, device);
+        return -1;
+    }
+
+    if (!bdrv_is_read_only(old_bs)) {
+        if (bdrv_flush(old_bs)) {
+            error_set_replace(errp, QERR_IO_ERROR);
+            return -1;
+        }
+    }
+
+    drv = bdrv_find_format(format);
+    if (!drv) {
+        error_set_replace(errp, QERR_INVALID_BLOCK_FORMAT, format);
+        return -1;
+    }
+    st_sync->external.format_drv = drv;
+
+    proto_drv = bdrv_find_protocol(new_image_file);
+    if (!proto_drv) {
+        error_set_replace(errp, QERR_INVALID_BLOCK_FORMAT, format);
+        return -1;
+    }
+    st_sync->external.step = BLK_SNAPSHOT_EXT_START;
+    return 0;
+}
+
+/* We don't do anything that commits us to the snapshot here, do it later. */
+static int blk_sn_create_external_sync(BlkTransactionStates *states,
+                                       Error **errp)
+{
+    BlkTransactionStatesSync *st_sync = &states->st_sync;
+    BlockDriverState *old_bs = st_sync->external.old_bs;
+    const char *format = st_sync->external.format;
+    BlockDriver *drv = st_sync->external.format_drv;
+    const char *new_image_file = st_sync->external.new_image_file;
+    bool use_existing = st_sync->use_existing;
+    BlockDriverState *new_bs;
+    Error *local_err = NULL;
+
+    int flags = old_bs->open_flags, ret;
+    /* create new image w/backing file */
+    if (!use_existing) {
+        bdrv_img_create(new_image_file, format,
+                        old_bs->filename,
+                        old_bs->drv->format_name,
+                        NULL, -1, flags, &local_err);
+        if (error_is_set(&local_err)) {
+            error_propagate(errp, local_err);
+            return -1;
+        }
+    }
+
+    /* We will manually add the backing_hd field to the bs later */
+    new_bs = bdrv_new("");
+    ret = bdrv_open(new_bs, new_image_file,
+                    flags | BDRV_O_NO_BACKING, drv);
+    if (ret != 0) {
+        error_set_replace(errp, QERR_OPEN_FILE_FAILED, new_image_file);
+        return -1;
+    }
+
+    st_sync->external.new_bs = new_bs;
+    st_sync->external.step = BLK_SNAPSHOT_EXT_CREATED;
+    return 0;
+}
+
+/* refresh the blk device's states to emulator, always succeed now. */
+static int blk_sn_invalid_external_sync(BlkTransactionStates *states,
+                                        Error **errp)
+{
+    BlkTransactionStatesSync *st_sync = &states->st_sync;
+    BlkSnapshotExternal *ext = &st_sync->external;
+    /* This removes our old bs from the bdrv_states, and adds the new
+     * bs */
+    bdrv_append(ext->new_bs, ext->old_bs);
+    /* We don't need (or want) to use the transactional
+     * bdrv_reopen_multiple() across all the entries at once, because
+     * we don't want to abort all of them if one of them fails the
+     * reopen */
+    bdrv_reopen(ext->new_bs,
+                ext->new_bs->open_flags & ~BDRV_O_RDWR,
+                NULL);
+
+    st_sync->external.step = BLK_SNAPSHOT_EXT_INVALIDATED;
+    return 0;
+}
+
+/* cancel the job if it have been done before */
+static int blk_sn_cancel_external_sync(BlkTransactionStates *states,
+                                       Error **errp)
+{
+    BlkTransactionStatesSync *st_sync = &states->st_sync;
+    BlkSnapshotExternal *ext = &st_sync->external;
+    if (ext->step == BLK_SNAPSHOT_EXT_INVALIDATED) {
+        /* revert the bind */
+        bdrv_deappend(ext->new_bs, ext->old_bs);
+        bdrv_reopen(ext->old_bs,
+                    ext->old_bs->open_flags & ~BDRV_O_RDWR,
+                    NULL);
+        ext->step = BLK_SNAPSHOT_EXT_CREATED;
+    }
+    if (ext->step == BLK_SNAPSHOT_EXT_CREATED) {
+        /* abandon new bs, and keep using the original bs.
+         * no need to delete the image? */
+        bdrv_delete(ext->new_bs);
+        ext->step = BLK_SNAPSHOT_EXT_CANCELED;
+    }
+    return 0;
+}
+
+/*
+ * caller's API to add, submit request, support mixed request.
+ */
+
+BlkTransactionStates *blk_trans_st_new(void)
+{
+    BlkTransactionStates *states = g_malloc0(sizeof(BlkTransactionStates));
+    return states;
+}
+
+void blk_trans_st_delete(BlkTransactionStates **p_st)
+{
+    if ((*p_st)->err != NULL) {
+        error_free((*p_st)->err);
+    }
+    g_free(*p_st);
+    *p_st = NULL;
+    return;
+}
+
+BlkTransactionStatesList *blk_trans_st_list_new(void)
+{
+    BlkTransactionStatesList *list =
+                      g_malloc0(sizeof(BlkTransactionStatesList));
+    QSIMPLEQ_INIT(list);
+    return list;
+}
+
+void blk_trans_st_list_delete(BlkTransactionStatesList **p_list)
+{
+    BlkTransactionStates *states, *next;
+    QSIMPLEQ_FOREACH_SAFE(states, (*p_list), entry, next) {
+        blk_trans_st_delete(&states);
+    }
+    g_free(*p_list);
+    *p_list = NULL;
+    return;
+}
+
+int add_transaction(BlkTransactionStatesList *list,
+                    BlkTransactionStates *states,
+                    Error **errp)
+{
+    int ret;
+
+    if (states->async) {
+        abort();
+    }
+
+    switch (states->st_sync.op) {
+    case BLK_SN_SYNC_CREATE:
+        if (states->st_sync.type == BLK_SNAPSHOT_INTERNAL) {
+            ret = blk_sn_check_create_internal_sync(states, errp);
+            if (ret < 0) {
+                return -1;
+            }
+            states->blk_trans_do = blk_sn_create_internal_sync;
+            states->blk_trans_cancel = blk_sn_cancel_internal_sync;
+        } else if (states->st_sync.type == BLK_SNAPSHOT_EXTERNAL) {
+            ret = blk_sn_check_create_external_sync(states, errp);
+            if (ret < 0) {
+                return -1;
+            }
+            states->blk_trans_do = blk_sn_create_external_sync;
+            states->blk_trans_invalid = blk_sn_invalid_external_sync;
+            states->blk_trans_cancel = blk_sn_cancel_external_sync;
+        } else {
+            error_setg_replace(errp, "Operation %d for type %d not supprted.",
+                               states->st_sync.op, states->st_sync.type);
+            return -1;
+        }
+        break;
+    case BLK_SN_SYNC_DELETE:
+        if (states->st_sync.type == BLK_SNAPSHOT_INTERNAL) {
+            ret = blk_sn_check_delete_internal_sync(states, errp);
+            if (ret < 0) {
+                return -1;
+            }
+            states->blk_trans_do = blk_sn_delete_internal_sync;
+            /* this operation can't be canceled. */
+        } else if (states->st_sync.type == BLK_SNAPSHOT_EXTERNAL) {
+            /* sync delete an external snapshot, not support now,
+               use blk commit instead. */
+            error_setg_replace(errp, "Operation %d for type %d not supprted.",
+                               states->st_sync.op, states->st_sync.type);
+            return -1;
+        } else {
+            error_setg_replace(errp, "Operation %d for type %d not supprted.",
+                               states->st_sync.op, states->st_sync.type);
+            return -1;
+        }
+        break;
+    default:
+        return -1;
+    }
+
+    /* Todo, in async case const char* member should be duplicated, but
+    we do not support it now so direct enlist the request. */
+    QSIMPLEQ_INSERT_TAIL(list, states, entry);
+    return 0;
+}
+
+int submit_transaction(BlkTransactionStatesList *list, Error **errp)
+{
+    BlkTransactionStates *states = NULL;
+    int ret = 0;
+    bool error_set = false;
+
+    /* drain all i/o before any snapshots */
+    bdrv_drain_all();
+
+    /* step 1, do the action, that is create/delete snapshots in sync case */
+    QSIMPLEQ_FOREACH(states, list, entry) {
+        if (states->async) {
+            abort();
+        } else {
+            ret = states->blk_trans_do(states, &states->err);
+            if (ret < 0) {
+                if ((!error_set) && (states->err)) {
+                    *errp = error_copy(states->err);
+                    error_set = TRUE;
+                }
+                goto delete_and_fail;
+            }
+        }
+    }
+
+    /* step 2, update emulator */
+    QSIMPLEQ_FOREACH(states, list, entry) {
+        if (states->async) {
+            abort();
+        } else {
+            if (states->blk_trans_invalid) {
+                ret = states->blk_trans_invalid(states, &states->err);
+                if (ret < 0) {
+                    if ((!error_set) && (states->err)) {
+                        *errp = error_copy(states->err);
+                        error_set = TRUE;
+                    }
+                    goto delete_and_fail;
+                }
+            }
+        }
+    }
+
+    /* success */
+    return 0;
+
+delete_and_fail:
+    /*
+     * failure, and it is all-or-none, cancel all if error got.
+     */
+    QSIMPLEQ_FOREACH(states, list, entry) {
+        if (states->async) {
+            abort();
+        } else {
+            if (states->blk_trans_cancel) {
+                ret = states->blk_trans_cancel(states, &states->err);
+                if ((ret < 0) && (!error_set) && (states->err)) {
+                    *errp = error_copy(states->err);
+                    error_set = TRUE;
+                }
+            }
+        }
+    }
+    return -1;
+}
+
 static void blockdev_do_action(int kind, void *data, Error **errp)
 {
     BlockdevAction action;
-- 
1.7.1

Re: [Qemu-devel] [PATCH 4/6] snapshot: implemention of common API to take snapshots

[Dietmar Maurer]

>   This patch is the implemention of transaction based snapshot internal API.

What exactly is the advantage of using qmp transactions (as opposed to the pve snapshot patches)?
Seems this makes it impossible to use external commands to make snapshots? 

Re: [Qemu-devel] [PATCH 4/6] snapshot: implemention of common API to take snapshots

[Wenchao Xia]

于 2012-12-17 14:36, Dietmar Maurer 写道:
>>    This patch is the implemention of transaction based snapshot internal API.
>
> What exactly is the advantage of using qmp transactions (as opposed to the pve snapshot patches)?
   Which pve snapshot patches you referring? In group case a check should
be done for all snapshot first, and I need to support qmp transaction
interface which is already there, so I build an internal transaction
layer below to make the interface unified and relative easier to add in
qmp transaction.

> Seems this makes it impossible to use external commands to make snapshots?
>
   what command? can you tip more about the case?


-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH 4/6] snapshot: implemention of common API to take snapshots

[Dietmar Maurer]

> 于 2012-12-17 14:36, Dietmar Maurer 写道:
> >>    This patch is the implemention of transaction based snapshot internal
> API.
> >
> > What exactly is the advantage of using qmp transactions (as opposed to the
> pve snapshot patches)?
>    Which pve snapshot patches you referring? 

I refer to:  

https://git.proxmox.com/?p=pve-qemu-kvm.git;a=blob;f=debian/patches/internal-snapshot-async.patch;h=6c86de3a6160c58d77baa41a7774c4a80e63639e;hb=HEAD

> In group case a check should be
> done for all snapshot first, and I need to support qmp transaction interface
> which is already there, so I build an internal transaction layer below to make
> the interface unified and relative easier to add in qmp transaction.

IMHO qmp transactions are a bit clumsy and inflexible. And as you can see with my patches,
there is no real need for them.

> > Seems this makes it impossible to use external commands to make
> snapshots?
> >
>    what command? can you tip more about the case?

For example, nexenta storage provides an API to create snapshots. We want to use
that. Another example would be to use lvcreate to create lvm snapshots.

- Dietmar

Re: [Qemu-devel] [PATCH 4/6] snapshot: implemention of common API to take snapshots

[Wenchao Xia]

于 2012-12-17 15:52, Dietmar Maurer 写道:
>> 于 2012-12-17 14:36, Dietmar Maurer 写道:
>>>>     This patch is the implemention of transaction based snapshot internal
>> API.
>>>
>>> What exactly is the advantage of using qmp transactions (as opposed to the
>> pve snapshot patches)?
>>     Which pve snapshot patches you referring?
>
> I refer to:
>
> https://git.proxmox.com/?p=pve-qemu-kvm.git;a=blob;f=debian/patches/internal-snapshot-async.patch;h=6c86de3a6160c58d77baa41a7774c4a80e63639e;hb=HEAD
>
>> In group case a check should be
>> done for all snapshot first, and I need to support qmp transaction interface
>> which is already there, so I build an internal transaction layer below to make
>> the interface unified and relative easier to add in qmp transaction.
>
> IMHO qmp transactions are a bit clumsy and inflexible. And as you can see with my patches,
> there is no real need for them.
>
   There are two layers, qmp transaction and BlkTransactionStates, user
can use qmp_transaction interface or simply qmp_snapshot_create
interface, which are both consumer of BlkTransactionStates.

   Compared to the pre patch, I think what different is for group
use case and integration with external snapshot code. mainly reason
are:
   1 in group case, it need a queue with element records each one's
states, so it can revert on fail, that date structure is
BlkTransactionStatesList.
   2 in group case check should be done first, so separate check,
execution, cancel functions should be there. And there are two
type of snapshot there with two type of operation(create and delete),
so implement code will be fragment, need to packaged them in unified
way, then upper level function such as savevm can use it gracefully.
BlkTransactionStates can hide the implement details.
   3 group internal and external snapshot have same logic, they can be
merged with callback functions, which is embbed in BlkTransactionStates,
otherwise there will be "if" in many place makes code ugly which way I
have tried and falled back from.

some other reason not related to pre patch:
   4 qmp_transaction interface is already there for external snapshot,
so I think internal snapshot should also support it.
BlkTransactionStates make it easier.
   5 BlkTransactionStatesList use qemu internal data types instead data
exposed in BlockDevAction, this allows other code inside qemu pass in
data not observable to user and hide some functions to user.


>>> Seems this makes it impossible to use external commands to make
>> snapshots?
>>>
>>     what command? can you tip more about the case?
>
> For example, nexenta storage provides an API to create snapshots. We want to use
> that. Another example would be to use lvcreate to create lvm snapshots.
>
   I am not familar with those tools, Patch 5 and 6 provides hmp and qmp
API, hope you can enlight more what is  missing if it can't work with
external tool.

> - Dietmar
>


-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH 4/6] snapshot: implemention of common API to take snapshots

[Dietmar Maurer]

> >>> Seems this makes it impossible to use external commands to make
> >> snapshots?
> >>>
> >>     what command? can you tip more about the case?
> >
> > For example, nexenta storage provides an API to create snapshots. We
> > want to use that. Another example would be to use lvcreate to create lvm
> snapshots.
> >
>    I am not familar with those tools, Patch 5 and 6 provides hmp and qmp API,
> hope you can enlight more what is  missing if it can't work with external tool.

We need the ability to create snapshots with external tools - your patch does not support that.
I think that is a major drawback.

Re: [Qemu-devel] [PATCH 4/6] snapshot: implemention of common API to take snapshots

[Eric Blake]

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

On 12/17/2012 02:58 AM, Dietmar Maurer wrote:
>>>>> Seems this makes it impossible to use external commands to make
>>>> snapshots?
>>>>>
>>>>     what command? can you tip more about the case?
>>>
>>> For example, nexenta storage provides an API to create snapshots. We
>>> want to use that. Another example would be to use lvcreate to create lvm
>> snapshots.
>>>
>>    I am not familar with those tools, Patch 5 and 6 provides hmp and qmp API,
>> hope you can enlight more what is  missing if it can't work with external tool.
> 
> We need the ability to create snapshots with external tools - your patch does not support that.
> I think that is a major drawback.

I think the ability to create snapshots with external tools is going to
require asynchronous snapshots, where the work is divided into several
phases.  You'll need qemu to prepare for the snapshot, then the external
tools to create the snapshot, then resume qemu with possibly different
file names as a result of the snapshot being created.  I'm certainly
interested in seeing what you come up with, and how libvirt will be able
to interact with it for external tool snapshots.

-- 
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: 619 bytes --]

Re: [Qemu-devel] [PATCH 4/6] snapshot: implemention of common API to take snapshots

[Wenchao Xia]

于 2012-12-21 6:19, Eric Blake 写道:
> On 12/17/2012 02:58 AM, Dietmar Maurer wrote:
>>>>>> Seems this makes it impossible to use external commands to make
>>>>> snapshots?
>>>>>>
>>>>>      what command? can you tip more about the case?
>>>>
>>>> For example, nexenta storage provides an API to create snapshots. We
>>>> want to use that. Another example would be to use lvcreate to create lvm
>>> snapshots.
>>>>
>>>     I am not familar with those tools, Patch 5 and 6 provides hmp and qmp API,
>>> hope you can enlight more what is  missing if it can't work with external tool.
>>
>> We need the ability to create snapshots with external tools - your patch does not support that.
>> I think that is a major drawback.
>
> I think the ability to create snapshots with external tools is going to
> require asynchronous snapshots, where the work is divided into several
> phases.  You'll need qemu to prepare for the snapshot, then the external
> tools to create the snapshot, then resume qemu with possibly different
> file names as a result of the snapshot being created.  I'm certainly
> interested in seeing what you come up with, and how libvirt will be able
> to interact with it for external tool snapshots.
>

   If libvirt could integrate external tool using code, that would be
great. For qemu, My understanding is just to take internal snapshot
and stop vm, then let management stack do the things remain. Dietmar,
do you think that is all what needed in qemu?

-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH 4/6] snapshot: implemention of common API to take snapshots

[Dietmar Maurer]

>    If libvirt could integrate external tool using code, that would be great. For
> qemu, My understanding is just to take internal snapshot and stop vm,

You just need to save VM state, flush all IO requests, then stop.

After that the management framework can issue command to:

* create internal snapshots (qmp)
* create external snapshots (qmp)
* create snapshot using external tools (lvcreate, nexenta API, btrfs?, ...)

> then let management stack do the things remain. Dietmar, do you think that is all
> what needed in qemu?

Basically yes. Maybe we need to provide some 'abort/cleanup' handler to restore
state if something fail (AFAIK external snapshots needs that).

Re: [Qemu-devel] [PATCH 4/6] snapshot: implemention of common API to take snapshots

[Stefan Hajnoczi]

On Fri, Dec 21, 2012 at 06:20:40AM +0000, Dietmar Maurer wrote:
> >    If libvirt could integrate external tool using code, that would be great. For
> > qemu, My understanding is just to take internal snapshot and stop vm,
> 
> You just need to save VM state, flush all IO requests, then stop.
> 
> After that the management framework can issue command to:
> 
> * create internal snapshots (qmp)
> * create external snapshots (qmp)
> * create snapshot using external tools (lvcreate, nexenta API, btrfs?, ...)
> 
> > then let management stack do the things remain. Dietmar, do you think that is all
> > what needed in qemu?
> 
> Basically yes. Maybe we need to provide some 'abort/cleanup' handler to restore
> state if something fail (AFAIK external snapshots needs that).

Yes, I agree that we several steps that can be orchestrated over QMP -
and remember the QEMU guest agent which can ensure consistent backups.

External snapshots (LVM, btrfs, etc) should happen outside of QEMU,
usually in the management stack.  It's not just beyond the scope of QEMU
to execute LVM, btrfs, etc tools but also not possible when SELinux or
file descriptor passing are used.

Stefan

Re: [Qemu-devel] [PATCH 4/6] snapshot: implemention of common API to take snapshots

[Dietmar Maurer]

> > For example, nexenta storage provides an API to create snapshots. We
> > want to use that. Another example would be to use lvcreate to create lvm
> snapshots.
> >
>    I am not familar with those tools

You do not know LVM?
 

Re: [Qemu-devel] [PATCH 4/6] snapshot: implemention of common API to take snapshots

[Wenchao Xia]

于 2012-12-17 18:32, Dietmar Maurer 写道:
>>> For example, nexenta storage provides an API to create snapshots. We
>>> want to use that. Another example would be to use lvcreate to create lvm
>> snapshots.
>>>
>>     I am not familar with those tools
>
> You do not know LVM?
>
   Honest speaking I use LVM tools little. I wonder why lvcreate can't
be used, for block internal snapshot I think this patch have same
function as your previous patch, what is missing?

>


-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH 4/6] snapshot: implemention of common API to take snapshots

[Dietmar Maurer]

> >>     I am not familar with those tools
> >
> > You do not know LVM?
> >
>    Honest speaking I use LVM tools little. I wonder why lvcreate can't be used,
> for block internal snapshot I think this patch have same function as your
> previous patch, what is missing?

Qemu does not have any information about the underlying storage. So creating
lvm snapshot must be done from the management software.

Re: [Qemu-devel] [PATCH 4/6] snapshot: implemention of common API to take snapshots

[Wenchao Xia]

于 2012-12-18 18:36, Dietmar Maurer 写道:
>>>>      I am not familar with those tools
>>>
>>> You do not know LVM?
>>>
>>     Honest speaking I use LVM tools little. I wonder why lvcreate can't be used,
>> for block internal snapshot I think this patch have same function as your
>> previous patch, what is missing?
> 
> Qemu does not have any information about the underlying storage. So creating
> lvm snapshot must be done from the management software.
> 
  OK, I think underlying storage is another level of issue, just
wondering what this patch miss to stop you use external tool, did
you called a LVM/nexenta C API in previous patch?

-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH 4/6] snapshot: implemention of common API to take snapshots

[Dietmar Maurer]

>   OK, I think underlying storage is another level of issue, just wondering what
> this patch miss to stop you use external tool, did you called a LVM/nexenta C
> API in previous patch?

The trick is to do thing step by step

1.) save state
2.) create internal snapshots
3.) call external tools to create snapshots
...

1 and 2 is done inside qemu, 3 is done from the management tool.

You use qmp_transactions, which makes that impossible.

Re: [Qemu-devel] [PATCH 4/6] snapshot: implemention of common API to take snapshots

[Wenchao Xia]

于 2012-12-19 12:55, Dietmar Maurer 写道:
>>    OK, I think underlying storage is another level of issue, just wondering what
>> this patch miss to stop you use external tool, did you called a LVM/nexenta C
>> API in previous patch?
>
> The trick is to do thing step by step
>
> 1.) save state
> 2.) create internal snapshots
> 3.) call external tools to create snapshots
> ...
>
> 1 and 2 is done inside qemu, 3 is done from the management tool.
>
> You use qmp_transactions, which makes that impossible.
>

   There is simple HMP/QMP API for 2), in patch 5,6, not in the
form of qmp_transaction, simply blkdev_snapshot() form.
1) will be added later, which have some issue need to solve now.

qmp_transaction() or blkdev_snapshot() call the common API
in this patch. "common API" in this patch is for qemu not user,
it is hidden, maybe that caused misunderstanding.


-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH 4/6] snapshot: implemention of common API to take snapshots

[Juan Quintela]

Wenchao Xia <xiawenc@linux.vnet.ibm.com> wrote:
>   This patch is the implemention of transaction based snapshot
> internal API. Internal snapshot for specified block device
> is added, which can be used as part of functionality in one way
> to live full back up vm seperating internal block snapshot and
> vmstate(vmstate goes to another file, not implemented in this
> patch).
>   Every request's handler need to have three function:
> execution, updating, cancelling. Also another check function

canceling

> could be provided to check if request is valid before execition.
>   internal snapshot implemention was based on the code from
> dietmar@proxmox.com.
>
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> Signed-off-by: Dietmar Maurer <dietmar@proxmox.com>
> ---
>  blockdev.c |  515 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 515 insertions(+), 0 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index 9a05e57..1c38c67 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -660,6 +660,521 @@ void do_commit(Monitor *mon, const QDict *qdict)
>      }
>  }
>  
> +/*   snapshot functions.
> + *   following are implemention around core struct BlkTransactionStates.

Normally qemu commonts dont' start with one space.
Same for all comments in the series

> + * to start, invalidate, cancel the action.
> + */
> +
> +/* Block internal snapshot(qcow2, sheepdog image...) support.
> +   checking and execution was splitted to enable a check for every device
> +before execution in group case. */
> +
> +SNTime get_sn_time(void)
> +{
> +    SNTime time;
> +    /* is uint32_t big enough to get time_t value on other machine ? */
> +#ifdef _WIN32
> +    struct _timeb tb;
> +    _ftime(&tb);
> +    time.date_sec = tb.time;
> +    time.date_nsec = tb.millitm * 1000000;
> +#else
> +    struct timeval tv;
> +    gettimeofday(&tv, NULL);
> +    time.date_sec = tv.tv_sec;
> +    time.date_nsec = tv.tv_usec * 1000;
> +#endif

Can we move this bit of code os-lib-win32.c?
(yes, the mess already existed before this patch)

> +    time.vm_clock_nsec = qemu_get_clock_ns(vm_clock);
> +    return time;
> +}
> +
> +void generate_sn_name_from_time(SNTime *time, char *time_str, int size)
> +{
> +#ifdef _WIN32
> +    time_t t = time->date_sec;
> +    struct tm *ptm = localtime(&t);
> +    strftime(time_str, size, "vm-%Y%m%d%H%M%S", ptm);
> +#else
> +    /* cast below needed for OpenBSD where tv_sec is still 'long' */
> +    time_t second = time->date_sec;
> +    struct tm tm;
> +    localtime_r((const time_t *)&second, &tm);
> +    strftime(time_str, size, "vm-%Y%m%d%H%M%S", &tm);
> +#endif

can we use localtime_r() also for windows?  We have one implementation
on os-lib-win32.c?  It says that it miss locking, should we look at
improving it instead?

> +}
> +
> +static int blk_sn_check_create_internal_sync(BlkTransactionStates *states,
> +                                             Error **errp)
> +{
> +    BlkTransactionStatesSync *st_sync = &states->st_sync;
> +    BlockDriverState *bs = st_sync->internal.bs;
> +    const char *device = bdrv_get_device_name(bs);
> +    QEMUSnapshotInfo *old_sn = &st_sync->internal.old_sn;
> +    const char *name = st_sync->internal.sn_name;
> +    bool use_existing = st_sync->use_existing;
> +    int ret;
> +
> +    if (!bdrv_is_inserted(bs)) {
> +        error_set_replace(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
> +        return -1;
> +    }
> +
> +    if (bdrv_is_read_only(bs)) {
> +        error_set_replace(errp, QERR_DEVICE_IS_READ_ONLY, device);
> +        return -1;
> +    }
> +
> +    if (!bdrv_can_snapshot(bs)) {
> +        error_set_replace(errp, QERR_NOT_SUPPORTED);
> +        return -1;
> +    }
> +
> +    ret = bdrv_snapshot_find(bs, old_sn, name);
> +    if (ret >= 0) {
> +        st_sync->internal.old_sn_exist = TRUE;
> +    } else {
> +        if (use_existing) {
> +            /* caller require use existing one */
> +            error_set_replace(errp, ERROR_CLASS_GENERIC_ERROR,
> +                              "snapshot '%s' not exists on '%s' but caller "
> +                              "want to use it.",
> +                              name, device);
> +            return -1;
> +        }
> +    }
> +
> +    st_sync->internal.step = BLK_SNAPSHOT_INT_START;
> +    return 0;
> +}
> +
> +static int blk_sn_create_internal_sync(BlkTransactionStates *states,
> +                                       Error **errp)
> +{
> +    BlkTransactionStatesSync *st_sync = &states->st_sync;
> +    BlockDriverState *bs = st_sync->internal.bs;
> +    const char *name = st_sync->internal.sn_name;
> +    QEMUSnapshotInfo *sn = &st_sync->internal.sn;
> +    int ret = 0;
> +    const char *device = bdrv_get_device_name(bs);
> +    QEMUSnapshotInfo *old_sn = &st_sync->internal.old_sn;
> +
> +    SNTime *time = &st_sync->internal.time;
> +    if (time->vm_clock_nsec == 0) {
> +        /* not preset, it need to be set */
> +        error_setg_replace(errp,
> +                           "Invalid timestamp was set on for '%s' on '%s'\n",
> +                           name, device);
> +        return -1;
> +    }
> +
> +    sn->date_sec = time->date_sec;
> +    sn->date_nsec = time->date_nsec;
> +    sn->vm_clock_nsec = time->vm_clock_nsec;
> +    sn->vm_state_size = st_sync->internal.vm_state_size;
> +
> +    if (st_sync->internal.old_sn_exist) {
> +        ret = bdrv_snapshot_delete(bs, old_sn->id_str);
> +        if (ret < 0) {
> +            error_setg_replace(errp,
> +                       "Error in deleting existing snapshot %s on '%s'\n",
> +                       name, device);
> +            return -1;
> +        }
> +        pstrcpy(sn->name, sizeof(sn->name), old_sn->name);
> +        pstrcpy(sn->id_str, sizeof(sn->id_str), old_sn->id_str);
> +    } else {
> +        pstrcpy(sn->name, sizeof(sn->name), name);
> +    }
> +
> +    ret = bdrv_snapshot_create(bs, sn);
> +    if (ret < 0) {
> +        error_set_replace(errp, ERROR_CLASS_GENERIC_ERROR,
> +                  "Error while creating snapshot on '%s'\n", device);
> +        return -1;
> +    }
> +
> +    st_sync->internal.step = BLK_SNAPSHOT_INT_CREATED;
> +    return 0;
> +}
> +
> +static int blk_sn_cancel_internal_sync(BlkTransactionStates *states,
> +                                       Error **errp)
> +{
> +    BlkTransactionStatesSync *st_sync = &states->st_sync;
> +    const char *name = st_sync->internal.sn_name;
> +    BlockDriverState *bs = st_sync->internal.bs;
> +    const char *device = bdrv_get_device_name(bs);
> +    int ret;
> +
> +    if (st_sync->internal.step == BLK_SNAPSHOT_INT_CREATED) {
> +        if (st_sync->internal.old_sn_exist) {
> +            error_setg_replace(errp,
> +                       "sn '%s' on '%s' exist originally at it have been "
> +                       "overwritten, can't cancel the action.\n",
> +                       name, device);
> +            return -1;
> +        }
> +        ret = bdrv_snapshot_delete(bs, name);
> +        if (ret < 0) {
> +            error_setg_replace(errp,
> +                       "Error while deleting snapshot '%s' on '%s' to "
> +                       "cancel the operation.\n", name, device);
> +            return -1;
> +        }
> +        st_sync->internal.step = BLK_SNAPSHOT_INT_CANCELED;
> +    }
> +    return 0;
> +}
> +
> +static int blk_sn_check_delete_internal_sync(BlkTransactionStates *states,
> +                                             Error **errp)
> +{
> +    BlkTransactionStatesSync *st_sync = &states->st_sync;
> +    BlockDriverState *bs = st_sync->internal.bs;
> +    QEMUSnapshotInfo *old_sn = &st_sync->internal.old_sn;
> +    const char *device = bdrv_get_device_name(bs);
> +    const char *name = st_sync->internal.sn_name;
> +    int ret;
> +
> +    if (bdrv_is_read_only(bs)) {
> +        error_set_replace(errp, QERR_DEVICE_IS_READ_ONLY, device);
> +        return -1;
> +    }
> +
> +    if (!bdrv_can_snapshot(bs)) {
> +        error_set_replace(errp, QERR_NOT_SUPPORTED);
> +        return -1;
> +    }
> +
> +    ret = bdrv_snapshot_find(bs, old_sn, name);
> +    if (ret < 0) {
> +        /* caller require use existing one */
> +        error_set_replace(errp, ERROR_CLASS_GENERIC_ERROR,
> +                          "snapshot '%s' not exists on '%s'.",
> +                          name, device);
> +        return -1;
> +    }
> +    return 0;
> +}
> +
> +static int blk_sn_delete_internal_sync(BlkTransactionStates *states,
> +                                       Error **errp)
> +{
> +    BlkTransactionStatesSync *st_sync = &states->st_sync;
> +    int ret = 0;
> +    const char *name = st_sync->internal.sn_name;
> +    BlockDriverState *bs = st_sync->internal.bs;
> +    const char *device = bdrv_get_device_name(bs);
> +
> +    /* no need to check snapshot existence? */
> +    ret = bdrv_snapshot_delete(bs, name);
> +    if (ret < 0) {
> +        error_setg_replace(errp,
> +                  "Error while deleting snapshot on '%s'\n", device);
> +        return -1;
> +    }
> +    return 0;
> +}
> +
> +
> +/* Block external snapshot(backing file chain) support. */
> +
> +static int blk_sn_check_create_external_sync(BlkTransactionStates *states,
> +                                             Error **errp)
> +{
> +    BlkTransactionStatesSync *st_sync = &states->st_sync;
> +    BlockDriverState *old_bs = st_sync->external.old_bs;
> +    const char *device = bdrv_get_device_name(old_bs);
> +    const char *format = st_sync->external.format;
> +    const char *new_image_file = st_sync->external.new_image_file;
> +    BlockDriver *proto_drv;
> +    BlockDriver *drv;
> +
> +    if (!bdrv_is_inserted(old_bs)) {
> +        error_set_replace(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
> +        return -1;
> +    }
> +
> +    if (bdrv_in_use(old_bs)) {
> +        error_set_replace(errp, QERR_DEVICE_IN_USE, device);
> +        return -1;
> +    }
> +
> +    if (!bdrv_is_read_only(old_bs)) {
> +        if (bdrv_flush(old_bs)) {
> +            error_set_replace(errp, QERR_IO_ERROR);
> +            return -1;
> +        }
> +    }
> +
> +    drv = bdrv_find_format(format);
> +    if (!drv) {
> +        error_set_replace(errp, QERR_INVALID_BLOCK_FORMAT, format);
> +        return -1;
> +    }
> +    st_sync->external.format_drv = drv;
> +
> +    proto_drv = bdrv_find_protocol(new_image_file);
> +    if (!proto_drv) {
> +        error_set_replace(errp, QERR_INVALID_BLOCK_FORMAT, format);
> +        return -1;
> +    }
> +    st_sync->external.step = BLK_SNAPSHOT_EXT_START;
> +    return 0;
> +}
> +
> +/* We don't do anything that commits us to the snapshot here, do it later. */
> +static int blk_sn_create_external_sync(BlkTransactionStates *states,
> +                                       Error **errp)
> +{
> +    BlkTransactionStatesSync *st_sync = &states->st_sync;
> +    BlockDriverState *old_bs = st_sync->external.old_bs;
> +    const char *format = st_sync->external.format;
> +    BlockDriver *drv = st_sync->external.format_drv;
> +    const char *new_image_file = st_sync->external.new_image_file;
> +    bool use_existing = st_sync->use_existing;
> +    BlockDriverState *new_bs;
> +    Error *local_err = NULL;
> +
> +    int flags = old_bs->open_flags, ret;
> +    /* create new image w/backing file */
> +    if (!use_existing) {
> +        bdrv_img_create(new_image_file, format,
> +                        old_bs->filename,
> +                        old_bs->drv->format_name,
> +                        NULL, -1, flags, &local_err);
> +        if (error_is_set(&local_err)) {
> +            error_propagate(errp, local_err);
> +            return -1;
> +        }
> +    }
> +
> +    /* We will manually add the backing_hd field to the bs later */
> +    new_bs = bdrv_new("");
> +    ret = bdrv_open(new_bs, new_image_file,
> +                    flags | BDRV_O_NO_BACKING, drv);
> +    if (ret != 0) {
> +        error_set_replace(errp, QERR_OPEN_FILE_FAILED, new_image_file);
> +        return -1;
> +    }
> +
> +    st_sync->external.new_bs = new_bs;
> +    st_sync->external.step = BLK_SNAPSHOT_EXT_CREATED;
> +    return 0;
> +}
> +
> +/* refresh the blk device's states to emulator, always succeed now. */
> +static int blk_sn_invalid_external_sync(BlkTransactionStates *states,
> +                                        Error **errp)
> +{
> +    BlkTransactionStatesSync *st_sync = &states->st_sync;
> +    BlkSnapshotExternal *ext = &st_sync->external;
> +    /* This removes our old bs from the bdrv_states, and adds the new
> +     * bs */
> +    bdrv_append(ext->new_bs, ext->old_bs);
> +    /* We don't need (or want) to use the transactional
> +     * bdrv_reopen_multiple() across all the entries at once, because
> +     * we don't want to abort all of them if one of them fails the
> +     * reopen */
> +    bdrv_reopen(ext->new_bs,
> +                ext->new_bs->open_flags & ~BDRV_O_RDWR,
> +                NULL);
> +
> +    st_sync->external.step = BLK_SNAPSHOT_EXT_INVALIDATED;
> +    return 0;
> +}
> +
> +/* cancel the job if it have been done before */
> +static int blk_sn_cancel_external_sync(BlkTransactionStates *states,
> +                                       Error **errp)
> +{
> +    BlkTransactionStatesSync *st_sync = &states->st_sync;
> +    BlkSnapshotExternal *ext = &st_sync->external;
> +    if (ext->step == BLK_SNAPSHOT_EXT_INVALIDATED) {
> +        /* revert the bind */
> +        bdrv_deappend(ext->new_bs, ext->old_bs);
> +        bdrv_reopen(ext->old_bs,
> +                    ext->old_bs->open_flags & ~BDRV_O_RDWR,
> +                    NULL);
> +        ext->step = BLK_SNAPSHOT_EXT_CREATED;
> +    }
> +    if (ext->step == BLK_SNAPSHOT_EXT_CREATED) {
> +        /* abandon new bs, and keep using the original bs.
> +         * no need to delete the image? */
> +        bdrv_delete(ext->new_bs);
> +        ext->step = BLK_SNAPSHOT_EXT_CANCELED;
> +    }
> +    return 0;
> +}
> +
> +/*
> + * caller's API to add, submit request, support mixed request.
> + */
> +
> +BlkTransactionStates *blk_trans_st_new(void)
> +{
> +    BlkTransactionStates *states = g_malloc0(sizeof(BlkTransactionStates));
> +    return states;
> +}
> +
> +void blk_trans_st_delete(BlkTransactionStates **p_st)
> +{
> +    if ((*p_st)->err != NULL) {
> +        error_free((*p_st)->err);
> +    }
> +    g_free(*p_st);
> +    *p_st = NULL;
> +    return;
> +}
> +
> +BlkTransactionStatesList *blk_trans_st_list_new(void)
> +{
> +    BlkTransactionStatesList *list =
> +                      g_malloc0(sizeof(BlkTransactionStatesList));
> +    QSIMPLEQ_INIT(list);
> +    return list;
> +}
> +
> +void blk_trans_st_list_delete(BlkTransactionStatesList **p_list)
> +{
> +    BlkTransactionStates *states, *next;
> +    QSIMPLEQ_FOREACH_SAFE(states, (*p_list), entry, next) {
> +        blk_trans_st_delete(&states);
> +    }
> +    g_free(*p_list);
> +    *p_list = NULL;
> +    return;
> +}
> +
> +int add_transaction(BlkTransactionStatesList *list,
> +                    BlkTransactionStates *states,
> +                    Error **errp)
> +{
> +    int ret;
> +
> +    if (states->async) {
> +        abort();
> +    }
> +
> +    switch (states->st_sync.op) {
> +    case BLK_SN_SYNC_CREATE:
> +        if (states->st_sync.type == BLK_SNAPSHOT_INTERNAL) {

This is spelled:

switch(states-s>st_sync.type) {
    case BLK_SNAPSHOT_INTERNAL:
    .....
}

> +            ret = blk_sn_check_create_internal_sync(states, errp);
> +            if (ret < 0) {
> +                return -1;
> +            }
> +            states->blk_trans_do = blk_sn_create_internal_sync;
> +            states->blk_trans_cancel = blk_sn_cancel_internal_sync;
> +        } else if (states->st_sync.type == BLK_SNAPSHOT_EXTERNAL) {
> +            ret = blk_sn_check_create_external_sync(states, errp);
> +            if (ret < 0) {
> +                return -1;
> +            }
> +            states->blk_trans_do = blk_sn_create_external_sync;
> +            states->blk_trans_invalid = blk_sn_invalid_external_sync;
> +            states->blk_trans_cancel = blk_sn_cancel_external_sync;
> +        } else {
> +            error_setg_replace(errp, "Operation %d for type %d not supprted.",
> +                               states->st_sync.op, states->st_sync.type);
> +            return -1;
> +        }
> +        break;
> +    case BLK_SN_SYNC_DELETE:
> +        if (states->st_sync.type == BLK_SNAPSHOT_INTERNAL) {
> +            ret = blk_sn_check_delete_internal_sync(states, errp);
> +            if (ret < 0) {
> +                return -1;
> +            }
> +            states->blk_trans_do = blk_sn_delete_internal_sync;
> +            /* this operation can't be canceled. */
> +        } else if (states->st_sync.type == BLK_SNAPSHOT_EXTERNAL) {
> +            /* sync delete an external snapshot, not support now,
> +               use blk commit instead. */
> +            error_setg_replace(errp, "Operation %d for type %d not supprted.",
> +                               states->st_sync.op, states->st_sync.type);
> +            return -1;
> +        } else {
> +            error_setg_replace(errp, "Operation %d for type %d not supprted.",
> +                               states->st_sync.op, states->st_sync.type);
> +            return -1;
> +        }
> +        break;
> +    default:
> +        return -1;
> +    }
> +
> +    /* Todo, in async case const char* member should be duplicated, but
> +    we do not support it now so direct enlist the request. */
> +    QSIMPLEQ_INSERT_TAIL(list, states, entry);
> +    return 0;
> +}
> +
> +int submit_transaction(BlkTransactionStatesList *list, Error **errp)
> +{
> +    BlkTransactionStates *states = NULL;
> +    int ret = 0;
> +    bool error_set = false;
> +
> +    /* drain all i/o before any snapshots */
> +    bdrv_drain_all();
> +
> +    /* step 1, do the action, that is create/delete snapshots in sync case */
> +    QSIMPLEQ_FOREACH(states, list, entry) {
> +        if (states->async) {
> +            abort();
> +        } else {
> +            ret = states->blk_trans_do(states, &states->err);
> +            if (ret < 0) {
> +                if ((!error_set) && (states->err)) {

Parens are not needed here
                  if (!error_set && states->err) {

> +                    *errp = error_copy(states->err);
> +                    error_set = TRUE;

TRUE is a constant, here you want "true" lowercase.


> +                }
> +                goto delete_and_fail;
> +            }
> +        }
> +    }
> +
> +    /* step 2, update emulator */
> +    QSIMPLEQ_FOREACH(states, list, entry) {
> +        if (states->async) {
> +            abort();
> +        } else {
> +            if (states->blk_trans_invalid) {
> +                ret = states->blk_trans_invalid(states, &states->err);
> +                if (ret < 0) {

> +                    if ((!error_set) && (states->err)) {
> +                        *errp = error_copy(states->err);
> +                        error_set = TRUE;

This snip is repeated in all the loops, can't we factor it out?

> +                    }
> +                    goto delete_and_fail;
> +                }
> +            }
> +        }
> +    }
> +
> +    /* success */
> +    return 0;
> +
> +delete_and_fail:
> +    /*
> +     * failure, and it is all-or-none, cancel all if error got.
> +     */
> +    QSIMPLEQ_FOREACH(states, list, entry) {
> +        if (states->async) {
> +            abort();
> +        } else {
> +            if (states->blk_trans_cancel) {
> +                ret = states->blk_trans_cancel(states, &states->err);
> +                if ((ret < 0) && (!error_set) && (states->err)) {
> +                    *errp = error_copy(states->err);
> +                    error_set = TRUE;
> +                }
> +            }
> +        }
> +    }
> +    return -1;
> +}
> +
>  static void blockdev_do_action(int kind, void *data, Error **errp)
>  {
>      BlockdevAction action;

Re: [Qemu-devel] [PATCH 4/6] snapshot: implemention of common API to take snapshots

[Wenchao Xia]

>>    Every request's handler need to have three function:
>> execution, updating, cancelling. Also another check function
> 
> canceling
> 
  OK.

>> could be provided to check if request is valid before execition.
>>    internal snapshot implemention was based on the code from
>> dietmar@proxmox.com.
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> Signed-off-by: Dietmar Maurer <dietmar@proxmox.com>
>> ---
>>   blockdev.c |  515 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 files changed, 515 insertions(+), 0 deletions(-)
>>
>> diff --git a/blockdev.c b/blockdev.c
>> index 9a05e57..1c38c67 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -660,6 +660,521 @@ void do_commit(Monitor *mon, const QDict *qdict)
>>       }
>>   }
>>   
>> +/*   snapshot functions.
>> + *   following are implemention around core struct BlkTransactionStates.
> 
> Normally qemu commonts dont' start with one space.
> Same for all comments in the series
> 
  OK, will watch for it.

>> + * to start, invalidate, cancel the action.
>> + */
>> +
>> +/* Block internal snapshot(qcow2, sheepdog image...) support.
>> +   checking and execution was splitted to enable a check for every device
>> +before execution in group case. */
>> +
>> +SNTime get_sn_time(void)
>> +{
>> +    SNTime time;
>> +    /* is uint32_t big enough to get time_t value on other machine ? */
>> +#ifdef _WIN32
>> +    struct _timeb tb;
>> +    _ftime(&tb);
>> +    time.date_sec = tb.time;
>> +    time.date_nsec = tb.millitm * 1000000;
>> +#else
>> +    struct timeval tv;
>> +    gettimeofday(&tv, NULL);
>> +    time.date_sec = tv.tv_sec;
>> +    time.date_nsec = tv.tv_usec * 1000;
>> +#endif
> 
> Can we move this bit of code os-lib-win32.c?
> (yes, the mess already existed before this patch)
> 
  Sure, that is where it should belong.

>> +    time.vm_clock_nsec = qemu_get_clock_ns(vm_clock);
>> +    return time;
>> +}
>> +
>> +void generate_sn_name_from_time(SNTime *time, char *time_str, int size)
>> +{
>> +#ifdef _WIN32
>> +    time_t t = time->date_sec;
>> +    struct tm *ptm = localtime(&t);
>> +    strftime(time_str, size, "vm-%Y%m%d%H%M%S", ptm);
>> +#else
>> +    /* cast below needed for OpenBSD where tv_sec is still 'long' */
>> +    time_t second = time->date_sec;
>> +    struct tm tm;
>> +    localtime_r((const time_t *)&second, &tm);
>> +    strftime(time_str, size, "vm-%Y%m%d%H%M%S", &tm);
>> +#endif
> 
> can we use localtime_r() also for windows?  We have one implementation
> on os-lib-win32.c?  It says that it miss locking, should we look at
> improving it instead?
> 
  let me have a check.

>> +int add_transaction(BlkTransactionStatesList *list,
>> +                    BlkTransactionStates *states,
>> +                    Error **errp)
>> +{
>> +    int ret;
>> +
>> +    if (states->async) {
>> +        abort();
>> +    }
>> +
>> +    switch (states->st_sync.op) {
>> +    case BLK_SN_SYNC_CREATE:
>> +        if (states->st_sync.type == BLK_SNAPSHOT_INTERNAL) {
> 
> This is spelled:
> 
> switch(states-s>st_sync.type) {
>      case BLK_SNAPSHOT_INTERNAL:
>      .....
> }
> 
  OK.

>> +int submit_transaction(BlkTransactionStatesList *list, Error **errp)
>> +{
>> +    BlkTransactionStates *states = NULL;
>> +    int ret = 0;
>> +    bool error_set = false;
>> +
>> +    /* drain all i/o before any snapshots */
>> +    bdrv_drain_all();
>> +
>> +    /* step 1, do the action, that is create/delete snapshots in sync case */
>> +    QSIMPLEQ_FOREACH(states, list, entry) {
>> +        if (states->async) {
>> +            abort();
>> +        } else {
>> +            ret = states->blk_trans_do(states, &states->err);
>> +            if (ret < 0) {
>> +                if ((!error_set) && (states->err)) {
> 
> Parens are not needed here
>                    if (!error_set && states->err) {
>
  OK.


>> +                    *errp = error_copy(states->err);
>> +                    error_set = TRUE;
> 
> TRUE is a constant, here you want "true" lowercase.
> 
  OK.

> 
>> +                }
>> +                goto delete_and_fail;
>> +            }
>> +        }
>> +    }
>> +
>> +    /* step 2, update emulator */
>> +    QSIMPLEQ_FOREACH(states, list, entry) {
>> +        if (states->async) {
>> +            abort();
>> +        } else {
>> +            if (states->blk_trans_invalid) {
>> +                ret = states->blk_trans_invalid(states, &states->err);
>> +                if (ret < 0) {
> 
>> +                    if ((!error_set) && (states->err)) {
>> +                        *errp = error_copy(states->err);
>> +                        error_set = TRUE;
> 
> This snip is repeated in all the loops, can't we factor it out?
> 
  Sure, will sort them out.


-- 
Best Regards

Wenchao Xia

[Qemu-devel] [PATCH 5/6] snapshot: qmp interface

[Wenchao Xia]

  This patch changes the implemtion of external block snapshot
to use internal unified interface, now qmp handler just do
a translation of request and submit.
  Also internal block snapshot qmp interface was added.
  Now add external snapshot, add/delete internal snapshot
can be started in their own qmp interface or a group of
BlockAction in qmp transaction interface.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 blockdev.c       |  352 +++++++++++++++++++++++++++++++-----------------------
 qapi-schema.json |  102 ++++++++++++++--
 2 files changed, 292 insertions(+), 162 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 1c38c67..d1bbe23 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1175,6 +1175,195 @@ delete_and_fail:
     return -1;
 }
 
+/* translation from qmp commands */
+static int fill_blk_trsact_create_sync(BlockdevSnapshot *create_sync,
+                                       BlkTransactionStatesSync *st_sync,
+                                       SNTime *time,
+                                       const char *time_str,
+                                       Error **errp)
+{
+    const char *format = "qcow2";
+    enum NewImageMode mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;
+    enum SnapshotType type = SNAPSHOT_TYPE_EXTERNAL;
+    BlockDriverState *bs;
+
+    const char *device = create_sync->device;
+    const char *name = create_sync->snapshot_file;
+    if (create_sync->has_mode) {
+        mode = create_sync->mode;
+    }
+    if (create_sync->has_format) {
+        format = create_sync->format;
+    }
+    if (create_sync->has_type) {
+        type = create_sync->type;
+    }
+
+    /* find the target bs */
+    bs = bdrv_find(device);
+    if (!bs) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+        return -1;
+    }
+
+    switch (type) {
+    case SNAPSHOT_TYPE_INTERNAL:
+        st_sync->type = BLK_SNAPSHOT_INTERNAL;
+        break;
+    case SNAPSHOT_TYPE_EXTERNAL:
+        st_sync->type = BLK_SNAPSHOT_EXTERNAL;
+        break;
+    default:
+        st_sync->type = BLK_SNAPSHOT_NOSUPPORT;
+        error_setg(errp, "Device %s requested invalid snapshot"
+                         " type %d.", device, type);
+        return -1;
+    }
+
+    switch (mode) {
+    case NEW_IMAGE_MODE_EXISTING:
+        st_sync->use_existing = TRUE;
+        break;
+    case NEW_IMAGE_MODE_ABSOLUTE_PATHS:
+        st_sync->use_existing = FALSE;
+        break;
+    default:
+        error_setg(errp, "Device %s requested invalid snapshot"
+                         " mode %d.", device, mode);
+        return -1;
+    }
+
+    if (st_sync->type == BLK_SNAPSHOT_INTERNAL) {
+        /* internal case, if caller need create new one with default string */
+        if (((name == NULL) || (name[0] == '\0')) &&
+           (!st_sync->use_existing)) {
+            st_sync->internal.sn_name = time_str;
+        } else {
+            st_sync->internal.sn_name = name;
+        }
+        st_sync->internal.bs = bs;
+        st_sync->internal.time = *time;
+    } else if (st_sync->type == BLK_SNAPSHOT_EXTERNAL) {
+        st_sync->external.new_image_file = name;
+        st_sync->external.format = format;
+        st_sync->external.old_bs = bs;
+    }
+
+    return 0;
+}
+static int fill_blk_trsact_delete_sync(BlockdevSnapshotDelete *delete_sync,
+                                       BlkTransactionStatesSync *st_sync,
+                                       Error **errp)
+{
+    enum SnapshotType type = SNAPSHOT_TYPE_EXTERNAL;
+    BlockDriverState *bs;
+
+    const char *device = delete_sync->device;
+    const char *name = delete_sync->snapshot_file;
+    if (delete_sync->has_type) {
+        type = delete_sync->type;
+    }
+
+    /* find the target bs */
+    bs = bdrv_find(device);
+    if (!bs) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+        return -1;
+    }
+
+    switch (type) {
+    case SNAPSHOT_TYPE_INTERNAL:
+        st_sync->type = BLK_SNAPSHOT_INTERNAL;
+        break;
+    case SNAPSHOT_TYPE_EXTERNAL:
+        st_sync->type = BLK_SNAPSHOT_EXTERNAL;
+        break;
+    default:
+        st_sync->type = BLK_SNAPSHOT_NOSUPPORT;
+        error_setg(errp, "Device %s requested invalid snapshot"
+                         " type %d.", device, type);
+        return -1;
+    }
+
+    if (st_sync->type == BLK_SNAPSHOT_INTERNAL) {
+        st_sync->internal.sn_name = name;
+        st_sync->internal.bs = bs;
+    } else if (st_sync->type == BLK_SNAPSHOT_EXTERNAL) {
+        st_sync->external.new_image_file = name;
+        st_sync->external.old_bs = bs;
+    }
+
+    return 0;
+}
+
+static int fill_blk_trsact(BlockdevAction *dev_info,
+                           BlkTransactionStates *states,
+                           SNTime *time,
+                           const char *time_str,
+                           Error **errp)
+{
+    switch (dev_info->kind) {
+    case BLOCKDEV_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC:
+        states->st_sync.op = BLK_SN_SYNC_CREATE;
+        states->async = FALSE;
+        return fill_blk_trsact_create_sync(dev_info->blockdev_snapshot_sync,
+                                   &states->st_sync, time, time_str, errp);
+        break;
+    case BLOCKDEV_ACTION_KIND_BLOCKDEV_SNAPSHOT_DELETE_SYNC:
+        states->st_sync.op = BLK_SN_SYNC_DELETE;
+        states->async = FALSE;
+        return fill_blk_trsact_delete_sync(
+                                    dev_info->blockdev_snapshot_delete_sync,
+                                    &states->st_sync, errp);
+        break;
+    default:
+        abort();
+    }
+    return 0;
+}
+
+/* Here this funtion prepare the request list, submit for atomic snapshot. */
+void qmp_transaction(BlockdevActionList *dev_list, Error **errp)
+{
+    BlockdevActionList *dev_entry = dev_list;
+    BlkTransactionStates *states;
+    int ret;
+
+    BlkTransactionStatesList *snap_bdrv_states = blk_trans_st_list_new();
+
+    /* translate qmp request */
+    /* for group snapshot create we use same time stamp here */
+    SNTime time = get_sn_time();
+    char time_str[256];
+    generate_sn_name_from_time(&time, time_str, sizeof(time_str));
+    while (NULL != dev_entry) {
+        BlockdevAction *dev_info = NULL;
+
+        dev_info = dev_entry->value;
+        dev_entry = dev_entry->next;
+
+        states = blk_trans_st_new();
+        ret = fill_blk_trsact(dev_info, states, &time, time_str, errp);
+        if (ret < 0) {
+            blk_trans_st_delete(&states);
+            goto exit;
+        }
+
+        ret = add_transaction(snap_bdrv_states, states, errp);
+        if (ret < 0) {
+            blk_trans_st_delete(&states);
+            goto exit;
+        }
+    }
+
+    /* submit to internal API, no need to check return for no following
+       action now. */
+    submit_transaction(snap_bdrv_states, errp);
+
+exit:
+    blk_trans_st_list_delete(&snap_bdrv_states);
+}
+
 static void blockdev_do_action(int kind, void *data, Error **errp)
 {
     BlockdevAction action;
@@ -1190,6 +1379,7 @@ static void blockdev_do_action(int kind, void *data, Error **errp)
 void qmp_blockdev_snapshot_sync(const char *device, const char *snapshot_file,
                                 bool has_format, const char *format,
                                 bool has_mode, enum NewImageMode mode,
+                                bool has_type, enum SnapshotType type,
                                 Error **errp)
 {
     BlockdevSnapshot snapshot = {
@@ -1199,162 +1389,28 @@ void qmp_blockdev_snapshot_sync(const char *device, const char *snapshot_file,
         .format = (char *) format,
         .has_mode = has_mode,
         .mode = mode,
+        .has_type = has_type,
+        .type = type,
     };
     blockdev_do_action(BLOCKDEV_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC, &snapshot,
                        errp);
 }
 
-
-/* New and old BlockDriverState structs for group snapshots */
-typedef struct BlkTransactionStates {
-    BlockDriverState *old_bs;
-    BlockDriverState *new_bs;
-    QSIMPLEQ_ENTRY(BlkTransactionStates) entry;
-} BlkTransactionStates;
-
-/*
- * 'Atomic' group snapshots.  The snapshots are taken as a set, and if any fail
- *  then we do not pivot any of the devices in the group, and abandon the
- *  snapshots
- */
-void qmp_transaction(BlockdevActionList *dev_list, Error **errp)
+void qmp_blockdev_snapshot_delete_sync(const char *device,
+                                       const char *snapshot_file,
+                                       bool has_type, enum SnapshotType type,
+                                       Error **errp)
 {
-    int ret = 0;
-    BlockdevActionList *dev_entry = dev_list;
-    BlkTransactionStates *states, *next;
-    Error *local_err = NULL;
-
-    QSIMPLEQ_HEAD(snap_bdrv_states, BlkTransactionStates) snap_bdrv_states;
-    QSIMPLEQ_INIT(&snap_bdrv_states);
-
-    /* drain all i/o before any snapshots */
-    bdrv_drain_all();
-
-    /* We don't do anything in this loop that commits us to the snapshot */
-    while (NULL != dev_entry) {
-        BlockdevAction *dev_info = NULL;
-        BlockDriver *proto_drv;
-        BlockDriver *drv;
-        int flags;
-        enum NewImageMode mode;
-        const char *new_image_file;
-        const char *device;
-        const char *format = "qcow2";
-
-        dev_info = dev_entry->value;
-        dev_entry = dev_entry->next;
-
-        states = g_malloc0(sizeof(BlkTransactionStates));
-        QSIMPLEQ_INSERT_TAIL(&snap_bdrv_states, states, entry);
-
-        switch (dev_info->kind) {
-        case BLOCKDEV_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC:
-            device = dev_info->blockdev_snapshot_sync->device;
-            if (!dev_info->blockdev_snapshot_sync->has_mode) {
-                dev_info->blockdev_snapshot_sync->mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;
-            }
-            new_image_file = dev_info->blockdev_snapshot_sync->snapshot_file;
-            if (dev_info->blockdev_snapshot_sync->has_format) {
-                format = dev_info->blockdev_snapshot_sync->format;
-            }
-            mode = dev_info->blockdev_snapshot_sync->mode;
-            break;
-        default:
-            abort();
-        }
-
-        drv = bdrv_find_format(format);
-        if (!drv) {
-            error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
-            goto delete_and_fail;
-        }
-
-        states->old_bs = bdrv_find(device);
-        if (!states->old_bs) {
-            error_set(errp, QERR_DEVICE_NOT_FOUND, device);
-            goto delete_and_fail;
-        }
-
-        if (!bdrv_is_inserted(states->old_bs)) {
-            error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
-            goto delete_and_fail;
-        }
-
-        if (bdrv_in_use(states->old_bs)) {
-            error_set(errp, QERR_DEVICE_IN_USE, device);
-            goto delete_and_fail;
-        }
-
-        if (!bdrv_is_read_only(states->old_bs)) {
-            if (bdrv_flush(states->old_bs)) {
-                error_set(errp, QERR_IO_ERROR);
-                goto delete_and_fail;
-            }
-        }
-
-        flags = states->old_bs->open_flags;
-
-        proto_drv = bdrv_find_protocol(new_image_file);
-        if (!proto_drv) {
-            error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
-            goto delete_and_fail;
-        }
-
-        /* create new image w/backing file */
-        if (mode != NEW_IMAGE_MODE_EXISTING) {
-            bdrv_img_create(new_image_file, format,
-                            states->old_bs->filename,
-                            states->old_bs->drv->format_name,
-                            NULL, -1, flags, &local_err);
-            if (error_is_set(&local_err)) {
-                error_propagate(errp, local_err);
-                goto delete_and_fail;
-            }
-        }
-
-        /* We will manually add the backing_hd field to the bs later */
-        states->new_bs = bdrv_new("");
-        ret = bdrv_open(states->new_bs, new_image_file,
-                        flags | BDRV_O_NO_BACKING, drv);
-        if (ret != 0) {
-            error_set(errp, QERR_OPEN_FILE_FAILED, new_image_file);
-            goto delete_and_fail;
-        }
-    }
-
-
-    /* Now we are going to do the actual pivot.  Everything up to this point
-     * is reversible, but we are committed at this point */
-    QSIMPLEQ_FOREACH(states, &snap_bdrv_states, entry) {
-        /* This removes our old bs from the bdrv_states, and adds the new bs */
-        bdrv_append(states->new_bs, states->old_bs);
-        /* We don't need (or want) to use the transactional
-         * bdrv_reopen_multiple() across all the entries at once, because we
-         * don't want to abort all of them if one of them fails the reopen */
-        bdrv_reopen(states->new_bs, states->new_bs->open_flags & ~BDRV_O_RDWR,
-                    NULL);
-    }
-
-    /* success */
-    goto exit;
-
-delete_and_fail:
-    /*
-    * failure, and it is all-or-none; abandon each new bs, and keep using
-    * the original bs for all images
-    */
-    QSIMPLEQ_FOREACH(states, &snap_bdrv_states, entry) {
-        if (states->new_bs) {
-             bdrv_delete(states->new_bs);
-        }
-    }
-exit:
-    QSIMPLEQ_FOREACH_SAFE(states, &snap_bdrv_states, entry, next) {
-        g_free(states);
-    }
+    BlockdevSnapshotDelete snapshot_delete = {
+        .device = (char *) device,
+        .snapshot_file = (char *) snapshot_file,
+        .has_type = has_type,
+        .type = type,
+    };
+    blockdev_do_action(BLOCKDEV_ACTION_KIND_BLOCKDEV_SNAPSHOT_DELETE_SYNC,
+                       &snapshot_delete, errp);
 }
 
-
 static void eject_device(BlockDriverState *bs, int force, Error **errp)
 {
     if (bdrv_in_use(bs)) {
diff --git a/qapi-schema.json b/qapi-schema.json
index 5dfa052..46f4c6b 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1458,17 +1458,36 @@
 { 'command': 'block_resize', 'data': { 'device': 'str', 'size': 'int' }}
 
 ##
+# @SnapshotType
+#
+# An enumeration that tells QEMU what type of snapshot to access.
+#
+# @internal: QEMU should use internal snapshot in format such as qcow2.
+#
+# @external: QEMU should use backing file chain.
+#
+# Since: 1.4.
+##
+{ 'enum': 'SnapshotType'
+  'data': [ 'internal', 'external' ] }
+
+##
 # @NewImageMode
 #
 # An enumeration that tells QEMU how to set the backing file path in
-# a new image file.
+# a new image file, or how to use internal snapshot record.
 #
-# @existing: QEMU should look for an existing image file.
+# @existing: QEMU should look for an existing image file or internal snapshot
+#            record. In external snapshot case, qemu will skip create new image
+#            file, In internal snapshot case qemu will try use the existing
+#            one. if not found operation would fail.
 #
-# @absolute-paths: QEMU should create a new image with absolute paths
-# for the backing file.
+# @absolute-paths: QEMU should create a new image with absolute paths for
+#                  the backing file in external snapshot case, or create a new
+#                  snapshot record in internal snapshot case which will
+#                  overwrite internal snapshot record if it already exist.
 #
-# Since: 1.1
+# Since: 1.1, internal support since 1.4.
 ##
 { 'enum': 'NewImageMode'
   'data': [ 'existing', 'absolute-paths' ] }
@@ -1478,16 +1497,39 @@
 #
 # @device:  the name of the device to generate the snapshot from.
 #
-# @snapshot-file: the target of the new image. A new file will be created.
+# @snapshot-file: the target name of the snapshot. In external case, it is
+#                 the new file's name, A new file will be created. In internal
+#                 case, it is the internal snapshot record's name and if it is
+#                 'blank' name will be generated according to time.
 #
 # @format: #optional the format of the snapshot image, default is 'qcow2'.
 #
-# @mode: #optional whether and how QEMU should create a new image, default is
-#        'absolute-paths'.
+# @mode: #optional whether QEMU should create a new snapshot or use existing
+#        one, default is 'absolute-paths'.
+#
+# @type: #optional internal snapshot or external, default is 'external'.
+#
 ##
 { 'type': 'BlockdevSnapshot',
   'data': { 'device': 'str', 'snapshot-file': 'str', '*format': 'str',
-            '*mode': 'NewImageMode' } }
+            '*mode': 'NewImageMode', '*type': 'SnapshotType'} }
+
+##
+# @BlockdevSnapshotDelete
+#
+# @device:  the name of the device to delete the snapshot from.
+#
+# @snapshot-file: the target name of the snapshot. In external case, it is
+#                 the file's name to be merged, In internal case, it is the
+#                 internal snapshot record's name.
+#
+# @type: #optional internal snapshot or external, default is
+#        'external', note that delete 'external' snapshot is not supported
+#        now for that it is the same to commit it.
+##
+{ 'type': 'BlockdevSnapshotDelete',
+  'data': { 'device': 'str', 'snapshot-file': 'str',
+            '*type': 'SnapshotType'} }
 
 ##
 # @BlockdevAction
@@ -1498,6 +1540,7 @@
 { 'union': 'BlockdevAction',
   'data': {
        'blockdev-snapshot-sync': 'BlockdevSnapshot',
+       'blockdev-snapshot-delete-sync': 'BlockdevSnapshotDelete',
    } }
 
 ##
@@ -1530,23 +1573,54 @@
 #
 # @device:  the name of the device to generate the snapshot from.
 #
-# @snapshot-file: the target of the new image. If the file exists, or if it
-#                 is a device, the snapshot will be created in the existing
-#                 file/device. If does not exist, a new file will be created.
+# @snapshot-file: the target name of the snapshot. In external case, it is
+#                 the new file's name, A new file will be created. If the file
+#                 exists, or if it is a device, the snapshot will be created in
+#                 the existing file/device. If does not exist, a new file will
+#                 be created. In internal case, it is the internal snapshot
+#                 record's name, if it is 'blank' name will be generated
+#                 according to time.
 #
 # @format: #optional the format of the snapshot image, default is 'qcow2'.
 #
 # @mode: #optional whether and how QEMU should create a new image, default is
 #        'absolute-paths'.
 #
+# @type: #optional internal snapshot or external, default is
+#        'external'.
+#
 # Returns: nothing on success
 #          If @device is not a valid block device, DeviceNotFound
 #
-# Since 0.14.0
+# Since 0.14.0, internal snapshot supprt since 1.4.
 ##
 { 'command': 'blockdev-snapshot-sync',
   'data': { 'device': 'str', 'snapshot-file': 'str', '*format': 'str',
-            '*mode': 'NewImageMode'} }
+            '*mode': 'NewImageMode', '*type': 'SnapshotType'} }
+
+##
+# @blockdev-snapshot-delete-sync
+#
+# Delete a synchronous snapshot of a block device.
+#
+# @device:  the name of the device to delete the snapshot from.
+#
+# @snapshot-file: the target name of the snapshot. In external case, it is
+#                 the file's name to be merged, In internal case, it is the
+#                 internal snapshot record's name.
+#
+# @type: #optional internal snapshot or external, default is
+#        'external', note that delete 'external' snapshot is not supported
+#        now for that it is the same to commit it.
+#
+# Returns: nothing on success
+#          If @device is not a valid block device, DeviceNotFound
+#
+# Since 1.4
+##
+{ 'command': 'blockdev-snapshot-delete-sync',
+  'data': { 'device': 'str', 'snapshot-file': 'str',
+            '*type': 'SnapshotType'} }
 
 ##
 # @human-monitor-command:
-- 
1.7.1

Re: [Qemu-devel] [PATCH 5/6] snapshot: qmp interface

[Eric Blake]

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

On 12/16/2012 11:25 PM, Wenchao Xia wrote:
>   This patch changes the implemtion of external block snapshot

s/implemtion/implementation/

> to use internal unified interface, now qmp handler just do

s/do/does/

> a translation of request and submit.
>   Also internal block snapshot qmp interface was added.
>   Now add external snapshot, add/delete internal snapshot
> can be started in their own qmp interface or a group of
> BlockAction in qmp transaction interface.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---

> +++ b/qapi-schema.json

I didn't look at the code, because I want to make sure we get the
interface right, first.

> @@ -1458,17 +1458,36 @@
>  { 'command': 'block_resize', 'data': { 'device': 'str', 'size': 'int' }}
>  
>  ##
> +# @SnapshotType
> +#
> +# An enumeration that tells QEMU what type of snapshot to access.
> +#
> +# @internal: QEMU should use internal snapshot in format such as qcow2.
> +#
> +# @external: QEMU should use backing file chain.
> +#
> +# Since: 1.4.
> +##
> +{ 'enum': 'SnapshotType'
> +  'data': [ 'internal', 'external' ] }
> +
> +##
>  # @NewImageMode
>  #
>  # An enumeration that tells QEMU how to set the backing file path in
> -# a new image file.
> +# a new image file, or how to use internal snapshot record.
>  #
> -# @existing: QEMU should look for an existing image file.
> +# @existing: QEMU should look for an existing image file or internal snapshot
> +#            record. In external snapshot case, qemu will skip create new image
> +#            file, In internal snapshot case qemu will try use the existing

s/In/in/

> +#            one. if not found operation would fail.

s/. if/. If/; s/would/will/

>  #
> -# @absolute-paths: QEMU should create a new image with absolute paths
> -# for the backing file.
> +# @absolute-paths: QEMU should create a new image with absolute paths for
> +#                  the backing file in external snapshot case, or create a new
> +#                  snapshot record in internal snapshot case which will
> +#                  overwrite internal snapshot record if it already exist.

Doesn't quite make sense - internal snapshots don't record a path, so
why is absolute-paths the right mode for requesting the creation of a
new snapshot?   I think it would make more sense if you add a new mode,
and then declare that absolute-paths is invalid for internal snapshots,
and that the new mode is invalid for external snapshots.

>  #
> -# Since: 1.1
> +# Since: 1.1, internal support since 1.4.
>  ##
>  { 'enum': 'NewImageMode'
>    'data': [ 'existing', 'absolute-paths' ] }
> @@ -1478,16 +1497,39 @@
>  #
>  # @device:  the name of the device to generate the snapshot from.
>  #
> -# @snapshot-file: the target of the new image. A new file will be created.
> +# @snapshot-file: the target name of the snapshot. In external case, it is
> +#                 the new file's name, A new file will be created. In internal

s/A/a/

and a new file is only created according to mode.

> +#                 case, it is the internal snapshot record's name and if it is
> +#                 'blank' name will be generated according to time.

Ugg.  Passing an empty string for snapshot-file as a special case seems
awkward; it might be better to make it an optional argument via
'*snapshot-file', where the argument is mandatory for external, but
omitting the argument on internal allows the fallback naming.  Or why do
you even need to worry about fallback naming?  Requiring the user to
always provide a record name may be easier to support (certainly fewer
corner cases to worry about).

>  #
>  # @format: #optional the format of the snapshot image, default is 'qcow2'.
>  #
> -# @mode: #optional whether and how QEMU should create a new image, default is
> -#        'absolute-paths'.
> +# @mode: #optional whether QEMU should create a new snapshot or use existing
> +#        one, default is 'absolute-paths'.

Does this default still make sense for internal snapshots, or do you
need to document that the default mode differs depending on the type of
snapshot being taken?

> +#
> +# @type: #optional internal snapshot or external, default is 'external'.

Mention that this field is new since 1.4.

> +#
>  ##
>  { 'type': 'BlockdevSnapshot',
>    'data': { 'device': 'str', 'snapshot-file': 'str', '*format': 'str',
> -            '*mode': 'NewImageMode' } }
> +            '*mode': 'NewImageMode', '*type': 'SnapshotType'} }
> +
> +##
> +# @BlockdevSnapshotDelete
> +#
> +# @device:  the name of the device to delete the snapshot from.
> +#
> +# @snapshot-file: the target name of the snapshot. In external case, it is
> +#                 the file's name to be merged, In internal case, it is the
> +#                 internal snapshot record's name.

What happens if there is no record name (since the qcow2 file does not
require one)?

> +#
> +# @type: #optional internal snapshot or external, default is
> +#        'external', note that delete 'external' snapshot is not supported
> +#        now for that it is the same to commit it.

If external is not supported, then why do you even need this parameter?
 Besides, shouldn't it be pretty obvious from the snapshot-file argument
whether the snapshot exists internally or externally, without needing
the user to tell you that?

> +##
> +{ 'type': 'BlockdevSnapshotDelete',
> +  'data': { 'device': 'str', 'snapshot-file': 'str',
> +            '*type': 'SnapshotType'} }
>  
>  ##
>  # @BlockdevAction
> @@ -1498,6 +1540,7 @@
>  { 'union': 'BlockdevAction',
>    'data': {
>         'blockdev-snapshot-sync': 'BlockdevSnapshot',
> +       'blockdev-snapshot-delete-sync': 'BlockdevSnapshotDelete',
>     } }
>  
>  ##
> @@ -1530,23 +1573,54 @@
>  #
>  # @device:  the name of the device to generate the snapshot from.
>  #
> -# @snapshot-file: the target of the new image. If the file exists, or if it
> -#                 is a device, the snapshot will be created in the existing
> -#                 file/device. If does not exist, a new file will be created.
> +# @snapshot-file: the target name of the snapshot. In external case, it is
> +#                 the new file's name, A new file will be created. If the file
> +#                 exists, or if it is a device, the snapshot will be created in
> +#                 the existing file/device. If does not exist, a new file will
> +#                 be created. In internal case, it is the internal snapshot
> +#                 record's name, if it is 'blank' name will be generated
> +#                 according to time.

Again, special casing the empty string seems awkward; making the field
optional for the internal case might make more sense.

>  #
>  # @format: #optional the format of the snapshot image, default is 'qcow2'.

Is this field compatible with internal snapshots?

>  #
>  # @mode: #optional whether and how QEMU should create a new image, default is
>  #        'absolute-paths'.
>  #
> +# @type: #optional internal snapshot or external, default is
> +#        'external'.

Mention that this field was added in 1.4.

> +#
>  # Returns: nothing on success
>  #          If @device is not a valid block device, DeviceNotFound
>  #
> -# Since 0.14.0
> +# Since 0.14.0, internal snapshot supprt since 1.4.
>  ##
>  { 'command': 'blockdev-snapshot-sync',
>    'data': { 'device': 'str', 'snapshot-file': 'str', '*format': 'str',
> -            '*mode': 'NewImageMode'} }
> +            '*mode': 'NewImageMode', '*type': 'SnapshotType'} }
> +
> +##
> +# @blockdev-snapshot-delete-sync
> +#
> +# Delete a synchronous snapshot of a block device.

Wrong.  Rather, this is a synchronous operation that deletes a snapshot,
regardless of whether the snapshot was created synchronously or
asynchronously.

Remember, the original goal was to come up with a way to take snapshots
in a way that didn't require blocking until the operation was done; and
once such an interface is present, then that becomes a new operation
blockdev-snapshot-async (or more than one command, in order to drive the
sequence of operations needed).  blockdev-snapshot-sync would then be
rewritten as a wrapper that calls the underlying sequence of operations
in one command, blocking until complete.

But for deletion, we might as well do it right from the beginning.  I
wonder if you can even design things to just have
'blockdev-snapshot-delete' without needing to distinguish between a sync
or async operation.

> +#
> +# @device:  the name of the device to delete the snapshot from.
> +#
> +# @snapshot-file: the target name of the snapshot. In external case, it is
> +#                 the file's name to be merged, In internal case, it is the
> +#                 internal snapshot record's name.
> +#
> +# @type: #optional internal snapshot or external, default is
> +#        'external', note that delete 'external' snapshot is not supported
> +#        now for that it is the same to commit it.

Again, do you really need this field, or can it be inferred from
snapshot-file?

> +#
> +# Returns: nothing on success
> +#          If @device is not a valid block device, DeviceNotFound
> +#
> +# Since 1.4
> +##
> +{ 'command': 'blockdev-snapshot-delete-sync',
> +  'data': { 'device': 'str', 'snapshot-file': 'str',
> +            '*type': 'SnapshotType'} }
>  
>  ##
>  # @human-monitor-command:
> 

-- 
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: 619 bytes --]

Re: [Qemu-devel] [PATCH 5/6] snapshot: qmp interface

[Wenchao Xia]

   Thanks for reviewing, I'll correct the spelling.

>> +
>> +##
>>   # @NewImageMode
>>   #
>>   # An enumeration that tells QEMU how to set the backing file path in
>> -# a new image file.
>> +# a new image file, or how to use internal snapshot record.
>>   #
>> -# @existing: QEMU should look for an existing image file.
>> +# @existing: QEMU should look for an existing image file or internal snapshot
>> +#            record. In external snapshot case, qemu will skip create new image
>> +#            file, In internal snapshot case qemu will try use the existing
>
> s/In/in/
>
>> +#            one. if not found operation would fail.
>
> s/. if/. If/; s/would/will/
>
>>   #
>> -# @absolute-paths: QEMU should create a new image with absolute paths
>> -# for the backing file.
>> +# @absolute-paths: QEMU should create a new image with absolute paths for
>> +#                  the backing file in external snapshot case, or create a new
>> +#                  snapshot record in internal snapshot case which will
>> +#                  overwrite internal snapshot record if it already exist.
>
> Doesn't quite make sense - internal snapshots don't record a path, so
> why is absolute-paths the right mode for requesting the creation of a
> new snapshot?   I think it would make more sense if you add a new mode,
> and then declare that absolute-paths is invalid for internal snapshots,
> and that the new mode is invalid for external snapshots.
>
   OK,

>>   #
>> -# Since: 1.1
>> +# Since: 1.1, internal support since 1.4.
>>   ##
>>   { 'enum': 'NewImageMode'
>>     'data': [ 'existing', 'absolute-paths' ] }
>> @@ -1478,16 +1497,39 @@
>>   #
>>   # @device:  the name of the device to generate the snapshot from.
>>   #
>> -# @snapshot-file: the target of the new image. A new file will be created.
>> +# @snapshot-file: the target name of the snapshot. In external case, it is
>> +#                 the new file's name, A new file will be created. In internal
>
> s/A/a/
>
> and a new file is only created according to mode.
>
   OK.

>> +#                 case, it is the internal snapshot record's name and if it is
>> +#                 'blank' name will be generated according to time.
>
> Ugg.  Passing an empty string for snapshot-file as a special case seems
> awkward; it might be better to make it an optional argument via
> '*snapshot-file', where the argument is mandatory for external, but
   I think *snapshot-file is great, but it will change the interface
which already exist triggering compatialbility issue, is this allowed?

> omitting the argument on internal allows the fallback naming.  Or why do
> you even need to worry about fallback naming?  Requiring the user to
> always provide a record name may be easier to support (certainly fewer
> corner cases to worry about).
>
   If cost is low, I think providing it is not bad, and thus hmp/qmp
can have same capability.

>>   #
>>   # @format: #optional the format of the snapshot image, default is 'qcow2'.
>>   #
>> -# @mode: #optional whether and how QEMU should create a new image, default is
>> -#        'absolute-paths'.
>> +# @mode: #optional whether QEMU should create a new snapshot or use existing
>> +#        one, default is 'absolute-paths'.
>
> Does this default still make sense for internal snapshots, or do you
> need to document that the default mode differs depending on the type of
> snapshot being taken?
>
   I'll add another optional argument and keeps mode only for external
snapshot.

>> +#
>> +# @type: #optional internal snapshot or external, default is 'external'.
>
> Mention that this field is new since 1.4.
>
   OK.

>> +#
>>   ##
>>   { 'type': 'BlockdevSnapshot',
>>     'data': { 'device': 'str', 'snapshot-file': 'str', '*format': 'str',
>> -            '*mode': 'NewImageMode' } }
>> +            '*mode': 'NewImageMode', '*type': 'SnapshotType'} }
>> +
>> +##
>> +# @BlockdevSnapshotDelete
>> +#
>> +# @device:  the name of the device to delete the snapshot from.
>> +#
>> +# @snapshot-file: the target name of the snapshot. In external case, it is
>> +#                 the file's name to be merged, In internal case, it is the
>> +#                 internal snapshot record's name.
>
> What happens if there is no record name (since the qcow2 file does not
> require one)?
   I think snapshot id should be provided then.

>> +#
>> +# @type: #optional internal snapshot or external, default is
>> +#        'external', note that delete 'external' snapshot is not supported
>> +#        now for that it is the same to commit it.
>
> If external is not supported, then why do you even need this parameter?
>   Besides, shouldn't it be pretty obvious from the snapshot-file argument
> whether the snapshot exists internally or externally, without needing
> the user to tell you that?
>
   I thought it is not easy to tell the difference just from file, could
u tip how?

>> +##
>> +{ 'type': 'BlockdevSnapshotDelete',
>> +  'data': { 'device': 'str', 'snapshot-file': 'str',
>> +            '*type': 'SnapshotType'} }
>>
>>   ##
>>   # @BlockdevAction
>> @@ -1498,6 +1540,7 @@
>>   { 'union': 'BlockdevAction',
>>     'data': {
>>          'blockdev-snapshot-sync': 'BlockdevSnapshot',
>> +       'blockdev-snapshot-delete-sync': 'BlockdevSnapshotDelete',
>>      } }
>>
>>   ##
>> @@ -1530,23 +1573,54 @@
>>   #
>>   # @device:  the name of the device to generate the snapshot from.
>>   #
>> -# @snapshot-file: the target of the new image. If the file exists, or if it
>> -#                 is a device, the snapshot will be created in the existing
>> -#                 file/device. If does not exist, a new file will be created.
>> +# @snapshot-file: the target name of the snapshot. In external case, it is
>> +#                 the new file's name, A new file will be created. If the file
>> +#                 exists, or if it is a device, the snapshot will be created in
>> +#                 the existing file/device. If does not exist, a new file will
>> +#                 be created. In internal case, it is the internal snapshot
>> +#                 record's name, if it is 'blank' name will be generated
>> +#                 according to time.
>
> Again, special casing the empty string seems awkward; making the field
> optional for the internal case might make more sense.
>
   OK, if API compatible issue is acceptable.


>>   #
>>   # @format: #optional the format of the snapshot image, default is 'qcow2'.
>
> Is this field compatible with internal snapshots?
>
   no, I'll doc it.

>>   #
>>   # @mode: #optional whether and how QEMU should create a new image, default is
>>   #        'absolute-paths'.
>>   #
>> +# @type: #optional internal snapshot or external, default is
>> +#        'external'.
>
> Mention that this field was added in 1.4.
>
   OK.

>> +#
>>   # Returns: nothing on success
>>   #          If @device is not a valid block device, DeviceNotFound
>>   #
>> -# Since 0.14.0
>> +# Since 0.14.0, internal snapshot supprt since 1.4.
>>   ##
>>   { 'command': 'blockdev-snapshot-sync',
>>     'data': { 'device': 'str', 'snapshot-file': 'str', '*format': 'str',
>> -            '*mode': 'NewImageMode'} }
>> +            '*mode': 'NewImageMode', '*type': 'SnapshotType'} }
>> +
>> +##
>> +# @blockdev-snapshot-delete-sync
>> +#
>> +# Delete a synchronous snapshot of a block device.
>
> Wrong.  Rather, this is a synchronous operation that deletes a snapshot,
> regardless of whether the snapshot was created synchronously or
> asynchronously.
>
   OK, the doc will be changed to tip better.

> Remember, the original goal was to come up with a way to take snapshots
> in a way that didn't require blocking until the operation was done; and
> once such an interface is present, then that becomes a new operation
> blockdev-snapshot-async (or more than one command, in order to drive the
> sequence of operations needed).  blockdev-snapshot-sync would then be
> rewritten as a wrapper that calls the underlying sequence of operations
> in one command, blocking until complete.
>
> But for deletion, we might as well do it right from the beginning.  I
> wonder if you can even design things to just have
> 'blockdev-snapshot-delete' without needing to distinguish between a sync
> or async operation.
>
   There is already API as blockdev-snapshot-sync, this will break
the mirror, do you think it is OK? Actually I think it is OK
to redesign API including old one as blockdev-snapshot,
  blockdev-snapshot-delete, but again it brings compatible issue.
Or another solution which keep API unchanged as:
blockdev-snapshot-sync
blockdev-snapshot-delete-sync
blockdev-snapshot-async
blockdev-snapshot-delete-async

>> +#
>> +# @device:  the name of the device to delete the snapshot from.
>> +#
>> +# @snapshot-file: the target name of the snapshot. In external case, it is
>> +#                 the file's name to be merged, In internal case, it is the
>> +#                 internal snapshot record's name.
>> +#
>> +# @type: #optional internal snapshot or external, default is
>> +#        'external', note that delete 'external' snapshot is not supported
>> +#        now for that it is the same to commit it.
>
> Again, do you really need this field, or can it be inferred from
> snapshot-file?
>
>> +#
>> +# Returns: nothing on success
>> +#          If @device is not a valid block device, DeviceNotFound
>> +#
>> +# Since 1.4
>> +##
>> +{ 'command': 'blockdev-snapshot-delete-sync',
>> +  'data': { 'device': 'str', 'snapshot-file': 'str',
>> +            '*type': 'SnapshotType'} }
>>
>>   ##
>>   # @human-monitor-command:
>>
>


-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH 5/6] snapshot: qmp interface

[Eric Blake]

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

On 01/03/2013 11:02 PM, Wenchao Xia wrote:

> 
>>> +#                 case, it is the internal snapshot record's name
>>> and if it is
>>> +#                 'blank' name will be generated according to time.
>>
>> Ugg.  Passing an empty string for snapshot-file as a special case seems
>> awkward; it might be better to make it an optional argument via
>> '*snapshot-file', where the argument is mandatory for external, but
>   I think *snapshot-file is great, but it will change the interface
> which already exist triggering compatialbility issue, is this allowed?

Making a formerly-required argument optional is NOT an API break - all
old callers will already be providing the argument, and only new callers
will take advantage of the ability to omit the argument.  So yes, this
change is allowed.  Furthermore, if you properly issue an error in the
cases where a name is required (external snapshots), then marking the
argument optional only affects the cases where it makes sense (internal
snapshots).

>> omitting the argument on internal allows the fallback naming.  Or why do
>> you even need to worry about fallback naming?  Requiring the user to
>> always provide a record name may be easier to support (certainly fewer
>> corner cases to worry about).
>>
>   If cost is low, I think providing it is not bad, and thus hmp/qmp
> can have same capability.

Do we want to support omitting the name in qcow2?  If so, then you have
to provide that capability.  If not, then HMP can still provide the
capability, by generating a name before calling into QMP, while still
making QMP simpler by always requiring a name.  I don't care either way
- when libvirt generates a snapshot, it will always provide a name (as
nameless snapshots are harder to deal with).  Note, however, that even
if we make creation always require a name, we still have to worry about
deletion being able to use a nameless snapshot, thanks to pre-existing
qcow2 files that had no name.  And since there are pre-existing files
with no name, you could argue that taking away the ability to create a
nameless snapshot would be a regression, so maybe you have to support
creation of a nameless snapshot via QMP after all.

>>   Besides, shouldn't it be pretty obvious from the snapshot-file argument
>> whether the snapshot exists internally or externally, without needing
>> the user to tell you that?
>>
>   I thought it is not easy to tell the difference just from file, could
> u tip how?

If a snapshot name is given but no indication of whether it is external
or internal, then the obvious implementation is to iterate through all
internal names, and if any of them match, then it was internal;
otherwise assume it is external.  If it is possible to create both an
internal and an external snapshot using the same name (that is, if
internal names can look like valid file names), then the user would have
to be explicit which one they want; or you could check if an external
file exists with the same name as an internal and error out that a name
is ambiguous if they user was not explicit in that case.


>>> +##
>>> +# @blockdev-snapshot-delete-sync
>>> +#
>>> +# Delete a synchronous snapshot of a block device.
>>
>> Wrong.  Rather, this is a synchronous operation that deletes a snapshot,
>> regardless of whether the snapshot was created synchronously or
>> asynchronously.
>>
>   OK, the doc will be changed to tip better.
> 
>> Remember, the original goal was to come up with a way to take snapshots
>> in a way that didn't require blocking until the operation was done; and
>> once such an interface is present, then that becomes a new operation
>> blockdev-snapshot-async (or more than one command, in order to drive the
>> sequence of operations needed).  blockdev-snapshot-sync would then be
>> rewritten as a wrapper that calls the underlying sequence of operations
>> in one command, blocking until complete.
>>
>> But for deletion, we might as well do it right from the beginning.  I
>> wonder if you can even design things to just have
>> 'blockdev-snapshot-delete' without needing to distinguish between a sync
>> or async operation.
>>
>   There is already API as blockdev-snapshot-sync, this will break
> the mirror, do you think it is OK? Actually I think it is OK
> to redesign API including old one as blockdev-snapshot,

Asymmetric naming is not a problem.  The names should be descriptive of
what they do.  Right now, we only have one command, which makes a
blockdev snapshot, and does so synchronously (blocks for an indefinite
amount of time).  We've left the door open for a new command (or more
likely, a sequence of new commands, one to start, one to abort, and one
to check progress, as well as an event when things are complete) for
doing a snapshot without an arbitrary blocking downtime.  But whether a
snapshot was created synchronously or asynchronously is not recorded in
the snapshot itself, so if deletion can always be done without blocking,
then we don't need two different styles of delete, and a -sync or -async
suffix in the delete command doesn't make any difference.

>  blockdev-snapshot-delete, but again it brings compatible issue.
> Or another solution which keep API unchanged as:
> blockdev-snapshot-sync
> blockdev-snapshot-delete-sync
> blockdev-snapshot-async
> blockdev-snapshot-delete-async

Symmetry in the naming is not important.  Does deleting a snapshot
require an arbitrary length of time?  If so, we should plan for how to
make it async, and track it's progress or abort it early.  If not, then
a single command that always completes fast and does the deletion is
sufficient, and we can name the command just blockdev-snapshot-delete.

I think it would help if you take a step back and give a higher-level
overview of how your commands will be used - what sequence of QMP
commands does a monitor app issue to do various snapshot actions?  Once
that sequence is determined, we can come up with names to fit.

-- 
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: 619 bytes --]

Re: [Qemu-devel] [PATCH 5/6] snapshot: qmp interface

[Stefan Hajnoczi]

On Mon, Dec 17, 2012 at 02:25:08PM +0800, Wenchao Xia wrote:
> @@ -1478,16 +1497,39 @@
>  #
>  # @device:  the name of the device to generate the snapshot from.
>  #
> -# @snapshot-file: the target of the new image. A new file will be created.
> +# @snapshot-file: the target name of the snapshot. In external case, it is
> +#                 the new file's name, A new file will be created. In internal
> +#                 case, it is the internal snapshot record's name and if it is
> +#                 'blank' name will be generated according to time.
>  #
>  # @format: #optional the format of the snapshot image, default is 'qcow2'.
>  #
> -# @mode: #optional whether and how QEMU should create a new image, default is
> -#        'absolute-paths'.
> +# @mode: #optional whether QEMU should create a new snapshot or use existing
> +#        one, default is 'absolute-paths'.
> +#
> +# @type: #optional internal snapshot or external, default is 'external'.
> +#
>  ##
>  { 'type': 'BlockdevSnapshot',
>    'data': { 'device': 'str', 'snapshot-file': 'str', '*format': 'str',
> -            '*mode': 'NewImageMode' } }
> +            '*mode': 'NewImageMode', '*type': 'SnapshotType'} }

It feels cleaner to define a new BlockdevInternalSnapshot type instead
of overloading the field semantics.  The naming doesn't fit for internal
snapshots (e.g. "snapshot-file", "absolute-paths", etc).

Treat internal snapshots as a new transaction action.  That way we can
cleanly add internal snapshot specific fields in the future.  The
documentation will be clearer because users won't have to consider the
"if type is 'external', then ..., otherwise ...".

Stefan

Re: [Qemu-devel] [PATCH 5/6] snapshot: qmp interface

[Wenchao Xia]

于 2013-1-5 0:22, Stefan Hajnoczi 写道:
> On Mon, Dec 17, 2012 at 02:25:08PM +0800, Wenchao Xia wrote:
>> @@ -1478,16 +1497,39 @@
>>   #
>>   # @device:  the name of the device to generate the snapshot from.
>>   #
>> -# @snapshot-file: the target of the new image. A new file will be created.
>> +# @snapshot-file: the target name of the snapshot. In external case, it is
>> +#                 the new file's name, A new file will be created. In internal
>> +#                 case, it is the internal snapshot record's name and if it is
>> +#                 'blank' name will be generated according to time.
>>   #
>>   # @format: #optional the format of the snapshot image, default is 'qcow2'.
>>   #
>> -# @mode: #optional whether and how QEMU should create a new image, default is
>> -#        'absolute-paths'.
>> +# @mode: #optional whether QEMU should create a new snapshot or use existing
>> +#        one, default is 'absolute-paths'.
>> +#
>> +# @type: #optional internal snapshot or external, default is 'external'.
>> +#
>>   ##
>>   { 'type': 'BlockdevSnapshot',
>>     'data': { 'device': 'str', 'snapshot-file': 'str', '*format': 'str',
>> -            '*mode': 'NewImageMode' } }
>> +            '*mode': 'NewImageMode', '*type': 'SnapshotType'} }
>
> It feels cleaner to define a new BlockdevInternalSnapshot type instead
> of overloading the field semantics.  The naming doesn't fit for internal
> snapshots (e.g. "snapshot-file", "absolute-paths", etc).
>
> Treat internal snapshots as a new transaction action.  That way we can
> cleanly add internal snapshot specific fields in the future.  The
> documentation will be clearer because users won't have to consider the
> "if type is 'external', then ..., otherwise ...".
>
> Stefan
>
   After struggling about the parameter meaning, I guess this is what
make things better.



-- 
Best Regards

Wenchao Xia

[Qemu-devel] [PATCH 6/6] snapshot: human monitor interface

[Wenchao Xia]

  This patch add support in human monitor to create/delete/check
internal snapshot on a single blk device.
  To make info command get parameter, added a new info handler
type which can take QDict as parameter.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 hmp-commands.hx |   50 +++++++++++++++++++++++++++++++++++++-------------
 hmp.c           |   30 +++++++++++++++++++++++++-----
 hmp.h           |    1 +
 monitor.c       |   21 +++++++++++++++------
 savevm.c        |   55 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 sysemu.h        |    2 +-
 6 files changed, 133 insertions(+), 26 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 010b8c9..ee74723 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -983,17 +983,22 @@ ETEXI
 
     {
         .name       = "snapshot_blkdev",
-        .args_type  = "reuse:-n,device:B,snapshot-file:s?,format:s?",
-        .params     = "[-n] device [new-image-file] [format]",
-        .help       = "initiates a live snapshot\n\t\t\t"
-                      "of device. If a new image file is specified, the\n\t\t\t"
-                      "new image file will become the new root image.\n\t\t\t"
-                      "If format is specified, the snapshot file will\n\t\t\t"
-                      "be created in that format. Otherwise the\n\t\t\t"
-                      "snapshot will be internal! (currently unsupported).\n\t\t\t"
-                      "The default format is qcow2.  The -n flag requests QEMU\n\t\t\t"
-                      "to reuse the image found in new-image-file, instead of\n\t\t\t"
-                      "recreating it from scratch.",
+        .args_type  = "internal:-i,reuse:-n,device:B,snapshot-file:s?,format:s?",
+        .params     = "[-i] [-n] device [new-image-file] [format]",
+        .help       = "initiates a live snapshot of device.\n\t\t\t"
+                      "  The -i flag requests QEMU to create internal snapshot\n\t\t\t"
+                      "instead of external one.\n\t\t\t"
+                      "  The -n flag requests QEMU to use existing snapshot\n\t\t\t"
+                      "instead of creating new snapshot, which would fails if\n\t\t\t"
+                      "snapshot does not exist ahead.\n\t\t\t"
+                      "  new-image-file is the snapshot's name, in external case\n\t\t\t"
+                      "it is the new image's name which will become the new root\n\t\t\t"
+                      "image and must be specified, in internal case it is the\n\t\t\t"
+                      "record's name and if not specified QEMU will create\n\t\t\t"
+                      "internal snapshot with name generated according to time.\n\t\t\t"
+                      "  format is only valid in external case, which is the new\n\t\t\t"
+                      "snapshot image's format. If not sepcified default format\n\t\t\t"
+                      "qcow2 will be used.",
         .mhandler.cmd = hmp_snapshot_blkdev,
     },
 
@@ -1004,6 +1009,25 @@ Snapshot device, using snapshot file as target if provided
 ETEXI
 
     {
+        .name       = "snapshot_delete_blkdev",
+        .args_type  = "internal:-i,device:B,snapshot-file:s",
+        .params     = "[-i] device new-image-file",
+        .help       = "delete a snapshot  synchronous.\n\t\t\t"
+                      "  The -i flag requests QEMU to delete internal snapshot\n\t\t\t"
+                      "instead of external one.\n\t\t\t"
+                      "  new-image-file is the snapshot's name, in external case\n\t\t\t"
+                      "it is the image's name which is not supported now.\n\t\t\t"
+                      "in internal case it is the snapshot record's id or name.",
+        .mhandler.cmd = hmp_snapshot_delete_blkdev,
+    },
+
+STEXI
+@item snapshot_delete_blkdev
+@findex snapshot_delete_blkdev
+Delete a snapshot on a block device.
+ETEXI
+
+    {
         .name       = "drive_mirror",
         .args_type  = "reuse:-n,full:-f,device:B,target:s,format:s?",
         .params     = "[-n] [-f] device target [format]",
@@ -1486,8 +1510,8 @@ ETEXI
 
     {
         .name       = "info",
-        .args_type  = "item:s?",
-        .params     = "[subcommand]",
+        .args_type  = "item:s?,params:s?",
+        .params     = "[subcommand] [params]",
         .help       = "show various information about the system state",
         .mhandler.cmd = do_info,
     },
diff --git a/hmp.c b/hmp.c
index 180ba2b..f247f51 100644
--- a/hmp.c
+++ b/hmp.c
@@ -806,20 +806,40 @@ void hmp_snapshot_blkdev(Monitor *mon, const QDict *qdict)
     const char *filename = qdict_get_try_str(qdict, "snapshot-file");
     const char *format = qdict_get_try_str(qdict, "format");
     int reuse = qdict_get_try_bool(qdict, "reuse", 0);
+    int internal = qdict_get_try_bool(qdict, "internal", 0);
     enum NewImageMode mode;
+    enum SnapshotType type;
     Error *errp = NULL;
 
-    if (!filename) {
-        /* In the future, if 'snapshot-file' is not specified, the snapshot
-           will be taken internally. Today it's actually required. */
+    if ((!internal) && (!filename)) {
+        /* in external case filename must be set, should we generate
+         it automatically? */
         error_set(&errp, QERR_MISSING_PARAMETER, "snapshot-file");
         hmp_handle_error(mon, &errp);
         return;
     }
-
+    type = internal ? SNAPSHOT_TYPE_INTERNAL : SNAPSHOT_TYPE_EXTERNAL;
     mode = reuse ? NEW_IMAGE_MODE_EXISTING : NEW_IMAGE_MODE_ABSOLUTE_PATHS;
     qmp_blockdev_snapshot_sync(device, filename, !!format, format,
-                               true, mode, &errp);
+                               true, mode, true, type, &errp);
+    hmp_handle_error(mon, &errp);
+}
+
+void hmp_snapshot_delete_blkdev(Monitor *mon, const QDict *qdict)
+{
+    const char *device = qdict_get_str(qdict, "device");
+    const char *filename = qdict_get_try_str(qdict, "snapshot-file");
+    int internal = qdict_get_try_bool(qdict, "internal", 0);
+    enum SnapshotType type;
+    Error *errp = NULL;
+
+    if (!internal) {
+        error_setg(&errp, "external snapshot delete not supported now.");
+        hmp_handle_error(mon, &errp);
+        return;
+    }
+    type = internal ? SNAPSHOT_TYPE_INTERNAL : SNAPSHOT_TYPE_EXTERNAL;
+    qmp_blockdev_snapshot_delete_sync(device, filename, true, type, &errp);
     hmp_handle_error(mon, &errp);
 }
 
diff --git a/hmp.h b/hmp.h
index 0ab03be..2ea67be 100644
--- a/hmp.h
+++ b/hmp.h
@@ -51,6 +51,7 @@ void hmp_block_passwd(Monitor *mon, const QDict *qdict);
 void hmp_balloon(Monitor *mon, const QDict *qdict);
 void hmp_block_resize(Monitor *mon, const QDict *qdict);
 void hmp_snapshot_blkdev(Monitor *mon, const QDict *qdict);
+void hmp_snapshot_delete_blkdev(Monitor *mon, const QDict *qdict);
 void hmp_drive_mirror(Monitor *mon, const QDict *qdict);
 void hmp_migrate_cancel(Monitor *mon, const QDict *qdict);
 void hmp_migrate_set_downtime(Monitor *mon, const QDict *qdict);
diff --git a/monitor.c b/monitor.c
index c0e32d6..81de470 100644
--- a/monitor.c
+++ b/monitor.c
@@ -124,11 +124,14 @@ typedef struct mon_cmd_t {
     void (*user_print)(Monitor *mon, const QObject *data);
     union {
         void (*info)(Monitor *mon);
+        void (*info_qdict)(Monitor *mon, const QDict *qdict);
         void (*cmd)(Monitor *mon, const QDict *qdict);
-        int  (*cmd_new)(Monitor *mon, const QDict *params, QObject **ret_data);
+        int  (*cmd_new)(Monitor *mon, const QDict *params,
+                        QObject **ret_data);
         int  (*cmd_async)(Monitor *mon, const QDict *params,
                           MonitorCompletion *cb, void *opaque);
     } mhandler;
+    int info_cmd_need_qdict;
     int flags;
 } mon_cmd_t;
 
@@ -824,7 +827,11 @@ static void do_info(Monitor *mon, const QDict *qdict)
         goto help;
     }
 
-    cmd->mhandler.info(mon);
+    if (cmd->info_cmd_need_qdict) {
+        cmd->mhandler.info_qdict(mon, qdict);
+    } else {
+        cmd->mhandler.info(mon);
+    }
     return;
 
 help:
@@ -2605,10 +2612,12 @@ static mon_cmd_t info_cmds[] = {
     },
     {
         .name       = "snapshots",
-        .args_type  = "",
-        .params     = "",
-        .help       = "show the currently saved VM snapshots",
-        .mhandler.info = do_info_snapshots,
+        .args_type  = "device:B?",
+        .params     = "[device]",
+        .help       = "show the currently saved VM snapshots or snapshots on "
+                      "a single device.",
+        .mhandler.info_qdict = do_info_snapshots,
+        .info_cmd_need_qdict = 1,
     },
     {
         .name       = "status",
diff --git a/savevm.c b/savevm.c
index c027529..5982aa9 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2336,7 +2336,7 @@ void do_delvm(Monitor *mon, const QDict *qdict)
     }
 }
 
-void do_info_snapshots(Monitor *mon)
+static void do_info_snapshots_vm(Monitor *mon)
 {
     BlockDriverState *bs, *bs1;
     QEMUSnapshotInfo *sn_tab, *sn, s, *sn_info = &s;
@@ -2400,6 +2400,59 @@ void do_info_snapshots(Monitor *mon)
 
 }
 
+static void do_info_snapshots_blk(Monitor *mon, const char *device)
+{
+    BlockDriverState *bs;
+    QEMUSnapshotInfo *sn_tab, *sn;
+    int nb_sns, i;
+    char buf[256];
+
+    /* find the target bs */
+    bs = bdrv_find(device);
+    if (!bs) {
+        monitor_printf(mon, "Device '%s' not found.\n", device);
+        return ;
+    }
+
+    if (!bdrv_can_snapshot(bs)) {
+        monitor_printf(mon, "Device '%s' can't have snapshot.\n", device);
+        return ;
+    }
+
+    nb_sns = bdrv_snapshot_list(bs, &sn_tab);
+    if (nb_sns < 0) {
+        monitor_printf(mon, "Device %s bdrv_snapshot_list: error %d\n",
+                       device, nb_sns);
+        return;
+    }
+
+    if (nb_sns == 0) {
+        monitor_printf(mon, "There is no snapshot available.\n");
+        return;
+    }
+
+    monitor_printf(mon, "Device %s:\n", device);
+    monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL));
+    for (i = 0; i < nb_sns; i++) {
+        sn = &sn_tab[i];
+        monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), sn));
+    }
+    g_free(sn_tab);
+    return;
+}
+
+void do_info_snapshots(Monitor *mon, const QDict *qdict)
+{
+    /* Todo, there should be a layer rebuild qdict before enter this func. */
+    const char *device = qdict_get_try_str(qdict, "params");
+    if (!device) {
+        do_info_snapshots_vm(mon);
+    } else {
+        do_info_snapshots_blk(mon, device);
+    }
+    return;
+}
+
 void vmstate_register_ram(MemoryRegion *mr, DeviceState *dev)
 {
     qemu_ram_set_idstr(memory_region_get_ram_addr(mr) & TARGET_PAGE_MASK,
diff --git a/sysemu.h b/sysemu.h
index 1b6add2..a1254bf 100644
--- a/sysemu.h
+++ b/sysemu.h
@@ -68,7 +68,7 @@ void qemu_add_machine_init_done_notifier(Notifier *notify);
 void do_savevm(Monitor *mon, const QDict *qdict);
 int load_vmstate(const char *name);
 void do_delvm(Monitor *mon, const QDict *qdict);
-void do_info_snapshots(Monitor *mon);
+void do_info_snapshots(Monitor *mon, const QDict *qdict);
 
 void qemu_announce_self(void);
 
-- 
1.7.1

Re: [Qemu-devel] [PATCH 6/6] snapshot: human monitor interface

[Stefan Hajnoczi]

On Mon, Dec 17, 2012 at 02:25:09PM +0800, Wenchao Xia wrote:
> @@ -983,17 +983,22 @@ ETEXI
>  
>      {
>          .name       = "snapshot_blkdev",
> -        .args_type  = "reuse:-n,device:B,snapshot-file:s?,format:s?",
> -        .params     = "[-n] device [new-image-file] [format]",
> -        .help       = "initiates a live snapshot\n\t\t\t"
> -                      "of device. If a new image file is specified, the\n\t\t\t"
> -                      "new image file will become the new root image.\n\t\t\t"
> -                      "If format is specified, the snapshot file will\n\t\t\t"
> -                      "be created in that format. Otherwise the\n\t\t\t"
> -                      "snapshot will be internal! (currently unsupported).\n\t\t\t"
> -                      "The default format is qcow2.  The -n flag requests QEMU\n\t\t\t"
> -                      "to reuse the image found in new-image-file, instead of\n\t\t\t"
> -                      "recreating it from scratch.",
> +        .args_type  = "internal:-i,reuse:-n,device:B,snapshot-file:s?,format:s?",
> +        .params     = "[-i] [-n] device [new-image-file] [format]",

Please rename snapshot-file and new-image-file because it is now either
the external snapshot filename or the internal snapshot name - it's not
always a file!

> +        .help       = "initiates a live snapshot of device.\n\t\t\t"
> +                      "  The -i flag requests QEMU to create internal snapshot\n\t\t\t"
> +                      "instead of external one.\n\t\t\t"
> +                      "  The -n flag requests QEMU to use existing snapshot\n\t\t\t"
> +                      "instead of creating new snapshot, which would fails if\n\t\t\t"
> +                      "snapshot does not exist ahead.\n\t\t\t"

"which fails if the snapshot does not exist already"

> +                      "  new-image-file is the snapshot's name, in external case\n\t\t\t"
> +                      "it is the new image's name which will become the new root\n\t\t\t"
> +                      "image and must be specified, in internal case it is the\n\t\t\t"
> +                      "record's name and if not specified QEMU will create\n\t\t\t"
> +                      "internal snapshot with name generated according to time.\n\t\t\t"
> +                      "  format is only valid in external case, which is the new\n\t\t\t"
> +                      "snapshot image's format. If not sepcified default format\n\t\t\t"
> +                      "qcow2 will be used.",

"If not specified, the default format is qcow2."

>          .mhandler.cmd = hmp_snapshot_blkdev,
>      },
>  
> @@ -1004,6 +1009,25 @@ Snapshot device, using snapshot file as target if provided
>  ETEXI
>  
>      {
> +        .name       = "snapshot_delete_blkdev",

Internal snapshots can already be deleted with delvm but there is no
existing command for external snapshots.

> +        .args_type  = "internal:-i,device:B,snapshot-file:s",
> +        .params     = "[-i] device new-image-file",
> +        .help       = "delete a snapshot  synchronous.\n\t\t\t"

"synchronous" is implied for HMP commands, they are all like this so I
don't think it's necessary to mention it.

> +                      "  The -i flag requests QEMU to delete internal snapshot\n\t\t\t"
> +                      "instead of external one.\n\t\t\t"
> +                      "  new-image-file is the snapshot's name, in external case\n\t\t\t"
> +                      "it is the image's name which is not supported now.\n\t\t\t"

I'm not sure how useful this interface is.  If we have no implementation
for deleting external snapshots and libvirt already uses delvm, then I'd
prefer you drop this command from the patch series.

Later on, when there is code to implement external snapshot deletion it
can be added.  Then there is no risk that this command design doesn't
work and we have to change it again.  (Remember libvirt already uses
delvm so adding snapshot_delete_blkdev without external snapshot
deletion just adds code churn.)

> @@ -1486,8 +1510,8 @@ ETEXI
>  
>      {
>          .name       = "info",
> -        .args_type  = "item:s?",
> -        .params     = "[subcommand]",
> +        .args_type  = "item:s?,params:s?",
> +        .params     = "[subcommand] [params]",
>          .help       = "show various information about the system state",
>          .mhandler.cmd = do_info,
>      },

What does this change do?

> diff --git a/hmp.c b/hmp.c
> index 180ba2b..f247f51 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -806,20 +806,40 @@ void hmp_snapshot_blkdev(Monitor *mon, const QDict *qdict)
>      const char *filename = qdict_get_try_str(qdict, "snapshot-file");
>      const char *format = qdict_get_try_str(qdict, "format");
>      int reuse = qdict_get_try_bool(qdict, "reuse", 0);
> +    int internal = qdict_get_try_bool(qdict, "internal", 0);
>      enum NewImageMode mode;
> +    enum SnapshotType type;
>      Error *errp = NULL;
>  
> -    if (!filename) {
> -        /* In the future, if 'snapshot-file' is not specified, the snapshot
> -           will be taken internally. Today it's actually required. */
> +    if ((!internal) && (!filename)) {
> +        /* in external case filename must be set, should we generate
> +         it automatically? */

Picking a filename would mainly be useful to humans.  libvirt and other
management tools will want full control anyway.  So the question is: is
there a naming policy that will be useful to most human users?

If yes, please implement it.  If no, please drop this comment.

> diff --git a/monitor.c b/monitor.c
> index c0e32d6..81de470 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -124,11 +124,14 @@ typedef struct mon_cmd_t {
>      void (*user_print)(Monitor *mon, const QObject *data);
>      union {
>          void (*info)(Monitor *mon);
> +        void (*info_qdict)(Monitor *mon, const QDict *qdict);
>          void (*cmd)(Monitor *mon, const QDict *qdict);
> -        int  (*cmd_new)(Monitor *mon, const QDict *params, QObject **ret_data);
> +        int  (*cmd_new)(Monitor *mon, const QDict *params,
> +                        QObject **ret_data);
>          int  (*cmd_async)(Monitor *mon, const QDict *params,
>                            MonitorCompletion *cb, void *opaque);
>      } mhandler;
> +    int info_cmd_need_qdict;
>      int flags;

The union discriminator is the flags field (e.g.  MONITOR_CMD_ASYNC).
Please follow that style.

(If you use a boolean variable, please use the bool type instead of
int.)

>  } mon_cmd_t;
>  
> @@ -824,7 +827,11 @@ static void do_info(Monitor *mon, const QDict *qdict)
>          goto help;
>      }
>  
> -    cmd->mhandler.info(mon);
> +    if (cmd->info_cmd_need_qdict) {
> +        cmd->mhandler.info_qdict(mon, qdict);
> +    } else {
> +        cmd->mhandler.info(mon);
> +    }
>      return;
>  
>  help:

The generic monitor changes need to go in a separate commit.

> @@ -2605,10 +2612,12 @@ static mon_cmd_t info_cmds[] = {
>      },
>      {
>          .name       = "snapshots",
> -        .args_type  = "",
> -        .params     = "",
> -        .help       = "show the currently saved VM snapshots",
> -        .mhandler.info = do_info_snapshots,
> +        .args_type  = "device:B?",
> +        .params     = "[device]",
> +        .help       = "show the currently saved VM snapshots or snapshots on "
> +                      "a single device.",
> +        .mhandler.info_qdict = do_info_snapshots,
> +        .info_cmd_need_qdict = 1,
>      },
>      {
>          .name       = "status",

This change to the info snapshots command needs to go in a separate
commit.

> diff --git a/savevm.c b/savevm.c
> index c027529..5982aa9 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -2336,7 +2336,7 @@ void do_delvm(Monitor *mon, const QDict *qdict)
>      }
>  }
>  
> -void do_info_snapshots(Monitor *mon)
> +static void do_info_snapshots_vm(Monitor *mon)
>  {
>      BlockDriverState *bs, *bs1;
>      QEMUSnapshotInfo *sn_tab, *sn, s, *sn_info = &s;
> @@ -2400,6 +2400,59 @@ void do_info_snapshots(Monitor *mon)
>  
>  }
>  
> +static void do_info_snapshots_blk(Monitor *mon, const char *device)
> +{
> +    BlockDriverState *bs;
> +    QEMUSnapshotInfo *sn_tab, *sn;
> +    int nb_sns, i;
> +    char buf[256];
> +
> +    /* find the target bs */
> +    bs = bdrv_find(device);
> +    if (!bs) {
> +        monitor_printf(mon, "Device '%s' not found.\n", device);
> +        return ;
> +    }
> +
> +    if (!bdrv_can_snapshot(bs)) {
> +        monitor_printf(mon, "Device '%s' can't have snapshot.\n", device);
> +        return ;
> +    }
> +
> +    nb_sns = bdrv_snapshot_list(bs, &sn_tab);
> +    if (nb_sns < 0) {
> +        monitor_printf(mon, "Device %s bdrv_snapshot_list: error %d\n",
> +                       device, nb_sns);
> +        return;
> +    }
> +
> +    if (nb_sns == 0) {
> +        monitor_printf(mon, "There is no snapshot available.\n");
> +        return;
> +    }
> +
> +    monitor_printf(mon, "Device %s:\n", device);
> +    monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL));
> +    for (i = 0; i < nb_sns; i++) {
> +        sn = &sn_tab[i];
> +        monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), sn));
> +    }
> +    g_free(sn_tab);
> +    return;
> +}

Return at the end of a void function is not necessary and QEMU code
doesn't use it.  Please use QEMU style.

Stefan

Re: [Qemu-devel] [PATCH 6/6] snapshot: human monitor interface

[Wenchao Xia]

 > On Mon, Dec 17, 2012 at 02:25:09PM +0800, Wenchao Xia wrote:
>> @@ -983,17 +983,22 @@ ETEXI
>>
>>       {
>>           .name       = "snapshot_blkdev",
>> -        .args_type  = "reuse:-n,device:B,snapshot-file:s?,format:s?",
>> -        .params     = "[-n] device [new-image-file] [format]",
>> -        .help       = "initiates a live snapshot\n\t\t\t"
>> -                      "of device. If a new image file is specified, the\n\t\t\t"
>> -                      "new image file will become the new root image.\n\t\t\t"
>> -                      "If format is specified, the snapshot file will\n\t\t\t"
>> -                      "be created in that format. Otherwise the\n\t\t\t"
>> -                      "snapshot will be internal! (currently unsupported).\n\t\t\t"
>> -                      "The default format is qcow2.  The -n flag requests QEMU\n\t\t\t"
>> -                      "to reuse the image found in new-image-file, instead of\n\t\t\t"
>> -                      "recreating it from scratch.",
>> +        .args_type  = "internal:-i,reuse:-n,device:B,snapshot-file:s?,format:s?",
>> +        .params     = "[-i] [-n] device [new-image-file] [format]",
>
> Please rename snapshot-file and new-image-file because it is now either
> the external snapshot filename or the internal snapshot name - it's not
> always a file!
>
   OK.

>> +        .help       = "initiates a live snapshot of device.\n\t\t\t"
>> +                      "  The -i flag requests QEMU to create internal snapshot\n\t\t\t"
>> +                      "instead of external one.\n\t\t\t"
>> +                      "  The -n flag requests QEMU to use existing snapshot\n\t\t\t"
>> +                      "instead of creating new snapshot, which would fails if\n\t\t\t"
>> +                      "snapshot does not exist ahead.\n\t\t\t"
>
> "which fails if the snapshot does not exist already"
>
   Will use this, thanks.

>> +                      "  new-image-file is the snapshot's name, in external case\n\t\t\t"
>> +                      "it is the new image's name which will become the new root\n\t\t\t"
>> +                      "image and must be specified, in internal case it is the\n\t\t\t"
>> +                      "record's name and if not specified QEMU will create\n\t\t\t"
>> +                      "internal snapshot with name generated according to time.\n\t\t\t"
>> +                      "  format is only valid in external case, which is the new\n\t\t\t"
>> +                      "snapshot image's format. If not sepcified default format\n\t\t\t"
>> +                      "qcow2 will be used.",
>
> "If not specified, the default format is qcow2."
>
>>           .mhandler.cmd = hmp_snapshot_blkdev,
>>       },
>>
>> @@ -1004,6 +1009,25 @@ Snapshot device, using snapshot file as target if provided
>>   ETEXI
>>
>>       {
>> +        .name       = "snapshot_delete_blkdev",
>
> Internal snapshots can already be deleted with delvm but there is no
> existing command for external snapshots.
>
>> +        .args_type  = "internal:-i,device:B,snapshot-file:s",
>> +        .params     = "[-i] device new-image-file",
>> +        .help       = "delete a snapshot  synchronous.\n\t\t\t"
>
> "synchronous" is implied for HMP commands, they are all like this so I
> don't think it's necessary to mention it.
>
>> +                      "  The -i flag requests QEMU to delete internal snapshot\n\t\t\t"
>> +                      "instead of external one.\n\t\t\t"
>> +                      "  new-image-file is the snapshot's name, in external case\n\t\t\t"
>> +                      "it is the image's name which is not supported now.\n\t\t\t"
>
> I'm not sure how useful this interface is.  If we have no implementation
> for deleting external snapshots and libvirt already uses delvm, then I'd
> prefer you drop this command from the patch series.
>
> Later on, when there is code to implement external snapshot deletion it
> can be added.  Then there is no risk that this command design doesn't
> work and we have to change it again.  (Remember libvirt already uses
> delvm so adding snapshot_delete_blkdev without external snapshot
> deletion just adds code churn.)
>
   OK, this interface will be dropped to make things simpler.

>> @@ -1486,8 +1510,8 @@ ETEXI
>>
>>       {
>>           .name       = "info",
>> -        .args_type  = "item:s?",
>> -        .params     = "[subcommand]",
>> +        .args_type  = "item:s?,params:s?",
>> +        .params     = "[subcommand] [params]",
>>           .help       = "show various information about the system state",
>>           .mhandler.cmd = do_info,
>>       },
>
> What does this change do?
>
   Pls ignore this, another serial was sent which extend hmp info
infra first.

>> diff --git a/hmp.c b/hmp.c
>> index 180ba2b..f247f51 100644
>> --- a/hmp.c
>> +++ b/hmp.c
>> @@ -806,20 +806,40 @@ void hmp_snapshot_blkdev(Monitor *mon, const QDict *qdict)
>>       const char *filename = qdict_get_try_str(qdict, "snapshot-file");
>>       const char *format = qdict_get_try_str(qdict, "format");
>>       int reuse = qdict_get_try_bool(qdict, "reuse", 0);
>> +    int internal = qdict_get_try_bool(qdict, "internal", 0);
>>       enum NewImageMode mode;
>> +    enum SnapshotType type;
>>       Error *errp = NULL;
>>
>> -    if (!filename) {
>> -        /* In the future, if 'snapshot-file' is not specified, the snapshot
>> -           will be taken internally. Today it's actually required. */
>> +    if ((!internal) && (!filename)) {
>> +        /* in external case filename must be set, should we generate
>> +         it automatically? */
>
> Picking a filename would mainly be useful to humans.  libvirt and other
> management tools will want full control anyway.  So the question is: is
> there a naming policy that will be useful to most human users?
>
> If yes, please implement it.  If no, please drop this comment.
>
   It will be dropped.

>> diff --git a/monitor.c b/monitor.c
>> index c0e32d6..81de470 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -124,11 +124,14 @@ typedef struct mon_cmd_t {
>>       void (*user_print)(Monitor *mon, const QObject *data);
>>       union {
>>           void (*info)(Monitor *mon);
>> +        void (*info_qdict)(Monitor *mon, const QDict *qdict);
>>           void (*cmd)(Monitor *mon, const QDict *qdict);
>> -        int  (*cmd_new)(Monitor *mon, const QDict *params, QObject **ret_data);
>> +        int  (*cmd_new)(Monitor *mon, const QDict *params,
>> +                        QObject **ret_data);
>>           int  (*cmd_async)(Monitor *mon, const QDict *params,
>>                             MonitorCompletion *cb, void *opaque);
>>       } mhandler;
>> +    int info_cmd_need_qdict;
>>       int flags;
>
> The union discriminator is the flags field (e.g.  MONITOR_CMD_ASYNC).
> Please follow that style.
>
> (If you use a boolean variable, please use the bool type instead of
> int.)
>
   There would be another way to extend hmp info command.

>>   } mon_cmd_t;
>>
>> @@ -824,7 +827,11 @@ static void do_info(Monitor *mon, const QDict *qdict)
>>           goto help;
>>       }
>>
>> -    cmd->mhandler.info(mon);
>> +    if (cmd->info_cmd_need_qdict) {
>> +        cmd->mhandler.info_qdict(mon, qdict);
>> +    } else {
>> +        cmd->mhandler.info(mon);
>> +    }
>>       return;
>>
>>   help:
>
> The generic monitor changes need to go in a separate commit.
>
>> @@ -2605,10 +2612,12 @@ static mon_cmd_t info_cmds[] = {
>>       },
>>       {
>>           .name       = "snapshots",
>> -        .args_type  = "",
>> -        .params     = "",
>> -        .help       = "show the currently saved VM snapshots",
>> -        .mhandler.info = do_info_snapshots,
>> +        .args_type  = "device:B?",
>> +        .params     = "[device]",
>> +        .help       = "show the currently saved VM snapshots or snapshots on "
>> +                      "a single device.",
>> +        .mhandler.info_qdict = do_info_snapshots,
>> +        .info_cmd_need_qdict = 1,
>>       },
>>       {
>>           .name       = "status",
>
> This change to the info snapshots command needs to go in a separate
> commit.
>
   It will go to hmp serial.

>> diff --git a/savevm.c b/savevm.c
>> index c027529..5982aa9 100644
>> --- a/savevm.c
>> +++ b/savevm.c
>> @@ -2336,7 +2336,7 @@ void do_delvm(Monitor *mon, const QDict *qdict)
>>       }
>>   }
>>
>> -void do_info_snapshots(Monitor *mon)
>> +static void do_info_snapshots_vm(Monitor *mon)
>>   {
>>       BlockDriverState *bs, *bs1;
>>       QEMUSnapshotInfo *sn_tab, *sn, s, *sn_info = &s;
>> @@ -2400,6 +2400,59 @@ void do_info_snapshots(Monitor *mon)
>>
>>   }
>>
>> +static void do_info_snapshots_blk(Monitor *mon, const char *device)
>> +{
>> +    BlockDriverState *bs;
>> +    QEMUSnapshotInfo *sn_tab, *sn;
>> +    int nb_sns, i;
>> +    char buf[256];
>> +
>> +    /* find the target bs */
>> +    bs = bdrv_find(device);
>> +    if (!bs) {
>> +        monitor_printf(mon, "Device '%s' not found.\n", device);
>> +        return ;
>> +    }
>> +
>> +    if (!bdrv_can_snapshot(bs)) {
>> +        monitor_printf(mon, "Device '%s' can't have snapshot.\n", device);
>> +        return ;
>> +    }
>> +
>> +    nb_sns = bdrv_snapshot_list(bs, &sn_tab);
>> +    if (nb_sns < 0) {
>> +        monitor_printf(mon, "Device %s bdrv_snapshot_list: error %d\n",
>> +                       device, nb_sns);
>> +        return;
>> +    }
>> +
>> +    if (nb_sns == 0) {
>> +        monitor_printf(mon, "There is no snapshot available.\n");
>> +        return;
>> +    }
>> +
>> +    monitor_printf(mon, "Device %s:\n", device);
>> +    monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL));
>> +    for (i = 0; i < nb_sns; i++) {
>> +        sn = &sn_tab[i];
>> +        monitor_printf(mon, "%s\n", bdrv_snapshot_dump(buf, sizeof(buf), sn));
>> +    }
>> +    g_free(sn_tab);
>> +    return;
>> +}
>
> Return at the end of a void function is not necessary and QEMU code
> doesn't use it.  Please use QEMU style.
>
   OK.

> Stefan
>


-- 
Best Regards

Wenchao Xia