[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [ [PATCH 1/2] cutils:change strtosz_suffix_unit functio
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [ [PATCH 1/2] cutils:change strtosz_suffix_unit function |
Date: |
Tue, 11 Dec 2012 16:06:28 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.1 (gnu/linux) |
Stefan Hajnoczi <address@hidden> 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 <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.
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
>>
>>