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...@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
>
--
Eduardo
