[Qemu-devel] [PATCH 4/7] block: distinguish id and name in bdrv_find_snapshot()

[Wenchao Xia]

To make it clear about id and name in searching, the API is changed
a bit to distinguish them, and caller can choose to search by id or name.
If not found, *errp will be set to tip why.

Note that the caller logic is changed a bit:
1) In del_existing_snapshots() called by do_savevm(), it travers twice
to find the snapshot, instead once, so matching sequence may change
if there are unwisely chosen, mixed id and names.
2) In do_savevm(), same with del_existing_snapshot(), when it tries to
find the snapshot to overwrite, matching sequence may change for same
reason.
3) In load_vmstate(), first when it tries to find the snapshot to be loaded,
sequence may change for the same reason of above. Later in validation, the
logic is changed to be more strict to require both id and name matching.
4) In do_info_snapshot(), in validation, the logic is changed to be more
strict to require both id and name matching.

Savevm, loadvm logic may need to be improved later, to avoid mixing of them.

Some code is borrowed from Pavel's patch.

Signed-off-by: Wenchao Xia <address@hidden>
Signed-off-by: Pavel Hrdina <address@hidden>
---
 block/snapshot.c         |   72 +++++++++++++++++++++++++++++++++++++++-------
 include/block/snapshot.h |    5 ++-
 savevm.c                 |   35 ++++++++++++----------
 3 files changed, 83 insertions(+), 29 deletions(-)

diff --git a/block/snapshot.c b/block/snapshot.c
index cdc2a76..fa02c4e 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -25,25 +25,75 @@
 #include "block/snapshot.h"
 #include "block/block_int.h"
 
-int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
-                       const char *name)
+/**
+ * Look up an internal snapshot by @id and @name.
+ * @bs: block device to search
+ * @sn_info: location to store information on the snapshot found
+ * @id: unique snapshot ID, or NULL
+ * @name: snapshot name, or NULL
+ * @errp: location to store error
+ *
+ * This function will travers 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, return false.
+ *
+ * Returns: true when a snapshot is found and @sn_info will be filled, false
+ * when error or not found with @errp filled if errp != NULL.
+ */
+bool bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
+                        const char *id, const char *name, Error **errp)
 {
     QEMUSnapshotInfo *sn_tab, *sn;
-    int nb_sns, i, ret;
+    int nb_sns, i;
+    bool ret = false;
 
-    ret = -ENOENT;
     nb_sns = bdrv_snapshot_list(bs, &sn_tab);
     if (nb_sns < 0) {
-        return ret;
+        error_setg_errno(errp, -nb_sns, "Failed to get a snapshot list");
+        return false;
+    } else if (nb_sns == 0) {
+        error_setg(errp, "Device has no snapshots");
+        return false;
     }
-    for (i = 0; i < nb_sns; i++) {
-        sn = &sn_tab[i];
-        if (!strcmp(sn->id_str, name) || !strcmp(sn->name, name)) {
-            *sn_info = *sn;
-            ret = 0;
-            break;
+
+
+    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;
+            }
         }
     }
+
+    if (!ret) {
+        error_setg(errp, "Device have no matching snapshot");
+    }
+
     g_free(sn_tab);
     return ret;
 }
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index 96d4a50..6aac5ed 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 */
@@ -38,8 +39,8 @@ typedef struct QEMUSnapshotInfo {
     uint64_t vm_clock_nsec; /* VM clock relative to boot */
 } QEMUSnapshotInfo;
 
-int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
-                       const char *name);
+bool bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
+                        const char *id, const char *name, Error **errp);
 int bdrv_can_snapshot(BlockDriverState *bs);
 int bdrv_is_snapshot(BlockDriverState *bs);
 int bdrv_snapshot_create(BlockDriverState *bs,
diff --git a/savevm.c b/savevm.c
index ab53a02..5dd2d14 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2286,8 +2286,8 @@ static int del_existing_snapshots(Monitor *mon, const 
char *name)
     bs = NULL;
     while ((bs = bdrv_next(bs))) {
         if (bdrv_can_snapshot(bs) &&
-            bdrv_snapshot_find(bs, snapshot, name) >= 0)
-        {
+            (bdrv_snapshot_find(bs, snapshot, name, NULL, NULL) ||
+             bdrv_snapshot_find(bs, snapshot, NULL, name, NULL))) {
             ret = bdrv_snapshot_delete(bs, name);
             if (ret < 0) {
                 monitor_printf(mon,
@@ -2346,8 +2346,8 @@ void do_savevm(Monitor *mon, const QDict *qdict)
     sn->vm_clock_nsec = qemu_get_clock_ns(vm_clock);
 
     if (name) {
-        ret = bdrv_snapshot_find(bs, old_sn, name);
-        if (ret >= 0) {
+        if (bdrv_snapshot_find(bs, old_sn, name, NULL, NULL) ||
+            bdrv_snapshot_find(bs, old_sn, NULL, name, NULL)) {
             pstrcpy(sn->name, sizeof(sn->name), old_sn->name);
             pstrcpy(sn->id_str, sizeof(sn->id_str), old_sn->id_str);
         } else {
@@ -2437,12 +2437,14 @@ int load_vmstate(const char *name)
     }
 
     /* Don't even try to load empty VM states */
-    ret = bdrv_snapshot_find(bs_vm_state, &sn, name);
-    if (ret < 0) {
-        return ret;
-    } else if (sn.vm_state_size == 0) {
-        error_report("This is a disk-only snapshot. Revert to it offline "
-            "using qemu-img.");
+    if (bdrv_snapshot_find(bs_vm_state, &sn, name, NULL, NULL) ||
+        bdrv_snapshot_find(bs_vm_state, &sn, NULL, name, NULL)) {
+            if (sn.vm_state_size == 0) {
+                error_report("This is a disk-only snapshot. Revert to it "
+                             "offline using qemu-img.");
+                return -EINVAL;
+            }
+    } else {
         return -EINVAL;
     }
 
@@ -2461,11 +2463,11 @@ int load_vmstate(const char *name)
             return -ENOTSUP;
         }
 
-        ret = bdrv_snapshot_find(bs, &sn, name);
-        if (ret < 0) {
+        /* vm snapshot will always have same id and name, check do_savevm(). */
+        if (!bdrv_snapshot_find(bs, &sn, sn.id_str, sn.name, NULL)) {
             error_report("Device '%s' does not have the requested snapshot 
'%s'",
                            bdrv_get_device_name(bs), name);
-            return ret;
+            return -ENOENT;
         }
     }
 
@@ -2536,7 +2538,7 @@ void do_info_snapshots(Monitor *mon, const QDict *qdict)
 {
     BlockDriverState *bs, *bs1;
     QEMUSnapshotInfo *sn_tab, *sn, s, *sn_info = &s;
-    int nb_sns, i, ret, available;
+    int nb_sns, i, available;
     int total;
     int *available_snapshots;
     char buf[256];
@@ -2567,8 +2569,9 @@ void do_info_snapshots(Monitor *mon, const QDict *qdict)
 
         while ((bs1 = bdrv_next(bs1))) {
             if (bdrv_can_snapshot(bs1) && bs1 != bs) {
-                ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str);
-                if (ret < 0) {
+                /* vm snapshot will always have same id and name */
+                if (!bdrv_snapshot_find(bs1, sn_info,
+                                        sn->id_str, sn->name, NULL)) {
                     available = 0;
                     break;
                 }
-- 
1.7.1