| Message ID | 1280345424-12918-2-git-send-email-miguel.filho@gmail.com |
|---|---|
| State | New |
| Headers | show |
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); }
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); > } >
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);
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(-)