qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 04/11] snapshot: new function bdrv_snapshot_find


From: Wenchao Xia
Subject: Re: [Qemu-devel] [PATCH 04/11] snapshot: new function bdrv_snapshot_find_by_id_and_name()
Date: Sun, 09 Jun 2013 10:33:52 +0800
User-agent: Mozilla/5.0 (Windows NT 5.1; rv:17.0) Gecko/20130509 Thunderbird/17.0.6

于 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 <address@hidden>
Signed-off-by: Pavel Hrdina <address@hidden>
---
  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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]