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

[Markus Armbruster <armbru@redhat.com>]

Stefan Hajnoczi <stefanha@gmail.com> writes:

> 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 <lig.fnst@cn.fujitsu.com>
>> ---
>>  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.

Zero is a legitimate value, so it cannot be used to indicate overflow.

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

The appropriate error code is ERANGE, following strtol() precedence.

Nitpick: function doesn't return zero on succes, it returns a
non-negative value.

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

Only if callers are interested in differentiating between under- and
overflow.  Which I doubt.

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