From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=46960 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Oepog-0002X2-RW for qemu-devel@nongnu.org; Fri, 30 Jul 2010 09:40:04 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1Oepnk-0001Ri-63 for qemu-devel@nongnu.org; Fri, 30 Jul 2010 09:39:05 -0400 Received: from mx1.redhat.com ([209.132.183.28]:41059) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1Oepnj-0001RO-Ux for qemu-devel@nongnu.org; Fri, 30 Jul 2010 09:39:04 -0400 Date: Fri, 30 Jul 2010 10:38:50 -0300 From: Luiz Capitulino Subject: Re: [Qemu-devel] [PATCH 1/3] cleanup: bdrv_snaphost_find() returns zero or -ENOENT Message-ID: <20100730103850.5ab26ee2@redhat.com> In-Reply-To: References: <1280345424-12918-1-git-send-email-miguel.filho@gmail.com> <1280345424-12918-2-git-send-email-miguel.filho@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: kwolf@redhat.com, Miguel Di Ciurcio Filho , qemu-devel@nongnu.org On Fri, 30 Jul 2010 11:44:26 +0200 Markus Armbruster wrote: > Miguel Di Ciurcio Filho 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 > > --- > > 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); > } >