| Message ID | 20191020111125.27659-2-tao3.xu@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | Build ACPI Heterogeneous Memory Attribute Table (HMAT) | expand |
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 >
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.
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.
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.
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
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.
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.
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); +}
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(+)