[v13,01/12] util/cutils: Add qemu_strtotime_ps()
diff mbox series

Message ID 20191020111125.27659-2-tao3.xu@intel.com
State New
Headers show
Series
  • Build ACPI Heterogeneous Memory Attribute Table (HMAT)
Related show

Commit Message

Tao Xu Oct. 20, 2019, 11:11 a.m. UTC
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(+)

Comments

Eduardo Habkost Oct. 23, 2019, 1:08 a.m. UTC | #1
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.


> +    } else if (*endptr) {
> +        retval = -EINVAL;
> +    }
> +
> +    return retval;
> +}
> +
> +int qemu_strtotime_ps(const char *nptr, const char **end, uint64_t *result)
> +{
> +    return do_strtotime(nptr, end, "ps", result);
> +}
> -- 
> 2.20.1
>
Eric Blake Oct. 23, 2019, 1:13 a.m. UTC | #2
On 10/20/19 6:11 AM, 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.

I haven't yet reviewed the patch itself, but my off-hand observation:

picosecond is probably too narrow to ever be useful.  POSIX interfaces 
only go down to nanoseconds, and when you start adding in vmexit delay 
times and such, we're lucky when we get anything better than microsecond 
accuracies.  Supporting just three sub-second suffixes instead of four 
would slightly simplify the code, and not cost you any real precision.
Tao Xu Oct. 23, 2019, 1:50 a.m. UTC | #3
On 10/23/2019 9:13 AM, Eric Blake wrote:
> On 10/20/19 6:11 AM, 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.
> 
> I haven't yet reviewed the patch itself, but my off-hand observation:
> 
> picosecond is probably too narrow to ever be useful.  POSIX interfaces
> only go down to nanoseconds, and when you start adding in vmexit delay
> times and such, we're lucky when we get anything better than microsecond
> accuracies.  Supporting just three sub-second suffixes instead of four
> would slightly simplify the code, and not cost you any real precision.
> 
Thank you for your suggestion.
Tao Xu Oct. 23, 2019, 6:07 a.m. UTC | #4
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.
Daniel P. Berrangé Oct. 24, 2019, 9:54 a.m. UTC | #5
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(+)

This really ought to have an addition to the unit tests to validating
the parsing, both success and error scenarios, so that we're clear on
exactly what strings are accepted & rejected.


Regards,
Daniel
Eduardo Habkost Oct. 24, 2019, 1:20 p.m. UTC | #6
On Thu, Oct 24, 2019 at 10:54:57AM +0100, Daniel P. Berrangé wrote:
> 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(+)
> 
> This really ought to have an addition to the unit tests to validating
> the parsing, both success and error scenarios, so that we're clear on
> exactly what strings are accepted & rejected.

Unit tests are in patch 02/12.  It's a good idea to squash
patches 01 and 02 together.
Tao Xu Oct. 25, 2019, 1:22 a.m. UTC | #7
On 10/24/2019 9:20 PM, Eduardo Habkost wrote:
> On Thu, Oct 24, 2019 at 10:54:57AM +0100, Daniel P. Berrangé wrote:
>> 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(+)
>>
>> This really ought to have an addition to the unit tests to validating
>> the parsing, both success and error scenarios, so that we're clear on
>> exactly what strings are accepted & rejected.
> 
> Unit tests are in patch 02/12.  It's a good idea to squash
> patches 01 and 02 together.
> 
Yes it is in 02/12. OK I will squash them.

Patch
diff mbox series

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;
+    } 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;
+    } else {
+        return -1;
+    }
+}
+
+
+/*
+ * 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;
+    } 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;
+    } else if (*endptr) {
+        retval = -EINVAL;
+    }
+
+    return retval;
+}
+
+int qemu_strtotime_ps(const char *nptr, const char **end, uint64_t *result)
+{
+    return do_strtotime(nptr, end, "ps", result);
+}