[Qemu-devel] [PATCH 00/11] add internal snapshot support at block device level

[Qemu-devel] [PATCH 00/11] add internal snapshot support at block device level

[Wenchao Xia]

  This series brings internal snapshot support at block devices level, now we
have two main methods to do snapshot: 1) backing chain and 2) internal one, and
3) Stefan's unmerged backing up approach. Compared with 1), internal snapshot
is faster in R/W/Delete. Compared with 3), it complete faster, take less space
in total, but take more space in local image. As a summary, When user want to
take a local snapshot quickly, it is encouraged to use internal snapshot when
possible for performance reason. If user want to duplicate or backup vm to
another place, it is encouraged to use backing up approach.

  A full view may be: it can take snapshot by 1), 2), and the snapshot can
be exported to 3rd place. it can also directly export/backup data by 3).
export means transfering the data out of qemu/libvirt's scope, let 3rd party
take over.

host A                       backup server B
 vm         --------->
  |           backup
 \|/                     Data that can distinguish base/delta, disk/memory 
snapshot v0   export
snapshot v1 --------->

Next steps to better full VM snapshot:
  Add support to export internal snapshot data lively, possible use qemu-img
or qemu-nbd.
  Better vmstate saving.

Note:
  I think in most case, saving vmstate in an standalone file is better than
saving it inside qcow2, So suggest treat internal snapshot as block level
methods and not encourage user to savevm in qcow2 any more. We can
have "internal snapshot (or backing chain) + standing alone vmstate file" as
replacement.  


Some details:
  To avoid trouble, this serial have hide ID in interfaces, this make sure
no chaos of ID and name will be introduced by these interfaces.
  There is one patch may be common to Pavel's savvm transaction, patch 4/11,
others are not quite related. Patch 4/11 will not set errp when no snapshot
find, since patch 7/11 need to distinguish real error case.

Thanks Kevin to give advancement about how add it in qmp_transaction, oldest
version comes drom Dietmar Maurer.

Stefan Hajnoczi (3):
  1 blockdev: drop redundant proto_drv check
  2 blockdev: rename BlkTransactionStates to singular
  3 blockdev: allow BdrvActionOps->commit() to be NULL

Wenchao Xia (8):
  4 snapshot: new function bdrv_snapshot_find_by_id_and_name()
  5 snapshot: add paired functions for internal snapshot id and name
  6 snapshot: distinguish id and name in snapshot delete
  7 qmp: add internal snapshot support in qmp_transaction
  8 qmp: add interface blockdev-snapshot-internal-sync
  9 qmp: add interface blockdev-snapshot-delete-internal-sync
  10 hmp: add interface hmp_snapshot_internal_blkdev
  11 hmp: add interface hmp_snapshot_delete_internal_blkdev

 block/qcow2-snapshot.c    |   55 +++++++---
 block/qcow2.h             |    5 +-
 block/rbd.c               |   19 +++-
 block/sheepdog.c          |    5 +-
 block/snapshot.c          |  143 +++++++++++++++++++++++-
 blockdev.c                |  278 ++++++++++++++++++++++++++++++++++-----------
 hmp-commands.hx           |   43 +++++++-
 hmp.c                     |   20 ++++
 hmp.h                     |    2 +
 include/block/block_int.h |    5 +-
 include/block/snapshot.h  |   14 ++-
 qapi-schema.json          |   56 +++++++++
 qemu-img.c                |    5 +-
 qmp-commands.hx           |   93 ++++++++++++++-
 savevm.c                  |   10 ++-
 15 files changed, 649 insertions(+), 104 deletions(-)

[Qemu-devel] [PATCH 01/11] blockdev: drop redundant proto_drv check

[Wenchao Xia]

From: Stefan Hajnoczi <stefanha@redhat.com>

It is not necessary to check that we can find a protocol block driver
since we create or open the image file.  This produces the error that we
need anyway.

Besides, the QERR_INVALID_BLOCK_FORMAT is inappropriate since the
protocol is incorrect rather than the format.

Also drop an empty line between bdrv_open() and checking its return
value.  This may be due to copy-pasting from earlier code that performed
other operations before handling errors.

Reported-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 blockdev.c |   15 ---------------
 1 files changed, 0 insertions(+), 15 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 9937311..e70225b 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -818,7 +818,6 @@ typedef struct ExternalSnapshotStates {
 static void external_snapshot_prepare(BlkTransactionStates *common,
                                       Error **errp)
 {
-    BlockDriver *proto_drv;
     BlockDriver *drv;
     int flags, ret;
     Error *local_err = NULL;
@@ -874,12 +873,6 @@ static void external_snapshot_prepare(BlkTransactionStates *common,
 
     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);
-        return;
-    }
-
     /* create new image w/backing file */
     if (mode != NEW_IMAGE_MODE_EXISTING) {
         bdrv_img_create(new_image_file, format,
@@ -1372,7 +1365,6 @@ void qmp_drive_mirror(const char *device, const char *target,
 {
     BlockDriverState *bs;
     BlockDriverState *source, *target_bs;
-    BlockDriver *proto_drv;
     BlockDriver *drv = NULL;
     Error *local_err = NULL;
     int flags;
@@ -1440,12 +1432,6 @@ void qmp_drive_mirror(const char *device, const char *target,
         sync = MIRROR_SYNC_MODE_FULL;
     }
 
-    proto_drv = bdrv_find_protocol(target);
-    if (!proto_drv) {
-        error_set(errp, QERR_INVALID_BLOCK_FORMAT, format);
-        return;
-    }
-
     bdrv_get_geometry(bs, &size);
     size *= 512;
     if (sync == MIRROR_SYNC_MODE_FULL && mode != NEW_IMAGE_MODE_EXISTING) {
@@ -1480,7 +1466,6 @@ void qmp_drive_mirror(const char *device, const char *target,
      */
     target_bs = bdrv_new("");
     ret = bdrv_open(target_bs, target, NULL, flags | BDRV_O_NO_BACKING, drv);
-
     if (ret < 0) {
         bdrv_delete(target_bs);
         error_set(errp, QERR_OPEN_FILE_FAILED, target);
-- 
1.7.1

[Qemu-devel] [PATCH 02/11] blockdev: rename BlkTransactionStates to singular

[Wenchao Xia]

From: Stefan Hajnoczi <stefanha@redhat.com>

The QMP 'transaction' command keeps a list of in-flight transactions.
The transaction state structure is called BlkTransactionStates even
though it only deals with a single transaction.  The only plural thing
is the linked list of transaction states.

I find it confusing to call the single structure "States".  This patch
renames it to "State", just like BlockDriverState is singular.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 blockdev.c |  104 ++++++++++++++++++++++++++++++------------------------------
 1 files changed, 52 insertions(+), 52 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index e70225b..46a43b0 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -780,7 +780,7 @@ void qmp_blockdev_snapshot_sync(const char *device, const char *snapshot_file,
 
 /* New and old BlockDriverState structs for group snapshots */
 
-typedef struct BlkTransactionStates BlkTransactionStates;
+typedef struct BlkTransactionState BlkTransactionState;
 
 /* Only prepare() may fail. In a single transaction, only one of commit() or
    abort() will be called, clean() will always be called if it present. */
@@ -788,13 +788,13 @@ typedef struct BdrvActionOps {
     /* Size of state struct, in bytes. */
     size_t instance_size;
     /* Prepare the work, must NOT be NULL. */
-    void (*prepare)(BlkTransactionStates *common, Error **errp);
+    void (*prepare)(BlkTransactionState *common, Error **errp);
     /* Commit the changes, must NOT be NULL. */
-    void (*commit)(BlkTransactionStates *common);
+    void (*commit)(BlkTransactionState *common);
     /* Abort the changes on fail, can be NULL. */
-    void (*abort)(BlkTransactionStates *common);
+    void (*abort)(BlkTransactionState *common);
     /* Clean up resource in the end, can be NULL. */
-    void (*clean)(BlkTransactionStates *common);
+    void (*clean)(BlkTransactionState *common);
 } BdrvActionOps;
 
 /*
@@ -802,20 +802,20 @@ typedef struct BdrvActionOps {
  * that compiler will also arrange it to the same address with parent instance.
  * Later it will be used in free().
  */
-struct BlkTransactionStates {
+struct BlkTransactionState {
     TransactionAction *action;
     const BdrvActionOps *ops;
-    QSIMPLEQ_ENTRY(BlkTransactionStates) entry;
+    QSIMPLEQ_ENTRY(BlkTransactionState) entry;
 };
 
 /* external snapshot private data */
-typedef struct ExternalSnapshotStates {
-    BlkTransactionStates common;
+typedef struct ExternalSnapshotState {
+    BlkTransactionState common;
     BlockDriverState *old_bs;
     BlockDriverState *new_bs;
-} ExternalSnapshotStates;
+} ExternalSnapshotState;
 
-static void external_snapshot_prepare(BlkTransactionStates *common,
+static void external_snapshot_prepare(BlkTransactionState *common,
                                       Error **errp)
 {
     BlockDriver *drv;
@@ -825,8 +825,8 @@ static void external_snapshot_prepare(BlkTransactionStates *common,
     const char *new_image_file;
     const char *format = "qcow2";
     enum NewImageMode mode = NEW_IMAGE_MODE_ABSOLUTE_PATHS;
-    ExternalSnapshotStates *states =
-                             DO_UPCAST(ExternalSnapshotStates, common, common);
+    ExternalSnapshotState *state =
+                             DO_UPCAST(ExternalSnapshotState, common, common);
     TransactionAction *action = common->action;
 
     /* get parameters */
@@ -848,36 +848,36 @@ static void external_snapshot_prepare(BlkTransactionStates *common,
         return;
     }
 
-    states->old_bs = bdrv_find(device);
-    if (!states->old_bs) {
+    state->old_bs = bdrv_find(device);
+    if (!state->old_bs) {
         error_set(errp, QERR_DEVICE_NOT_FOUND, device);
         return;
     }
 
-    if (!bdrv_is_inserted(states->old_bs)) {
+    if (!bdrv_is_inserted(state->old_bs)) {
         error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
         return;
     }
 
-    if (bdrv_in_use(states->old_bs)) {
+    if (bdrv_in_use(state->old_bs)) {
         error_set(errp, QERR_DEVICE_IN_USE, device);
         return;
     }
 
-    if (!bdrv_is_read_only(states->old_bs)) {
-        if (bdrv_flush(states->old_bs)) {
+    if (!bdrv_is_read_only(state->old_bs)) {
+        if (bdrv_flush(state->old_bs)) {
             error_set(errp, QERR_IO_ERROR);
             return;
         }
     }
 
-    flags = states->old_bs->open_flags;
+    flags = state->old_bs->open_flags;
 
     /* 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,
+                        state->old_bs->filename,
+                        state->old_bs->drv->format_name,
                         NULL, -1, flags, &local_err, false);
         if (error_is_set(&local_err)) {
             error_propagate(errp, local_err);
@@ -886,42 +886,42 @@ static void external_snapshot_prepare(BlkTransactionStates *common,
     }
 
     /* We will manually add the backing_hd field to the bs later */
-    states->new_bs = bdrv_new("");
+    state->new_bs = bdrv_new("");
     /* TODO Inherit bs->options or only take explicit options with an
      * extended QMP command? */
-    ret = bdrv_open(states->new_bs, new_image_file, NULL,
+    ret = bdrv_open(state->new_bs, new_image_file, NULL,
                     flags | BDRV_O_NO_BACKING, drv);
     if (ret != 0) {
         error_set(errp, QERR_OPEN_FILE_FAILED, new_image_file);
     }
 }
 
-static void external_snapshot_commit(BlkTransactionStates *common)
+static void external_snapshot_commit(BlkTransactionState *common)
 {
-    ExternalSnapshotStates *states =
-                             DO_UPCAST(ExternalSnapshotStates, common, common);
+    ExternalSnapshotState *state =
+                             DO_UPCAST(ExternalSnapshotState, common, common);
 
-    /* This removes our old bs from the bdrv_states, and adds the new bs */
-    bdrv_append(states->new_bs, states->old_bs);
+    /* This removes our old bs and adds the new bs */
+    bdrv_append(state->new_bs, state->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,
+    bdrv_reopen(state->new_bs, state->new_bs->open_flags & ~BDRV_O_RDWR,
                 NULL);
 }
 
-static void external_snapshot_abort(BlkTransactionStates *common)
+static void external_snapshot_abort(BlkTransactionState *common)
 {
-    ExternalSnapshotStates *states =
-                             DO_UPCAST(ExternalSnapshotStates, common, common);
-    if (states->new_bs) {
-        bdrv_delete(states->new_bs);
+    ExternalSnapshotState *state =
+                             DO_UPCAST(ExternalSnapshotState, common, common);
+    if (state->new_bs) {
+        bdrv_delete(state->new_bs);
     }
 }
 
 static const BdrvActionOps actions[] = {
     [TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_SYNC] = {
-        .instance_size = sizeof(ExternalSnapshotStates),
+        .instance_size = sizeof(ExternalSnapshotState),
         .prepare  = external_snapshot_prepare,
         .commit   = external_snapshot_commit,
         .abort = external_snapshot_abort,
@@ -936,10 +936,10 @@ static const BdrvActionOps actions[] = {
 void qmp_transaction(TransactionActionList *dev_list, Error **errp)
 {
     TransactionActionList *dev_entry = dev_list;
-    BlkTransactionStates *states, *next;
+    BlkTransactionState *state, *next;
     Error *local_err = NULL;
 
-    QSIMPLEQ_HEAD(snap_bdrv_states, BlkTransactionStates) snap_bdrv_states;
+    QSIMPLEQ_HEAD(snap_bdrv_states, BlkTransactionState) snap_bdrv_states;
     QSIMPLEQ_INIT(&snap_bdrv_states);
 
     /* drain all i/o before any snapshots */
@@ -956,20 +956,20 @@ void qmp_transaction(TransactionActionList *dev_list, Error **errp)
         assert(dev_info->kind < ARRAY_SIZE(actions));
 
         ops = &actions[dev_info->kind];
-        states = g_malloc0(ops->instance_size);
-        states->ops = ops;
-        states->action = dev_info;
-        QSIMPLEQ_INSERT_TAIL(&snap_bdrv_states, states, entry);
+        state = g_malloc0(ops->instance_size);
+        state->ops = ops;
+        state->action = dev_info;
+        QSIMPLEQ_INSERT_TAIL(&snap_bdrv_states, state, entry);
 
-        states->ops->prepare(states, &local_err);
+        state->ops->prepare(state, &local_err);
         if (error_is_set(&local_err)) {
             error_propagate(errp, local_err);
             goto delete_and_fail;
         }
     }
 
-    QSIMPLEQ_FOREACH(states, &snap_bdrv_states, entry) {
-        states->ops->commit(states);
+    QSIMPLEQ_FOREACH(state, &snap_bdrv_states, entry) {
+        state->ops->commit(state);
     }
 
     /* success */
@@ -980,17 +980,17 @@ 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->ops->abort) {
-            states->ops->abort(states);
+    QSIMPLEQ_FOREACH(state, &snap_bdrv_states, entry) {
+        if (state->ops->abort) {
+            state->ops->abort(state);
         }
     }
 exit:
-    QSIMPLEQ_FOREACH_SAFE(states, &snap_bdrv_states, entry, next) {
-        if (states->ops->clean) {
-            states->ops->clean(states);
+    QSIMPLEQ_FOREACH_SAFE(state, &snap_bdrv_states, entry, next) {
+        if (state->ops->clean) {
+            state->ops->clean(state);
         }
-        g_free(states);
+        g_free(state);
     }
 }
 
-- 
1.7.1

[Qemu-devel] [PATCH 03/11] blockdev: allow BdrvActionOps->commit() to be NULL

[Wenchao Xia]

From: Stefan Hajnoczi <stefanha@redhat.com>

Some QMP 'transaction' types don't need to do anything on .commit().
Make .commit() optional just like .abort().

The "drive-backup" action will take advantage of this, it only needs to
cancel the block job on .abort().  Other block job actions will probably
follow the same pattern, so allow .commit() to be NULL.

Suggested-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 blockdev.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 46a43b0..4bd6cbc 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -789,7 +789,7 @@ typedef struct BdrvActionOps {
     size_t instance_size;
     /* Prepare the work, must NOT be NULL. */
     void (*prepare)(BlkTransactionState *common, Error **errp);
-    /* Commit the changes, must NOT be NULL. */
+    /* Commit the changes, can be NULL. */
     void (*commit)(BlkTransactionState *common);
     /* Abort the changes on fail, can be NULL. */
     void (*abort)(BlkTransactionState *common);
@@ -969,7 +969,9 @@ void qmp_transaction(TransactionActionList *dev_list, Error **errp)
     }
 
     QSIMPLEQ_FOREACH(state, &snap_bdrv_states, entry) {
-        state->ops->commit(state);
+        if (state->ops->commit) {
+            state->ops->commit(state);
+        }
     }
 
     /* success */
-- 
1.7.1

[Qemu-devel] [PATCH 04/11] snapshot: new function bdrv_snapshot_find_by_id_and_name()

[Wenchao Xia]

To make it clear about id and name in searching, add this API
to distinguish them. Caller can choose to search by id or name,
*errp will be set only for exception.

Some code are modified based on Pavel's patch.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 block/snapshot.c         |   74 ++++++++++++++++++++++++++++++++++++++++++++++
 include/block/snapshot.h |    6 ++++
 2 files changed, 80 insertions(+), 0 deletions(-)

diff --git a/block/snapshot.c b/block/snapshot.c
index 6c6d9de..0a9af4e 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -48,6 +48,80 @@ int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
     return ret;
 }
 
+/**
+ * Look up an internal snapshot by @id and @name.
+ * @bs: block device to search
+ * @id: unique snapshot ID, or NULL
+ * @name: snapshot name, or NULL
+ * @sn_info: location to store information on the snapshot found
+ * @errp: location to store error, will be set only for exception
+ *
+ * This function will traverse snapshot list in @bs to search the matching
+ * one, @id and @name are the matching condition:
+ * If both @id and @name are specified, find the first one with id @id and
+ * name @name.
+ * If only @id is specified, find the first one with id @id.
+ * If only @name is specified, find the first one with name @name.
+ * if none is specified, abort().
+ *
+ * Returns: true when a snapshot is found and @sn_info will be filled, false
+ * when error or not found. If all operation succeed but no matching one is
+ * found, @errp will NOT be set.
+ */
+bool bdrv_snapshot_find_by_id_and_name(BlockDriverState *bs,
+                                       const char *id,
+                                       const char *name,
+                                       QEMUSnapshotInfo *sn_info,
+                                       Error **errp)
+{
+    QEMUSnapshotInfo *sn_tab, *sn;
+    int nb_sns, i;
+    bool ret = false;
+
+    nb_sns = bdrv_snapshot_list(bs, &sn_tab);
+    if (nb_sns < 0) {
+        error_setg_errno(errp, -nb_sns, "Failed to get a snapshot list");
+        return false;
+    } else if (nb_sns == 0) {
+        return false;
+    }
+
+    if (id && name) {
+        for (i = 0; i < nb_sns; i++) {
+            sn = &sn_tab[i];
+            if (!strcmp(sn->id_str, id) && !strcmp(sn->name, name)) {
+                *sn_info = *sn;
+                ret = true;
+                break;
+            }
+        }
+    } else if (id) {
+        for (i = 0; i < nb_sns; i++) {
+            sn = &sn_tab[i];
+            if (!strcmp(sn->id_str, id)) {
+                *sn_info = *sn;
+                ret = true;
+                break;
+            }
+        }
+    } else if (name) {
+        for (i = 0; i < nb_sns; i++) {
+            sn = &sn_tab[i];
+            if (!strcmp(sn->name, name)) {
+                *sn_info = *sn;
+                ret = true;
+                break;
+            }
+        }
+    } else {
+        /* program error */
+        abort();
+    }
+
+    g_free(sn_tab);
+    return ret;
+}
+
 int bdrv_can_snapshot(BlockDriverState *bs)
 {
     BlockDriver *drv = bs->drv;
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index eaf61f0..9d06dc1 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -26,6 +26,7 @@
 #define SNAPSHOT_H
 
 #include "qemu-common.h"
+#include "qapi/error.h"
 
 typedef struct QEMUSnapshotInfo {
     char id_str[128]; /* unique snapshot id */
@@ -40,6 +41,11 @@ typedef struct QEMUSnapshotInfo {
 
 int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
                        const char *name);
+bool bdrv_snapshot_find_by_id_and_name(BlockDriverState *bs,
+                                       const char *id,
+                                       const char *name,
+                                       QEMUSnapshotInfo *sn_info,
+                                       Error **errp);
 int bdrv_can_snapshot(BlockDriverState *bs);
 int bdrv_snapshot_create(BlockDriverState *bs,
                          QEMUSnapshotInfo *sn_info);
-- 
1.7.1

[Qemu-devel] [PATCH 05/11] snapshot: add paired functions for internal snapshot id and name

[Wenchao Xia]

Internal snapshot's ID and name concept are both visible in general
block level, they are observed by user in "info snapshots", so it is
possible to mess them up. Although we can separate the two concept in
programming, but if they can be distinguished in string itself, things
will be simple and clear, so introduce two functions to do it.

The implemention, qcow2 snapshot calls snapshot_id_string_generate() to
make sure it follows the rule in driver. If caller or user give a check
with snapshot_name_wellformed() before create snapshot, then the ID
and name will never mess up. The check can be also taken in
qcow2_snapshot_create(), but require it to return error reason.

For rbd, it have no ID, so have no impact.
For sheepdog, ID can't be determined in qemu, so still can't guarantee
that no more mess up for ID and name.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 block/qcow2-snapshot.c   |    2 +-
 block/snapshot.c         |   33 +++++++++++++++++++++++++++++++++
 include/block/snapshot.h |    3 +++
 3 files changed, 37 insertions(+), 1 deletions(-)

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 992a5c8..7108d46 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -284,7 +284,7 @@ static void find_new_snapshot_id(BlockDriverState *bs,
         if (id > id_max)
             id_max = id;
     }
-    snprintf(id_str, id_str_size, "%d", id_max + 1);
+    snapshot_id_string_generate(id_max + 1, id_str, id_str_size);
 }
 
 static int find_snapshot_by_id(BlockDriverState *bs, const char *id_str)
diff --git a/block/snapshot.c b/block/snapshot.c
index 0a9af4e..6fa59e0 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -229,3 +229,36 @@ int bdrv_snapshot_load_tmp(BlockDriverState *bs,
     }
     return -ENOTSUP;
 }
+
+/*
+ * Every internal snapshot have an ID used by qemu block layer, this function
+ * check whether name used by user mess up with ID. An empty string is also
+ * invalid.
+ */
+bool snapshot_name_wellformed(const char *name)
+{
+    char *p;
+    /* variable v is used to remove gcc warning of "ignoring return value" and
+       "set but not used" */
+    unsigned long v;
+
+    if (*name == '\0') {
+        return false;
+    }
+
+    v = strtoul(name, &p, 10);
+    v++;
+
+    if (*p == '\0') {
+        /* Numeric string */
+        return false;
+    }
+    return true;
+}
+
+/* Following function generate id string, used by block driver, such as qcow2.
+   Since no better place to go, place the funtion here for now. */
+void snapshot_id_string_generate(int id, char *id_str, int id_str_size)
+{
+    snprintf(id_str, id_str_size, "%d", id);
+}
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index 9d06dc1..3d93719 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -56,4 +56,7 @@ int bdrv_snapshot_list(BlockDriverState *bs,
                        QEMUSnapshotInfo **psn_info);
 int bdrv_snapshot_load_tmp(BlockDriverState *bs,
                            const char *snapshot_name);
+
+bool snapshot_name_wellformed(const char *name);
+void snapshot_id_string_generate(int id, char *id_str, int id_str_size);
 #endif
-- 
1.7.1

[Qemu-devel] [PATCH 06/11] snapshot: distinguish id and name in snapshot delete

[Wenchao Xia]

Snapshot creation actually already distinguish id and name since it take
a structured parameter *sn, but delete can't. Later an accurate delete
is needed in qmp_transaction abort and blockdev-snapshot-delete-sync,
so change its prototype. Also *errp is added to tip error, but return
value is kepted to let caller check what kind of error happens. Existing
caller for it are savevm, delvm and qemu-img, they are not impacted by
check the return value and do the operations again.

Before this patch:
  For qcow2, it search id first then name to find the one to delete.
  For rbd, it search name.
  For sheepdog, it does nothing.

After this patch:
  For qcow2, logic is the same by call it twice in caller.
  For rbd, it always fails in delete with id, but still search for name
in second try, no change for user.

Some code for *errp is based on Pavel's patch.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 block/qcow2-snapshot.c    |   53 ++++++++++++++++++++++++++++++++------------
 block/qcow2.h             |    5 +++-
 block/rbd.c               |   19 +++++++++++++++-
 block/sheepdog.c          |    5 +++-
 block/snapshot.c          |   36 ++++++++++++++++++++++++++++--
 include/block/block_int.h |    5 +++-
 include/block/snapshot.h  |    5 +++-
 qemu-img.c                |    5 +++-
 savevm.c                  |   10 ++++++-
 9 files changed, 117 insertions(+), 26 deletions(-)

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 7108d46..4c0f552 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -287,31 +287,45 @@ static void find_new_snapshot_id(BlockDriverState *bs,
     snapshot_id_string_generate(id_max + 1, id_str, id_str_size);
 }
 
-static int find_snapshot_by_id(BlockDriverState *bs, const char *id_str)
+static int find_snapshot_by_id_and_name(BlockDriverState *bs,
+                                        const char *id,
+                                        const char *name)
 {
     BDRVQcowState *s = bs->opaque;
     int i;
 
-    for(i = 0; i < s->nb_snapshots; i++) {
-        if (!strcmp(s->snapshots[i].id_str, id_str))
-            return i;
+    if (id && name) {
+        for (i = 0; i < s->nb_snapshots; i++) {
+            if (!strcmp(s->snapshots[i].id_str, id) &&
+                !strcmp(s->snapshots[i].name, name)) {
+                return i;
+            }
+        }
+    } else if (id) {
+        for (i = 0; i < s->nb_snapshots; i++) {
+            if (!strcmp(s->snapshots[i].id_str, id)) {
+                return i;
+            }
+        }
+    } else if (name) {
+        for (i = 0; i < s->nb_snapshots; i++) {
+            if (!strcmp(s->snapshots[i].name, name)) {
+                return i;
+            }
+        }
     }
+
     return -1;
 }
 
 static int find_snapshot_by_id_or_name(BlockDriverState *bs, const char *name)
 {
-    BDRVQcowState *s = bs->opaque;
-    int i, ret;
+    int ret;
 
-    ret = find_snapshot_by_id(bs, name);
+    ret = find_snapshot_by_id_and_name(bs, name, NULL);
     if (ret >= 0)
         return ret;
-    for(i = 0; i < s->nb_snapshots; i++) {
-        if (!strcmp(s->snapshots[i].name, name))
-            return i;
-    }
-    return -1;
+    return find_snapshot_by_id_and_name(bs, NULL, name);
 }
 
 /* if no id is provided, a new one is constructed */
@@ -333,7 +347,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
     }
 
     /* Check that the ID is unique */
-    if (find_snapshot_by_id(bs, sn_info->id_str) >= 0) {
+    if (find_snapshot_by_id_and_name(bs, sn_info->id_str, NULL) >= 0) {
         return -EEXIST;
     }
 
@@ -530,15 +544,21 @@ fail:
     return ret;
 }
 
-int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
+int qcow2_snapshot_delete(BlockDriverState *bs,
+                          const char *snapshot_id,
+                          const char *name,
+                          Error **errp)
 {
     BDRVQcowState *s = bs->opaque;
     QCowSnapshot sn;
     int snapshot_index, ret;
 
     /* Search the snapshot */
-    snapshot_index = find_snapshot_by_id_or_name(bs, snapshot_id);
+    snapshot_index = find_snapshot_by_id_and_name(bs, snapshot_id, name);
     if (snapshot_index < 0) {
+        error_setg(errp,
+                   "Can't find a snapshot with ID %s and name %s",
+                   snapshot_id, name);
         return -ENOENT;
     }
     sn = s->snapshots[snapshot_index];
@@ -550,6 +570,7 @@ int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
     s->nb_snapshots--;
     ret = qcow2_write_snapshots(bs);
     if (ret < 0) {
+        error_setg(errp, "Failed to remove it from snapshot list");
         return ret;
     }
 
@@ -567,6 +588,7 @@ int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
     ret = qcow2_update_snapshot_refcount(bs, sn.l1_table_offset,
                                          sn.l1_size, -1);
     if (ret < 0) {
+        error_setg(errp, "Failed to free the cluster and L1 table");
         return ret;
     }
     qcow2_free_clusters(bs, sn.l1_table_offset, sn.l1_size * sizeof(uint64_t));
@@ -574,6 +596,7 @@ int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
     /* must update the copied flag on the current cluster offsets */
     ret = qcow2_update_snapshot_refcount(bs, s->l1_table_offset, s->l1_size, 0);
     if (ret < 0) {
+        error_setg(errp, "Failed to update snapshot status in disk");
         return ret;
     }
 
diff --git a/block/qcow2.h b/block/qcow2.h
index 6959c6a..5e39f78 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -385,7 +385,10 @@ int qcow2_zero_clusters(BlockDriverState *bs, uint64_t offset, int nb_sectors);
 /* qcow2-snapshot.c functions */
 int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info);
 int qcow2_snapshot_goto(BlockDriverState *bs, const char *snapshot_id);
-int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id);
+int qcow2_snapshot_delete(BlockDriverState *bs,
+                          const char *snapshot_id,
+                          const char *name,
+                          Error **errp);
 int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab);
 int qcow2_snapshot_load_tmp(BlockDriverState *bs, const char *snapshot_name);
 
diff --git a/block/rbd.c b/block/rbd.c
index 0f2608b..b4839e6 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -903,12 +903,29 @@ static int qemu_rbd_snap_create(BlockDriverState *bs,
 }
 
 static int qemu_rbd_snap_remove(BlockDriverState *bs,
-                                const char *snapshot_name)
+                                const char *snapshot_id,
+                                const char *snapshot_name,
+                                Error **errp)
 {
     BDRVRBDState *s = bs->opaque;
     int r;
 
+    if (snapshot_id && snapshot_id[0] != '\0') {
+        error_setg(errp, "rbd do not support snapshot id");
+        return -EINVAL;
+    }
+    /* then snapshot_id == NULL or it contain an empty line, ignore it */
+
+    if (!snapshot_name) {
+        error_setg(errp, "rbd need a valid snapshot name");
+        return -EINVAL;
+    }
+
     r = rbd_snap_remove(s->image, snapshot_name);
+    if (r < 0) {
+        error_setg_errno(errp, -r, "Failed to remove snapshot '%s'",
+                         snapshot_name);
+    }
     return r;
 }
 
diff --git a/block/sheepdog.c b/block/sheepdog.c
index 21a4edf..972d59b 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -2092,7 +2092,10 @@ out:
     return ret;
 }
 
-static int sd_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
+static int sd_snapshot_delete(BlockDriverState *bs,
+                              const char *snapshot_id,
+                              const char *name,
+                              Error **errp)
 {
     /* FIXME: Delete specified snapshot id.  */
     return 0;
diff --git a/block/snapshot.c b/block/snapshot.c
index 6fa59e0..58454f8 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -183,18 +183,48 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
     return -ENOTSUP;
 }
 
-int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
+/**
+ * Delete an internal snapshot by @snapshot_id and @name.
+ * @bs: block device used in the operation
+ * @snapshot_id: unique snapshot ID, or NULL
+ * @name: snapshot name, or NULL
+ * @errp: location to store error
+ *
+ * If both @snapshot_id and @name are specified, delete the first one with
+ * id @snapshot_id and name @name.
+ * If only @snapshot_id is specified, delete the first one with id
+ * @snapshot_id.
+ * If only @name is specified, delete the first one with name @name.
+ * if none is specified, return -ENINVAL.
+ *
+ * Returns: 0 on success, -errno on fail. If @bs is not inserted, return
+ * -ENOMEDIUM. If can't find one matching @id and @name, return -ENOENT.
+ * If @bs did not support internal snapshot, return -ENOTSUP. If @bs do not
+ * support parameter @snapshot_id or @name, return -EINVAL.
+ */
+int bdrv_snapshot_delete(BlockDriverState *bs,
+                         const char *snapshot_id,
+                         const char *name,
+                         Error **errp)
 {
     BlockDriver *drv = bs->drv;
     if (!drv) {
+        error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, bdrv_get_device_name(bs));
         return -ENOMEDIUM;
     }
+    if (!snapshot_id && !name) {
+        error_setg(errp, "snapshot_id and name are both NULL");
+        return -EINVAL;
+    }
     if (drv->bdrv_snapshot_delete) {
-        return drv->bdrv_snapshot_delete(bs, snapshot_id);
+        return drv->bdrv_snapshot_delete(bs, snapshot_id, name, errp);
     }
     if (bs->file) {
-        return bdrv_snapshot_delete(bs->file, snapshot_id);
+        return bdrv_snapshot_delete(bs->file, snapshot_id, name, errp);
     }
+    error_set(errp, QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED,
+              drv->format_name, bdrv_get_device_name(bs),
+              "internal snapshot");
     return -ENOTSUP;
 }
 
diff --git a/include/block/block_int.h b/include/block/block_int.h
index ba52247..532acef 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -157,7 +157,10 @@ struct BlockDriver {
                                 QEMUSnapshotInfo *sn_info);
     int (*bdrv_snapshot_goto)(BlockDriverState *bs,
                               const char *snapshot_id);
-    int (*bdrv_snapshot_delete)(BlockDriverState *bs, const char *snapshot_id);
+    int (*bdrv_snapshot_delete)(BlockDriverState *bs,
+                                const char *snapshot_id,
+                                const char *name,
+                                Error **errp);
     int (*bdrv_snapshot_list)(BlockDriverState *bs,
                               QEMUSnapshotInfo **psn_info);
     int (*bdrv_snapshot_load_tmp)(BlockDriverState *bs,
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index 3d93719..fbc03c0 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -51,7 +51,10 @@ int bdrv_snapshot_create(BlockDriverState *bs,
                          QEMUSnapshotInfo *sn_info);
 int bdrv_snapshot_goto(BlockDriverState *bs,
                        const char *snapshot_id);
-int bdrv_snapshot_delete(BlockDriverState *bs, const char *snapshot_id);
+int bdrv_snapshot_delete(BlockDriverState *bs,
+                         const char *snapshot_id,
+                         const char *name,
+                         Error **errp);
 int bdrv_snapshot_list(BlockDriverState *bs,
                        QEMUSnapshotInfo **psn_info);
 int bdrv_snapshot_load_tmp(BlockDriverState *bs,
diff --git a/qemu-img.c b/qemu-img.c
index 809b4f1..b660b39 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1883,7 +1883,10 @@ static int img_snapshot(int argc, char **argv)
         break;
 
     case SNAPSHOT_DELETE:
-        ret = bdrv_snapshot_delete(bs, snapshot_name);
+        ret = bdrv_snapshot_delete(bs, snapshot_name, NULL, NULL);
+        if (ret == -ENOENT || ret == -EINVAL) {
+            ret = bdrv_snapshot_delete(bs, NULL, snapshot_name, NULL);
+        }
         if (ret) {
             error_report("Could not delete snapshot '%s': %d (%s)",
                 snapshot_name, ret, strerror(-ret));
diff --git a/savevm.c b/savevm.c
index 2ce439f..91c36df 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2289,7 +2289,10 @@ static int del_existing_snapshots(Monitor *mon, const char *name)
         if (bdrv_can_snapshot(bs) &&
             bdrv_snapshot_find(bs, snapshot, name) >= 0)
         {
-            ret = bdrv_snapshot_delete(bs, name);
+            ret = bdrv_snapshot_delete(bs, name, NULL, NULL);
+            if (ret == -ENOENT || ret == -EINVAL) {
+                ret = bdrv_snapshot_delete(bs, NULL, name, NULL);
+            }
             if (ret < 0) {
                 monitor_printf(mon,
                                "Error while deleting snapshot on '%s'\n",
@@ -2519,7 +2522,10 @@ void do_delvm(Monitor *mon, const QDict *qdict)
     bs1 = NULL;
     while ((bs1 = bdrv_next(bs1))) {
         if (bdrv_can_snapshot(bs1)) {
-            ret = bdrv_snapshot_delete(bs1, name);
+            ret = bdrv_snapshot_delete(bs1, name, NULL, NULL);
+            if (ret == -ENOENT || ret == -EINVAL) {
+                ret = bdrv_snapshot_delete(bs, NULL, name, NULL);
+            }
             if (ret < 0) {
                 if (ret == -ENOTSUP)
                     monitor_printf(mon,
-- 
1.7.1

[Qemu-devel] [PATCH 07/11] qmp: add internal snapshot support in qmp_transaction

[Wenchao Xia]

Unlike savevm, the qmp_transaction interface will not generate
snapshot name automatically, saving trouble to return information
of the new created snapshot. The snapshot name should not mess up
with snapshot ID, there is a check for it.

Although qcow2 support storing multiple snapshots with same name
but different ID, here it will fail when an snapshot with that name
already exist before the operation. Format such as rbd do not support
ID at all, and in most case, it means trouble to user when he faces
multiple snapshots with same name, so ban that case.

Snapshot ID can't be specified in this interface.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 blockdev.c       |  118 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qapi-schema.json |   16 +++++++
 qmp-commands.hx  |   32 +++++++++++---
 3 files changed, 159 insertions(+), 7 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 4bd6cbc..aaeb0e8 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -808,6 +808,119 @@ struct BlkTransactionState {
     QSIMPLEQ_ENTRY(BlkTransactionState) entry;
 };
 
+/* internal snapshot private data */
+typedef struct InternalSnapshotState {
+    BlkTransactionState common;
+    BlockDriverState *bs;
+    QEMUSnapshotInfo sn;
+} InternalSnapshotState;
+
+static void internal_snapshot_prepare(BlkTransactionState *common,
+                                      Error **errp)
+{
+    const char *device;
+    const char *name;
+    BlockDriverState *bs;
+    QEMUSnapshotInfo sn, *sn1;
+    bool ret;
+    qemu_timeval tv;
+    BlockdevSnapshotInternal *internal;
+    InternalSnapshotState *state;
+
+    g_assert(common->action->kind ==
+             TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_INTERNAL_SYNC);
+    internal = common->action->blockdev_snapshot_internal_sync;
+    state = DO_UPCAST(InternalSnapshotState, common, common);
+
+    /* 1. parse input */
+    device = internal->device;
+    name = internal->name;
+
+    /* 2. check for validation */
+    bs = bdrv_find(device);
+    if (!bs) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+        return;
+    }
+
+    if (!bdrv_is_inserted(bs)) {
+        error_set(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
+        return;
+    }
+
+    if (bdrv_is_read_only(bs)) {
+        error_set(errp, QERR_DEVICE_IS_READ_ONLY, device);
+        return;
+    }
+
+    if (!bdrv_can_snapshot(bs)) {
+        error_set(errp, QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED,
+                  bs->drv->format_name, device, "internal snapshot");
+        return;
+    }
+
+    /* check whether a snapshot with name exist, no need to check id, since
+       name will be checked later to make sure it does not mess up with id. */
+    ret = bdrv_snapshot_find_by_id_and_name(bs, NULL, name, &sn, errp);
+    if (error_is_set(errp)) {
+        return;
+    }
+    if (ret) {
+        error_setg(errp, "Snapshot with name %s already exist on device %s",
+                   name, device);
+        return;
+    }
+
+    /* Forbid having a name similar to id, empty name is also forbidden. */
+    if (!snapshot_name_wellformed(name)) {
+        error_setg(errp, "Name %s on device %s is not a valid one",
+                   name, device);
+        return;
+    }
+
+    /* 3. take the snapshot */
+    sn1 = &state->sn;
+    pstrcpy(sn1->name, sizeof(sn1->name), name);
+    qemu_gettimeofday(&tv);
+    sn1->date_sec = tv.tv_sec;
+    sn1->date_nsec = tv.tv_usec * 1000;
+    /* not good to use vm_clock in block layer, but that is what we can do now,
+       or drop it later, since it is an emulater concept. */
+    sn1->vm_clock_nsec = qemu_get_clock_ns(vm_clock);
+
+    if (bdrv_snapshot_create(bs, sn1) < 0) {
+        error_setg(errp, "Failed to create snapshot %s on device %s",
+                   name, device);
+        return;
+    }
+
+    /* 4. succeed, mark a snapshot is created */
+    state->bs = bs;
+}
+
+static void internal_snapshot_abort(BlkTransactionState *common)
+{
+    InternalSnapshotState *state =
+                             DO_UPCAST(InternalSnapshotState, common, common);
+    BlockDriverState *bs = state->bs;
+    QEMUSnapshotInfo *sn = &state->sn;
+    Error *local_error = NULL;
+
+    if (!bs) {
+        return;
+    }
+
+    if (bdrv_snapshot_delete(bs, sn->id_str, sn->name, &local_error) < 0) {
+        error_report("Failed to delete snapshot with id %s and name %s on "
+                     "device %s in abort, reason is: %s",
+                     sn->id_str,
+                     sn->name,
+                     bdrv_get_device_name(bs),
+                     error_get_pretty(local_error));
+        error_free(local_error);
+    }
+}
+
 /* external snapshot private data */
 typedef struct ExternalSnapshotState {
     BlkTransactionState common;
@@ -926,6 +1039,11 @@ static const BdrvActionOps actions[] = {
         .commit   = external_snapshot_commit,
         .abort = external_snapshot_abort,
     },
+    [TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_INTERNAL_SYNC] = {
+        .instance_size = sizeof(InternalSnapshotState),
+        .prepare  = internal_snapshot_prepare,
+        .abort = internal_snapshot_abort,
+    },
 };
 
 /*
diff --git a/qapi-schema.json b/qapi-schema.json
index 5ad6894..9e143a8 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1613,6 +1613,21 @@
 { 'type': 'BlockdevSnapshot',
   'data': { 'device': 'str', 'snapshot-file': 'str', '*format': 'str',
             '*mode': 'NewImageMode' } }
+##
+# @BlockdevSnapshotInternal
+#
+# @device: the name of the device to generate the snapshot from
+#
+# @name: the name of the internal snapshot to be created
+#
+# Notes: In transaction, if any snapshot matching @name exist, the operation
+#        will fail. If the name is a numeric string, it will also fail. Only
+#        some image format support it, for example, qcow2, rbd, and sheepdog.
+#
+# Since: 1.6
+##
+{ 'type': 'BlockdevSnapshotInternal',
+  'data': { 'device': 'str', 'name': 'str' } }
 
 ##
 # @TransactionAction
@@ -1623,6 +1638,7 @@
 { 'union': 'TransactionAction',
   'data': {
        'blockdev-snapshot-sync': 'BlockdevSnapshot'
+       'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal'
    } }
 
 ##
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 8cea5e5..d31f518 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -948,13 +948,13 @@ transaction
 -----------
 
 Atomically operate on one or more block devices.  The only supported
-operation for now is snapshotting.  If there is any failure performing
-any of the operations, all snapshots for the group are abandoned, and
-the original disks pre-snapshot attempt are used.
+operations for now are internal and external snapshotting.  A list of
+dictionaries is accepted, that contains the actions to be performed.  The
+sequence of the requests will not affect the result.  If there is any failure
+performing any of the operations, all operations for the group are abandoned.
 
-A list of dictionaries is accepted, that contains the actions to be performed.
-For snapshots this is the device, the file to use for the new snapshot,
-and the format.  The default format, if not specified, is qcow2.
+For external snapshot, The dictionary is the device, the file to use for the
+new snapshot, and the format.  The default format, if not specified, is qcow2.
 
 Each new snapshot defaults to being created by QEMU (wiping any
 contents if the file already exists), but it is also possible to reuse
@@ -963,6 +963,18 @@ the new image file has the same contents as the current one; QEMU cannot
 perform any meaningful check.  Typically this is achieved by using the
 current image file as the backing file for the new image.
 
+On fail the original disks pre-snapshot attempt will be used.
+
+For internal snapshot, The dictionary is the device and the snapshot's name.
+If name is a numeric string which will mess up with ID, the request will be
+rejected.  For example, name "99" is not a valid name.  If an internal snapshot
+matching name already exist, the request will be also rejected.  Only some
+image format support it, for example, qcow2, rbd, and sheepdog.
+
+On fail, qemu will try delete new created internal snapshot in the
+transaction.  When I/O error make delete fail, user need to fix it later
+with qemu-img or other command.
+
 Arguments:
 
 actions array:
@@ -975,6 +987,9 @@ actions array:
       - "format": format of new image (json-string, optional)
       - "mode": whether and how QEMU should create the snapshot file
         (NewImageMode, optional, default "absolute-paths")
+      When "type" is "blockdev-snapshot-internal-sync":
+      - "device": device name to snapshot (json-string)
+      - "name": name of the new snapshot (json-string)
 
 Example:
 
@@ -986,7 +1001,10 @@ Example:
          { 'type': 'blockdev-snapshot-sync', 'data' : { "device": "ide-hd1",
                                          "snapshot-file": "/some/place/my-image2",
                                          "mode": "existing",
-                                         "format": "qcow2" } } ] } }
+                                         "format": "qcow2" } },
+         { 'type': 'blockdev-snapshot-internal-sync', 'data' : {
+                                         "device": "ide-hd2",
+                                         "name": "snapshot0" } } ] } }
 <- { "return": {} }
 
 EQMP
-- 
1.7.1

[Qemu-devel] [PATCH 08/11] qmp: add interface blockdev-snapshot-internal-sync

[Wenchao Xia]

This interface can generate snapshot name automatically if it is not
specified, since it is a single opertion.

Snapshot ID can't be specified in this interface.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 blockdev.c       |   25 +++++++++++++++++++++++++
 qapi-schema.json |   21 +++++++++++++++++++++
 qmp-commands.hx  |   31 +++++++++++++++++++++++++++++++
 3 files changed, 77 insertions(+), 0 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index aaeb0e8..6a952cd 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -777,6 +777,31 @@ void qmp_blockdev_snapshot_sync(const char *device, const char *snapshot_file,
                        &snapshot, errp);
 }
 
+void qmp_blockdev_snapshot_internal_sync(const char *device,
+                                         bool has_name, const char *name,
+                                         Error **errp)
+{
+    qemu_timeval tv;
+    struct tm tm;
+    char name1[128];
+
+    BlockdevSnapshotInternal snapshot = {
+        .device = (char *) device,
+    };
+
+    if (has_name) {
+        snapshot.name = (char *) name;
+    } else {
+        qemu_gettimeofday(&tv);
+        localtime_r((const time_t *)&tv.tv_sec, &tm);
+        strftime(name1, sizeof(name1), "vm-%Y%m%d%H%M%S", &tm);
+        snapshot.name = name1;
+    }
+
+    blockdev_do_action(TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_INTERNAL_SYNC,
+                       &snapshot, errp);
+}
+
 
 /* New and old BlockDriverState structs for group snapshots */
 
diff --git a/qapi-schema.json b/qapi-schema.json
index 9e143a8..fd2f8ce 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1689,6 +1689,27 @@
             '*mode': 'NewImageMode'} }
 
 ##
+# @blockdev-snapshot-internal-sync
+#
+# Generates a synchronous internal snapshot of a block device, when the format
+# of the image used support it.
+#
+# @device: the name of the device to generate the snapshot from
+#
+# @name: #optional the new snapshot name. If not specified, a name will be
+# generated according to time by qemu
+#
+# Returns: nothing on success
+#          If @device is not a valid block device, DeviceNotFound
+#          If any snapshot matching @name exist, or the name is a numeric which
+#          may mess up with snapshot ID, generic error will be returned
+#
+# Since 1.6
+##
+{ 'command': 'blockdev-snapshot-internal-sync',
+  'data': { 'device': 'str', '*name': 'str'} }
+
+##
 # @human-monitor-command:
 #
 # Execute a command on the human monitor and return the output.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index d31f518..e41e98a 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1044,6 +1044,37 @@ Example:
 EQMP
 
     {
+        .name       = "blockdev-snapshot-internal-sync",
+        .args_type  = "device:B,name:s?",
+        .mhandler.cmd_new = qmp_marshal_input_blockdev_snapshot_internal_sync,
+    },
+
+SQMP
+blockdev-snapshot-internal-sync
+-------------------------------
+
+Synchronously take an internal snapshot of a block device when the format of
+image used support it.  If name is not specified, it will be automatically
+generated by qemu according to host time.  If the name is a numeric string
+which may mess up with ID, such as "19", the operation will fail.  If a
+snapshot with name already exist, the operation will fail.
+
+Arguments:
+
+- "device": device name to snapshot (json-string)
+- "name": name of the new snapshot (json-string, optional)
+
+Example:
+
+-> { "execute": "blockdev-snapshot-internal-sync",
+                "arguments": { "device": "ide-hd0",
+                               "name": "snapshot0" }
+   }
+<- { "return": {} }
+
+EQMP
+
+    {
         .name       = "drive-mirror",
         .args_type  = "sync:s,device:B,target:s,speed:i?,mode:s?,format:s?,"
                       "on-source-error:s?,on-target-error:s?,"
-- 
1.7.1

[Qemu-devel] [PATCH 09/11] qmp: add interface blockdev-snapshot-delete-internal-sync

[Wenchao Xia]

Snapshot ID can't be specified in this interface.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 blockdev.c       |   12 ++++++++++++
 qapi-schema.json |   19 +++++++++++++++++++
 qmp-commands.hx  |   30 ++++++++++++++++++++++++++++++
 3 files changed, 61 insertions(+), 0 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 6a952cd..0277482 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -802,6 +802,18 @@ void qmp_blockdev_snapshot_internal_sync(const char *device,
                        &snapshot, errp);
 }
 
+void qmp_blockdev_snapshot_delete_internal_sync(const char *device,
+                                                const char *name,
+                                                Error **errp)
+{
+    BlockDriverState *bs = bdrv_find(device);
+    if (!bs) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, device);
+        return;
+    };
+
+    bdrv_snapshot_delete(bs, NULL, name, errp);
+}
 
 /* New and old BlockDriverState structs for group snapshots */
 
diff --git a/qapi-schema.json b/qapi-schema.json
index fd2f8ce..1860d31 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1710,6 +1710,25 @@
   'data': { 'device': 'str', '*name': 'str'} }
 
 ##
+# @blockdev-snapshot-delete-internal-sync
+#
+# Synchronously delete an internal snapshot of a block device, when the format
+# of the image used support it.
+#
+# @device: the name of the device to delete the snapshot from
+#
+# @name: the snapshot's name to be deleted
+#
+# Returns: nothing on success
+#          If @device is not a valid block device, DeviceNotFound
+#          If snapshot not found, generic error will be returned
+#
+# Since 1.6
+##
+{ 'command': 'blockdev-snapshot-delete-internal-sync',
+  'data': { 'device': 'str', 'name': 'str'} }
+
+##
 # @human-monitor-command:
 #
 # Execute a command on the human monitor and return the output.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index e41e98a..675753f 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -1075,6 +1075,36 @@ Example:
 EQMP
 
     {
+        .name       = "blockdev-snapshot-delete-internal-sync",
+        .args_type  = "device:B,name:s",
+        .mhandler.cmd_new =
+                      qmp_marshal_input_blockdev_snapshot_delete_internal_sync,
+    },
+
+SQMP
+blockdev-snapshot-delete-internal-sync
+--------------------------------------
+
+Synchronously delete an internal snapshot of a block device when the format of
+image used support it.  If the snapshot matching name is not found, operation
+will fail.
+
+Arguments:
+
+- "device": device name (json-string)
+- "name": name of the snapshot (json-string)
+
+Example:
+
+-> { "execute": "blockdev-snapshot-delete-internal-sync",
+                "arguments": { "device": "ide-hd0",
+                               "name": "snapshot0" }
+   }
+<- { "return": {} }
+
+EQMP
+
+    {
         .name       = "drive-mirror",
         .args_type  = "sync:s,device:B,target:s,speed:i?,mode:s?,format:s?,"
                       "on-source-error:s?,on-target-error:s?,"
-- 
1.7.1

[Qemu-devel] [PATCH 10/11] hmp: add interface hmp_snapshot_internal_blkdev

[Wenchao Xia]

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 hmp-commands.hx |   24 ++++++++++++++++++++++--
 hmp.c           |   10 ++++++++++
 hmp.h           |    1 +
 3 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 396691a..65fb94d 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1025,8 +1025,7 @@ ETEXI
                       "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"
+                      "be created in that format.\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.",
@@ -1040,6 +1039,27 @@ Snapshot device, using snapshot file as target if provided
 ETEXI
 
     {
+        .name       = "snapshot_internal_blkdev",
+        .args_type  = "device:B,name:s?",
+        .params     = "device [name]",
+        .help       = "take an internal snapshot of device.\n\t\t\t"
+                      "Name is the snapshot's name, if not specified\n\t\t\t"
+                      "qemu will generate one according to time.\n\t\t\t"
+                      "If any internal snapshot matching name already\n\t\t\t"
+                      "exist, or the name is an numeric string,\n\t\t\t"
+                      "the operation will fail.\n\t\t\t"
+                      "The format of the image used by device must\n\t\t\t"
+                      "support it, such as qcow2.\n\t\t\t",
+        .mhandler.cmd = hmp_snapshot_internal_blkdev,
+    },
+
+STEXI
+@item snapshot_internal_blkdev
+@findex snapshot_internal_blkdev
+Take an internal snapshot on device if it support
+ETEXI
+
+    {
         .name       = "drive_mirror",
         .args_type  = "reuse:-n,full:-f,device:B,target:s,format:s?",
         .params     = "[-n] [-f] device target [format]",
diff --git a/hmp.c b/hmp.c
index 494a9aa..810e4c5 100644
--- a/hmp.c
+++ b/hmp.c
@@ -912,6 +912,16 @@ void hmp_snapshot_blkdev(Monitor *mon, const QDict *qdict)
     hmp_handle_error(mon, &errp);
 }
 
+void hmp_snapshot_internal_blkdev(Monitor *mon, const QDict *qdict)
+{
+    const char *device = qdict_get_str(qdict, "device");
+    const char *name = qdict_get_try_str(qdict, "name");
+    Error *errp = NULL;
+
+    qmp_blockdev_snapshot_internal_sync(device, !!name, name, &errp);
+    hmp_handle_error(mon, &errp);
+}
+
 void hmp_migrate_cancel(Monitor *mon, const QDict *qdict)
 {
     qmp_migrate_cancel(NULL);
diff --git a/hmp.h b/hmp.h
index 56d2e92..e7593e9 100644
--- a/hmp.h
+++ b/hmp.h
@@ -54,6 +54,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_internal_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);
-- 
1.7.1

[Qemu-devel] [PATCH 11/11] hmp: add interface hmp_snapshot_delete_internal_blkdev

[Wenchao Xia]

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 hmp-commands.hx |   19 +++++++++++++++++++
 hmp.c           |   10 ++++++++++
 hmp.h           |    1 +
 3 files changed, 30 insertions(+), 0 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 65fb94d..9d9229c 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -1060,6 +1060,25 @@ Take an internal snapshot on device if it support
 ETEXI
 
     {
+        .name       = "snapshot_delete_internal_blkdev",
+        .args_type  = "device:B,name:s",
+        .params     = "device name",
+        .help       = "delete an internal snapshot of device.\n\t\t\t"
+                      "Name is the snapshot's name.\n\t\t\t"
+                      "If no snapshot matching name found,\n\t\t\t"
+                      "the operation will fail.\n\t\t\t"
+                      "The format of the image used by device must\n\t\t\t"
+                      "support it, such as qcow2.\n\t\t\t",
+        .mhandler.cmd = hmp_snapshot_delete_internal_blkdev,
+    },
+
+STEXI
+@item snapshot_delete_internal_blkdev
+@findex snapshot_delete_internal_blkdev
+Delete an internal snapshot on device if it support
+ETEXI
+
+    {
         .name       = "drive_mirror",
         .args_type  = "reuse:-n,full:-f,device:B,target:s,format:s?",
         .params     = "[-n] [-f] device target [format]",
diff --git a/hmp.c b/hmp.c
index 810e4c5..1de6d25 100644
--- a/hmp.c
+++ b/hmp.c
@@ -922,6 +922,16 @@ void hmp_snapshot_internal_blkdev(Monitor *mon, const QDict *qdict)
     hmp_handle_error(mon, &errp);
 }
 
+void hmp_snapshot_delete_internal_blkdev(Monitor *mon, const QDict *qdict)
+{
+    const char *device = qdict_get_str(qdict, "device");
+    const char *name = qdict_get_try_str(qdict, "name");
+    Error *errp = NULL;
+
+    qmp_blockdev_snapshot_delete_internal_sync(device, name, &errp);
+    hmp_handle_error(mon, &errp);
+}
+
 void hmp_migrate_cancel(Monitor *mon, const QDict *qdict)
 {
     qmp_migrate_cancel(NULL);
diff --git a/hmp.h b/hmp.h
index e7593e9..a562bf1 100644
--- a/hmp.h
+++ b/hmp.h
@@ -55,6 +55,7 @@ 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_internal_blkdev(Monitor *mon, const QDict *qdict);
+void hmp_snapshot_delete_internal_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);
-- 
1.7.1

Re: [Qemu-devel] [PATCH 04/11] snapshot: new function bdrv_snapshot_find_by_id_and_name()

[Fam Zheng]

On Sat, 06/08 14:58, Wenchao Xia wrote:
> To make it clear about id and name in searching, add this API
> to distinguish them. Caller can choose to search by id or name,
> *errp will be set only for exception.
> 
> Some code are modified based on Pavel's patch.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>  block/snapshot.c         |   74 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/block/snapshot.h |    6 ++++
>  2 files changed, 80 insertions(+), 0 deletions(-)
> 
> diff --git a/block/snapshot.c b/block/snapshot.c
> index 6c6d9de..0a9af4e 100644
> --- a/block/snapshot.c
> +++ b/block/snapshot.c
> @@ -48,6 +48,80 @@ int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
>      return ret;
>  }
>  
> +/**
> + * Look up an internal snapshot by @id and @name.
> + * @bs: block device to search
> + * @id: unique snapshot ID, or NULL
> + * @name: snapshot name, or NULL
> + * @sn_info: location to store information on the snapshot found
> + * @errp: location to store error, will be set only for exception
> + *
> + * This function will traverse snapshot list in @bs to search the matching
> + * one, @id and @name are the matching condition:
> + * If both @id and @name are specified, find the first one with id @id and
> + * name @name.
> + * If only @id is specified, find the first one with id @id.
> + * If only @name is specified, find the first one with name @name.
> + * if none is specified, abort().
> + *
> + * Returns: true when a snapshot is found and @sn_info will be filled, false
> + * when error or not found. If all operation succeed but no matching one is
> + * found, @errp will NOT be set.
> + */
> +bool bdrv_snapshot_find_by_id_and_name(BlockDriverState *bs,
> +                                       const char *id,
> +                                       const char *name,
> +                                       QEMUSnapshotInfo *sn_info,
> +                                       Error **errp)
> +{
> +    QEMUSnapshotInfo *sn_tab, *sn;
> +    int nb_sns, i;
> +    bool ret = false;
> +
> +    nb_sns = bdrv_snapshot_list(bs, &sn_tab);
> +    if (nb_sns < 0) {
> +        error_setg_errno(errp, -nb_sns, "Failed to get a snapshot list");
> +        return false;
> +    } else if (nb_sns == 0) {
> +        return false;
> +    }
> +
> +    if (id && name) {
> +        for (i = 0; i < nb_sns; i++) {
> +            sn = &sn_tab[i];
> +            if (!strcmp(sn->id_str, id) && !strcmp(sn->name, name)) {
> +                *sn_info = *sn;
> +                ret = true;
> +                break;
> +            }
> +        }
> +    } else if (id) {
> +        for (i = 0; i < nb_sns; i++) {
> +            sn = &sn_tab[i];
> +            if (!strcmp(sn->id_str, id)) {
> +                *sn_info = *sn;
> +                ret = true;
> +                break;
> +            }
> +        }
> +    } else if (name) {
> +        for (i = 0; i < nb_sns; i++) {
> +            sn = &sn_tab[i];
> +            if (!strcmp(sn->name, name)) {
> +                *sn_info = *sn;
> +                ret = true;
> +                break;
> +            }
> +        }
> +    } else {
> +        /* program error */
> +        abort();
> +    }

Looks duplicated. How about:

    if (id || name) {
        for (i = 0; i < nb_sns; i++) {
            sn = &sn_tab[i];
            if ((!id || !strcmp(sn->id_str, id)) &&
                (!name || !strcmp(sn->name, name))) {
                *sn_info = *sn;
                ret = true;
                break;
            }
        }
    } else {
        abort();
    }

And why do we have to abort here? It is not completely nonsense to me to
return first snapshot with id == NULL and name == NULL.

-- 
Fam

Re: [Qemu-devel] [PATCH 06/11] snapshot: distinguish id and name in snapshot delete

[Fam Zheng]

On Sat, 06/08 14:58, Wenchao Xia wrote:
>  
> -static int find_snapshot_by_id(BlockDriverState *bs, const char *id_str)
> +static int find_snapshot_by_id_and_name(BlockDriverState *bs,
> +                                        const char *id,
> +                                        const char *name)
>  {
>      BDRVQcowState *s = bs->opaque;
>      int i;
>  
> -    for(i = 0; i < s->nb_snapshots; i++) {
> -        if (!strcmp(s->snapshots[i].id_str, id_str))
> -            return i;
> +    if (id && name) {
> +        for (i = 0; i < s->nb_snapshots; i++) {
> +            if (!strcmp(s->snapshots[i].id_str, id) &&
> +                !strcmp(s->snapshots[i].name, name)) {
> +                return i;
> +            }
> +        }
> +    } else if (id) {
> +        for (i = 0; i < s->nb_snapshots; i++) {
> +            if (!strcmp(s->snapshots[i].id_str, id)) {
> +                return i;
> +            }
> +        }
> +    } else if (name) {
> +        for (i = 0; i < s->nb_snapshots; i++) {
> +            if (!strcmp(s->snapshots[i].name, name)) {
> +                return i;
> +            }
> +        }
>      }
Can be simplified the same way with patch 4.
>  
> -int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
> +int qcow2_snapshot_delete(BlockDriverState *bs,
> +                          const char *snapshot_id,
> +                          const char *name,
> +                          Error **errp)
>  {
>      BDRVQcowState *s = bs->opaque;
>      QCowSnapshot sn;
>      int snapshot_index, ret;
>  
>      /* Search the snapshot */
> -    snapshot_index = find_snapshot_by_id_or_name(bs, snapshot_id);
> +    snapshot_index = find_snapshot_by_id_and_name(bs, snapshot_id, name);
>      if (snapshot_index < 0) {
> +        error_setg(errp,
> +                   "Can't find a snapshot with ID %s and name %s",
> +                   snapshot_id, name);
>          return -ENOENT;
>      }
>      sn = s->snapshots[snapshot_index];
> @@ -550,6 +570,7 @@ int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
>      s->nb_snapshots--;
>      ret = qcow2_write_snapshots(bs);
>      if (ret < 0) {
> +        error_setg(errp, "Failed to remove it from snapshot list");
Maybe put name and id in error message, as above?

Re: [Qemu-devel] [PATCH 04/11] snapshot: new function bdrv_snapshot_find_by_id_and_name()

[Wenchao Xia]

于 2013-6-8 15:31, Fam Zheng 写道:
> On Sat, 06/08 14:58, Wenchao Xia wrote:
>> To make it clear about id and name in searching, add this API
>> to distinguish them. Caller can choose to search by id or name,
>> *errp will be set only for exception.
>>
>> Some code are modified based on Pavel's patch.
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
>> ---
>>   block/snapshot.c         |   74 ++++++++++++++++++++++++++++++++++++++++++++++
>>   include/block/snapshot.h |    6 ++++
>>   2 files changed, 80 insertions(+), 0 deletions(-)
>>
>> diff --git a/block/snapshot.c b/block/snapshot.c
>> index 6c6d9de..0a9af4e 100644
>> --- a/block/snapshot.c
>> +++ b/block/snapshot.c
>> @@ -48,6 +48,80 @@ int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
>>       return ret;
>>   }
>>
>> +/**
>> + * Look up an internal snapshot by @id and @name.
>> + * @bs: block device to search
>> + * @id: unique snapshot ID, or NULL
>> + * @name: snapshot name, or NULL
>> + * @sn_info: location to store information on the snapshot found
>> + * @errp: location to store error, will be set only for exception
>> + *
>> + * This function will traverse snapshot list in @bs to search the matching
>> + * one, @id and @name are the matching condition:
>> + * If both @id and @name are specified, find the first one with id @id and
>> + * name @name.
>> + * If only @id is specified, find the first one with id @id.
>> + * If only @name is specified, find the first one with name @name.
>> + * if none is specified, abort().
>> + *
>> + * Returns: true when a snapshot is found and @sn_info will be filled, false
>> + * when error or not found. If all operation succeed but no matching one is
>> + * found, @errp will NOT be set.
>> + */
>> +bool bdrv_snapshot_find_by_id_and_name(BlockDriverState *bs,
>> +                                       const char *id,
>> +                                       const char *name,
>> +                                       QEMUSnapshotInfo *sn_info,
>> +                                       Error **errp)
>> +{
>> +    QEMUSnapshotInfo *sn_tab, *sn;
>> +    int nb_sns, i;
>> +    bool ret = false;
>> +
>> +    nb_sns = bdrv_snapshot_list(bs, &sn_tab);
>> +    if (nb_sns < 0) {
>> +        error_setg_errno(errp, -nb_sns, "Failed to get a snapshot list");
>> +        return false;
>> +    } else if (nb_sns == 0) {
>> +        return false;
>> +    }
>> +
>> +    if (id && name) {
>> +        for (i = 0; i < nb_sns; i++) {
>> +            sn = &sn_tab[i];
>> +            if (!strcmp(sn->id_str, id) && !strcmp(sn->name, name)) {
>> +                *sn_info = *sn;
>> +                ret = true;
>> +                break;
>> +            }
>> +        }
>> +    } else if (id) {
>> +        for (i = 0; i < nb_sns; i++) {
>> +            sn = &sn_tab[i];
>> +            if (!strcmp(sn->id_str, id)) {
>> +                *sn_info = *sn;
>> +                ret = true;
>> +                break;
>> +            }
>> +        }
>> +    } else if (name) {
>> +        for (i = 0; i < nb_sns; i++) {
>> +            sn = &sn_tab[i];
>> +            if (!strcmp(sn->name, name)) {
>> +                *sn_info = *sn;
>> +                ret = true;
>> +                break;
>> +            }
>> +        }
>> +    } else {
>> +        /* program error */
>> +        abort();
>> +    }
>
> Looks duplicated. How about:
>
>      if (id || name) {
>          for (i = 0; i < nb_sns; i++) {
>              sn = &sn_tab[i];
>              if ((!id || !strcmp(sn->id_str, id)) &&
>                  (!name || !strcmp(sn->name, name))) {
>                  *sn_info = *sn;
>                  ret = true;
>                  break;
>              }
>          }
>      } else {
>          abort();
>      }
>
   Less code, but slightly slower since more "if" inside "for". I think
three "for" also show more clear about judgement logic.

> And why do we have to abort here? It is not completely nonsense to me to
> return first snapshot with id == NULL and name == NULL.
>
   Just to tip program error. An snapshot with id == NULL and name ==
NULL is not possible, isn't it?.


-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH 08/11] qmp: add interface blockdev-snapshot-internal-sync

[Fam Zheng]

On Sat, 06/08 14:58, Wenchao Xia wrote:
> This interface can generate snapshot name automatically if it is not
> specified, since it is a single opertion.
> 
> Snapshot ID can't be specified in this interface.
> 
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  blockdev.c       |   25 +++++++++++++++++++++++++
>  qapi-schema.json |   21 +++++++++++++++++++++
>  qmp-commands.hx  |   31 +++++++++++++++++++++++++++++++
>  3 files changed, 77 insertions(+), 0 deletions(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index aaeb0e8..6a952cd 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -777,6 +777,31 @@ void qmp_blockdev_snapshot_sync(const char *device, const char *snapshot_file,
>                         &snapshot, errp);
>  }
>  
> +void qmp_blockdev_snapshot_internal_sync(const char *device,
> +                                         bool has_name, const char *name,
> +                                         Error **errp)
> +{
> +    qemu_timeval tv;
> +    struct tm tm;
> +    char name1[128];
> +
> +    BlockdevSnapshotInternal snapshot = {
> +        .device = (char *) device,
> +    };
> +
> +    if (has_name) {
> +        snapshot.name = (char *) name;
> +    } else {
> +        qemu_gettimeofday(&tv);
> +        localtime_r((const time_t *)&tv.tv_sec, &tm);
> +        strftime(name1, sizeof(name1), "vm-%Y%m%d%H%M%S", &tm);
> +        snapshot.name = name1;
> +    }
> +
> +    blockdev_do_action(TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_INTERNAL_SYNC,
> +                       &snapshot, errp);
> +}
> +
>  
>  /* New and old BlockDriverState structs for group snapshots */
>  
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 9e143a8..fd2f8ce 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1689,6 +1689,27 @@
>              '*mode': 'NewImageMode'} }
>  
>  ##
> +# @blockdev-snapshot-internal-sync
> +#
> +# Generates a synchronous internal snapshot of a block device, when the format
> +# of the image used support it.
Confusing to say synchronous internal snapshot, "Synchronously take an
internal snapshot" from below is better. And s/support/supports/.  Maybe
document the return value for unsupported format too?
> +#
> +# @device: the name of the device to generate the snapshot from
> +#
> +# @name: #optional the new snapshot name. If not specified, a name will be
> +# generated according to time by qemu
> +#
> +# Returns: nothing on success
> +#          If @device is not a valid block device, DeviceNotFound
> +#          If any snapshot matching @name exist, or the name is a numeric which
> +#          may mess up with snapshot ID, generic error will be returned
> +#
> +# Since 1.6
> +##
> +{ 'command': 'blockdev-snapshot-internal-sync',
> +  'data': { 'device': 'str', '*name': 'str'} }
> +
> +##
>  # @human-monitor-command:
>  #
>  # Execute a command on the human monitor and return the output.
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index d31f518..e41e98a 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -1044,6 +1044,37 @@ Example:
>  EQMP
>  
>      {
> +        .name       = "blockdev-snapshot-internal-sync",
> +        .args_type  = "device:B,name:s?",
> +        .mhandler.cmd_new = qmp_marshal_input_blockdev_snapshot_internal_sync,
> +    },
> +
> +SQMP
> +blockdev-snapshot-internal-sync
> +-------------------------------
> +
> +Synchronously take an internal snapshot of a block device when the format of
> +image used support it.  If name is not specified, it will be automatically
I meant here: This is easier to understand.
s/support/supports/
> +generated by qemu according to host time.  If the name is a numeric string
> +which may mess up with ID, such as "19", the operation will fail.  If a
> +snapshot with name already exist, the operation will fail.
s/exist/exists/
> +
> +Arguments:
> +
> +- "device": device name to snapshot (json-string)
> +- "name": name of the new snapshot (json-string, optional)
> +
> +Example:
> +
> +-> { "execute": "blockdev-snapshot-internal-sync",
> +                "arguments": { "device": "ide-hd0",
> +                               "name": "snapshot0" }
> +   }
> +<- { "return": {} }
> +
> +EQMP
> +
> +    {
>          .name       = "drive-mirror",
>          .args_type  = "sync:s,device:B,target:s,speed:i?,mode:s?,format:s?,"
>                        "on-source-error:s?,on-target-error:s?,"
> -- 
> 1.7.1
> 
> 
> 

-- 
Fam

Re: [Qemu-devel] [PATCH 10/11] hmp: add interface hmp_snapshot_internal_blkdev

[Fam Zheng]

On Sat, 06/08 14:58, Wenchao Xia wrote:
> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> ---
>  hmp-commands.hx |   24 ++++++++++++++++++++++--
>  hmp.c           |   10 ++++++++++
>  hmp.h           |    1 +
>  3 files changed, 33 insertions(+), 2 deletions(-)
> 
> diff --git a/hmp-commands.hx b/hmp-commands.hx
> index 396691a..65fb94d 100644
> --- a/hmp-commands.hx
> +++ b/hmp-commands.hx
> @@ -1025,8 +1025,7 @@ ETEXI
>                        "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"
> +                      "be created in that format.\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.",
> @@ -1040,6 +1039,27 @@ Snapshot device, using snapshot file as target if provided
>  ETEXI
>  
>      {
> +        .name       = "snapshot_internal_blkdev",
Hmm, Just a boring naming question:

snapshot_blkdev reads like a verb-object phrase "snapshot the block
device", but snapshot_internal_blkdev is becoming awkward: "snapshot a
internal block device"?

> +        .args_type  = "device:B,name:s?",
> +        .params     = "device [name]",
> +        .help       = "take an internal snapshot of device.\n\t\t\t"
> +                      "Name is the snapshot's name, if not specified\n\t\t\t"
> +                      "qemu will generate one according to time.\n\t\t\t"
> +                      "If any internal snapshot matching name already\n\t\t\t"
> +                      "exist, or the name is an numeric string,\n\t\t\t"
> +                      "the operation will fail.\n\t\t\t"
> +                      "The format of the image used by device must\n\t\t\t"
> +                      "support it, such as qcow2.\n\t\t\t",
> +        .mhandler.cmd = hmp_snapshot_internal_blkdev,
> +    },
> +
> +STEXI
> +@item snapshot_internal_blkdev
> +@findex snapshot_internal_blkdev
> +Take an internal snapshot on device if it support
> +ETEXI
> +
> +    {
>          .name       = "drive_mirror",
>          .args_type  = "reuse:-n,full:-f,device:B,target:s,format:s?",
>          .params     = "[-n] [-f] device target [format]",
> diff --git a/hmp.c b/hmp.c
> index 494a9aa..810e4c5 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -912,6 +912,16 @@ void hmp_snapshot_blkdev(Monitor *mon, const QDict *qdict)
>      hmp_handle_error(mon, &errp);
>  }
>  
> +void hmp_snapshot_internal_blkdev(Monitor *mon, const QDict *qdict)
> +{
> +    const char *device = qdict_get_str(qdict, "device");
> +    const char *name = qdict_get_try_str(qdict, "name");
> +    Error *errp = NULL;
> +
> +    qmp_blockdev_snapshot_internal_sync(device, !!name, name, &errp);
> +    hmp_handle_error(mon, &errp);
> +}
> +
>  void hmp_migrate_cancel(Monitor *mon, const QDict *qdict)
>  {
>      qmp_migrate_cancel(NULL);
> diff --git a/hmp.h b/hmp.h
> index 56d2e92..e7593e9 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -54,6 +54,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_internal_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);
> -- 
> 1.7.1
> 
> 
> 

-- 
Fam

Re: [Qemu-devel] [PATCH 04/11] snapshot: new function bdrv_snapshot_find_by_id_and_name()

[Fam Zheng]

On Sat, 06/08 15:58, Wenchao Xia wrote:
> 于 2013-6-8 15:31, Fam Zheng 写道:
> >On Sat, 06/08 14:58, Wenchao Xia wrote:
> >>To make it clear about id and name in searching, add this API
> >>to distinguish them. Caller can choose to search by id or name,
> >>*errp will be set only for exception.
> >>
> >>Some code are modified based on Pavel's patch.
> >>
> >>Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> >>Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> >>---
> >>  block/snapshot.c         |   74 ++++++++++++++++++++++++++++++++++++++++++++++
> >>  include/block/snapshot.h |    6 ++++
> >>  2 files changed, 80 insertions(+), 0 deletions(-)
> >>
> >>diff --git a/block/snapshot.c b/block/snapshot.c
> >>index 6c6d9de..0a9af4e 100644
> >>--- a/block/snapshot.c
> >>+++ b/block/snapshot.c
> >>@@ -48,6 +48,80 @@ int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
> >>      return ret;
> >>  }
> >>
> >>+/**
> >>+ * Look up an internal snapshot by @id and @name.
> >>+ * @bs: block device to search
> >>+ * @id: unique snapshot ID, or NULL
> >>+ * @name: snapshot name, or NULL
> >>+ * @sn_info: location to store information on the snapshot found
> >>+ * @errp: location to store error, will be set only for exception
> >>+ *
> >>+ * This function will traverse snapshot list in @bs to search the matching
> >>+ * one, @id and @name are the matching condition:
> >>+ * If both @id and @name are specified, find the first one with id @id and
> >>+ * name @name.
> >>+ * If only @id is specified, find the first one with id @id.
> >>+ * If only @name is specified, find the first one with name @name.
> >>+ * if none is specified, abort().
> >>+ *
> >>+ * Returns: true when a snapshot is found and @sn_info will be filled, false
> >>+ * when error or not found. If all operation succeed but no matching one is
> >>+ * found, @errp will NOT be set.
> >>+ */
> >>+bool bdrv_snapshot_find_by_id_and_name(BlockDriverState *bs,
> >>+                                       const char *id,
> >>+                                       const char *name,
> >>+                                       QEMUSnapshotInfo *sn_info,
> >>+                                       Error **errp)
> >>+{
> >>+    QEMUSnapshotInfo *sn_tab, *sn;
> >>+    int nb_sns, i;
> >>+    bool ret = false;
> >>+
> >>+    nb_sns = bdrv_snapshot_list(bs, &sn_tab);
> >>+    if (nb_sns < 0) {
> >>+        error_setg_errno(errp, -nb_sns, "Failed to get a snapshot list");
> >>+        return false;
> >>+    } else if (nb_sns == 0) {
> >>+        return false;
> >>+    }
> >>+
> >>+    if (id && name) {
> >>+        for (i = 0; i < nb_sns; i++) {
> >>+            sn = &sn_tab[i];
> >>+            if (!strcmp(sn->id_str, id) && !strcmp(sn->name, name)) {
> >>+                *sn_info = *sn;
> >>+                ret = true;
> >>+                break;
> >>+            }
> >>+        }
> >>+    } else if (id) {
> >>+        for (i = 0; i < nb_sns; i++) {
> >>+            sn = &sn_tab[i];
> >>+            if (!strcmp(sn->id_str, id)) {
> >>+                *sn_info = *sn;
> >>+                ret = true;
> >>+                break;
> >>+            }
> >>+        }
> >>+    } else if (name) {
> >>+        for (i = 0; i < nb_sns; i++) {
> >>+            sn = &sn_tab[i];
> >>+            if (!strcmp(sn->name, name)) {
> >>+                *sn_info = *sn;
> >>+                ret = true;
> >>+                break;
> >>+            }
> >>+        }
> >>+    } else {
> >>+        /* program error */
> >>+        abort();
> >>+    }
> >
> >Looks duplicated. How about:
> >
> >     if (id || name) {
> >         for (i = 0; i < nb_sns; i++) {
> >             sn = &sn_tab[i];
> >             if ((!id || !strcmp(sn->id_str, id)) &&
> >                 (!name || !strcmp(sn->name, name))) {
> >                 *sn_info = *sn;
> >                 ret = true;
> >                 break;
> >             }
> >         }
> >     } else {
> >         abort();
> >     }
> >
>   Less code, but slightly slower since more "if" inside "for". I think
> three "for" also show more clear about judgement logic.

No I don't think if-in-for or for-in-if *here* makes any meaningful
difference in performance, if we really need it fast, we'd better sort the
list it first and binary search. And I don't see it clearer to duplicate
the same logic for three times, If I want to understand it, I need to
compare if#1 and if#2 to get they are the same, and then compare #2 and
#3 again, just to know that the three are no different.

> 
> >And why do we have to abort here? It is not completely nonsense to me to
> >return first snapshot with id == NULL and name == NULL.
> >
>   Just to tip program error. An snapshot with id == NULL and name ==
> NULL is not possible, isn't it?.

OK.

-- 
Fam

Re: [Qemu-devel] [PATCH 04/11] snapshot: new function bdrv_snapshot_find_by_id_and_name()

[Wenchao Xia]

于 2013-6-8 16:35, Fam Zheng 写道:
> On Sat, 06/08 15:58, Wenchao Xia wrote:
>> 于 2013-6-8 15:31, Fam Zheng 写道:
>>> On Sat, 06/08 14:58, Wenchao Xia wrote:
>>>> To make it clear about id and name in searching, add this API
>>>> to distinguish them. Caller can choose to search by id or name,
>>>> *errp will be set only for exception.
>>>>
>>>> Some code are modified based on Pavel's patch.
>>>>
>>>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>>>> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
>>>> ---
>>>>   block/snapshot.c         |   74 ++++++++++++++++++++++++++++++++++++++++++++++
>>>>   include/block/snapshot.h |    6 ++++
>>>>   2 files changed, 80 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/block/snapshot.c b/block/snapshot.c
>>>> index 6c6d9de..0a9af4e 100644
>>>> --- a/block/snapshot.c
>>>> +++ b/block/snapshot.c
>>>> @@ -48,6 +48,80 @@ int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
>>>>       return ret;
>>>>   }
>>>>
>>>> +/**
>>>> + * Look up an internal snapshot by @id and @name.
>>>> + * @bs: block device to search
>>>> + * @id: unique snapshot ID, or NULL
>>>> + * @name: snapshot name, or NULL
>>>> + * @sn_info: location to store information on the snapshot found
>>>> + * @errp: location to store error, will be set only for exception
>>>> + *
>>>> + * This function will traverse snapshot list in @bs to search the matching
>>>> + * one, @id and @name are the matching condition:
>>>> + * If both @id and @name are specified, find the first one with id @id and
>>>> + * name @name.
>>>> + * If only @id is specified, find the first one with id @id.
>>>> + * If only @name is specified, find the first one with name @name.
>>>> + * if none is specified, abort().
>>>> + *
>>>> + * Returns: true when a snapshot is found and @sn_info will be filled, false
>>>> + * when error or not found. If all operation succeed but no matching one is
>>>> + * found, @errp will NOT be set.
>>>> + */
>>>> +bool bdrv_snapshot_find_by_id_and_name(BlockDriverState *bs,
>>>> +                                       const char *id,
>>>> +                                       const char *name,
>>>> +                                       QEMUSnapshotInfo *sn_info,
>>>> +                                       Error **errp)
>>>> +{
>>>> +    QEMUSnapshotInfo *sn_tab, *sn;
>>>> +    int nb_sns, i;
>>>> +    bool ret = false;
>>>> +
>>>> +    nb_sns = bdrv_snapshot_list(bs, &sn_tab);
>>>> +    if (nb_sns < 0) {
>>>> +        error_setg_errno(errp, -nb_sns, "Failed to get a snapshot list");
>>>> +        return false;
>>>> +    } else if (nb_sns == 0) {
>>>> +        return false;
>>>> +    }
>>>> +
>>>> +    if (id && name) {
>>>> +        for (i = 0; i < nb_sns; i++) {
>>>> +            sn = &sn_tab[i];
>>>> +            if (!strcmp(sn->id_str, id) && !strcmp(sn->name, name)) {
>>>> +                *sn_info = *sn;
>>>> +                ret = true;
>>>> +                break;
>>>> +            }
>>>> +        }
>>>> +    } else if (id) {
>>>> +        for (i = 0; i < nb_sns; i++) {
>>>> +            sn = &sn_tab[i];
>>>> +            if (!strcmp(sn->id_str, id)) {
>>>> +                *sn_info = *sn;
>>>> +                ret = true;
>>>> +                break;
>>>> +            }
>>>> +        }
>>>> +    } else if (name) {
>>>> +        for (i = 0; i < nb_sns; i++) {
>>>> +            sn = &sn_tab[i];
>>>> +            if (!strcmp(sn->name, name)) {
>>>> +                *sn_info = *sn;
>>>> +                ret = true;
>>>> +                break;
>>>> +            }
>>>> +        }
>>>> +    } else {
>>>> +        /* program error */
>>>> +        abort();
>>>> +    }
>>>
>>> Looks duplicated. How about:
>>>
>>>      if (id || name) {
>>>          for (i = 0; i < nb_sns; i++) {
>>>              sn = &sn_tab[i];
>>>              if ((!id || !strcmp(sn->id_str, id)) &&
>>>                  (!name || !strcmp(sn->name, name))) {
>>>                  *sn_info = *sn;
>>>                  ret = true;
>>>                  break;
>>>              }
>>>          }
>>>      } else {
>>>          abort();
>>>      }
>>>
>>    Less code, but slightly slower since more "if" inside "for". I think
>> three "for" also show more clear about judgement logic.
>
> No I don't think if-in-for or for-in-if *here* makes any meaningful
> difference in performance, if we really need it fast, we'd better sort the
   My instinct is forbid if in for, just my custom.

> list it first and binary search. And I don't see it clearer to duplicate
> the same logic for three times, If I want to understand it, I need to
   Three cases makes work flow clear, and it is easy when I want to
change a logic in one case.

> compare if#1 and if#2 to get they are the same, and then compare #2 and
> #3 again, just to know that the three are no different.
>
   There is difference requiring reader think and extend it out into
three cases in his mind:
               if ((!id || !strcmp(sn->id_str, id)) &&
                   (!name || !strcmp(sn->name, name)))

It is a coding style issue, usually I don't mind to write more C lines 
to make things simple. Hope to get maintainer's idea.

>>
>>> And why do we have to abort here? It is not completely nonsense to me to
>>> return first snapshot with id == NULL and name == NULL.
>>>
>>    Just to tip program error. An snapshot with id == NULL and name ==
>> NULL is not possible, isn't it?.
>
> OK.
>


-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH 08/11] qmp: add interface blockdev-snapshot-internal-sync

[Wenchao Xia]

于 2013-6-8 16:05, Fam Zheng 写道:
> On Sat, 06/08 14:58, Wenchao Xia wrote:
>> This interface can generate snapshot name automatically if it is not
>> specified, since it is a single opertion.
>>
>> Snapshot ID can't be specified in this interface.
>>
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>>   blockdev.c       |   25 +++++++++++++++++++++++++
>>   qapi-schema.json |   21 +++++++++++++++++++++
>>   qmp-commands.hx  |   31 +++++++++++++++++++++++++++++++
>>   3 files changed, 77 insertions(+), 0 deletions(-)
>>
>> diff --git a/blockdev.c b/blockdev.c
>> index aaeb0e8..6a952cd 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -777,6 +777,31 @@ void qmp_blockdev_snapshot_sync(const char *device, const char *snapshot_file,
>>                          &snapshot, errp);
>>   }
>>
>> +void qmp_blockdev_snapshot_internal_sync(const char *device,
>> +                                         bool has_name, const char *name,
>> +                                         Error **errp)
>> +{
>> +    qemu_timeval tv;
>> +    struct tm tm;
>> +    char name1[128];
>> +
>> +    BlockdevSnapshotInternal snapshot = {
>> +        .device = (char *) device,
>> +    };
>> +
>> +    if (has_name) {
>> +        snapshot.name = (char *) name;
>> +    } else {
>> +        qemu_gettimeofday(&tv);
>> +        localtime_r((const time_t *)&tv.tv_sec, &tm);
>> +        strftime(name1, sizeof(name1), "vm-%Y%m%d%H%M%S", &tm);
>> +        snapshot.name = name1;
>> +    }
>> +
>> +    blockdev_do_action(TRANSACTION_ACTION_KIND_BLOCKDEV_SNAPSHOT_INTERNAL_SYNC,
>> +                       &snapshot, errp);
>> +}
>> +
>>
>>   /* New and old BlockDriverState structs for group snapshots */
>>
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index 9e143a8..fd2f8ce 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -1689,6 +1689,27 @@
>>               '*mode': 'NewImageMode'} }
>>
>>   ##
>> +# @blockdev-snapshot-internal-sync
>> +#
>> +# Generates a synchronous internal snapshot of a block device, when the format
>> +# of the image used support it.
> Confusing to say synchronous internal snapshot, "Synchronously take an
> internal snapshot" from below is better. And s/support/supports/.  Maybe
> document the return value for unsupported format too?
   OK, will change it.


>> +#
>> +# @device: the name of the device to generate the snapshot from
>> +#
>> +# @name: #optional the new snapshot name. If not specified, a name will be
>> +# generated according to time by qemu
>> +#
>> +# Returns: nothing on success
>> +#          If @device is not a valid block device, DeviceNotFound
>> +#          If any snapshot matching @name exist, or the name is a numeric which
>> +#          may mess up with snapshot ID, generic error will be returned
>> +#
>> +# Since 1.6
>> +##
>> +{ 'command': 'blockdev-snapshot-internal-sync',
>> +  'data': { 'device': 'str', '*name': 'str'} }
>> +
>> +##
>>   # @human-monitor-command:
>>   #
>>   # Execute a command on the human monitor and return the output.
>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>> index d31f518..e41e98a 100644
>> --- a/qmp-commands.hx
>> +++ b/qmp-commands.hx
>> @@ -1044,6 +1044,37 @@ Example:
>>   EQMP
>>
>>       {
>> +        .name       = "blockdev-snapshot-internal-sync",
>> +        .args_type  = "device:B,name:s?",
>> +        .mhandler.cmd_new = qmp_marshal_input_blockdev_snapshot_internal_sync,
>> +    },
>> +
>> +SQMP
>> +blockdev-snapshot-internal-sync
>> +-------------------------------
>> +
>> +Synchronously take an internal snapshot of a block device when the format of
>> +image used support it.  If name is not specified, it will be automatically
> I meant here: This is easier to understand.
> s/support/supports/
>> +generated by qemu according to host time.  If the name is a numeric string
>> +which may mess up with ID, such as "19", the operation will fail.  If a
>> +snapshot with name already exist, the operation will fail.
> s/exist/exists/
   Will change it, thanks for examination.

>> +
>> +Arguments:
>> +
>> +- "device": device name to snapshot (json-string)
>> +- "name": name of the new snapshot (json-string, optional)
>> +
>> +Example:
>> +
>> +-> { "execute": "blockdev-snapshot-internal-sync",
>> +                "arguments": { "device": "ide-hd0",
>> +                               "name": "snapshot0" }
>> +   }
>> +<- { "return": {} }
>> +
>> +EQMP
>> +
>> +    {
>>           .name       = "drive-mirror",
>>           .args_type  = "sync:s,device:B,target:s,speed:i?,mode:s?,format:s?,"
>>                         "on-source-error:s?,on-target-error:s?,"
>> --
>> 1.7.1
>>
>>
>>
>


-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH 10/11] hmp: add interface hmp_snapshot_internal_blkdev

[Wenchao Xia]

于 2013-6-8 16:21, Fam Zheng 写道:
> On Sat, 06/08 14:58, Wenchao Xia wrote:
>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>> ---
>>   hmp-commands.hx |   24 ++++++++++++++++++++++--
>>   hmp.c           |   10 ++++++++++
>>   hmp.h           |    1 +
>>   3 files changed, 33 insertions(+), 2 deletions(-)
>>
>> diff --git a/hmp-commands.hx b/hmp-commands.hx
>> index 396691a..65fb94d 100644
>> --- a/hmp-commands.hx
>> +++ b/hmp-commands.hx
>> @@ -1025,8 +1025,7 @@ ETEXI
>>                         "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"
>> +                      "be created in that format.\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.",
>> @@ -1040,6 +1039,27 @@ Snapshot device, using snapshot file as target if provided
>>   ETEXI
>>
>>       {
>> +        .name       = "snapshot_internal_blkdev",
> Hmm, Just a boring naming question:
>
> snapshot_blkdev reads like a verb-object phrase "snapshot the block
> device", but snapshot_internal_blkdev is becoming awkward: "snapshot a
> internal block device"?
   Actually in my draft it is named as snapshot_blkdev_internal, but
seems not so good also. I am short of English words, how about
snapshot_internally_blkdev?


>
>> +        .args_type  = "device:B,name:s?",
>> +        .params     = "device [name]",
>> +        .help       = "take an internal snapshot of device.\n\t\t\t"
>> +                      "Name is the snapshot's name, if not specified\n\t\t\t"
>> +                      "qemu will generate one according to time.\n\t\t\t"
>> +                      "If any internal snapshot matching name already\n\t\t\t"
>> +                      "exist, or the name is an numeric string,\n\t\t\t"
>> +                      "the operation will fail.\n\t\t\t"
>> +                      "The format of the image used by device must\n\t\t\t"
>> +                      "support it, such as qcow2.\n\t\t\t",
>> +        .mhandler.cmd = hmp_snapshot_internal_blkdev,
>> +    },
>> +
>> +STEXI
>> +@item snapshot_internal_blkdev
>> +@findex snapshot_internal_blkdev
>> +Take an internal snapshot on device if it support
>> +ETEXI
>> +
>> +    {
>>           .name       = "drive_mirror",
>>           .args_type  = "reuse:-n,full:-f,device:B,target:s,format:s?",
>>           .params     = "[-n] [-f] device target [format]",
>> diff --git a/hmp.c b/hmp.c
>> index 494a9aa..810e4c5 100644
>> --- a/hmp.c
>> +++ b/hmp.c
>> @@ -912,6 +912,16 @@ void hmp_snapshot_blkdev(Monitor *mon, const QDict *qdict)
>>       hmp_handle_error(mon, &errp);
>>   }
>>
>> +void hmp_snapshot_internal_blkdev(Monitor *mon, const QDict *qdict)
>> +{
>> +    const char *device = qdict_get_str(qdict, "device");
>> +    const char *name = qdict_get_try_str(qdict, "name");
>> +    Error *errp = NULL;
>> +
>> +    qmp_blockdev_snapshot_internal_sync(device, !!name, name, &errp);
>> +    hmp_handle_error(mon, &errp);
>> +}
>> +
>>   void hmp_migrate_cancel(Monitor *mon, const QDict *qdict)
>>   {
>>       qmp_migrate_cancel(NULL);
>> diff --git a/hmp.h b/hmp.h
>> index 56d2e92..e7593e9 100644
>> --- a/hmp.h
>> +++ b/hmp.h
>> @@ -54,6 +54,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_internal_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);
>> --
>> 1.7.1
>>
>>
>>
>


-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH 10/11] hmp: add interface hmp_snapshot_internal_blkdev

[Fam Zheng]

On Sun, 06/09 10:39, Wenchao Xia wrote:
> 于 2013-6-8 16:21, Fam Zheng 写道:
> >On Sat, 06/08 14:58, Wenchao Xia wrote:
> >>Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
> >>---
> >>  hmp-commands.hx |   24 ++++++++++++++++++++++--
> >>  hmp.c           |   10 ++++++++++
> >>  hmp.h           |    1 +
> >>  3 files changed, 33 insertions(+), 2 deletions(-)
> >>
> >>diff --git a/hmp-commands.hx b/hmp-commands.hx
> >>index 396691a..65fb94d 100644
> >>--- a/hmp-commands.hx
> >>+++ b/hmp-commands.hx
> >>@@ -1025,8 +1025,7 @@ ETEXI
> >>                        "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"
> >>+                      "be created in that format.\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.",
> >>@@ -1040,6 +1039,27 @@ Snapshot device, using snapshot file as target if provided
> >>  ETEXI
> >>
> >>      {
> >>+        .name       = "snapshot_internal_blkdev",
> >Hmm, Just a boring naming question:
> >
> >snapshot_blkdev reads like a verb-object phrase "snapshot the block
> >device", but snapshot_internal_blkdev is becoming awkward: "snapshot a
> >internal block device"?
>   Actually in my draft it is named as snapshot_blkdev_internal, but
> seems not so good also. I am short of English words, how about
> snapshot_internally_blkdev?
> 

I prefer snapshot_blkdev_internal, personally.

-- 
Fam

Re: [Qemu-devel] [PATCH 04/11] snapshot: new function bdrv_snapshot_find_by_id_and_name()

[Stefan Hajnoczi]

On Sat, Jun 08, 2013 at 02:58:00PM +0800, Wenchao Xia wrote:
> +    if (id && name) {
> +        for (i = 0; i < nb_sns; i++) {
> +            sn = &sn_tab[i];
> +            if (!strcmp(sn->id_str, id) && !strcmp(sn->name, name)) {
> +                *sn_info = *sn;
> +                ret = true;
> +                break;
> +            }
> +        }
> +    } else if (id) {
> +        for (i = 0; i < nb_sns; i++) {
> +            sn = &sn_tab[i];
> +            if (!strcmp(sn->id_str, id)) {
> +                *sn_info = *sn;
> +                ret = true;
> +                break;
> +            }
> +        }
> +    } else if (name) {
> +        for (i = 0; i < nb_sns; i++) {
> +            sn = &sn_tab[i];
> +            if (!strcmp(sn->name, name)) {
> +                *sn_info = *sn;
> +                ret = true;
> +                break;
> +            }
> +        }
> +    } else {
> +        /* program error */
> +        abort();
> +    }

If you respin, this would be a little clearer:

assert(id || name);

if (id && name) {
    ...
} else if (id) {
    ...
} else if (name) {
    ...
}

The advantage is that the assert(3) condition is included in the error
message that gets printed.

Stefan

Re: [Qemu-devel] [PATCH 05/11] snapshot: add paired functions for internal snapshot id and name

[Stefan Hajnoczi]

On Sat, Jun 08, 2013 at 02:58:01PM +0800, Wenchao Xia wrote:
> +/*
> + * Every internal snapshot have an ID used by qemu block layer, this function
> + * check whether name used by user mess up with ID. An empty string is also
> + * invalid.
> + */
> +bool snapshot_name_wellformed(const char *name)
> +{
> +    char *p;
> +    /* variable v is used to remove gcc warning of "ignoring return value" and
> +       "set but not used" */
> +    unsigned long v;
> +
> +    if (*name == '\0') {
> +        return false;
> +    }
> +
> +    v = strtoul(name, &p, 10);
> +    v++;
> +
> +    if (*p == '\0') {
> +        /* Numeric string */
> +        return false;
> +    }
> +    return true;
> +}

Shorter function with the same behavior and a rewritten doc comment:

/*
 * Return true if the given internal snapshot name is valid, false
 * otherwise.
 *
 * To prevent clashes with internal snapshot IDs, names consisting only
 * of digits are rejected.  Empty strings are also rejected.
 */
bool snapshot_name_wellformed(const char *name)
{
    return strspn(name, "0123456789") != strlen(name);
}

> +
> +/* Following function generate id string, used by block driver, such as qcow2.
> +   Since no better place to go, place the funtion here for now. */
> +void snapshot_id_string_generate(int id, char *id_str, int id_str_size)
> +{
> +    snprintf(id_str, id_str_size, "%d", id);
> +}

Since the caller has to manage id, this function doesn't really abstract
anything.  I would keep the snprintf() inline, there's only one caller.

Re: [Qemu-devel] [PATCH 06/11] snapshot: distinguish id and name in snapshot delete

[Stefan Hajnoczi]

On Sat, Jun 08, 2013 at 02:58:02PM +0800, Wenchao Xia wrote:
>  static int find_snapshot_by_id_or_name(BlockDriverState *bs, const char *name)

I suggest renaming the argument to make it less confusing: const char *name_or_id

>  {
> -    BDRVQcowState *s = bs->opaque;
> -    int i, ret;
> +    int ret;
>  
> -    ret = find_snapshot_by_id(bs, name);
> +    ret = find_snapshot_by_id_and_name(bs, name, NULL);
>      if (ret >= 0)
>          return ret;

Since you're touching the other lines in this function you could add {}.

> -    for(i = 0; i < s->nb_snapshots; i++) {
> -        if (!strcmp(s->snapshots[i].name, name))
> -            return i;
> -    }
> -    return -1;
> +    return find_snapshot_by_id_and_name(bs, NULL, name);
>  }
>  
>  /* if no id is provided, a new one is constructed */
> @@ -333,7 +347,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
>      }
>  
>      /* Check that the ID is unique */
> -    if (find_snapshot_by_id(bs, sn_info->id_str) >= 0) {
> +    if (find_snapshot_by_id_and_name(bs, sn_info->id_str, NULL) >= 0) {
>          return -EEXIST;
>      }
>  
> @@ -530,15 +544,21 @@ fail:
>      return ret;
>  }
>  
> -int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
> +int qcow2_snapshot_delete(BlockDriverState *bs,
> +                          const char *snapshot_id,
> +                          const char *name,
> +                          Error **errp)

This patch will fail to compile.  You haven't changed the
.bdrv_snapshot_delete() prototype.

Please make sure every patch compiles.

>  {
>      BDRVQcowState *s = bs->opaque;
>      QCowSnapshot sn;
>      int snapshot_index, ret;
>  
>      /* Search the snapshot */
> -    snapshot_index = find_snapshot_by_id_or_name(bs, snapshot_id);
> +    snapshot_index = find_snapshot_by_id_and_name(bs, snapshot_id, name);
>      if (snapshot_index < 0) {
> +        error_setg(errp,
> +                   "Can't find a snapshot with ID %s and name %s",
> +                   snapshot_id, name);

Are both snapshot_id and name guaranteed to be non-NULL here?  It is
dangerous to pass NULL strings to sprintf() functions.  IIRC some libc
implementations will crash, others will print "(null)" like Linux.

Re: [Qemu-devel] [PATCH 04/11] snapshot: new function bdrv_snapshot_find_by_id_and_name()

[Wenchao Xia]

于 2013-6-11 16:26, Stefan Hajnoczi 写道:
> On Sat, Jun 08, 2013 at 02:58:00PM +0800, Wenchao Xia wrote:
>> +    if (id && name) {
>> +        for (i = 0; i < nb_sns; i++) {
>> +            sn = &sn_tab[i];
>> +            if (!strcmp(sn->id_str, id) && !strcmp(sn->name, name)) {
>> +                *sn_info = *sn;
>> +                ret = true;
>> +                break;
>> +            }
>> +        }
>> +    } else if (id) {
>> +        for (i = 0; i < nb_sns; i++) {
>> +            sn = &sn_tab[i];
>> +            if (!strcmp(sn->id_str, id)) {
>> +                *sn_info = *sn;
>> +                ret = true;
>> +                break;
>> +            }
>> +        }
>> +    } else if (name) {
>> +        for (i = 0; i < nb_sns; i++) {
>> +            sn = &sn_tab[i];
>> +            if (!strcmp(sn->name, name)) {
>> +                *sn_info = *sn;
>> +                ret = true;
>> +                break;
>> +            }
>> +        }
>> +    } else {
>> +        /* program error */
>> +        abort();
>> +    }
>
> If you respin, this would be a little clearer:
>
> assert(id || name);
>
> if (id && name) {
>      ...
> } else if (id) {
>      ...
> } else if (name) {
>      ...
> }
>
> The advantage is that the assert(3) condition is included in the error
> message that gets printed.
>
> Stefan
>
   OK, will do it in next version.

-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH 05/11] snapshot: add paired functions for internal snapshot id and name

[Wenchao Xia]

于 2013-6-11 17:14, Stefan Hajnoczi 写道:
> On Sat, Jun 08, 2013 at 02:58:01PM +0800, Wenchao Xia wrote:
>> +/*
>> + * Every internal snapshot have an ID used by qemu block layer, this function
>> + * check whether name used by user mess up with ID. An empty string is also
>> + * invalid.
>> + */
>> +bool snapshot_name_wellformed(const char *name)
>> +{
>> +    char *p;
>> +    /* variable v is used to remove gcc warning of "ignoring return value" and
>> +       "set but not used" */
>> +    unsigned long v;
>> +
>> +    if (*name == '\0') {
>> +        return false;
>> +    }
>> +
>> +    v = strtoul(name, &p, 10);
>> +    v++;
>> +
>> +    if (*p == '\0') {
>> +        /* Numeric string */
>> +        return false;
>> +    }
>> +    return true;
>> +}
>
> Shorter function with the same behavior and a rewritten doc comment:
>
> /*
>   * Return true if the given internal snapshot name is valid, false
>   * otherwise.
>   *
>   * To prevent clashes with internal snapshot IDs, names consisting only
>   * of digits are rejected.  Empty strings are also rejected.
>   */
> bool snapshot_name_wellformed(const char *name)
> {
>      return strspn(name, "0123456789") != strlen(name);
> }
>
   much nicer, will use it, thanks!

>> +
>> +/* Following function generate id string, used by block driver, such as qcow2.
>> +   Since no better place to go, place the funtion here for now. */
>> +void snapshot_id_string_generate(int id, char *id_str, int id_str_size)
>> +{
>> +    snprintf(id_str, id_str_size, "%d", id);
>> +}
>
> Since the caller has to manage id, this function doesn't really abstract
> anything.  I would keep the snprintf() inline, there's only one caller.
>
   Its purpose is move the id rules from one implemention(qcow2) into
generic. If not, it would be a question why snapshot_name_wellformed()
could make sure name not conflict with ID, then reader find the answer
in qcow2...
   I think at least a document is needed about how should implemention
under ./block generate snapshot ID.

-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH 06/11] snapshot: distinguish id and name in snapshot delete

[Wenchao Xia]

于 2013-6-11 17:25, Stefan Hajnoczi 写道:
> On Sat, Jun 08, 2013 at 02:58:02PM +0800, Wenchao Xia wrote:
>>   static int find_snapshot_by_id_or_name(BlockDriverState *bs, const char *name)
>
> I suggest renaming the argument to make it less confusing: const char *name_or_id
>
   will rename it.

>>   {
>> -    BDRVQcowState *s = bs->opaque;
>> -    int i, ret;
>> +    int ret;
>>
>> -    ret = find_snapshot_by_id(bs, name);
>> +    ret = find_snapshot_by_id_and_name(bs, name, NULL);
>>       if (ret >= 0)
>>           return ret;
>
> Since you're touching the other lines in this function you could add {}.
>
  OK.

>> -    for(i = 0; i < s->nb_snapshots; i++) {
>> -        if (!strcmp(s->snapshots[i].name, name))
>> -            return i;
>> -    }
>> -    return -1;
>> +    return find_snapshot_by_id_and_name(bs, NULL, name);
>>   }
>>
>>   /* if no id is provided, a new one is constructed */
>> @@ -333,7 +347,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
>>       }
>>
>>       /* Check that the ID is unique */
>> -    if (find_snapshot_by_id(bs, sn_info->id_str) >= 0) {
>> +    if (find_snapshot_by_id_and_name(bs, sn_info->id_str, NULL) >= 0) {
>>           return -EEXIST;
>>       }
>>
>> @@ -530,15 +544,21 @@ fail:
>>       return ret;
>>   }
>>
>> -int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
>> +int qcow2_snapshot_delete(BlockDriverState *bs,
>> +                          const char *snapshot_id,
>> +                          const char *name,
>> +                          Error **errp)
>
> This patch will fail to compile.  You haven't changed the
> .bdrv_snapshot_delete() prototype.
>
> Please make sure every patch compiles.
>
   shouldn't following line in this patch changed the prototype?

--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -157,7 +157,10 @@ struct BlockDriver {
                                  QEMUSnapshotInfo *sn_info);
      int (*bdrv_snapshot_goto)(BlockDriverState *bs,
                                const char *snapshot_id);
-    int (*bdrv_snapshot_delete)(BlockDriverState *bs, const char 
*snapshot_id);
+    int (*bdrv_snapshot_delete)(BlockDriverState *bs,
+                                const char *snapshot_id,
+                                const char *name,
+                                Error **errp);

   let me retry.

>>   {
>>       BDRVQcowState *s = bs->opaque;
>>       QCowSnapshot sn;
>>       int snapshot_index, ret;
>>
>>       /* Search the snapshot */
>> -    snapshot_index = find_snapshot_by_id_or_name(bs, snapshot_id);
>> +    snapshot_index = find_snapshot_by_id_and_name(bs, snapshot_id, name);
>>       if (snapshot_index < 0) {
>> +        error_setg(errp,
>> +                   "Can't find a snapshot with ID %s and name %s",
>> +                   snapshot_id, name);
>
> Are both snapshot_id and name guaranteed to be non-NULL here?  It is
> dangerous to pass NULL strings to sprintf() functions.  IIRC some libc
> implementations will crash, others will print "(null)" like Linux.
>
   OK, will check it.


-- 
Best Regards

Wenchao Xia

Re: [Qemu-devel] [PATCH 05/11] snapshot: add paired functions for internal snapshot id and name

[Stefan Hajnoczi]

On Thu, Jun 13, 2013 at 01:33:29PM +0800, Wenchao Xia wrote:
> 于 2013-6-11 17:14, Stefan Hajnoczi 写道:
> >On Sat, Jun 08, 2013 at 02:58:01PM +0800, Wenchao Xia wrote:
> >>+
> >>+/* Following function generate id string, used by block driver, such as qcow2.
> >>+   Since no better place to go, place the funtion here for now. */
> >>+void snapshot_id_string_generate(int id, char *id_str, int id_str_size)
> >>+{
> >>+    snprintf(id_str, id_str_size, "%d", id);
> >>+}
> >
> >Since the caller has to manage id, this function doesn't really abstract
> >anything.  I would keep the snprintf() inline, there's only one caller.
> >
>   Its purpose is move the id rules from one implemention(qcow2) into
> generic. If not, it would be a question why snapshot_name_wellformed()
> could make sure name not conflict with ID, then reader find the answer
> in qcow2...
>   I think at least a document is needed about how should implemention
> under ./block generate snapshot ID.

I see your point.  Maybe keep the function.  I was not sure because the
caller still has the int id and has to increment it.  Therefore it
doesn't fully handle id generation.

Stefan