Patchwork [V12,02/18] block: distinguish id and name in bdrv_find_snapshot()

login
register
mail settings
Submitter Wayne Xia
Date April 13, 2013, 8:56 a.m.
Message ID <1365843407-16504-3-git-send-email-xiawenc@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/236321/
State New
Headers show

Comments

Wayne Xia - April 13, 2013, 8:56 a.m.
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.
Searching will be done with higher priority of id. This function also
returns negative value from bdrv_snapshot_list() instead of -ENOENT on
error now.
  Note that the logic is changed a bit: now it traverse twice, first search
for id, second for name, but original code traverse only once to search
them at the same time, so matching sequence may be different. As a result,
do_savevm(), del_existing_snapshots(), load_vmsate() may behaviors differently
if there are unwisely chosen name mixed with id. In do_info_snapshots(),
the caller is changed to search id only, which should be the correct behavior.

Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
---
 block/snapshot.c         |   46 ++++++++++++++++++++++++++++++++++++++--------
 include/block/snapshot.h |    2 +-
 savevm.c                 |   10 +++++-----
 3 files changed, 44 insertions(+), 14 deletions(-)
Eric Blake - April 17, 2013, 1:09 a.m.
On 04/13/2013 02:56 AM, Wenchao Xia wrote:
>   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.
> Searching will be done with higher priority of id. This function also
> returns negative value from bdrv_snapshot_list() instead of -ENOENT on
> error now.
>   Note that the logic is changed a bit: now it traverse twice, first search
> for id, second for name, but original code traverse only once to search
> them at the same time, so matching sequence may be different. As a result,
> do_savevm(), del_existing_snapshots(), load_vmsate() may behaviors differently
> if there are unwisely chosen name mixed with id. In do_info_snapshots(),
> the caller is changed to search id only, which should be the correct behavior.

I just realized that you are trying to do the same thing as Pavel, but
that your two implementations differ.

https://lists.gnu.org/archive/html/qemu-devel/2013-04/msg03289.html

Rather than spending my time reviewing two competing versions, it would
be really nice if one of you could rebase patches on top of the other,
and present a unified series containing both of your improvements as a
single series, so that we are only changing bdrv_snapshot_list semantics
once.
Eric Blake - April 17, 2013, 6:50 p.m.
On 04/16/2013 07:09 PM, Eric Blake wrote:
> On 04/13/2013 02:56 AM, Wenchao Xia wrote:
>>   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.
>> Searching will be done with higher priority of id. This function also
>> returns negative value from bdrv_snapshot_list() instead of -ENOENT on
>> error now.
>>   Note that the logic is changed a bit: now it traverse twice, first search
>> for id, second for name, but original code traverse only once to search
>> them at the same time, so matching sequence may be different. As a result,
>> do_savevm(), del_existing_snapshots(), load_vmsate() may behaviors differently
>> if there are unwisely chosen name mixed with id. In do_info_snapshots(),
>> the caller is changed to search id only, which should be the correct behavior.
> 
> I just realized that you are trying to do the same thing as Pavel, but
> that your two implementations differ.
> 
> https://lists.gnu.org/archive/html/qemu-devel/2013-04/msg03289.html
> 
> Rather than spending my time reviewing two competing versions, it would
> be really nice if one of you could rebase patches on top of the other,
> and present a unified series containing both of your improvements as a
> single series, so that we are only changing bdrv_snapshot_list semantics
> once.

I ended up reviewing both implementations, and not fully liking either
of them, to the point that I proposed yet a third version.  I guess we
need Kevin or Markus to speak up now...

https://lists.gnu.org/archive/html/qemu-devel/2013-04/msg03501.html

Patch

diff --git a/block/snapshot.c b/block/snapshot.c
index c47a899..d57d04a 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -24,8 +24,20 @@ 
 
 #include "block/snapshot.h"
 
+/**
+ * Look up an internal snapshot by @id, or else by @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
+ *
+ * If the snapshot with unique ID @id exists, find it.
+ * Else, if snapshots with name @name exists, find one of them.
+ *
+ * Returns: 0 when a snapshot is found, else -errno.
+ */
 int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
-                       const char *name)
+                       const char *id, const char *name)
 {
     QEMUSnapshotInfo *sn_tab, *sn;
     int nb_sns, i, ret;
@@ -33,16 +45,34 @@  int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
     ret = -ENOENT;
     nb_sns = bdrv_snapshot_list(bs, &sn_tab);
     if (nb_sns < 0) {
-        return ret;
+        return nb_sns;
+    }
+
+    /* search by id */
+    if (id) {
+        for (i = 0; i < nb_sns; i++) {
+            sn = &sn_tab[i];
+            if (!strcmp(sn->id_str, id)) {
+                *sn_info = *sn;
+                ret = 0;
+                goto out;
+            }
+        }
     }
-    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;
+
+    /* search by name */
+    if (name) {
+        for (i = 0; i < nb_sns; i++) {
+            sn = &sn_tab[i];
+            if (!strcmp(sn->name, name)) {
+                *sn_info = *sn;
+                ret = 0;
+                goto out;
+            }
         }
     }
+
+ out:
     g_free(sn_tab);
     return ret;
 }
diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index 4ad070c..a047a8e 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -33,5 +33,5 @@ 
 #include "block.h"
 
 int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
-                       const char *name);
+                       const char *id, const char *name);
 #endif
diff --git a/savevm.c b/savevm.c
index 528ba0d..ed6d74c 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2212,7 +2212,7 @@  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, name) >= 0)
         {
             ret = bdrv_snapshot_delete(bs, name);
             if (ret < 0) {
@@ -2272,7 +2272,7 @@  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);
+        ret = bdrv_snapshot_find(bs, old_sn, name, name);
         if (ret >= 0) {
             pstrcpy(sn->name, sizeof(sn->name), old_sn->name);
             pstrcpy(sn->id_str, sizeof(sn->id_str), old_sn->id_str);
@@ -2363,7 +2363,7 @@  int load_vmstate(const char *name)
     }
 
     /* Don't even try to load empty VM states */
-    ret = bdrv_snapshot_find(bs_vm_state, &sn, name);
+    ret = bdrv_snapshot_find(bs_vm_state, &sn, name, name);
     if (ret < 0) {
         return ret;
     } else if (sn.vm_state_size == 0) {
@@ -2387,7 +2387,7 @@  int load_vmstate(const char *name)
             return -ENOTSUP;
         }
 
-        ret = bdrv_snapshot_find(bs, &sn, name);
+        ret = bdrv_snapshot_find(bs, &sn, name, name);
         if (ret < 0) {
             error_report("Device '%s' does not have the requested snapshot '%s'",
                            bdrv_get_device_name(bs), name);
@@ -2493,7 +2493,7 @@  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);
+                ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str, NULL);
                 if (ret < 0) {
                     available = 0;
                     break;