Patchwork [1/3] cleanup: bdrv_snaphost_find() returns zero or -ENOENT

login
register
mail settings
Submitter Miguel Di Ciurcio Filho
Date July 28, 2010, 7:30 p.m.
Message ID <1280345424-12918-2-git-send-email-miguel.filho@gmail.com>
Download mbox | patch
Permalink /patch/60171/
State New
Headers show

Comments

Miguel Di Ciurcio Filho - July 28, 2010, 7:30 p.m.
The bdrv_snaphost_find() returns zero in case it finds an snapshot or -ENOENT in
case it doesn't.

Checking returning values as >= zero doesn't make sense.

Signed-off-by: Miguel Di Ciurcio Filho <miguel.filho@gmail.com>
---
 savevm.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)
Markus Armbruster - July 30, 2010, 9:44 a.m.
Miguel Di Ciurcio Filho <miguel.filho@gmail.com> writes:

> The bdrv_snaphost_find() returns zero in case it finds an snapshot or -ENOENT in
> case it doesn't.
>
> Checking returning values as >= zero doesn't make sense.

Debatable.  "RETVAL < 0" is an idiomatic check for error.  "RETVAL >= 0"
is merely its negation.

Anyway, I do prefer code like this

    ret = do_something();
    if (ret < 0) {
        handle error...
    }
    do more...

over

    ret = do_something();
    if (ret >= 0) {
        do more...
    }

>
> Signed-off-by: Miguel Di Ciurcio Filho <miguel.filho@gmail.com>
> ---
>  savevm.c |    7 ++++---
>  1 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/savevm.c b/savevm.c
> index 7a1de3c..6c6adb0 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -1768,7 +1768,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) == 0)
>          {
>              ret = bdrv_snapshot_delete(bs, name);
>              if (ret < 0) {
> @@ -1948,8 +1948,9 @@ int load_vmstate(const char *name)
>  
>      /* Don't even try to load empty VM states */
>      ret = bdrv_snapshot_find(bs, &sn, name);
> -    if ((ret >= 0) && (sn.vm_state_size == 0))
> -        return -EINVAL;
> +    if ((ret == 0) && (sn.vm_state_size == 0)) {
> +            return -EINVAL;
> +    }
>  
>      /* restore the VM state */
>      f = qemu_fopen_bdrv(bs, 0);

I wonder what happens if bdrv_snapshot_find() fails.  Why is it safe to
ignore that and continue?


do_savevm() has another one:

        ret = bdrv_snapshot_find(bs, old_sn, 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);
        } else {
            pstrcpy(sn->name, sizeof(sn->name), name);
        }
Luiz Capitulino - July 30, 2010, 1:38 p.m.
On Fri, 30 Jul 2010 11:44:26 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Miguel Di Ciurcio Filho <miguel.filho@gmail.com> writes:
> 
> > The bdrv_snaphost_find() returns zero in case it finds an snapshot or -ENOENT in
> > case it doesn't.
> >
> > Checking returning values as >= zero doesn't make sense.
> 
> Debatable.  "RETVAL < 0" is an idiomatic check for error.  "RETVAL >= 0"
> is merely its negation.

True, but Miguel's change makes the code less confusing, IMHO. I'm always
under the impression that this kind of code is counting on impossible
things (ie. on RETVAL > 0).

Idioms don't have this problem, obviously.

> Anyway, I do prefer code like this
> 
>     ret = do_something();
>     if (ret < 0) {
>         handle error...
>     }
>     do more...
> 
> over
> 
>     ret = do_something();
>     if (ret >= 0) {
>         do more...
>     }

Agreed, not worth fixing now though.

> > Signed-off-by: Miguel Di Ciurcio Filho <miguel.filho@gmail.com>
> > ---
> >  savevm.c |    7 ++++---
> >  1 files changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/savevm.c b/savevm.c
> > index 7a1de3c..6c6adb0 100644
> > --- a/savevm.c
> > +++ b/savevm.c
> > @@ -1768,7 +1768,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) == 0)
> >          {
> >              ret = bdrv_snapshot_delete(bs, name);
> >              if (ret < 0) {
> > @@ -1948,8 +1948,9 @@ int load_vmstate(const char *name)
> >  
> >      /* Don't even try to load empty VM states */
> >      ret = bdrv_snapshot_find(bs, &sn, name);
> > -    if ((ret >= 0) && (sn.vm_state_size == 0))
> > -        return -EINVAL;
> > +    if ((ret == 0) && (sn.vm_state_size == 0)) {
> > +            return -EINVAL;
> > +    }
> >  
> >      /* restore the VM state */
> >      f = qemu_fopen_bdrv(bs, 0);
> 
> I wonder what happens if bdrv_snapshot_find() fails.  Why is it safe to
> ignore that and continue?

Good point, turns out I wondered the same and fixed it in an past
RFC series:

 http://lists.gnu.org/archive/html/qemu-devel/2010-04/msg01321.html

Miguel, I think it's worth going through that series and cherry picking
what makes sense. Of course that you don't have to worry about QError.

> 
> 
> do_savevm() has another one:
> 
>         ret = bdrv_snapshot_find(bs, old_sn, 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);
>         } else {
>             pstrcpy(sn->name, sizeof(sn->name), name);
>         }
>

Patch

diff --git a/savevm.c b/savevm.c
index 7a1de3c..6c6adb0 100644
--- a/savevm.c
+++ b/savevm.c
@@ -1768,7 +1768,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) == 0)
         {
             ret = bdrv_snapshot_delete(bs, name);
             if (ret < 0) {
@@ -1948,8 +1948,9 @@  int load_vmstate(const char *name)
 
     /* Don't even try to load empty VM states */
     ret = bdrv_snapshot_find(bs, &sn, name);
-    if ((ret >= 0) && (sn.vm_state_size == 0))
-        return -EINVAL;
+    if ((ret == 0) && (sn.vm_state_size == 0)) {
+            return -EINVAL;
+    }
 
     /* restore the VM state */
     f = qemu_fopen_bdrv(bs, 0);