Re: [Qemu-devel] [ [PATCH 1/2] cutils:change strtosz_suffix_unit function

[li guang]

在 2012-12-11二的 10:52 +0100，Stefan Hajnoczi写道：
> 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 <address@hidden>
> > ---
> >  cutils.c |    5 +++--
> >  1 files changed, 3 insertions(+), 2 deletions(-)
> 
> I don't understand what this patch is supposed to do.

little change to let the caller know the translated value overflow

> 
> Why change the type of mul from double to int64_t?

mul is multiplier generated by suffix_mul(return value type int64_t)
so double is incorrect.

> 
> Are you using retval = 0 as a special value to indicate overflow?  

yes, just an indicator for caller, not so normal return value

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

because of 'retval = 0' is only a hacking way to reflect an illegal
translated value, does it have to act in such an 'official' way?

> 
> Should the val < 0 case be checked earlier in the function?  It seems
> like a different failure case then INT64_MAX overflow.

maybe, but I think no one interested in difference between
under-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
> > 
> > 

-- 
regards!
li guang