From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:46715) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TiMW7-0002Yg-Fm for qemu-devel@nongnu.org; Tue, 11 Dec 2012 04:52:54 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TiMW6-0005qT-71 for qemu-devel@nongnu.org; Tue, 11 Dec 2012 04:52:47 -0500 Received: from mail-wg0-f43.google.com ([74.125.82.43]:46792) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TiMW6-0005q9-0g for qemu-devel@nongnu.org; Tue, 11 Dec 2012 04:52:46 -0500 Received: by mail-wg0-f43.google.com with SMTP id e12so1889614wge.10 for ; Tue, 11 Dec 2012 01:52:45 -0800 (PST) Date: Tue, 11 Dec 2012 10:52:41 +0100 From: Stefan Hajnoczi Message-ID: <20121211095241.GC796@stefanha-thinkpad.muc.redhat.com> References: <1354852190-15095-1-git-send-email-lig.fnst@cn.fujitsu.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1354852190-15095-1-git-send-email-lig.fnst@cn.fujitsu.com> Subject: Re: [Qemu-devel] [ [PATCH 1/2] cutils:change strtosz_suffix_unit function List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: liguang Cc: kwolf@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com On Fri, Dec 07, 2012 at 11:49:49AM +0800, liguang wrote: > if value to be translated is larger than INT64_MAX, > this function will not be convenient for caller to > be aware of it, so change a little for this. > > Signed-off-by: liguang > --- > cutils.c | 5 +++-- > 1 files changed, 3 insertions(+), 2 deletions(-) I don't understand what this patch is supposed to do. Why change the type of mul from double to int64_t? Are you using retval = 0 as a special value to indicate overflow? Then the new return value should be documented. But it would be better to change the function to return -errno values instead of 0/-1 so the caller knows the reason for specific failure cases (e.g. -E2BIG). Should the val < 0 case be checked earlier in the function? It seems like a different failure case then INT64_MAX overflow. > diff --git a/cutils.c b/cutils.c > index 4f0692f..8905b5e 100644 > --- a/cutils.c > +++ b/cutils.c > @@ -219,11 +219,11 @@ static int64_t suffix_mul(char suffix, int64_t unit) > int64_t strtosz_suffix_unit(const char *nptr, char **end, > const char default_suffix, int64_t unit) > { > - int64_t retval = -1; > + int64_t retval = -1, mul; > char *endptr; > unsigned char c; > int mul_required = 0; > - double val, mul, integral, fraction; > + double val, integral, fraction; > > errno = 0; > val = strtod(nptr, &endptr); > @@ -246,6 +246,7 @@ int64_t strtosz_suffix_unit(const char *nptr, char **end, > goto fail; > } > if ((val * mul >= INT64_MAX) || val < 0) { > + retval = 0; > goto fail; > } > retval = val * mul; > -- > 1.7.2.5 > >