Re: [Qemu-devel] [PATCH 1/3] cleanup: bdrv_snaphost_find() returns zero or -ENOENT

[Luiz Capitulino <lcapitulino@redhat.com>]

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