Patchwork [03/11] savevm: update bdrv_snapshot_find() to find snapshot by id or name

login
register
mail settings
Submitter Pavel Hrdina
Date April 16, 2013, 4:05 p.m.
Message ID <17626585faf740878394cb2cbdc733cfb0c535e8.1366127809.git.phrdina@redhat.com>
Download mbox | patch
Permalink /patch/237018/
State New
Headers show

Comments

Pavel Hrdina - April 16, 2013, 4:05 p.m.
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 1 otherwise
it returns 0.

Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
---
 savevm.c | 73 ++++++++++++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 50 insertions(+), 23 deletions(-)
Eric Blake - April 16, 2013, 5:34 p.m.
On 04/16/2013 10:05 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.

Fair enough, even if it is a bit of churn on the signature and callers.

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

If you are only returning a yes/no result, bool is better than int.

> 
> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>  savevm.c | 73 ++++++++++++++++++++++++++++++++++++++++++++--------------------
>  1 file changed, 50 insertions(+), 23 deletions(-)
> 
> diff --git a/savevm.c b/savevm.c
> index 6af84fd..96a2340 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -2197,25 +2197,55 @@ out:
>  }
>  
>  static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,

Return bool...

> -                              const char *name)
> +                              const char *name, const char *id, bool old_match)
>  {
>      QEMUSnapshotInfo *sn_tab, *sn;
> -    int nb_sns, i, ret;
> +    int nb_sns, i, found = 0;

... and initialize 'bool found = false'.

> +
> +    if (!name && !id) {
> +        return found;
> +    }

It may be appropriate to do assert(name || id), if you want to guarantee
that all callers supply at least one of the two arguments (and that the
QMP code where both arguments are optional does its own sanity checking
that the user supplied an argument.

>  
> -    ret = -ENOENT;
>      nb_sns = bdrv_snapshot_list(bs, &sn_tab);
> -    if (nb_sns < 0)
> -        return ret;
> +    if (nb_sns < 0) {
> +        return found;

Calling the return value 'found' makes the code read awkwardly when
reporting failure; maybe keeping it named 'ret' is better after all.

> +    }
> +
>      for(i = 0; i < nb_sns; i++) {

As long as you are touching this code, you could insert the space after
'for'.

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

Note that this says that if both name and id are specified, then that
takes priority, no matter what was specified for old_match.

>          }
>      }
> +
>      g_free(sn_tab);
> -    return ret;
> +    return found;
>  }
>  
>  /*
> @@ -2229,8 +2259,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)
> +        if (bdrv_can_snapshot(bs)
> +                && bdrv_snapshot_find(bs, snapshot, name, name, true))

Needless style change.  The old style was sufficient, with:

    if (bdrv_can_snapshot(bs) &&
        bdrv_snapshot_find(bs, snapshot, name, name, true))

Except that bdrv_snapshot_find(bs, snapshot, name, name, anything)
results in a lookup that requires sn->id_str and sn->name be the same
string, which is not what you intended.

You really want to be using bdrv_snapshot_find(bs, snapshot, NULL, name,
true), if you want the old_match parameter to make a difference.

...
> @@ -2474,7 +2501,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];
> @@ -2505,8 +2532,8 @@ 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) {
> +                if (!bdrv_snapshot_find(bs1, sn_info, sn->name, sn->id_str,
> +                                        true)) {

This was the only conversion that passed both id and name that looked
correct.  All the other conversions to 'name, name, true' need fixing.
Kevin Wolf - April 18, 2013, 1:17 p.m.
Am 16.04.2013 um 18:05 hat Pavel Hrdina geschrieben:
> 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 1 otherwise
> it returns 0.

Sounds like it should be true/false rather than 1/0.

Also, isn't there a difference between "there is no snapshot foo" and "we
couldn't find out if there is a snapshot foo because there was an
error"?

> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
> ---
>  savevm.c | 73 ++++++++++++++++++++++++++++++++++++++++++++--------------------
>  1 file changed, 50 insertions(+), 23 deletions(-)
> 
> diff --git a/savevm.c b/savevm.c
> index 6af84fd..96a2340 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -2197,25 +2197,55 @@ out:
>  }
>  
>  static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
> -                              const char *name)
> +                              const char *name, const char *id, bool old_match)
>  {
>      QEMUSnapshotInfo *sn_tab, *sn;
> -    int nb_sns, i, ret;
> +    int nb_sns, i, found = 0;
> +
> +    if (!name && !id) {
> +        return found;
> +    }

Why not just an explicit return 0? Or actually, while you touch the
prototype you could make the function return a bool and return false.

>  
> -    ret = -ENOENT;
>      nb_sns = bdrv_snapshot_list(bs, &sn_tab);
> -    if (nb_sns < 0)
> -        return ret;
> +    if (nb_sns < 0) {
> +        return found;
> +    }

Same here.

> +
>      for(i = 0; i < nb_sns; i++) {

You can add a space after 'for' while touching the function.

>          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 = 1;
> +                break;
> +            }
> +        } else if (name) {
> +            /* for compatibility for old bdrv_snapshot_find call
> +             * will be removed */
> +            if (old_match) {
> +                if (!strcmp(sn->id_str, id) || !strcmp(sn->name, name)) {
> +                    *sn_info = *sn;
> +                    found = 1;
> +                    break;
> +                }
> +            } else {
> +                if (!strcmp(sn->name, name)) {
> +                    *sn_info = *sn;
> +                    found = 1;
> +                    break;
> +                }
> +            }
> +        } else if (id) {
> +            if (!strcmp(sn->id_str, id)) {
> +                *sn_info = *sn;
> +                found = 1;
> +                break;
> +            }
>          }
>      }
> +
>      g_free(sn_tab);
> -    return ret;
> +    return found;
>  }
>  
>  /*
> @@ -2229,8 +2259,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)
> +        if (bdrv_can_snapshot(bs)
> +                && bdrv_snapshot_find(bs, snapshot, name, name, true))

It would only retain the old behaviour if you specified NULL for id.
Now _both_ name and id must match, which will happen almost never.

How did you test this patch? Sounds like something that a qemu-iotests
case using the Python QMP methods could easily test.

>          {
>              bdrv_snapshot_delete(bs, name, &local_err);
>              if (error_is_set(&local_err)) {
> @@ -2289,8 +2319,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);
> -        if (ret >= 0) {
> +        if (bdrv_snapshot_find(bs, old_sn, name, name, true)) {

Same problem.

>              pstrcpy(sn->name, sizeof(sn->name), old_sn->name);
>              pstrcpy(sn->id_str, sizeof(sn->id_str), old_sn->id_str);
>          } else {
> @@ -2380,9 +2409,8 @@ 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;
> +    if (!bdrv_snapshot_find(bs_vm_state, &sn, name, name, true)) {

And again.

> +        return -ENOENT;

We're losing the real error message here. The old code said exactly what
happened, the new one always claims that the snapshot doesn't exist even
in case of I/O errors or anything.

>      } else if (sn.vm_state_size == 0) {
>          error_report("This is a disk-only snapshot. Revert to it offline "
>              "using qemu-img.");
> @@ -2404,11 +2432,10 @@ int load_vmstate(const char *name)
>              return -ENOTSUP;
>          }
>  
> -        ret = bdrv_snapshot_find(bs, &sn, name);
> -        if (ret < 0) {
> +        if (!bdrv_snapshot_find(bs, &sn, name, name, true)) {
>              error_report("Device '%s' does not have the requested snapshot '%s'",
>                             bdrv_get_device_name(bs), name);
> -            return ret;
> +            return -ENOENT;
>          }
>      }
>  
> @@ -2474,7 +2501,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];
> @@ -2505,8 +2532,8 @@ 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) {
> +                if (!bdrv_snapshot_find(bs1, sn_info, sn->name, sn->id_str,
> +                                        true)) {

Is it intention that both id and name must match here? I suspect that no
because you set old_match == true, which is however ignored because both
id and name are set.

>                      available = 0;
>                      break;
>                  }

Kevin

Patch

diff --git a/savevm.c b/savevm.c
index 6af84fd..96a2340 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2197,25 +2197,55 @@  out:
 }
 
 static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
-                              const char *name)
+                              const char *name, const char *id, bool old_match)
 {
     QEMUSnapshotInfo *sn_tab, *sn;
-    int nb_sns, i, ret;
+    int nb_sns, i, found = 0;
+
+    if (!name && !id) {
+        return found;
+    }
 
-    ret = -ENOENT;
     nb_sns = bdrv_snapshot_list(bs, &sn_tab);
-    if (nb_sns < 0)
-        return ret;
+    if (nb_sns < 0) {
+        return found;
+    }
+
     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 = 1;
+                break;
+            }
+        } else if (name) {
+            /* for compatibility for old bdrv_snapshot_find call
+             * will be removed */
+            if (old_match) {
+                if (!strcmp(sn->id_str, id) || !strcmp(sn->name, name)) {
+                    *sn_info = *sn;
+                    found = 1;
+                    break;
+                }
+            } else {
+                if (!strcmp(sn->name, name)) {
+                    *sn_info = *sn;
+                    found = 1;
+                    break;
+                }
+            }
+        } else if (id) {
+            if (!strcmp(sn->id_str, id)) {
+                *sn_info = *sn;
+                found = 1;
+                break;
+            }
         }
     }
+
     g_free(sn_tab);
-    return ret;
+    return found;
 }
 
 /*
@@ -2229,8 +2259,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)
+        if (bdrv_can_snapshot(bs)
+                && bdrv_snapshot_find(bs, snapshot, name, name, true))
         {
             bdrv_snapshot_delete(bs, name, &local_err);
             if (error_is_set(&local_err)) {
@@ -2289,8 +2319,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);
-        if (ret >= 0) {
+        if (bdrv_snapshot_find(bs, old_sn, name, name, true)) {
             pstrcpy(sn->name, sizeof(sn->name), old_sn->name);
             pstrcpy(sn->id_str, sizeof(sn->id_str), old_sn->id_str);
         } else {
@@ -2380,9 +2409,8 @@  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;
+    if (!bdrv_snapshot_find(bs_vm_state, &sn, name, name, true)) {
+        return -ENOENT;
     } else if (sn.vm_state_size == 0) {
         error_report("This is a disk-only snapshot. Revert to it offline "
             "using qemu-img.");
@@ -2404,11 +2432,10 @@  int load_vmstate(const char *name)
             return -ENOTSUP;
         }
 
-        ret = bdrv_snapshot_find(bs, &sn, name);
-        if (ret < 0) {
+        if (!bdrv_snapshot_find(bs, &sn, name, name, true)) {
             error_report("Device '%s' does not have the requested snapshot '%s'",
                            bdrv_get_device_name(bs), name);
-            return ret;
+            return -ENOENT;
         }
     }
 
@@ -2474,7 +2501,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];
@@ -2505,8 +2532,8 @@  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) {
+                if (!bdrv_snapshot_find(bs1, sn_info, sn->name, sn->id_str,
+                                        true)) {
                     available = 0;
                     break;
                 }