qemu-devel
[Top][All Lists]
Advanced

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

Re: [Qemu-devel] [PATCH v2 03/12] savevm: update bdrv_snapshot_find() to


From: Pavel Hrdina
Subject: Re: [Qemu-devel] [PATCH v2 03/12] savevm: update bdrv_snapshot_find() to find snapshot by id or name and add error parameter
Date: Thu, 25 Apr 2013 08:46:19 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130402 Thunderbird/17.0.5

On 24.4.2013 23:26, Eric Blake wrote:
On 04/24/2013 09:32 AM, Pavel Hrdina wrote:
Finding snapshot by a name which could also be an id isn't best way
how to do it. There will be rewrite of savevm, loadvm and delvm to
improve the behavior of these commands. The savevm and loadvm will
have their own patch series.

Now bdrv_snapshot_find takes more parameters. The name parameter will
be matched only against the name of the snapshot and the same applies
to id parameter.

There is one exception. If you set the last parameter, the name parameter
will be matched against the name or the id of a snapshot. This exception
is only for backward compatibility for other commands and it will be
dropped after all commands will be rewritten.

We only need to know if that snapshot exists or not. We don't care
about any error message. If snapshot exists it returns TRUE otherwise
it returns FALSE.

There is also new Error parameter which will containt error messeage if

double-typo and grammar:
s/also/also a/
s/containt error messeage/contain the error message/

something goes wrong.

Signed-off-by: Pavel Hrdina <address@hidden>
---
  savevm.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++------------------
  1 file changed, 67 insertions(+), 26 deletions(-)

diff --git a/savevm.c b/savevm.c
index ba97c41..1622c55 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2262,26 +2262,66 @@ out:
      return ret;
  }

-static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
-                              const char *name)
+static bool bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
+                               const char *name, const char *id, Error **errp,
+                               bool old_match)

I'd add a FIXME comment here documenting that you intend to remove the
old_match parameter after all callers have been updated to the new
semantics.

  {
      QEMUSnapshotInfo *sn_tab, *sn;
-    int nb_sns, i, ret;
+    int nb_sns, i;
+    bool found = false;

Bikeshedding: I don't think you need this variable, if you would instead
do...

+
+    assert(name || id);

-    ret = -ENOENT;
      nb_sns = bdrv_snapshot_list(bs, &sn_tab);
-    if (nb_sns < 0)
-        return ret;
-    for(i = 0; i < nb_sns; i++) {
+    if (nb_sns < 0) {
+        error_setg_errno(errp, -nb_sns, "Failed to get a snapshot list");
+        return found;

return false;

+    }
+
+    if (nb_sns == 0) {
+        error_setg(errp, "Device has no snapshots");
+        return found;

return false;

+    }

*sn_info = NULL;

You cannot do that. It should be sn_info = NULL, but if you look at the usage of the bdrv_snapshot_find you will see that you cannot do that too. And the same applies ...


+
+    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 (name && id) {
+            if (!strcmp(sn->id_str, id) && !strcmp(sn->name, name)) {
+                *sn_info = *sn;
+                found = true;

Drop this assignment and others like it...

+                break;
+            }
+        } else if (name) {
+            /* for compatibility for old bdrv_snapshot_find call
+             * will be removed */
+            if (old_match) {
+                if (!strcmp(sn->id_str, name) || !strcmp(sn->name, name)) {
+                    *sn_info = *sn;
+                    found = true;
+                    break;
+                }
+            } else {
+                if (!strcmp(sn->name, name)) {
+                    *sn_info = *sn;
+                    found = true;
+                    break;
+                }
+            }
+        } else if (id) {
+            if (!strcmp(sn->id_str, id)) {
+                *sn_info = *sn;
+                found = true;
+                break;
+            }
          }
      }
+
+    if (!found) {

use 'if (*sn_info)'

to this usage and ...


+        error_setg(errp, "Failed to find snapshot '%s'", name ? name : id);
+    }
+
      g_free(sn_tab);
-    return ret;
+    return found;

return *sn_info != NULL;

this one.


  }

If you _do_ decide to keep the boolean variable instead of hard-coding a
false return and avoiding redundancy by using other variables to
determine the result, then at least s/found/ret/, because I find 'return
found' as a way to intentionally fail rather odd-looking.

At any rate, I can live with this logic, and all the conversions of
existing call sites properly passed the given name, NULL id, and true
for old_match semantics; along with optional deciding whether to pass
NULL or a local error based on whether it would ignore lookup failure or
propagate it as a failure of the higher-level operation that needed a
lookup.


You are right that avoiding redundancy is better and I just think up a new solution.

static QEMUSnapshotInfo *bdrv_snapshot_find(BlockDriverState *bs,
                                            const char *name,
                                            const char *id,
                                            Error **errp,
                                            bool old_match /*FIXME*/)

And the bdrv_snapshot_find will return QEMUSnapshotInfo* on success and
on error it will set the error message and also return NULL.

Pavel





reply via email to

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