Re: [PATCH v13 01/12] util/cutils: Add qemu_strtotime_ps()

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

On 10/23/2019 9:08 AM, Eduardo Habkost wrote:
> Hi,
> 
> First of all, sorry for not reviewing this earlier.  I thought
> other people were already looking at the first 4 patches.
> 
> On Sun, Oct 20, 2019 at 07:11:14PM +0800, Tao Xu wrote:
>> To convert strings with time suffixes to numbers, support time unit are
>> "ps" for picosecond, "ns" for nanosecond, "us" for microsecond, "ms"
>> for millisecond or "s" for second.
>>
>> Signed-off-by: Tao Xu <tao3.xu@intel.com>
>> ---
>>
>> No changes in v13.
>> ---
>>   include/qemu/cutils.h |  1 +
>>   util/cutils.c         | 82 +++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 83 insertions(+)
>>
>> diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h
>> index b54c847e0f..7b6d106bdd 100644
>> --- a/include/qemu/cutils.h
>> +++ b/include/qemu/cutils.h
>> @@ -182,5 +182,6 @@ int uleb128_decode_small(const uint8_t *in, uint32_t *n);
>>    * *str1 is <, == or > than *str2.
>>    */
>>   int qemu_pstrcmp0(const char **str1, const char **str2);
>> +int qemu_strtotime_ps(const char *nptr, const char **end, uint64_t *result);
>>   
>>   #endif
>> diff --git a/util/cutils.c b/util/cutils.c
>> index fd591cadf0..a50c15f46a 100644
>> --- a/util/cutils.c
>> +++ b/util/cutils.c
>> @@ -847,3 +847,85 @@ int qemu_pstrcmp0(const char **str1, const char **str2)
>>   {
>>       return g_strcmp0(*str1, *str2);
>>   }
>> +
>> +static int64_t timeunit_mul(const char *unitstr)
>> +{
>> +    if (g_strcmp0(unitstr, "ps") == 0) {
>> +        return 1;
> 
> This makes do_strtotime("123ps,...", &end, ...) fail because of
> trailing data.
> 
>> +    } else if (g_strcmp0(unitstr, "ns") == 0) {
>> +        return 1000;
>> +    } else if (g_strcmp0(unitstr, "us") == 0) {
>> +        return 1000000;
>> +    } else if (g_strcmp0(unitstr, "ms") == 0) {
>> +        return 1000000000LL;
>> +    } else if (g_strcmp0(unitstr, "s") == 0) {
>> +        return 1000000000000LL;
> 
> Same as above, for the other suffixes.
> 
>> +    } else {
>> +        return -1;
> 
> But this makes do_strtotime("123,...", &end, ...) work as
> expected.
> 
> This is inconsistent.  I see that you are not testing non-NULL
> `end` argument at test_qemu_strtotime_ps_trailing(), so that's
> probably why this issue wasn't detected.
> 
>> +    }
>> +}
>> +
>> +
>> +/*
>> + * Convert string to time, support time unit are ps for picosecond,
>> + * ns for nanosecond, us for microsecond, ms for millisecond or s for second.
>> + * End pointer will be returned in *end, if not NULL. Return -ERANGE on
>> + * overflow, and -EINVAL on other error.
>> + */
>> +static int do_strtotime(const char *nptr, const char **end,
>> +                      const char *default_unit, uint64_t *result)
>> +{
>> +    int retval;
>> +    const char *endptr;
>> +    int mul_required = 0;
>> +    int64_t mul;
>> +    double val, integral, fraction;
>> +
>> +    retval = qemu_strtod_finite(nptr, &endptr, &val);
>> +    if (retval) {
>> +        goto out;
>> +    }
>> +    fraction = modf(val, &integral);
>> +    if (fraction != 0) {
>> +        mul_required = 1;
>> +    }
>> +
>> +    mul = timeunit_mul(endptr);
>> +
>> +    if (mul == 1000000000000LL) {
>> +        endptr++;
>> +    } else if (mul != -1) {
>> +        endptr += 2;
> 
> This is fragile.  It can break very easily if additional
> one-letter suffixes are added to timeunit_mul() in the future.
> 
> One option to make this safer is to make timeunit_mul() get a
> pointer to endptr.
> 
> 
>> +    } else {
>> +        mul = timeunit_mul(default_unit);
>> +        assert(mul >= 0);
>> +    }
>> +    if (mul == 1 && mul_required) {
>> +        retval = -EINVAL;
>> +        goto out;
>> +    }
>> +    /*
>> +     * Values >= 0xfffffffffffffc00 overflow uint64_t after their trip
>> +     * through double (53 bits of precision).
>> +     */
>> +    if ((val * (double)mul >= 0xfffffffffffffc00) || val < 0) {
>> +        retval = -ERANGE;
>> +        goto out;
>> +    }
>> +    *result = val * (double)mul;
>> +    retval = 0;
>> +
>> +out:
>> +    if (end) {
>> +        *end = endptr;
> 
> This indicates that having trailing data after the string is OK
> if `end` is not NULL, but timeunit_mul() doesn't take that into
> account.
> 
> Considering that this function is just a copy of do_strtosz(), I
> suggest making do_strtosz() and suffix_mul() get a
> suffix/multiplier table as input, instead of duplicating the
> code.
> 
> Thanks for your suggestion, I will try it.