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

Submitted by Miguel Di Ciurcio Filho on July 28, 2010, 7:30 p.m.

Details

Message ID 1280345424-12918-2-git-send-email-miguel.filho@gmail.com
State New
Headers show

Commit Message

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(-)

Comments

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 hide | download patch | download mbox

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);