Re: [PATCH] util/cutils: Expand do_strtosz parsing precision to 64 bits

[Tao Xu <tao3.xu@intel.com>]

On 12/18/2019 9:33 AM, Tao Xu wrote:
> On 12/17/2019 6:25 PM, Markus Armbruster wrote:
>> Tao Xu <tao3.xu@intel.com> writes:
>>
>>> On 12/5/19 11:29 PM, Markus Armbruster wrote:
>>>> Tao Xu <tao3.xu@intel.com> writes:
>>>>
>>>>> Parse input string both as a double and as a uint64_t, then use the
>>>>> method which consumes more characters. Update the related test cases.
>>>>>
>>>>> Signed-off-by: Tao Xu <tao3.xu@intel.com>
>>>>> ---
>>>> [...]
>>>>> diff --git a/util/cutils.c b/util/cutils.c
>>>>> index 77acadc70a..b08058c57c 100644
>>>>> --- a/util/cutils.c
>>>>> +++ b/util/cutils.c
>>>>> @@ -212,24 +212,43 @@ static int do_strtosz(const char *nptr, const 
>>>>> char **end,
>>>>>                          const char default_suffix, int64_t unit,
>>>>>                          uint64_t *result)
>>>>>    {
>>>>> -    int retval;
>>>>> -    const char *endptr;
>>>>> +    int retval, retd, retu;
>>>>> +    const char *suffix, *suffixd, *suffixu;
>>>>>        unsigned char c;
>>>>>        int mul_required = 0;
>>>>> -    double val, mul, integral, fraction;
>>>>> +    bool use_strtod;
>>>>> +    uint64_t valu;
>>>>> +    double vald, mul, integral, fraction;
>>>>
>>>> Note for later: @mul is double.
>>>>
>>>>> +
>>>>> +    retd = qemu_strtod_finite(nptr, &suffixd, &vald);
>>>>> +    retu = qemu_strtou64(nptr, &suffixu, 0, &valu);
>>
>> Note for later: passing 0 to base accepts octal and hexadecimal
>> integers.
>>
>>>>> +    use_strtod = strlen(suffixd) < strlen(suffixu);
>>>>> +
>>>>> +    /*
>>>>> +     * Parse @nptr both as a double and as a uint64_t, then use 
>>>>> the method
>>>>> +     * which consumes more characters.
>>>>> +     */
>>>>
>>>> The comment is in a funny place.  I'd put it right before the
>>>> qemu_strtod_finite() line.
>>>>
>>>>> +    if (use_strtod) {
>>>>> +        suffix = suffixd;
>>>>> +        retval = retd;
>>>>> +    } else {
>>>>> +        suffix = suffixu;
>>>>> +        retval = retu;
>>>>> +    }
>>>>>    -    retval = qemu_strtod_finite(nptr, &endptr, &val);
>>>>>        if (retval) {
>>>>>            goto out;
>>>>>        }
>>>>
>>>> This is even more subtle than it looks.
>>>>
>>>> A close reading of the function contracts leads to three cases for each
>>>> conversion:
>>>>
>>>> * parse error (including infinity and NaN)
>>>>
>>>>     @retu / @retd is -EINVAL
>>>>     @valu / @vald is uninitialized
>>>>     @suffixu / @suffixd is @nptr
>>>>
>>>> * range error
>>>>
>>>>     @retu / @retd is -ERANGE
>>>>     @valu / @vald is our best approximation of the conversion result
>>>>     @suffixu / @suffixd points to the first character not consumed 
>>>> by the
>>>>     conversion.
>>>>
>>>>     Sub-cases:
>>>>
>>>>     - uint64_t overflow
>>>>
>>>>       We know the conversion result exceeds UINT64_MAX.
>>>>
>>>>     - double overflow
>>>>
>>>>       we know the conversion result's magnitude exceeds the largest
>>>>       representable finite double DBL_MAX.
>>>>
>>>>     - double underflow
>>>>
>>>>       we know the conversion result is close to zero (closer than 
>>>> DBL_MIN,
>>>>       the smallest normalized positive double).
>>>>
>>>> * success
>>>>
>>>>     @retu / @retd is 0
>>>>     @valu / @vald is the conversion result
>>>>     @suffixu / @suffixd points to the first character not consumed 
>>>> by the
>>>>     conversion.
>>>>
>>>> This leads to a matrix (parse error, uint64_t overflow, success) x
>>>> (parse error, double overflow, double underflow, success).  We need to
>>>> check the code does what we want for each element of this matrix, and
>>>> document any behavior that's not perfectly obvious.
>>>>
>>>> (success, success): we pick uint64_t if qemu_strtou64() consumed more
>>>> characters than qemu_strtod_finite(), else double.  "More" is important
>>>> here; when they consume the same characters, we *need* to use the
>>>> uint64_t result.  Example: for "18446744073709551615", we need to use
>>>> uint64_t 18446744073709551615, not double 18446744073709551616.0.  But
>>>> for "18446744073709551616.", we need to use the double.  Good.
>>
>> Also fun: for "0123", we use uint64_t 83, not double 123.0.  But for
>> "0123.", we use 123.0, not 83.
>>
>> Do we really want to accept octal and hexadecimal integers?
>>
> 
> Thank you for reminding me. Octal and hexadecimal may bring more 
> confusion. I will use qemu_strtou64(nptr, &suffixu, 10, &valu) and add 
> test for input like "0123".
> 

Hi Markus,

After I use qemu_strtou64(nptr, &suffixu, 10, &valu), it cause another 
question. Because qemu_strtod_finite support hexadecimal input, so in 
this situation, it will parsed as double. It will also let large 
hexadecimal integers be rounded. So there may be two solution:

1: use qemu_strtou64(nptr, &suffixu, 0, &valu) and parse octal as 
decimal. This will keep hexadecimal valid as now.

"0123" --> 123; "0x123" --> 291

2: use qemu_strtou64(nptr, &suffixu, 10, &valu) and reject octal and 
decimal.

"0123" --> Error; "0x123" --> Error