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

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

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