[Qemu-devel] [PATCH v1 0/9] qapi: rewrite string-input-visitor

[Qemu-devel] [PATCH v1 0/9] qapi: rewrite string-input-visitor

[David Hildenbrand]

Rewrite string-input-visitor to be (hopefully) less ugly. Support
int and uint lists (including ranges, but not implemented via type
"Range").

Virtual walks are now supported and more errors are cought (and some bugs
fixed). Fix and extend the tests. Parsing of uint64_t is now properly
supported.

Importantly, when parsing a list we now return the list and not an
ordered set (we are not an ordered set parser after all). Whoever needs
that can add it on top. As far as I can see, current code can deal with
it but I'll have to look at the details.

While at it, introduce and use qemu_strtod() and qemu_strtod_finite().
However don't convert monitor.c as that seems to be too broken for me
(it ignores both conversion errors and range errors, and there is an
actual unit test in which conversion is expected to fail).

RFC - v1:
- Too much to name it all, just some highlights
- Pull some tests before the rework
- Introduce qemu_strtod_finite() and use it in more places
- More tests
- "qapi: rewrite string-input-visitor"
-- programming error -> assert
-- g_assert() -> assert(), g_assert_not_reached() -> abort()
-- Accept single-element ranges "1-1"
-- Split parsing and returning the next range element
-- Minor style fixes

David Hildenbrand (9):
  cutils: add qemu_strtod() and qemu_strtod_finite()
  cutils: use qemu_strtod_finite() in do_strtosz()
  qapi: use qemu_strtod_finite() in string-input-visitor
  qapi: use qemu_strtod_finite() in qobject-input-visitor
  test-string-input-visitor: add more tests
  qapi: rewrite string-input-visitor
  test-string-input-visitor: use virtual walk
  test-string-input-visitor: split off uint64 list tests
  test-string-input-visitor: add range overflow tests

 include/qapi/string-input-visitor.h |   4 +-
 include/qemu/cutils.h               |   8 +-
 monitor.c                           |   2 +-
 qapi/qobject-input-visitor.c        |   9 +-
 qapi/string-input-visitor.c         | 418 ++++++++++++++++------------
 tests/test-cutils.c                 |  14 +-
 tests/test-string-input-visitor.c   | 172 +++++++++---
 util/cutils.c                       |  52 +++-
 8 files changed, 432 insertions(+), 247 deletions(-)

-- 
2.17.2

[Qemu-devel] [PATCH v1 1/9] cutils: add qemu_strtod() and qemu_strtod_finite()

[David Hildenbrand]

Let's provide a wrapper for strtod().

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 include/qemu/cutils.h |  2 ++
 util/cutils.c         | 38 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 40 insertions(+)

diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h
index 7071bfe2d4..756b41c193 100644
--- a/include/qemu/cutils.h
+++ b/include/qemu/cutils.h
@@ -146,6 +146,8 @@ int qemu_strtoi64(const char *nptr, const char **endptr, int base,
                   int64_t *result);
 int qemu_strtou64(const char *nptr, const char **endptr, int base,
                   uint64_t *result);
+int qemu_strtod(const char *nptr, const char **endptr, double *result);
+int qemu_strtod_finite(const char *nptr, const char **endptr, double *result);
 
 int parse_uint(const char *s, unsigned long long *value, char **endptr,
                int base);
diff --git a/util/cutils.c b/util/cutils.c
index 698bd315bd..7868a683e8 100644
--- a/util/cutils.c
+++ b/util/cutils.c
@@ -544,6 +544,44 @@ int qemu_strtou64(const char *nptr, const char **endptr, int base,
     return check_strtox_error(nptr, ep, endptr, errno);
 }
 
+/**
+ * Convert string @nptr to a double.
+ *
+ * Works like qemu_strtoul(), except it stores +/-HUGE_VAL on
+ * overflow/underflow.
+ */
+int qemu_strtod(const char *nptr, const char **endptr, double *result)
+{
+    char *ep;
+
+    if (!nptr) {
+        if (endptr) {
+            *endptr = nptr;
+        }
+        return -EINVAL;
+    }
+
+    errno = 0;
+    *result = strtod(nptr, &ep);
+    return check_strtox_error(nptr, ep, endptr, errno);
+}
+
+/**
+ * Convert string @nptr to a finite double.
+ *
+ * Works like qemu_strtoul(), except it stores +/-HUGE_VAL on
+ * overflow/underflow. "NaN" or "inf" are rejcted with -EINVAL.
+ */
+int qemu_strtod_finite(const char *nptr, const char **endptr, double *result)
+{
+    int ret = qemu_strtod(nptr, endptr, result);
+
+    if (!ret && !isfinite(*result)) {
+        return -EINVAL;
+    }
+    return ret;
+}
+
 /**
  * Searches for the first occurrence of 'c' in 's', and returns a pointer
  * to the trailing null byte if none was found.
-- 
2.17.2

Re: [Qemu-devel] [PATCH v1 1/9] cutils: add qemu_strtod() and qemu_strtod_finite()

[Eric Blake]

On 11/15/18 8:04 AM, David Hildenbrand wrote:
> Let's provide a wrapper for strtod().
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>   include/qemu/cutils.h |  2 ++
>   util/cutils.c         | 38 ++++++++++++++++++++++++++++++++++++++
>   2 files changed, 40 insertions(+)
> 


> +
> +/**
> + * Convert string @nptr to a finite double.
> + *
> + * Works like qemu_strtoul(), except it stores +/-HUGE_VAL on
> + * overflow/underflow. "NaN" or "inf" are rejcted with -EINVAL.

s/rejcted/rejected/

> + */
> +int qemu_strtod_finite(const char *nptr, const char **endptr, double *result)
> +{
> +    int ret = qemu_strtod(nptr, endptr, result);

On overflow, result is set to HUGE_VAL (aka "inf") with ret set to 
-ERANGE.  (The C standard uses HUGE_VAL rather than directly requiring 
infinity on overflow, in order to cater to museum platforms where the 
largest representable double is still finite; but no one develops qemu 
on a non-IEEE machine these days so we know that HUGE_VAL == INF).

> +
> +    if (!ret && !isfinite(*result)) {
> +        return -EINVAL;
> +    }

This check means that overflow ("1e9999") fails with -ERANGE, while 
actual infinity ("inf") fails with -EINVAL, letting the user distinguish 
between the two.  Still, I wonder if assigning a non-finite value into 
result on -ERANGE is the wisest course of action.  We'll just have to 
see in the next patches that use this.

With the typo fix,

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH v1 1/9] cutils: add qemu_strtod() and qemu_strtod_finite()

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 11/15/18 8:04 AM, David Hildenbrand wrote:
>> Let's provide a wrapper for strtod().
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>   include/qemu/cutils.h |  2 ++
>>   util/cutils.c         | 38 ++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 40 insertions(+)
>>
>
>
>> +
>> +/**
>> + * Convert string @nptr to a finite double.
>> + *
>> + * Works like qemu_strtoul(), except it stores +/-HUGE_VAL on
>> + * overflow/underflow. "NaN" or "inf" are rejcted with -EINVAL.
>
> s/rejcted/rejected/

Also, just overflow.  Floating-point underflow is when a computation's
mathematical result is too close to zero to be represented without
extraordinary rounding error.

Skip this paragraph unless you're ready to nerd out.  IEEE 754 section
7.5 defines underflow to happen

    [...] either

    a) after rounding — when a non-zero result computed as though the
    exponent range were unbounded would lie strictly between ±b^emin,
    or

    b) before rounding — when a non-zero result computed as though both
    the exponent range and the precision were unbounded would lie
    strictly between ±b^emin.

where b^emin is the smallest normal number.  

The "Works like qemu_strtoul()" is a bit lazy.  I guess it works like
qemu_strtoul() in the sense that it adds to strtod() what qemu_strtoul()
adds to strtoul().  I consciously didn't take a similar shortcut in
commit 4295f879bec: I documented both qemu_strtol() and qemu_strtoul()
in longhand, and used "Works like" shorthand only where that's actually
the case: qemu_strtoll() works like qemu_strtol(), and qemu_strtoull()
works like qemu_strtoul().  I'd prefer longhand for qemu_strtod().  It
costs us a few lines, but it results in a clearer contract.

>> + */
>> +int qemu_strtod_finite(const char *nptr, const char **endptr, double *result)
>> +{
>> +    int ret = qemu_strtod(nptr, endptr, result);
>
> On overflow, result is set to HUGE_VAL (aka "inf") with ret set to
> -ERANGE.  (The C standard uses HUGE_VAL rather than directly requiring
> infinity on overflow, in order to cater to museum platforms where the
> largest representable double is still finite; but no one develops qemu
> on a non-IEEE machine these days so we know that HUGE_VAL == INF).

Aside: museum clauses like this one make the standard much harder to
read than necessary.  I wish they'll purge them from C2X.

>> +
>> +    if (!ret && !isfinite(*result)) {
>> +        return -EINVAL;
>> +    }

qemu_strtol() & friends leave *result alone when they return -EINVAL.
This one doesn't.  Unlikely to hurt anyone, but I'd prefer to keep them
consistent.

> This check means that overflow ("1e9999") fails with -ERANGE, while
> actual infinity ("inf") fails with -EINVAL, letting the user
> distinguish between the two.  Still, I wonder if assigning a
> non-finite value into result on -ERANGE is the wisest course of
> action.  We'll just have to see in the next patches that use this.

I guess it's about as "wise" as qemu_strtol() storing LONG_MAX on
integer overflow.

I'm fine with the semantics David picked, as long as they're spelled out
in the function contract.

> With the typo fix,
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Re: [Qemu-devel] [PATCH v1 1/9] cutils: add qemu_strtod() and qemu_strtod_finite()

[David Hildenbrand]

On 15.11.18 17:22, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> On 11/15/18 8:04 AM, David Hildenbrand wrote:
>>> Let's provide a wrapper for strtod().
>>>
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>> ---
>>>   include/qemu/cutils.h |  2 ++
>>>   util/cutils.c         | 38 ++++++++++++++++++++++++++++++++++++++
>>>   2 files changed, 40 insertions(+)
>>>
>>
>>
>>> +
>>> +/**
>>> + * Convert string @nptr to a finite double.
>>> + *
>>> + * Works like qemu_strtoul(), except it stores +/-HUGE_VAL on
>>> + * overflow/underflow. "NaN" or "inf" are rejcted with -EINVAL.
>>
>> s/rejcted/rejected/
> 
> Also, just overflow.  Floating-point underflow is when a computation's
> mathematical result is too close to zero to be represented without
> extraordinary rounding error.

Indeed, as the "man strod" states
"... would cause overflow, plus or minus HUGE_VAL (HUGE_VALF, HUGE_VALL)
is returned (according to the sign of the value)"

> 
> Skip this paragraph unless you're ready to nerd out.  IEEE 754 section
> 7.5 defines underflow to happen
> 
>     [...] either
> 
>     a) after rounding — when a non-zero result computed as though the
>     exponent range were unbounded would lie strictly between ±b^emin,
>     or
> 
>     b) before rounding — when a non-zero result computed as though both
>     the exponent range and the precision were unbounded would lie
>     strictly between ±b^emin.
> 
> where b^emin is the smallest normal number.  
> 
> The "Works like qemu_strtoul()" is a bit lazy.  I guess it works like
> qemu_strtoul() in the sense that it adds to strtod() what qemu_strtoul()
> adds to strtoul().  I consciously didn't take a similar shortcut in
> commit 4295f879bec: I documented both qemu_strtol() and qemu_strtoul()
> in longhand, and used "Works like" shorthand only where that's actually
> the case: qemu_strtoll() works like qemu_strtol(), and qemu_strtoull()
> works like qemu_strtoul().  I'd prefer longhand for qemu_strtod().  It
> costs us a few lines, but it results in a clearer contract.

/**
 * Convert string @nptr to a double.
  *
 * This is a wrapper around strtod() that is harder to misuse.
 * Semantics of @nptr and @endptr match strtod() with differences
 * noted below.
 *
 * @nptr may be null, and no conversion is performed then.
 *
 * If no conversion is performed, store @nptr in *@endptr and return
 * -EINVAL.
 *
 * If @endptr is null, and the string isn't fully converted, return
 * -EINVAL.  This is the case when the pointer that would be stored in
 * a non-null @endptr points to a character other than '\0'.
 *
 * If the conversion overflows @result, store +/-HUGE_VAL, depending on
 * the sign, in @result and return -ERANGE.
 *
 * Else store the converted value in @result, and return zero.
 */

> 
>>> + */
>>> +int qemu_strtod_finite(const char *nptr, const char **endptr, double *result)
>>> +{
>>> +    int ret = qemu_strtod(nptr, endptr, result);
>>
>> On overflow, result is set to HUGE_VAL (aka "inf") with ret set to
>> -ERANGE.  (The C standard uses HUGE_VAL rather than directly requiring
>> infinity on overflow, in order to cater to museum platforms where the
>> largest representable double is still finite; but no one develops qemu
>> on a non-IEEE machine these days so we know that HUGE_VAL == INF).
> 
> Aside: museum clauses like this one make the standard much harder to
> read than necessary.  I wish they'll purge them from C2X.
> 
>>> +
>>> +    if (!ret && !isfinite(*result)) {
>>> +        return -EINVAL;
>>> +    }
> 
> qemu_strtol() & friends leave *result alone when they return -EINVAL.
> This one doesn't.  Unlikely to hurt anyone, but I'd prefer to keep them
> consistent.

Will use a temporary and also properly set endptr in case we return
-EINVAL; And add a fully-blown description as noted above :)

> 
>> This check means that overflow ("1e9999") fails with -ERANGE, while
>> actual infinity ("inf") fails with -EINVAL, letting the user
>> distinguish between the two.  Still, I wonder if assigning a
>> non-finite value into result on -ERANGE is the wisest course of
>> action.  We'll just have to see in the next patches that use this.
> 
> I guess it's about as "wise" as qemu_strtol() storing LONG_MAX on
> integer overflow.
> 
> I'm fine with the semantics David picked, as long as they're spelled out
> in the function contract.

I think for now we're fine treating explicit "infinity" user input as
-EINVAL. We could return something like "HUGE_VAL - 1" along with
-ERANGE, but I guess for now this is overkill. Most callers will bail
out on -ERANGE either way. And if not, they have to make sure they can
deal with HUGE_VAL.

> 
>> With the typo fix,
>>
>> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v1 1/9] cutils: add qemu_strtod() and qemu_strtod_finite()

[Eric Blake]

On 11/15/18 11:25 AM, David Hildenbrand wrote:

>> Also, just overflow.  Floating-point underflow is when a computation's
>> mathematical result is too close to zero to be represented without
>> extraordinary rounding error.
> 

>> works like qemu_strtoul().  I'd prefer longhand for qemu_strtod().  It
>> costs us a few lines, but it results in a clearer contract.
> 
> /**
>   * Convert string @nptr to a double.
>    *
>   * This is a wrapper around strtod() that is harder to misuse.
>   * Semantics of @nptr and @endptr match strtod() with differences
>   * noted below.
>   *
>   * @nptr may be null, and no conversion is performed then.
>   *
>   * If no conversion is performed, store @nptr in *@endptr and return
>   * -EINVAL.
>   *
>   * If @endptr is null, and the string isn't fully converted, return
>   * -EINVAL.  This is the case when the pointer that would be stored in
>   * a non-null @endptr points to a character other than '\0'.
>   *
>   * If the conversion overflows @result, store +/-HUGE_VAL, depending on
>   * the sign, in @result and return -ERANGE.

Fails to mention underflow. Maybe add:

If the conversion underflows, store +/-0.0 in @result, depending on the 
sign, and return -ERANGE.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH v1 1/9] cutils: add qemu_strtod() and qemu_strtod_finite()

[David Hildenbrand]

On 15.11.18 19:02, Eric Blake wrote:
> If the conversion underflows, store ±0.0 in @result, depending on the 
> sign, and return -ERANGE.

Will do! Thanks!

-- 

Thanks,

David / dhildenb

[Qemu-devel] [PATCH v1 2/9] cutils: use qemu_strtod_finite() in do_strtosz()

[David Hildenbrand]

Let's use the new function. In order to do so, we have to convert all
users of qemu_strtosz*() to pass a "const char **end" ptr.

We will now also reject "inf" properly.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 include/qemu/cutils.h |  6 +++---
 monitor.c             |  2 +-
 tests/test-cutils.c   | 14 +++++++-------
 util/cutils.c         | 14 ++++++--------
 4 files changed, 17 insertions(+), 19 deletions(-)

diff --git a/include/qemu/cutils.h b/include/qemu/cutils.h
index 756b41c193..d2dad3057c 100644
--- a/include/qemu/cutils.h
+++ b/include/qemu/cutils.h
@@ -153,9 +153,9 @@ int parse_uint(const char *s, unsigned long long *value, char **endptr,
                int base);
 int parse_uint_full(const char *s, unsigned long long *value, int base);
 
-int qemu_strtosz(const char *nptr, char **end, uint64_t *result);
-int qemu_strtosz_MiB(const char *nptr, char **end, uint64_t *result);
-int qemu_strtosz_metric(const char *nptr, char **end, uint64_t *result);
+int qemu_strtosz(const char *nptr, const char **end, uint64_t *result);
+int qemu_strtosz_MiB(const char *nptr, const char **end, uint64_t *result);
+int qemu_strtosz_metric(const char *nptr, const char **end, uint64_t *result);
 
 /* used to print char* safely */
 #define STR_OR_NULL(str) ((str) ? (str) : "null")
diff --git a/monitor.c b/monitor.c
index d39390c2f2..ee9893c785 100644
--- a/monitor.c
+++ b/monitor.c
@@ -3231,7 +3231,7 @@ static QDict *monitor_parse_arguments(Monitor *mon,
             {
                 int ret;
                 uint64_t val;
-                char *end;
+                const char *end;
 
                 while (qemu_isspace(*p)) {
                     p++;
diff --git a/tests/test-cutils.c b/tests/test-cutils.c
index d85c3e0f6d..e21dcbf12b 100644
--- a/tests/test-cutils.c
+++ b/tests/test-cutils.c
@@ -1950,7 +1950,7 @@ static void test_qemu_strtou64_full_max(void)
 static void test_qemu_strtosz_simple(void)
 {
     const char *str;
-    char *endptr = NULL;
+    const char *endptr = NULL;
     int err;
     uint64_t res = 0xbaadf00d;
 
@@ -2017,7 +2017,7 @@ static void test_qemu_strtosz_units(void)
     const char *p = "1P";
     const char *e = "1E";
     int err;
-    char *endptr = NULL;
+    const char *endptr = NULL;
     uint64_t res = 0xbaadf00d;
 
     /* default is M */
@@ -2066,7 +2066,7 @@ static void test_qemu_strtosz_float(void)
 {
     const char *str = "12.345M";
     int err;
-    char *endptr = NULL;
+    const char *endptr = NULL;
     uint64_t res = 0xbaadf00d;
 
     err = qemu_strtosz(str, &endptr, &res);
@@ -2078,7 +2078,7 @@ static void test_qemu_strtosz_float(void)
 static void test_qemu_strtosz_invalid(void)
 {
     const char *str;
-    char *endptr = NULL;
+    const char *endptr = NULL;
     int err;
     uint64_t res = 0xbaadf00d;
 
@@ -2101,7 +2101,7 @@ static void test_qemu_strtosz_invalid(void)
 static void test_qemu_strtosz_trailing(void)
 {
     const char *str;
-    char *endptr = NULL;
+    const char *endptr = NULL;
     int err;
     uint64_t res = 0xbaadf00d;
 
@@ -2126,7 +2126,7 @@ static void test_qemu_strtosz_trailing(void)
 static void test_qemu_strtosz_erange(void)
 {
     const char *str;
-    char *endptr = NULL;
+    const char *endptr = NULL;
     int err;
     uint64_t res = 0xbaadf00d;
 
@@ -2160,7 +2160,7 @@ static void test_qemu_strtosz_metric(void)
 {
     const char *str = "12345k";
     int err;
-    char *endptr = NULL;
+    const char *endptr = NULL;
     uint64_t res = 0xbaadf00d;
 
     err = qemu_strtosz_metric(str, &endptr, &res);
diff --git a/util/cutils.c b/util/cutils.c
index 7868a683e8..dd61fb4558 100644
--- a/util/cutils.c
+++ b/util/cutils.c
@@ -206,19 +206,17 @@ static int64_t suffix_mul(char suffix, int64_t unit)
  * in *end, if not NULL. Return -ERANGE on overflow, Return -EINVAL on
  * other error.
  */
-static int do_strtosz(const char *nptr, char **end,
+static int do_strtosz(const char *nptr, const char **end,
                       const char default_suffix, int64_t unit,
                       uint64_t *result)
 {
     int retval;
-    char *endptr;
+    const char *endptr;
     unsigned char c;
     int mul_required = 0;
     double val, mul, integral, fraction;
 
-    errno = 0;
-    val = strtod(nptr, &endptr);
-    if (isnan(val) || endptr == nptr || errno != 0) {
+    if (qemu_strtod_finite(nptr, &endptr, &val)) {
         retval = -EINVAL;
         goto out;
     }
@@ -259,17 +257,17 @@ out:
     return retval;
 }
 
-int qemu_strtosz(const char *nptr, char **end, uint64_t *result)
+int qemu_strtosz(const char *nptr, const char **end, uint64_t *result)
 {
     return do_strtosz(nptr, end, 'B', 1024, result);
 }
 
-int qemu_strtosz_MiB(const char *nptr, char **end, uint64_t *result)
+int qemu_strtosz_MiB(const char *nptr, const char **end, uint64_t *result)
 {
     return do_strtosz(nptr, end, 'M', 1024, result);
 }
 
-int qemu_strtosz_metric(const char *nptr, char **end, uint64_t *result)
+int qemu_strtosz_metric(const char *nptr, const char **end, uint64_t *result)
 {
     return do_strtosz(nptr, end, 'B', 1000, result);
 }
-- 
2.17.2

Re: [Qemu-devel] [PATCH v1 2/9] cutils: use qemu_strtod_finite() in do_strtosz()

[Eric Blake]

On 11/15/18 8:04 AM, David Hildenbrand wrote:
> Let's use the new function. In order to do so, we have to convert all
> users of qemu_strtosz*() to pass a "const char **end" ptr.
> 
> We will now also reject "inf" properly.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>   include/qemu/cutils.h |  6 +++---
>   monitor.c             |  2 +-
>   tests/test-cutils.c   | 14 +++++++-------
>   util/cutils.c         | 14 ++++++--------
>   4 files changed, 17 insertions(+), 19 deletions(-)
> 

> +++ b/tests/test-cutils.c
> @@ -1950,7 +1950,7 @@ static void test_qemu_strtou64_full_max(void)
>   static void test_qemu_strtosz_simple(void)
>   {
>       const char *str;
> -    char *endptr = NULL;
> +    const char *endptr = NULL;
...
> diff --git a/util/cutils.c b/util/cutils.c

Conversion of call sites is good (in fact, you could drop the ' = NULL' 
initialization, since do_strtosz() guarantees it is set to something 
sane even on error).  But where's the added test coverage of rejecting 
"inf" as a size?

> index 7868a683e8..dd61fb4558 100644
> --- a/util/cutils.c
> +++ b/util/cutils.c
> @@ -206,19 +206,17 @@ static int64_t suffix_mul(char suffix, int64_t unit)
>    * in *end, if not NULL. Return -ERANGE on overflow, Return -EINVAL on
>    * other error.
>    */
> -static int do_strtosz(const char *nptr, char **end,
> +static int do_strtosz(const char *nptr, const char **end,
>                         const char default_suffix, int64_t unit,
>                         uint64_t *result)
>   {
>       int retval;
> -    char *endptr;
> +    const char *endptr;
>       unsigned char c;
>       int mul_required = 0;
>       double val, mul, integral, fraction;
>   
> -    errno = 0;
> -    val = strtod(nptr, &endptr);
> -    if (isnan(val) || endptr == nptr || errno != 0) {
> +    if (qemu_strtod_finite(nptr, &endptr, &val)) {
>           retval = -EINVAL;

This slams -ERANGE failure into -EINVAL.  Do we care?  Or would it have 
been better to just do:

retval = qemu_strtod_finite(...);
if (retval) {
     goto out;

>           goto out;
>       }
> @@ -259,17 +257,17 @@ out:

More context:

out:
     if (end) {
         *end = endptr;
     } else if (*endptr) {
         retval = -EINVAL;
     }

>       return retval;
>   }

Everything else looks okay.

Hmm - while touching this, is it worth making mul_required be a bool, to 
match its use?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH v1 2/9] cutils: use qemu_strtod_finite() in do_strtosz()

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 11/15/18 8:04 AM, David Hildenbrand wrote:
>> Let's use the new function. In order to do so, we have to convert all
>> users of qemu_strtosz*() to pass a "const char **end" ptr.

We shouldn't have qemu_strtol() "improve" on strtol() by splicing in
another const.  Since we did, we havr to do the same for qemu_strtod().

>>
>> We will now also reject "inf" properly.
>>

Ah, so this is a bug fix!  The commit message's title should tell.
Suggest something like:

    cutils: Fix qemu_strtosz() & friends to reject non-finite sizes

    qemu_strtosz() & friends reject NaNs, but happily accept inifities.
    They shouldn't.  Fix that.

    The fix makes use of qemu_strtod_finite().  To avoid ugly casts,
    change the @end parameter of qemu_strtosz() & friends from char **
    to const char **.

I'd split the patch into just the fix (with the ugly cast) and the
parameter type change (getting rid of the ugly cast), but I'm a
compulsive patch splitter.  Up to you.

>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>   include/qemu/cutils.h |  6 +++---
>>   monitor.c             |  2 +-
>>   tests/test-cutils.c   | 14 +++++++-------
>>   util/cutils.c         | 14 ++++++--------
>>   4 files changed, 17 insertions(+), 19 deletions(-)
>>
>
>> +++ b/tests/test-cutils.c
>> @@ -1950,7 +1950,7 @@ static void test_qemu_strtou64_full_max(void)
>>   static void test_qemu_strtosz_simple(void)
>>   {
>>       const char *str;
>> -    char *endptr = NULL;
>> +    const char *endptr = NULL;
> ...
>> diff --git a/util/cutils.c b/util/cutils.c
>
> Conversion of call sites is good (in fact, you could drop the ' =
> NULL' initialization, since do_strtosz() guarantees it is set to
> something sane even on error).

Yes.

>                                 But where's the added test coverage of
> rejecting "inf" as a size?

Let's stick it into test_qemu_strtosz_invalid().

>
>> index 7868a683e8..dd61fb4558 100644
>> --- a/util/cutils.c
>> +++ b/util/cutils.c
>> @@ -206,19 +206,17 @@ static int64_t suffix_mul(char suffix, int64_t unit)
>>    * in *end, if not NULL. Return -ERANGE on overflow, Return -EINVAL on
>>    * other error.
>>    */
>> -static int do_strtosz(const char *nptr, char **end,
>> +static int do_strtosz(const char *nptr, const char **end,
>>                         const char default_suffix, int64_t unit,
>>                         uint64_t *result)
>>   {
>>       int retval;
>> -    char *endptr;
>> +    const char *endptr;
>>       unsigned char c;
>>       int mul_required = 0;
>>       double val, mul, integral, fraction;
>>   -    errno = 0;
>> -    val = strtod(nptr, &endptr);
>> -    if (isnan(val) || endptr == nptr || errno != 0) {
>> +    if (qemu_strtod_finite(nptr, &endptr, &val)) {
>>           retval = -EINVAL;
>
> This slams -ERANGE failure into -EINVAL.  Do we care?  Or would it
> have been better to just do:
>
> retval = qemu_strtod_finite(...);
> if (retval) {
>     goto out;

If distinguishing between ERANGE and EINVAL makes sense for
qemu_strtod_finite(), it probably makes sense for qemu_strtod(), too.

>>           goto out;
>>       }
>> @@ -259,17 +257,17 @@ out:
>
> More context:
>
> out:
>     if (end) {
>         *end = endptr;
>     } else if (*endptr) {
>         retval = -EINVAL;
>     }
>
>>       return retval;
>>   }
>
> Everything else looks okay.
>
> Hmm - while touching this, is it worth making mul_required be a bool,
> to match its use?

Touches no line containing mul_required, so I wouldn't.

Re: [Qemu-devel] [PATCH v1 2/9] cutils: use qemu_strtod_finite() in do_strtosz()

[David Hildenbrand]

On 15.11.18 17:41, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> On 11/15/18 8:04 AM, David Hildenbrand wrote:
>>> Let's use the new function. In order to do so, we have to convert all
>>> users of qemu_strtosz*() to pass a "const char **end" ptr.
> 
> We shouldn't have qemu_strtol() "improve" on strtol() by splicing in
> another const.  Since we did, we havr to do the same for qemu_strtod().
> 
>>>
>>> We will now also reject "inf" properly.
>>>
> 
> Ah, so this is a bug fix!  The commit message's title should tell.
> Suggest something like:
> 
>     cutils: Fix qemu_strtosz() & friends to reject non-finite sizes
> 
>     qemu_strtosz() & friends reject NaNs, but happily accept inifities.
>     They shouldn't.  Fix that.
> 
>     The fix makes use of qemu_strtod_finite().  To avoid ugly casts,
>     change the @end parameter of qemu_strtosz() & friends from char **
>     to const char **.
> 
> I'd split the patch into just the fix (with the ugly cast) and the
> parameter type change (getting rid of the ugly cast), but I'm a
> compulsive patch splitter.  Up to you.

I'll leave it in one patch for now. (and will also add two test cases
for "inf" and "NaN"). If this patch grows even bigger, it makes sense to
split.

Commit message updated.

> 
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>> ---
>>>   include/qemu/cutils.h |  6 +++---
>>>   monitor.c             |  2 +-
>>>   tests/test-cutils.c   | 14 +++++++-------
>>>   util/cutils.c         | 14 ++++++--------
>>>   4 files changed, 17 insertions(+), 19 deletions(-)
>>>
>>
>>> +++ b/tests/test-cutils.c
>>> @@ -1950,7 +1950,7 @@ static void test_qemu_strtou64_full_max(void)
>>>   static void test_qemu_strtosz_simple(void)
>>>   {
>>>       const char *str;
>>> -    char *endptr = NULL;
>>> +    const char *endptr = NULL;
>> ...
>>> diff --git a/util/cutils.c b/util/cutils.c
>>
>> Conversion of call sites is good (in fact, you could drop the ' =
>> NULL' initialization, since do_strtosz() guarantees it is set to
>> something sane even on error).
> 
> Yes.
> 

Dropped.

>>                                 But where's the added test coverage of
>> rejecting "inf" as a size?
> 
> Let's stick it into test_qemu_strtosz_invalid().

Done.

@@ -2096,12 +2096,22 @@ static void test_qemu_strtosz_invalid(void)
     err = qemu_strtosz(str, &endptr, &res);
     g_assert_cmpint(err, ==, -EINVAL);
     g_assert(endptr == str);
+
+    str = "inf";
+    err = qemu_strtosz(str, &endptr, &res);
+    g_assert_cmpint(err, ==, -EINVAL);
+    g_assert(endptr == str);
+
+    str = "NaN";
+    err = qemu_strtosz(str, &endptr, &res);
+    g_assert_cmpint(err, ==, -EINVAL);
+    g_assert(endptr == str);
 }


> 
>>
>>> index 7868a683e8..dd61fb4558 100644
>>> --- a/util/cutils.c
>>> +++ b/util/cutils.c
>>> @@ -206,19 +206,17 @@ static int64_t suffix_mul(char suffix, int64_t unit)
>>>    * in *end, if not NULL. Return -ERANGE on overflow, Return -EINVAL on
>>>    * other error.
>>>    */
>>> -static int do_strtosz(const char *nptr, char **end,
>>> +static int do_strtosz(const char *nptr, const char **end,
>>>                         const char default_suffix, int64_t unit,
>>>                         uint64_t *result)
>>>   {
>>>       int retval;
>>> -    char *endptr;
>>> +    const char *endptr;
>>>       unsigned char c;
>>>       int mul_required = 0;
>>>       double val, mul, integral, fraction;
>>>   -    errno = 0;
>>> -    val = strtod(nptr, &endptr);
>>> -    if (isnan(val) || endptr == nptr || errno != 0) {
>>> +    if (qemu_strtod_finite(nptr, &endptr, &val)) {
>>>           retval = -EINVAL;
>>
>> This slams -ERANGE failure into -EINVAL.  Do we care?  Or would it
>> have been better to just do:
>>
>> retval = qemu_strtod_finite(...);
>> if (retval) {
>>     goto out;
> 
> If distinguishing between ERANGE and EINVAL makes sense for
> qemu_strtod_finite(), it probably makes sense for qemu_strtod(), too.

Yes, will do it like that.

> 
>>>           goto out;
>>>       }
>>> @@ -259,17 +257,17 @@ out:
>>
>> More context:
>>
>> out:
>>     if (end) {
>>         *end = endptr;
>>     } else if (*endptr) {
>>         retval = -EINVAL;
>>     }
>>
>>>       return retval;
>>>   }
>>
>> Everything else looks okay.
>>
>> Hmm - while touching this, is it worth making mul_required be a bool,
>> to match its use?
> 
> Touches no line containing mul_required, so I wouldn't.
> 

Alright, thanks to the both of you.

-- 

Thanks,

David / dhildenb

[Qemu-devel] [PATCH v1 3/9] qapi: use qemu_strtod_finite() in string-input-visitor

[David Hildenbrand]

Let's use the new function. "NaN" and "inf" are now properly rejected.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 qapi/string-input-visitor.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
index b3fdd0827d..b89c6c4e06 100644
--- a/qapi/string-input-visitor.c
+++ b/qapi/string-input-visitor.c
@@ -20,6 +20,7 @@
 #include "qemu/option.h"
 #include "qemu/queue.h"
 #include "qemu/range.h"
+#include "qemu/cutils.h"
 
 
 struct StringInputVisitor
@@ -313,12 +314,9 @@ static void parse_type_number(Visitor *v, const char *name, double *obj,
                               Error **errp)
 {
     StringInputVisitor *siv = to_siv(v);
-    char *endp = (char *) siv->string;
     double val;
 
-    errno = 0;
-    val = strtod(siv->string, &endp);
-    if (errno || endp == siv->string || *endp) {
+    if (qemu_strtod_finite(siv->string, NULL, &val)) {
         error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
                    "number");
         return;
-- 
2.17.2

Re: [Qemu-devel] [PATCH v1 3/9] qapi: use qemu_strtod_finite() in string-input-visitor

[Eric Blake]

On 11/15/18 8:04 AM, David Hildenbrand wrote:
> Let's use the new function. "NaN" and "inf" are now properly rejected.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>   qapi/string-input-visitor.c | 6 ++----
>   1 file changed, 2 insertions(+), 4 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

Should there be any added test coverage in this patch, or is it later in 
the series?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH v1 3/9] qapi: use qemu_strtod_finite() in string-input-visitor

[David Hildenbrand]

On 15.11.18 15:37, Eric Blake wrote:
> On 11/15/18 8:04 AM, David Hildenbrand wrote:
>> Let's use the new function. "NaN" and "inf" are now properly rejected.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>   qapi/string-input-visitor.c | 6 ++----
>>   1 file changed, 2 insertions(+), 4 deletions(-)
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> Should there be any added test coverage in this patch, or is it later in 
> the series?

Nope not yet, but can add.


-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v1 3/9] qapi: use qemu_strtod_finite() in string-input-visitor

[Markus Armbruster]

David Hildenbrand <david@redhat.com> writes:

> Let's use the new function. "NaN" and "inf" are now properly rejected.
>
> Signed-off-by: David Hildenbrand <david@redhat.com>

Whether this is a bug fix or just a change is debatable.  But the commit
message's title should highlight the change.  Perhaps you want to steal
from the one I suggested for the previous patch.

We need to assess backward compatibility impact.  I can do that.

> ---
>  qapi/string-input-visitor.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
> index b3fdd0827d..b89c6c4e06 100644
> --- a/qapi/string-input-visitor.c
> +++ b/qapi/string-input-visitor.c
> @@ -20,6 +20,7 @@
>  #include "qemu/option.h"
>  #include "qemu/queue.h"
>  #include "qemu/range.h"
> +#include "qemu/cutils.h"
>  
>  
>  struct StringInputVisitor
> @@ -313,12 +314,9 @@ static void parse_type_number(Visitor *v, const char *name, double *obj,
>                                Error **errp)
>  {
>      StringInputVisitor *siv = to_siv(v);
> -    char *endp = (char *) siv->string;
>      double val;
>  
> -    errno = 0;
> -    val = strtod(siv->string, &endp);
> -    if (errno || endp == siv->string || *endp) {
> +    if (qemu_strtod_finite(siv->string, NULL, &val)) {
>          error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
>                     "number");
>          return;

Re: [Qemu-devel] [PATCH v1 3/9] qapi: use qemu_strtod_finite() in string-input-visitor

[David Hildenbrand]

On 15.11.18 17:48, Markus Armbruster wrote:
> David Hildenbrand <david@redhat.com> writes:
> 
>> Let's use the new function. "NaN" and "inf" are now properly rejected.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
> 
> Whether this is a bug fix or just a change is debatable.  But the commit
> message's title should highlight the change.  Perhaps you want to steal
> from the one I suggested for the previous patch.

Yes, will copy+modify that one.

> 
> We need to assess backward compatibility impact.  I can do that.
Thanks! I assume this shouldn't be an issue.

-- 

Thanks,

David / dhildenb

[Qemu-devel] [PATCH v1 4/9] qapi: use qemu_strtod_finite() in qobject-input-visitor

[David Hildenbrand]

Let's use the new function. Just as current behavior, we have to
consume the whole string (now it's just way clearer what's going on).

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 qapi/qobject-input-visitor.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
index 3e88b27f9e..07465f9947 100644
--- a/qapi/qobject-input-visitor.c
+++ b/qapi/qobject-input-visitor.c
@@ -562,19 +562,20 @@ static void qobject_input_type_number_keyval(Visitor *v, const char *name,
 {
     QObjectInputVisitor *qiv = to_qiv(v);
     const char *str = qobject_input_get_keyval(qiv, name, errp);
-    char *endp;
+    double val;
 
     if (!str) {
         return;
     }
 
-    errno = 0;
-    *obj = strtod(str, &endp);
-    if (errno || endp == str || *endp || !isfinite(*obj)) {
+    if (qemu_strtod_finite(str, NULL, &val)) {
         /* TODO report -ERANGE more nicely */
         error_setg(errp, QERR_INVALID_PARAMETER_TYPE,
                    full_name(qiv, name), "number");
+        return;
     }
+
+    *obj = val;
 }
 
 static void qobject_input_type_any(Visitor *v, const char *name, QObject **obj,
-- 
2.17.2

Re: [Qemu-devel] [PATCH v1 4/9] qapi: use qemu_strtod_finite() in qobject-input-visitor

[Eric Blake]

On 11/15/18 8:04 AM, David Hildenbrand wrote:
> Let's use the new function. Just as current behavior, we have to
> consume the whole string (now it's just way clearer what's going on).
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>   qapi/qobject-input-visitor.c | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH v1 4/9] qapi: use qemu_strtod_finite() in qobject-input-visitor

[Markus Armbruster]

David Hildenbrand <david@redhat.com> writes:

> Let's use the new function. Just as current behavior, we have to
> consume the whole string (now it's just way clearer what's going on).
>
> Signed-off-by: David Hildenbrand <david@redhat.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

[Qemu-devel] [PATCH v1 5/9] test-string-input-visitor: add more tests

[David Hildenbrand]

Test that very big/small values are not accepted and that ranges with
only one element work.

Rename expect4 to expect5, as we will be moving that to a separate ulist
test after the rework.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 tests/test-string-input-visitor.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/tests/test-string-input-visitor.c b/tests/test-string-input-visitor.c
index 88e0e1aa9a..f55e0696c0 100644
--- a/tests/test-string-input-visitor.c
+++ b/tests/test-string-input-visitor.c
@@ -121,7 +121,8 @@ static void test_visitor_in_intList(TestInputVisitorData *data,
     int64_t expect1[] = { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 20 };
     int64_t expect2[] = { 32767, -32768, -32767 };
     int64_t expect3[] = { INT64_MAX, INT64_MIN };
-    uint64_t expect4[] = { UINT64_MAX };
+    int64_t expect4[] = { 1 };
+    uint64_t expect5[] = { UINT64_MAX };
     Error *err = NULL;
     int64List *res = NULL;
     int64List *tail;
@@ -140,8 +141,25 @@ static void test_visitor_in_intList(TestInputVisitorData *data,
                                 "-9223372036854775808,9223372036854775807");
     check_ilist(v, expect3, ARRAY_SIZE(expect3));
 
+    v = visitor_input_test_init(data, "1-1");
+    check_ilist(v, expect4, ARRAY_SIZE(expect4));
+
     v = visitor_input_test_init(data, "18446744073709551615");
-    check_ulist(v, expect4, ARRAY_SIZE(expect4));
+    check_ulist(v, expect5, ARRAY_SIZE(expect5));
+
+    /* Value too large */
+
+    v = visitor_input_test_init(data, "9223372036854775808");
+    visit_type_int64List(v, NULL, &res, &err);
+    error_free_or_abort(&err);
+    g_assert(!res);
+
+    /* Value too small */
+
+    v = visitor_input_test_init(data, "-9223372036854775809");
+    visit_type_int64List(v, NULL, &res, &err);
+    error_free_or_abort(&err);
+    g_assert(!res);
 
     /* Empty list */
 
-- 
2.17.2

Re: [Qemu-devel] [PATCH v1 5/9] test-string-input-visitor: add more tests

[Eric Blake]

On 11/15/18 8:04 AM, David Hildenbrand wrote:
> Test that very big/small values are not accepted and that ranges with
> only one element work.
> 
> Rename expect4 to expect5, as we will be moving that to a separate ulist
> test after the rework.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>   tests/test-string-input-visitor.c | 22 ++++++++++++++++++++--
>   1 file changed, 20 insertions(+), 2 deletions(-)
> 

I don't see a test for a range that wraps around (such as UINT_MAX-0); 
that's worth testing (whether it happens to work or is rejected as 
invalid).  Do we require ranges to be ascending, or does 6-5 result in 
the sequence 5, 6?  I also recall that our range code imposes a limit on 
the maximum elements present in a single range, in order to prevent 
denial-of-service attacks where a caller could request 0-INT_MAX to 
exhaust resources enumerating everything in the range; does our 
testsuite cover those limits?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH v1 5/9] test-string-input-visitor: add more tests

[David Hildenbrand]

On 15.11.18 18:13, Eric Blake wrote:
> On 11/15/18 8:04 AM, David Hildenbrand wrote:
>> Test that very big/small values are not accepted and that ranges with
>> only one element work.
>>
>> Rename expect4 to expect5, as we will be moving that to a separate ulist
>> test after the rework.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>   tests/test-string-input-visitor.c | 22 ++++++++++++++++++++--
>>   1 file changed, 20 insertions(+), 2 deletions(-)
>>
> 
> I don't see a test for a range that wraps around (such as UINT_MAX-0); 
> that's worth testing (whether it happens to work or is rejected as 
> invalid).  Do we require ranges to be ascending, or does 6-5 result in 
> the sequence 5, 6?  I also recall that our range code imposes a limit on 
> the maximum elements present in a single range, in order to prevent 
> denial-of-service attacks where a caller could request 0-INT_MAX to 
> exhaust resources enumerating everything in the range; does our 
> testsuite cover those limits?
>
Ranges have to be ascending and old code enforced that. New code still
enforces it. Wrapping ranges are AFAIC also not supported - not
ascending. I can add a test.

The limit is a good point. It is neither in the tests nor in the new
code. But now we finally have an explanation on the 65000-somewhat
thingy. I assume that we need such a limit?

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v1 5/9] test-string-input-visitor: add more tests

[Markus Armbruster]

David Hildenbrand <david@redhat.com> writes:

> On 15.11.18 18:13, Eric Blake wrote:
>> On 11/15/18 8:04 AM, David Hildenbrand wrote:
>>> Test that very big/small values are not accepted and that ranges with
>>> only one element work.
>>>
>>> Rename expect4 to expect5, as we will be moving that to a separate ulist
>>> test after the rework.
>>>
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>> ---
>>>   tests/test-string-input-visitor.c | 22 ++++++++++++++++++++--
>>>   1 file changed, 20 insertions(+), 2 deletions(-)
>>>
>> 
>> I don't see a test for a range that wraps around (such as UINT_MAX-0); 
>> that's worth testing (whether it happens to work or is rejected as 
>> invalid).  Do we require ranges to be ascending, or does 6-5 result in 
>> the sequence 5, 6?  I also recall that our range code imposes a limit on 
>> the maximum elements present in a single range, in order to prevent 
>> denial-of-service attacks where a caller could request 0-INT_MAX to 
>> exhaust resources enumerating everything in the range; does our 
>> testsuite cover those limits?
>>
> Ranges have to be ascending and old code enforced that. New code still
> enforces it. Wrapping ranges are AFAIC also not supported - not
> ascending. I can add a test.

Good.

> The limit is a good point. It is neither in the tests nor in the new
> code. But now we finally have an explanation on the 65000-somewhat
> thingy. I assume that we need such a limit?

Yes, we do.  We don't expect untrusted input here, but typo in the
monitor (say 0--1 parsed as uint64_t 0, UINT_MAX) killing the VM by
eating all memory is not a happy user experience.

[Qemu-devel] [PATCH v1 6/9] qapi: rewrite string-input-visitor

[David Hildenbrand]

The input visitor has some problems right now, especially
- unsigned type "Range" is used to process signed ranges, resulting in
  inconsistent behavior and ugly/magical code
- uint64_t are parsed like int64_t, so big uint64_t values are not
  supported and error messages are misleading
- lists/ranges of int64_t are accepted although no list is parsed and
  we should rather report an error
- lists/ranges are preparsed using int64_t, making it hard to
  implement uint64_t values or uint64_t lists
- types that don't support lists don't bail out
- visiting beyond the end of a list is not handled properly

So let's rewrite it by getting rid of usage of the type "Range" and
properly supporting lists of int64_t and uint64_t (including ranges of
both types), fixing the above mentioned issues.

Lists of other types are not supported and will properly report an
error. Virtual walks are now supported.

Tests have to be fixed up:
- Two BUGs were hardcoded that are fixed now
- The string-input-visitor now actually returns a parsed list and not
  an ordered set.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 include/qapi/string-input-visitor.h |   4 +-
 qapi/string-input-visitor.c         | 410 ++++++++++++++++------------
 tests/test-string-input-visitor.c   |  18 +-
 3 files changed, 239 insertions(+), 193 deletions(-)

diff --git a/include/qapi/string-input-visitor.h b/include/qapi/string-input-visitor.h
index 33551340e3..921f3875b9 100644
--- a/include/qapi/string-input-visitor.h
+++ b/include/qapi/string-input-visitor.h
@@ -19,8 +19,8 @@ typedef struct StringInputVisitor StringInputVisitor;
 
 /*
  * The string input visitor does not implement support for visiting
- * QAPI structs, alternates, null, or arbitrary QTypes.  It also
- * requires a non-null list argument to visit_start_list().
+ * QAPI structs, alternates, null, or arbitrary QTypes. Only flat lists
+ * of integers (except type "size") are supported.
  */
 Visitor *string_input_visitor_new(const char *str);
 
diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
index b89c6c4e06..4113be01fb 100644
--- a/qapi/string-input-visitor.c
+++ b/qapi/string-input-visitor.c
@@ -4,10 +4,10 @@
  * Copyright Red Hat, Inc. 2012-2016
  *
  * Author: Paolo Bonzini <pbonzini@redhat.com>
+ *         David Hildenbrand <david@redhat.com>
  *
  * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
  * See the COPYING.LIB file in the top-level directory.
- *
  */
 
 #include "qemu/osdep.h"
@@ -18,21 +18,39 @@
 #include "qapi/qmp/qerror.h"
 #include "qapi/qmp/qnull.h"
 #include "qemu/option.h"
-#include "qemu/queue.h"
-#include "qemu/range.h"
 #include "qemu/cutils.h"
 
+typedef enum ListMode {
+    /* no list parsing active / no list expected */
+    LM_NONE,
+    /* we have an unparsed string remaining */
+    LM_UNPARSED,
+    /* we have an unfinished int64 range */
+    LM_INT64_RANGE,
+    /* we have an unfinished uint64 range */
+    LM_UINT64_RANGE,
+    /* we have parsed the string completely and no range is remaining */
+    LM_END,
+} ListMode;
+
+typedef union RangeElement {
+    int64_t i64;
+    uint64_t u64;
+} RangeElement;
 
 struct StringInputVisitor
 {
     Visitor visitor;
 
-    GList *ranges;
-    GList *cur_range;
-    int64_t cur;
+    /* List parsing state */
+    ListMode lm;
+    RangeElement rangeNext;
+    RangeElement rangeEnd;
+    const char *unparsed_string;
+    void *list;
 
+    /* The original string to parse */
     const char *string;
-    void *list; /* Only needed for sanity checking the caller */
 };
 
 static StringInputVisitor *to_siv(Visitor *v)
@@ -40,136 +58,43 @@ static StringInputVisitor *to_siv(Visitor *v)
     return container_of(v, StringInputVisitor, visitor);
 }
 
-static void free_range(void *range, void *dummy)
-{
-    g_free(range);
-}
-
-static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
-{
-    char *str = (char *) siv->string;
-    long long start, end;
-    Range *cur;
-    char *endptr;
-
-    if (siv->ranges) {
-        return 0;
-    }
-
-    if (!*str) {
-        return 0;
-    }
-
-    do {
-        errno = 0;
-        start = strtoll(str, &endptr, 0);
-        if (errno == 0 && endptr > str) {
-            if (*endptr == '\0') {
-                cur = g_malloc0(sizeof(*cur));
-                range_set_bounds(cur, start, start);
-                siv->ranges = range_list_insert(siv->ranges, cur);
-                cur = NULL;
-                str = NULL;
-            } else if (*endptr == '-') {
-                str = endptr + 1;
-                errno = 0;
-                end = strtoll(str, &endptr, 0);
-                if (errno == 0 && endptr > str && start <= end &&
-                    (start > INT64_MAX - 65536 ||
-                     end < start + 65536)) {
-                    if (*endptr == '\0') {
-                        cur = g_malloc0(sizeof(*cur));
-                        range_set_bounds(cur, start, end);
-                        siv->ranges = range_list_insert(siv->ranges, cur);
-                        cur = NULL;
-                        str = NULL;
-                    } else if (*endptr == ',') {
-                        str = endptr + 1;
-                        cur = g_malloc0(sizeof(*cur));
-                        range_set_bounds(cur, start, end);
-                        siv->ranges = range_list_insert(siv->ranges, cur);
-                        cur = NULL;
-                    } else {
-                        goto error;
-                    }
-                } else {
-                    goto error;
-                }
-            } else if (*endptr == ',') {
-                str = endptr + 1;
-                cur = g_malloc0(sizeof(*cur));
-                range_set_bounds(cur, start, start);
-                siv->ranges = range_list_insert(siv->ranges, cur);
-                cur = NULL;
-            } else {
-                goto error;
-            }
-        } else {
-            goto error;
-        }
-    } while (str);
-
-    return 0;
-error:
-    g_list_foreach(siv->ranges, free_range, NULL);
-    g_list_free(siv->ranges);
-    siv->ranges = NULL;
-    error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null",
-               "an int64 value or range");
-    return -1;
-}
-
-static void
-start_list(Visitor *v, const char *name, GenericList **list, size_t size,
-           Error **errp)
+static void start_list(Visitor *v, const char *name, GenericList **list,
+                       size_t size, Error **errp)
 {
     StringInputVisitor *siv = to_siv(v);
 
-    /* We don't support visits without a list */
-    assert(list);
+    /* properly set the list parsing state */
+    assert(siv->lm == LM_NONE);
     siv->list = list;
+    siv->unparsed_string = siv->string;
 
-    if (parse_str(siv, name, errp) < 0) {
-        *list = NULL;
-        return;
-    }
-
-    siv->cur_range = g_list_first(siv->ranges);
-    if (siv->cur_range) {
-        Range *r = siv->cur_range->data;
-        if (r) {
-            siv->cur = range_lob(r);
+    if (!siv->string[0]) {
+        if (list) {
+            *list = NULL;
         }
-        *list = g_malloc0(size);
+        siv->lm = LM_END;
     } else {
-        *list = NULL;
+        if (list) {
+            *list = g_malloc0(size);
+        }
+        siv->lm = LM_UNPARSED;
     }
 }
 
 static GenericList *next_list(Visitor *v, GenericList *tail, size_t size)
 {
     StringInputVisitor *siv = to_siv(v);
-    Range *r;
 
-    if (!siv->ranges || !siv->cur_range) {
+    switch (siv->lm) {
+    case LM_END:
         return NULL;
-    }
-
-    r = siv->cur_range->data;
-    if (!r) {
-        return NULL;
-    }
-
-    if (!range_contains(r, siv->cur)) {
-        siv->cur_range = g_list_next(siv->cur_range);
-        if (!siv->cur_range) {
-            return NULL;
-        }
-        r = siv->cur_range->data;
-        if (!r) {
-            return NULL;
-        }
-        siv->cur = range_lob(r);
+    case LM_INT64_RANGE:
+    case LM_UINT64_RANGE:
+    case LM_UNPARSED:
+        /* we have an unparsed string or something left in a range */
+        break;
+    default:
+        abort();
     }
 
     tail->next = g_malloc0(size);
@@ -179,88 +104,215 @@ static GenericList *next_list(Visitor *v, GenericList *tail, size_t size)
 static void check_list(Visitor *v, Error **errp)
 {
     const StringInputVisitor *siv = to_siv(v);
-    Range *r;
-    GList *cur_range;
 
-    if (!siv->ranges || !siv->cur_range) {
+    switch (siv->lm) {
+    case LM_INT64_RANGE:
+    case LM_UINT64_RANGE:
+    case LM_UNPARSED:
+        error_setg(errp, "Fewer list elements expected");
         return;
-    }
-
-    r = siv->cur_range->data;
-    if (!r) {
+    case LM_END:
         return;
+    default:
+        abort();
     }
-
-    if (!range_contains(r, siv->cur)) {
-        cur_range = g_list_next(siv->cur_range);
-        if (!cur_range) {
-            return;
-        }
-        r = cur_range->data;
-        if (!r) {
-            return;
-        }
-    }
-
-    error_setg(errp, "Range contains too many values");
 }
 
 static void end_list(Visitor *v, void **obj)
 {
     StringInputVisitor *siv = to_siv(v);
 
+    assert(siv->lm != LM_NONE);
     assert(siv->list == obj);
+    siv->list = NULL;
+    siv->unparsed_string = NULL;
+    siv->lm = LM_NONE;
+}
+
+static int try_parse_int64_list_entry(StringInputVisitor *siv, int64_t *obj)
+{
+    const char *endptr;
+    int64_t start, end;
+
+    if (qemu_strtoi64(siv->unparsed_string, &endptr, 0, &start)) {
+        return -EINVAL;
+    }
+    end = start;
+
+    switch (endptr[0]) {
+    case '\0':
+        siv->unparsed_string = endptr;
+        break;
+    case ',':
+        siv->unparsed_string = endptr + 1;
+        break;
+    case '-':
+        /* parse the end of the range */
+        if (qemu_strtoi64(endptr + 1, &endptr, 0, &end)) {
+            return -EINVAL;
+        }
+        if (start > end) {
+            return -EINVAL;
+        }
+        switch (endptr[0]) {
+        case '\0':
+            siv->unparsed_string = endptr;
+            break;
+        case ',':
+            siv->unparsed_string = endptr + 1;
+            break;
+        default:
+            return -EINVAL;
+        }
+        break;
+    default:
+        return -EINVAL;
+    }
+
+    /* we have a proper range (with maybe only one element) */
+    siv->lm = LM_INT64_RANGE;
+    siv->rangeNext.i64 = start;
+    siv->rangeEnd.i64 = end;
+    return 0;
 }
 
 static void parse_type_int64(Visitor *v, const char *name, int64_t *obj,
                              Error **errp)
 {
     StringInputVisitor *siv = to_siv(v);
-
-    if (parse_str(siv, name, errp) < 0) {
+    int64_t val;
+
+    switch (siv->lm) {
+    case LM_NONE:
+        /* just parse a simple int64, bail out if not completely consumed */
+        if (qemu_strtoi64(siv->string, NULL, 0, &val)) {
+                error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
+                           name ? name : "null", "int64");
+            return;
+        }
+        *obj = val;
         return;
+    case LM_UNPARSED:
+        if (try_parse_int64_list_entry(siv, obj)) {
+            error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null",
+                       "list of int64 values or ranges");
+            return;
+        }
+        assert(siv->lm == LM_INT64_RANGE);
+        /* FALLTHROUGH */
+    case LM_INT64_RANGE:
+        /* return the next element in the range */
+        assert(siv->rangeNext.i64 <= siv->rangeEnd.i64);
+        *obj = siv->rangeNext.i64++;
+
+        if (siv->rangeNext.i64 > siv->rangeEnd.i64 || *obj == INT64_MAX) {
+            /* end of range, check if there is more to parse */
+            if (siv->unparsed_string[0]) {
+                siv->lm = LM_UNPARSED;
+            } else {
+                siv->lm = LM_END;
+            }
+        }
+        return;
+    case LM_END:
+        error_setg(errp, "Fewer list elements expected");
+        return;
+    default:
+        abort();
     }
+}
 
-    if (!siv->ranges) {
-        goto error;
-    }
-
-    if (!siv->cur_range) {
-        Range *r;
+static int try_parse_uint64_list_entry(StringInputVisitor *siv, uint64_t *obj)
+{
+    const char *endptr;
+    uint64_t start, end;
 
-        siv->cur_range = g_list_first(siv->ranges);
-        if (!siv->cur_range) {
-            goto error;
+    /* parse a simple uint64 or range */
+    if (qemu_strtou64(siv->unparsed_string, &endptr, 0, &start)) {
+        return -EINVAL;
+    }
+    end = start;
+
+    switch (endptr[0]) {
+    case '\0':
+        siv->unparsed_string = endptr;
+        break;
+    case ',':
+        siv->unparsed_string = endptr + 1;
+        break;
+    case '-':
+        /* parse the end of the range */
+        if (qemu_strtou64(endptr + 1, &endptr, 0, &end)) {
+            return -EINVAL;
         }
-
-        r = siv->cur_range->data;
-        if (!r) {
-            goto error;
+        if (start > end) {
+            return -EINVAL;
         }
-
-        siv->cur = range_lob(r);
+        switch (endptr[0]) {
+        case '\0':
+            siv->unparsed_string = endptr;
+            break;
+        case ',':
+            siv->unparsed_string = endptr + 1;
+            break;
+        default:
+            return -EINVAL;
+        }
+        break;
+    default:
+        return -EINVAL;
     }
 
-    *obj = siv->cur;
-    siv->cur++;
-    return;
-
-error:
-    error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null",
-               "an int64 value or range");
+    /* we have a proper range (with maybe only one element) */
+    siv->lm = LM_UINT64_RANGE;
+    siv->rangeNext.u64 = start;
+    siv->rangeEnd.u64 = end;
+    return 0;
 }
 
 static void parse_type_uint64(Visitor *v, const char *name, uint64_t *obj,
                               Error **errp)
 {
-    /* FIXME: parse_type_int64 mishandles values over INT64_MAX */
-    int64_t i;
-    Error *err = NULL;
-    parse_type_int64(v, name, &i, &err);
-    if (err) {
-        error_propagate(errp, err);
-    } else {
-        *obj = i;
+    StringInputVisitor *siv = to_siv(v);
+    uint64_t val;
+
+    switch (siv->lm) {
+    case LM_NONE:
+        /* just parse a simple uint64, bail out if not completely consumed */
+        if (qemu_strtou64(siv->string, NULL, 0, &val)) {
+            error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null",
+                       "uint64");
+            return;
+        }
+        *obj = val;
+        return;
+    case LM_UNPARSED:
+        if (try_parse_uint64_list_entry(siv, obj)) {
+            error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null",
+                       "list of uint64 values or ranges");
+            return;
+        }
+        assert(siv->lm == LM_UINT64_RANGE);
+        /* FALLTHROUGH */
+    case LM_UINT64_RANGE:
+        /* return the next element in the range */
+        assert(siv->rangeNext.u64 <= siv->rangeEnd.u64);
+        *obj = siv->rangeNext.u64++;
+
+        if (siv->rangeNext.u64 > siv->rangeEnd.u64 || *obj == UINT64_MAX) {
+            /* end of range, check if there is more to parse */
+            if (siv->unparsed_string[0]) {
+                siv->lm = LM_UNPARSED;
+            } else {
+                siv->lm = LM_END;
+            }
+        }
+        return;
+    case LM_END:
+        error_setg(errp, "Fewer list elements expected");
+        return;
+    default:
+        abort();
     }
 }
 
@@ -271,6 +323,7 @@ static void parse_type_size(Visitor *v, const char *name, uint64_t *obj,
     Error *err = NULL;
     uint64_t val;
 
+    assert(siv->lm == LM_NONE);
     parse_option_size(name, siv->string, &val, &err);
     if (err) {
         error_propagate(errp, err);
@@ -285,6 +338,7 @@ static void parse_type_bool(Visitor *v, const char *name, bool *obj,
 {
     StringInputVisitor *siv = to_siv(v);
 
+    assert(siv->lm == LM_NONE);
     if (!strcasecmp(siv->string, "on") ||
         !strcasecmp(siv->string, "yes") ||
         !strcasecmp(siv->string, "true")) {
@@ -307,6 +361,7 @@ static void parse_type_str(Visitor *v, const char *name, char **obj,
 {
     StringInputVisitor *siv = to_siv(v);
 
+    assert(siv->lm == LM_NONE);
     *obj = g_strdup(siv->string);
 }
 
@@ -316,6 +371,7 @@ static void parse_type_number(Visitor *v, const char *name, double *obj,
     StringInputVisitor *siv = to_siv(v);
     double val;
 
+    assert(siv->lm == LM_NONE);
     if (qemu_strtod_finite(siv->string, NULL, &val)) {
         error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
                    "number");
@@ -330,9 +386,10 @@ static void parse_type_null(Visitor *v, const char *name, QNull **obj,
 {
     StringInputVisitor *siv = to_siv(v);
 
+    assert(siv->lm == LM_NONE);
     *obj = NULL;
 
-    if (!siv->string || siv->string[0]) {
+    if (siv->string[0]) {
         error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
                    "null");
         return;
@@ -345,8 +402,6 @@ static void string_input_free(Visitor *v)
 {
     StringInputVisitor *siv = to_siv(v);
 
-    g_list_foreach(siv->ranges, free_range, NULL);
-    g_list_free(siv->ranges);
     g_free(siv);
 }
 
@@ -372,5 +427,6 @@ Visitor *string_input_visitor_new(const char *str)
     v->visitor.free = string_input_free;
 
     v->string = str;
+    v->lm = LM_NONE;
     return &v->visitor;
 }
diff --git a/tests/test-string-input-visitor.c b/tests/test-string-input-visitor.c
index f55e0696c0..a198aedfce 100644
--- a/tests/test-string-input-visitor.c
+++ b/tests/test-string-input-visitor.c
@@ -92,16 +92,6 @@ static void check_ulist(Visitor *v, uint64_t *expected, size_t n)
     uint64List *tail;
     int i;
 
-    /* BUG: unsigned numbers above INT64_MAX don't work */
-    for (i = 0; i < n; i++) {
-        if (expected[i] > INT64_MAX) {
-            Error *err = NULL;
-            visit_type_uint64List(v, NULL, &res, &err);
-            error_free_or_abort(&err);
-            return;
-        }
-    }
-
     visit_type_uint64List(v, NULL, &res, &error_abort);
     tail = res;
     for (i = 0; i < n; i++) {
@@ -117,10 +107,10 @@ static void check_ulist(Visitor *v, uint64_t *expected, size_t n)
 static void test_visitor_in_intList(TestInputVisitorData *data,
                                     const void *unused)
 {
-    /* Note: the visitor *sorts* ranges *unsigned* */
-    int64_t expect1[] = { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 20 };
+    int64_t expect1[] = { 1, 2, 0, 2, 3, 4, 20, 5, 6, 7,
+                          8, 9, 1, 2, 3, 4, 5, 6, 7, 8 };
     int64_t expect2[] = { 32767, -32768, -32767 };
-    int64_t expect3[] = { INT64_MAX, INT64_MIN };
+    int64_t expect3[] = { INT64_MIN, INT64_MAX };
     int64_t expect4[] = { 1 };
     uint64_t expect5[] = { UINT64_MAX };
     Error *err = NULL;
@@ -207,7 +197,7 @@ static void test_visitor_in_intList(TestInputVisitorData *data,
     visit_type_int64(v, NULL, &tail->value, &err);
     g_assert_cmpint(tail->value, ==, 0);
     visit_type_int64(v, NULL, &val, &err);
-    g_assert_cmpint(val, ==, 1); /* BUG */
+    error_free_or_abort(&err);
     visit_check_list(v, &error_abort);
     visit_end_list(v, (void **)&res);
 
-- 
2.17.2

Re: [Qemu-devel] [PATCH v1 6/9] qapi: rewrite string-input-visitor

[Markus Armbruster]

David Hildenbrand <david@redhat.com> writes:

> The input visitor has some problems right now, especially
> - unsigned type "Range" is used to process signed ranges, resulting in
>   inconsistent behavior and ugly/magical code
> - uint64_t are parsed like int64_t, so big uint64_t values are not
>   supported and error messages are misleading
> - lists/ranges of int64_t are accepted although no list is parsed and
>   we should rather report an error
> - lists/ranges are preparsed using int64_t, making it hard to
>   implement uint64_t values or uint64_t lists
> - types that don't support lists don't bail out
> - visiting beyond the end of a list is not handled properly

  - We don't actually parse lists, we parse *sets*: members are sorted,
    and duplicates eliminated

Reproducers for the problems would be nice.  Suggestion, not demand.

>
> So let's rewrite it by getting rid of usage of the type "Range" and
> properly supporting lists of int64_t and uint64_t (including ranges of
> both types), fixing the above mentioned issues.
>
> Lists of other types are not supported and will properly report an
> error. Virtual walks are now supported.
>
> Tests have to be fixed up:
> - Two BUGs were hardcoded that are fixed now
> - The string-input-visitor now actually returns a parsed list and not
>   an ordered set.

I'd expect this to necessitate an update of callers that expect a set, but...

> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  include/qapi/string-input-visitor.h |   4 +-
>  qapi/string-input-visitor.c         | 410 ++++++++++++++++------------
>  tests/test-string-input-visitor.c   |  18 +-
>  3 files changed, 239 insertions(+), 193 deletions(-)

... there's none.

Let me know if you need help finding them.  I think we tracked them down
during the discussion that led to this series.

>
> diff --git a/include/qapi/string-input-visitor.h b/include/qapi/string-input-visitor.h
> index 33551340e3..921f3875b9 100644
> --- a/include/qapi/string-input-visitor.h
> +++ b/include/qapi/string-input-visitor.h
> @@ -19,8 +19,8 @@ typedef struct StringInputVisitor StringInputVisitor;
>  
>  /*
>   * The string input visitor does not implement support for visiting
> - * QAPI structs, alternates, null, or arbitrary QTypes.  It also
> - * requires a non-null list argument to visit_start_list().
> + * QAPI structs, alternates, null, or arbitrary QTypes. Only flat lists
> + * of integers (except type "size") are supported.
>   */
>  Visitor *string_input_visitor_new(const char *str);
>  
> diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
> index b89c6c4e06..4113be01fb 100644
> --- a/qapi/string-input-visitor.c
> +++ b/qapi/string-input-visitor.c
> @@ -4,10 +4,10 @@
>   * Copyright Red Hat, Inc. 2012-2016
>   *
>   * Author: Paolo Bonzini <pbonzini@redhat.com>
> + *         David Hildenbrand <david@redhat.com>
>   *
>   * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
>   * See the COPYING.LIB file in the top-level directory.
> - *
>   */
>  
>  #include "qemu/osdep.h"
> @@ -18,21 +18,39 @@
>  #include "qapi/qmp/qerror.h"
>  #include "qapi/qmp/qnull.h"
>  #include "qemu/option.h"
> -#include "qemu/queue.h"
> -#include "qemu/range.h"
>  #include "qemu/cutils.h"
>  
> +typedef enum ListMode {
> +    /* no list parsing active / no list expected */
> +    LM_NONE,
> +    /* we have an unparsed string remaining */
> +    LM_UNPARSED,
> +    /* we have an unfinished int64 range */
> +    LM_INT64_RANGE,
> +    /* we have an unfinished uint64 range */
> +    LM_UINT64_RANGE,
> +    /* we have parsed the string completely and no range is remaining */
> +    LM_END,
> +} ListMode;
> +
> +typedef union RangeElement {
> +    int64_t i64;
> +    uint64_t u64;
> +} RangeElement;
>  
>  struct StringInputVisitor
>  {
>      Visitor visitor;
>  
> -    GList *ranges;
> -    GList *cur_range;
> -    int64_t cur;
> +    /* List parsing state */
> +    ListMode lm;
> +    RangeElement rangeNext;
> +    RangeElement rangeEnd;
> +    const char *unparsed_string;
> +    void *list;
>  
> +    /* The original string to parse */
>      const char *string;
> -    void *list; /* Only needed for sanity checking the caller */
>  };
>  
>  static StringInputVisitor *to_siv(Visitor *v)
> @@ -40,136 +58,43 @@ static StringInputVisitor *to_siv(Visitor *v)
>      return container_of(v, StringInputVisitor, visitor);
>  }
>  
> -static void free_range(void *range, void *dummy)
> -{
> -    g_free(range);
> -}
> -
> -static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
> -{
> -    char *str = (char *) siv->string;
> -    long long start, end;
> -    Range *cur;
> -    char *endptr;
> -
> -    if (siv->ranges) {
> -        return 0;
> -    }
> -
> -    if (!*str) {
> -        return 0;
> -    }
> -
> -    do {
> -        errno = 0;
> -        start = strtoll(str, &endptr, 0);
> -        if (errno == 0 && endptr > str) {
> -            if (*endptr == '\0') {
> -                cur = g_malloc0(sizeof(*cur));
> -                range_set_bounds(cur, start, start);
> -                siv->ranges = range_list_insert(siv->ranges, cur);
> -                cur = NULL;
> -                str = NULL;
> -            } else if (*endptr == '-') {
> -                str = endptr + 1;
> -                errno = 0;
> -                end = strtoll(str, &endptr, 0);
> -                if (errno == 0 && endptr > str && start <= end &&
> -                    (start > INT64_MAX - 65536 ||
> -                     end < start + 65536)) {
> -                    if (*endptr == '\0') {
> -                        cur = g_malloc0(sizeof(*cur));
> -                        range_set_bounds(cur, start, end);
> -                        siv->ranges = range_list_insert(siv->ranges, cur);
> -                        cur = NULL;
> -                        str = NULL;
> -                    } else if (*endptr == ',') {
> -                        str = endptr + 1;
> -                        cur = g_malloc0(sizeof(*cur));
> -                        range_set_bounds(cur, start, end);
> -                        siv->ranges = range_list_insert(siv->ranges, cur);
> -                        cur = NULL;
> -                    } else {
> -                        goto error;
> -                    }
> -                } else {
> -                    goto error;
> -                }
> -            } else if (*endptr == ',') {
> -                str = endptr + 1;
> -                cur = g_malloc0(sizeof(*cur));
> -                range_set_bounds(cur, start, start);
> -                siv->ranges = range_list_insert(siv->ranges, cur);
> -                cur = NULL;
> -            } else {
> -                goto error;
> -            }
> -        } else {
> -            goto error;
> -        }
> -    } while (str);
> -
> -    return 0;
> -error:
> -    g_list_foreach(siv->ranges, free_range, NULL);
> -    g_list_free(siv->ranges);
> -    siv->ranges = NULL;
> -    error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null",
> -               "an int64 value or range");
> -    return -1;
> -}
> -
> -static void
> -start_list(Visitor *v, const char *name, GenericList **list, size_t size,
> -           Error **errp)
> +static void start_list(Visitor *v, const char *name, GenericList **list,
> +                       size_t size, Error **errp)
>  {
>      StringInputVisitor *siv = to_siv(v);
>  
> -    /* We don't support visits without a list */
> -    assert(list);
> +    /* properly set the list parsing state */

This comment feels redundant.

> +    assert(siv->lm == LM_NONE);
>      siv->list = list;
> +    siv->unparsed_string = siv->string;
>  
> -    if (parse_str(siv, name, errp) < 0) {
> -        *list = NULL;
> -        return;
> -    }
> -
> -    siv->cur_range = g_list_first(siv->ranges);
> -    if (siv->cur_range) {
> -        Range *r = siv->cur_range->data;
> -        if (r) {
> -            siv->cur = range_lob(r);
> +    if (!siv->string[0]) {
> +        if (list) {
> +            *list = NULL;
>          }
> -        *list = g_malloc0(size);
> +        siv->lm = LM_END;
>      } else {
> -        *list = NULL;
> +        if (list) {
> +            *list = g_malloc0(size);
> +        }
> +        siv->lm = LM_UNPARSED;
>      }
>  }
>  
>  static GenericList *next_list(Visitor *v, GenericList *tail, size_t size)
>  {
>      StringInputVisitor *siv = to_siv(v);
> -    Range *r;
>  
> -    if (!siv->ranges || !siv->cur_range) {
> +    switch (siv->lm) {
> +    case LM_END:
>          return NULL;
> -    }
> -
> -    r = siv->cur_range->data;
> -    if (!r) {
> -        return NULL;
> -    }
> -
> -    if (!range_contains(r, siv->cur)) {
> -        siv->cur_range = g_list_next(siv->cur_range);
> -        if (!siv->cur_range) {
> -            return NULL;
> -        }
> -        r = siv->cur_range->data;
> -        if (!r) {
> -            return NULL;
> -        }
> -        siv->cur = range_lob(r);
> +    case LM_INT64_RANGE:
> +    case LM_UINT64_RANGE:
> +    case LM_UNPARSED:
> +        /* we have an unparsed string or something left in a range */
> +        break;
> +    default:
> +        abort();
>      }
>  
>      tail->next = g_malloc0(size);
> @@ -179,88 +104,215 @@ static GenericList *next_list(Visitor *v, GenericList *tail, size_t size)
>  static void check_list(Visitor *v, Error **errp)
>  {
>      const StringInputVisitor *siv = to_siv(v);
> -    Range *r;
> -    GList *cur_range;
>  
> -    if (!siv->ranges || !siv->cur_range) {
> +    switch (siv->lm) {
> +    case LM_INT64_RANGE:
> +    case LM_UINT64_RANGE:
> +    case LM_UNPARSED:
> +        error_setg(errp, "Fewer list elements expected");
>          return;
> -    }
> -
> -    r = siv->cur_range->data;
> -    if (!r) {
> +    case LM_END:
>          return;
> +    default:
> +        abort();
>      }
> -
> -    if (!range_contains(r, siv->cur)) {
> -        cur_range = g_list_next(siv->cur_range);
> -        if (!cur_range) {
> -            return;
> -        }
> -        r = cur_range->data;
> -        if (!r) {
> -            return;
> -        }
> -    }
> -
> -    error_setg(errp, "Range contains too many values");
>  }
>  
>  static void end_list(Visitor *v, void **obj)
>  {
>      StringInputVisitor *siv = to_siv(v);
>  
> +    assert(siv->lm != LM_NONE);
>      assert(siv->list == obj);
> +    siv->list = NULL;
> +    siv->unparsed_string = NULL;
> +    siv->lm = LM_NONE;
> +}
> +
> +static int try_parse_int64_list_entry(StringInputVisitor *siv, int64_t *obj)
> +{
> +    const char *endptr;
> +    int64_t start, end;
> +
> +    if (qemu_strtoi64(siv->unparsed_string, &endptr, 0, &start)) {
> +        return -EINVAL;
> +    }
> +    end = start;
> +
> +    switch (endptr[0]) {
> +    case '\0':
> +        siv->unparsed_string = endptr;
> +        break;
> +    case ',':
> +        siv->unparsed_string = endptr + 1;
> +        break;
> +    case '-':
> +        /* parse the end of the range */
> +        if (qemu_strtoi64(endptr + 1, &endptr, 0, &end)) {
> +            return -EINVAL;
> +        }
> +        if (start > end) {
> +            return -EINVAL;
> +        }
> +        switch (endptr[0]) {
> +        case '\0':
> +            siv->unparsed_string = endptr;
> +            break;
> +        case ',':
> +            siv->unparsed_string = endptr + 1;
> +            break;
> +        default:
> +            return -EINVAL;
> +        }
> +        break;
> +    default:
> +        return -EINVAL;
> +    }
> +
> +    /* we have a proper range (with maybe only one element) */
> +    siv->lm = LM_INT64_RANGE;
> +    siv->rangeNext.i64 = start;
> +    siv->rangeEnd.i64 = end;
> +    return 0;
>  }
>  
>  static void parse_type_int64(Visitor *v, const char *name, int64_t *obj,
>                               Error **errp)
>  {
>      StringInputVisitor *siv = to_siv(v);
> -
> -    if (parse_str(siv, name, errp) < 0) {
> +    int64_t val;
> +
> +    switch (siv->lm) {
> +    case LM_NONE:
> +        /* just parse a simple int64, bail out if not completely consumed */
> +        if (qemu_strtoi64(siv->string, NULL, 0, &val)) {
> +                error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
> +                           name ? name : "null", "int64");
> +            return;
> +        }
> +        *obj = val;
>          return;
> +    case LM_UNPARSED:
> +        if (try_parse_int64_list_entry(siv, obj)) {
> +            error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null",
> +                       "list of int64 values or ranges");
> +            return;
> +        }
> +        assert(siv->lm == LM_INT64_RANGE);
> +        /* FALLTHROUGH */

Please spell it /* fall through */, because:

$ git-grep -Ei '/\* *fall.?thr[a-z]* *\*/' | sed 's/.*\* *\(fall[^*]*\)\*.*/\1/i' | sort | uniq -c
      4 FALL THROUGH 
      8 FALLTHROUGH 
     61 FALLTHRU 
     36 Fall through 
     20 Fallthrough 
      4 Fallthru 
    237 fall through 
      1 fall-through 
     16 fallthrough 
     39 fallthru 

> +    case LM_INT64_RANGE:
> +        /* return the next element in the range */
> +        assert(siv->rangeNext.i64 <= siv->rangeEnd.i64);
> +        *obj = siv->rangeNext.i64++;
> +
> +        if (siv->rangeNext.i64 > siv->rangeEnd.i64 || *obj == INT64_MAX) {
> +            /* end of range, check if there is more to parse */
> +            if (siv->unparsed_string[0]) {
> +                siv->lm = LM_UNPARSED;
> +            } else {
> +                siv->lm = LM_END;
> +            }

I'd do

               siv->lm = siv->unparsed_string[0] ? LM_UNPARSED : LM_END;

Matter of taste, entirely up to you.

> +        }
> +        return;
> +    case LM_END:
> +        error_setg(errp, "Fewer list elements expected");
> +        return;
> +    default:
> +        abort();
>      }
> +}
>  
> -    if (!siv->ranges) {
> -        goto error;
> -    }
> -
> -    if (!siv->cur_range) {
> -        Range *r;
> +static int try_parse_uint64_list_entry(StringInputVisitor *siv, uint64_t *obj)
> +{
> +    const char *endptr;
> +    uint64_t start, end;
>  
> -        siv->cur_range = g_list_first(siv->ranges);
> -        if (!siv->cur_range) {
> -            goto error;
> +    /* parse a simple uint64 or range */

try_parse_int64_list_entry() doesn't have this comment.  I think either
both or none should have it.  You decide.

> +    if (qemu_strtou64(siv->unparsed_string, &endptr, 0, &start)) {
> +        return -EINVAL;
> +    }
> +    end = start;
> +
> +    switch (endptr[0]) {
> +    case '\0':
> +        siv->unparsed_string = endptr;
> +        break;
> +    case ',':
> +        siv->unparsed_string = endptr + 1;
> +        break;
> +    case '-':
> +        /* parse the end of the range */
> +        if (qemu_strtou64(endptr + 1, &endptr, 0, &end)) {
> +            return -EINVAL;
>          }
> -
> -        r = siv->cur_range->data;
> -        if (!r) {
> -            goto error;
> +        if (start > end) {
> +            return -EINVAL;
>          }
> -
> -        siv->cur = range_lob(r);
> +        switch (endptr[0]) {
> +        case '\0':
> +            siv->unparsed_string = endptr;
> +            break;
> +        case ',':
> +            siv->unparsed_string = endptr + 1;
> +            break;
> +        default:
> +            return -EINVAL;
> +        }
> +        break;
> +    default:
> +        return -EINVAL;
>      }
>  
> -    *obj = siv->cur;
> -    siv->cur++;
> -    return;
> -
> -error:
> -    error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null",
> -               "an int64 value or range");
> +    /* we have a proper range (with maybe only one element) */
> +    siv->lm = LM_UINT64_RANGE;
> +    siv->rangeNext.u64 = start;
> +    siv->rangeEnd.u64 = end;
> +    return 0;
>  }
>  
>  static void parse_type_uint64(Visitor *v, const char *name, uint64_t *obj,
>                                Error **errp)
>  {
> -    /* FIXME: parse_type_int64 mishandles values over INT64_MAX */
> -    int64_t i;
> -    Error *err = NULL;
> -    parse_type_int64(v, name, &i, &err);
> -    if (err) {
> -        error_propagate(errp, err);
> -    } else {
> -        *obj = i;
> +    StringInputVisitor *siv = to_siv(v);
> +    uint64_t val;
> +
> +    switch (siv->lm) {
> +    case LM_NONE:
> +        /* just parse a simple uint64, bail out if not completely consumed */
> +        if (qemu_strtou64(siv->string, NULL, 0, &val)) {
> +            error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null",
> +                       "uint64");
> +            return;
> +        }
> +        *obj = val;
> +        return;
> +    case LM_UNPARSED:
> +        if (try_parse_uint64_list_entry(siv, obj)) {
> +            error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null",
> +                       "list of uint64 values or ranges");
> +            return;
> +        }
> +        assert(siv->lm == LM_UINT64_RANGE);
> +        /* FALLTHROUGH */
> +    case LM_UINT64_RANGE:
> +        /* return the next element in the range */
> +        assert(siv->rangeNext.u64 <= siv->rangeEnd.u64);
> +        *obj = siv->rangeNext.u64++;
> +
> +        if (siv->rangeNext.u64 > siv->rangeEnd.u64 || *obj == UINT64_MAX) {
> +            /* end of range, check if there is more to parse */
> +            if (siv->unparsed_string[0]) {
> +                siv->lm = LM_UNPARSED;
> +            } else {
> +                siv->lm = LM_END;
> +            }
> +        }
> +        return;
> +    case LM_END:
> +        error_setg(errp, "Fewer list elements expected");
> +        return;
> +    default:
> +        abort();
>      }
>  }
>  
> @@ -271,6 +323,7 @@ static void parse_type_size(Visitor *v, const char *name, uint64_t *obj,
>      Error *err = NULL;
>      uint64_t val;
>  
> +    assert(siv->lm == LM_NONE);
>      parse_option_size(name, siv->string, &val, &err);
>      if (err) {
>          error_propagate(errp, err);
> @@ -285,6 +338,7 @@ static void parse_type_bool(Visitor *v, const char *name, bool *obj,
>  {
>      StringInputVisitor *siv = to_siv(v);
>  
> +    assert(siv->lm == LM_NONE);
>      if (!strcasecmp(siv->string, "on") ||
>          !strcasecmp(siv->string, "yes") ||
>          !strcasecmp(siv->string, "true")) {
> @@ -307,6 +361,7 @@ static void parse_type_str(Visitor *v, const char *name, char **obj,
>  {
>      StringInputVisitor *siv = to_siv(v);
>  
> +    assert(siv->lm == LM_NONE);
>      *obj = g_strdup(siv->string);
>  }
>  
> @@ -316,6 +371,7 @@ static void parse_type_number(Visitor *v, const char *name, double *obj,
>      StringInputVisitor *siv = to_siv(v);
>      double val;
>  
> +    assert(siv->lm == LM_NONE);
>      if (qemu_strtod_finite(siv->string, NULL, &val)) {
>          error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
>                     "number");
> @@ -330,9 +386,10 @@ static void parse_type_null(Visitor *v, const char *name, QNull **obj,
>  {
>      StringInputVisitor *siv = to_siv(v);
>  
> +    assert(siv->lm == LM_NONE);
>      *obj = NULL;
>  
> -    if (!siv->string || siv->string[0]) {
> +    if (siv->string[0]) {
>          error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
>                     "null");
>          return;
> @@ -345,8 +402,6 @@ static void string_input_free(Visitor *v)
>  {
>      StringInputVisitor *siv = to_siv(v);
>  
> -    g_list_foreach(siv->ranges, free_range, NULL);
> -    g_list_free(siv->ranges);
>      g_free(siv);
>  }
>  
> @@ -372,5 +427,6 @@ Visitor *string_input_visitor_new(const char *str)
>      v->visitor.free = string_input_free;
>  
>      v->string = str;
> +    v->lm = LM_NONE;
>      return &v->visitor;
>  }
> diff --git a/tests/test-string-input-visitor.c b/tests/test-string-input-visitor.c
> index f55e0696c0..a198aedfce 100644
> --- a/tests/test-string-input-visitor.c
> +++ b/tests/test-string-input-visitor.c
> @@ -92,16 +92,6 @@ static void check_ulist(Visitor *v, uint64_t *expected, size_t n)
>      uint64List *tail;
>      int i;
>  
> -    /* BUG: unsigned numbers above INT64_MAX don't work */
> -    for (i = 0; i < n; i++) {
> -        if (expected[i] > INT64_MAX) {
> -            Error *err = NULL;
> -            visit_type_uint64List(v, NULL, &res, &err);
> -            error_free_or_abort(&err);
> -            return;
> -        }
> -    }
> -
>      visit_type_uint64List(v, NULL, &res, &error_abort);
>      tail = res;
>      for (i = 0; i < n; i++) {
> @@ -117,10 +107,10 @@ static void check_ulist(Visitor *v, uint64_t *expected, size_t n)
>  static void test_visitor_in_intList(TestInputVisitorData *data,
>                                      const void *unused)
>  {
> -    /* Note: the visitor *sorts* ranges *unsigned* */
> -    int64_t expect1[] = { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 20 };
> +    int64_t expect1[] = { 1, 2, 0, 2, 3, 4, 20, 5, 6, 7,
> +                          8, 9, 1, 2, 3, 4, 5, 6, 7, 8 };
>      int64_t expect2[] = { 32767, -32768, -32767 };
> -    int64_t expect3[] = { INT64_MAX, INT64_MIN };
> +    int64_t expect3[] = { INT64_MIN, INT64_MAX };
>      int64_t expect4[] = { 1 };
>      uint64_t expect5[] = { UINT64_MAX };
>      Error *err = NULL;
> @@ -207,7 +197,7 @@ static void test_visitor_in_intList(TestInputVisitorData *data,
>      visit_type_int64(v, NULL, &tail->value, &err);
>      g_assert_cmpint(tail->value, ==, 0);
>      visit_type_int64(v, NULL, &val, &err);
> -    g_assert_cmpint(val, ==, 1); /* BUG */
> +    error_free_or_abort(&err);
>      visit_check_list(v, &error_abort);
>      visit_end_list(v, (void **)&res);

Lovely!

Re: [Qemu-devel] [PATCH v1 6/9] qapi: rewrite string-input-visitor

[David Hildenbrand]

>>
>> Tests have to be fixed up:
>> - Two BUGs were hardcoded that are fixed now
>> - The string-input-visitor now actually returns a parsed list and not
>>   an ordered set.
> 
> I'd expect this to necessitate an update of callers that expect a set, but...
> 
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  include/qapi/string-input-visitor.h |   4 +-
>>  qapi/string-input-visitor.c         | 410 ++++++++++++++++------------
>>  tests/test-string-input-visitor.c   |  18 +-
>>  3 files changed, 239 insertions(+), 193 deletions(-)
> 
> ... there's none.
> 
> Let me know if you need help finding them.  I think we tracked them down
> during the discussion that led to this series.
> 

Indeed, I missed to document that. So here is the outcome:

1. backends/hostmem.c:host_memory_backend_set_host_nodes()

-> calls visit_type_uint16List(via bitmap)
-> the code can deal with duplicates/unsorted lists (bitmap_set)

Side node: I am not sure if there should be some range checks, but maybe
the bitmap is large enough .... hm ...

2. qapi-visit.c::visit_type_Memdev_members()

-> calls visit_type_uint16List()
-> I think this never used for input, only for output / freeing

3. qapi-visit.c::visit_type_NumaNodeOptions_members()

-> calls visit_type_uint16List()
-> I think this never used for input, only for output / freeing

4. qapi-visit.c::visit_type_RockerOfDpaGroup_members

-> calls visit_type_uint32List()
-> I think this never used for input, only for output / freeing

5. qapi-visit.c::visit_type_RxFilterInfo_members()

-> calls visit_type_intList()
-> I think this never used for input, only for output / freeing

6. numa.c::query_memdev()

-> calls object_property_get_uint16List()
--> String parsed via visit_type_uint16List() into list
-> qmp_query_memdev() uses this list
--> Not relevant if unique or sorted
-> hmp_info_memdev() uses this list
--> List converted again to a string using string output visitor

-> I don't think unique/sorted is relevant here.


Am I missing anything / is any of my statements wrong?

Thanks!

>>
>> diff --git a/include/qapi/string-input-visitor.h b/include/qapi/string-input-visitor.h
>> index 33551340e3..921f3875b9 100644
>> --- a/include/qapi/string-input-visitor.h
>> +++ b/include/qapi/string-input-visitor.h
>> @@ -19,8 +19,8 @@ typedef struct StringInputVisitor StringInputVisitor;
>>  
>>  /*
>>   * The string input visitor does not implement support for visiting
>> - * QAPI structs, alternates, null, or arbitrary QTypes.  It also
>> - * requires a non-null list argument to visit_start_list().
>> + * QAPI structs, alternates, null, or arbitrary QTypes. Only flat lists
>> + * of integers (except type "size") are supported.
>>   */
>>  Visitor *string_input_visitor_new(const char *str);
>>  
>> diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
>> index b89c6c4e06..4113be01fb 100644
>> --- a/qapi/string-input-visitor.c
>> +++ b/qapi/string-input-visitor.c
>> @@ -4,10 +4,10 @@
>>   * Copyright Red Hat, Inc. 2012-2016
>>   *
>>   * Author: Paolo Bonzini <pbonzini@redhat.com>
>> + *         David Hildenbrand <david@redhat.com>
>>   *
>>   * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
>>   * See the COPYING.LIB file in the top-level directory.
>> - *
>>   */
>>  
>>  #include "qemu/osdep.h"
>> @@ -18,21 +18,39 @@
>>  #include "qapi/qmp/qerror.h"
>>  #include "qapi/qmp/qnull.h"
>>  #include "qemu/option.h"
>> -#include "qemu/queue.h"
>> -#include "qemu/range.h"
>>  #include "qemu/cutils.h"
>>  

[...]
>>  static void parse_type_int64(Visitor *v, const char *name, int64_t *obj,
>>                               Error **errp)
>>  {
>>      StringInputVisitor *siv = to_siv(v);
>> -
>> -    if (parse_str(siv, name, errp) < 0) {
>> +    int64_t val;
>> +
>> +    switch (siv->lm) {
>> +    case LM_NONE:
>> +        /* just parse a simple int64, bail out if not completely consumed */
>> +        if (qemu_strtoi64(siv->string, NULL, 0, &val)) {
>> +                error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
>> +                           name ? name : "null", "int64");
>> +            return;
>> +        }
>> +        *obj = val;
>>          return;
>> +    case LM_UNPARSED:
>> +        if (try_parse_int64_list_entry(siv, obj)) {
>> +            error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null",
>> +                       "list of int64 values or ranges");
>> +            return;
>> +        }
>> +        assert(siv->lm == LM_INT64_RANGE);
>> +        /* FALLTHROUGH */
> 
> Please spell it /* fall through */, because:
> 
> $ git-grep -Ei '/\* *fall.?thr[a-z]* *\*/' | sed 's/.*\* *\(fall[^*]*\)\*.*/\1/i' | sort | uniq -c
>       4 FALL THROUGH 
>       8 FALLTHROUGH 
>      61 FALLTHRU 
>      36 Fall through 
>      20 Fallthrough 
>       4 Fallthru 
>     237 fall through 
>       1 fall-through 
>      16 fallthrough 
>      39 fallthru 

Done!

[...]
> 
>> +    case LM_INT64_RANGE:
>> +        /* return the next element in the range */
>> +        assert(siv->rangeNext.i64 <= siv->rangeEnd.i64);
>> +        *obj = siv->rangeNext.i64++;
>> +
>> +        if (siv->rangeNext.i64 > siv->rangeEnd.i64 || *obj == INT64_MAX) {
>> +            /* end of range, check if there is more to parse */
>> +            if (siv->unparsed_string[0]) {
>> +                siv->lm = LM_UNPARSED;
>> +            } else {
>> +                siv->lm = LM_END;
>> +            }
> 
> I'd do
> 
>                siv->lm = siv->unparsed_string[0] ? LM_UNPARSED : LM_END;
> 
> Matter of taste, entirely up to you.

Yes, makes sense, thanks!

> 
>> +        }
>> +        return;
>> +    case LM_END:
>> +        error_setg(errp, "Fewer list elements expected");
>> +        return;
>> +    default:
>> +        abort();
>>      }
>> +}
>>  
>> -    if (!siv->ranges) {
>> -        goto error;
>> -    }
>> -
>> -    if (!siv->cur_range) {
>> -        Range *r;
>> +static int try_parse_uint64_list_entry(StringInputVisitor *siv, uint64_t *obj)
>> +{
>> +    const char *endptr;
>> +    uint64_t start, end;
>>  
>> -        siv->cur_range = g_list_first(siv->ranges);
>> -        if (!siv->cur_range) {
>> -            goto error;
>> +    /* parse a simple uint64 or range */
> 
> try_parse_int64_list_entry() doesn't have this comment.  I think either
> both or none should have it.  You decide.

Indeed, the comment got lost on the way. Will readd it.

[...]

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v1 6/9] qapi: rewrite string-input-visitor

[Markus Armbruster]

Copying Igor and Eduardo for a hostmem.c bug.  Search for "core dumped".

David Hildenbrand <david@redhat.com> writes:

>>>
>>> Tests have to be fixed up:
>>> - Two BUGs were hardcoded that are fixed now
>>> - The string-input-visitor now actually returns a parsed list and not
>>>   an ordered set.
>> 
>> I'd expect this to necessitate an update of callers that expect a set, but...
>> 
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>> ---
>>>  include/qapi/string-input-visitor.h |   4 +-
>>>  qapi/string-input-visitor.c         | 410 ++++++++++++++++------------
>>>  tests/test-string-input-visitor.c   |  18 +-
>>>  3 files changed, 239 insertions(+), 193 deletions(-)
>> 
>> ... there's none.
>> 
>> Let me know if you need help finding them.  I think we tracked them down
>> during the discussion that led to this series.
>> 
>
> Indeed, I missed to document that. So here is the outcome:
>
> 1. backends/hostmem.c:host_memory_backend_set_host_nodes()
>
> -> calls visit_type_uint16List(via bitmap)
> -> the code can deal with duplicates/unsorted lists (bitmap_set)

Yes.

> Side node: I am not sure if there should be some range checks, but maybe
> the bitmap is large enough .... hm ...

Fishy.  MAX_NODES is 128.  Tinker, tinker, ...

    $ upstream-qemu -nodefaults -object memory-backend-file,id=mem0,mem-path=x,size=4096,host-nodes=12345
    Segmentation fault (core dumped)

Igor, Eduardo, this is yours.

There's another use of visit_type_uint16List() is this file, but it's in
property getter host_memory_backend_get_host_nodes(), and property
getters aren't used with the string input visitor.

> 2. qapi-visit.c::visit_type_Memdev_members()
>
> -> calls visit_type_uint16List()
> -> I think this never used for input, only for output / freeing

Yes, it's used by query-memdev with the QObject output visitor to build
the value of @host-nodes.

> 3. qapi-visit.c::visit_type_NumaNodeOptions_members()
>
> -> calls visit_type_uint16List()
> -> I think this never used for input, only for output / freeing

It's used for input, but with the opts visitor, see parse_numa().

> 4. qapi-visit.c::visit_type_RockerOfDpaGroup_members
>
> -> calls visit_type_uint32List()
> -> I think this never used for input, only for output / freeing

Yes, it's used by query-rocker-of-dpa-groups with the QObject output
visitor to build the value of @group-ids.

> 5. qapi-visit.c::visit_type_RxFilterInfo_members()
>
> -> calls visit_type_intList()
> -> I think this never used for input, only for output / freeing

Yes, it's used by query-rx-filter with the QObject output visitor to
build the value of @vlan-table.

> 6. numa.c::query_memdev()
>
> -> calls object_property_get_uint16List()
> --> String parsed via visit_type_uint16List() into list

QOM, hard to understand.

The value of struct HostMemoryBackend member @host-nodes (a bitmap) is
first converted to a list (sorted, no duplicates) with
host_memory_backend_get_host_nodes() via object_property_get(), then
converted to a string with the string output visitor.  The resulting
string is then converted back to a list with the string input visitor.

Despite the shenanigans going on in the string output visitor, I'd
expect the resulting list to also be sorted and without duplicates.

> -> qmp_query_memdev() uses this list
> --> Not relevant if unique or sorted

Depends on the contract of QMP command query-memdev.  Here's the
relevant part.

    # @host-nodes: host nodes for its memory policy

Useless.

"Sorted, no duplicates" might have become de facto ABI.  Not sure.
However, I believe your patch won't affect it, as per the argument I
just made.

> -> hmp_info_memdev() uses this list
> --> List converted again to a string using string output visitor
>
> -> I don't think unique/sorted is relevant here.

HMP is not a stable interface.

> Am I missing anything / is any of my statements wrong?

Searching the QAPI schema for lists of integers coughs up block latency
histogram stuff, but that's unrelated, as far as I can tell.

Looks like we're good.  I didn't expect that :)

[...]

Re: [Qemu-devel] [PATCH v1 6/9] qapi: rewrite string-input-visitor

[David Hildenbrand]

On 19.11.18 20:51, Markus Armbruster wrote:
> Copying Igor and Eduardo for a hostmem.c bug.  Search for "core dumped".
> 
> David Hildenbrand <david@redhat.com> writes:
> 
>>>>
>>>> Tests have to be fixed up:
>>>> - Two BUGs were hardcoded that are fixed now
>>>> - The string-input-visitor now actually returns a parsed list and not
>>>>   an ordered set.
>>>
>>> I'd expect this to necessitate an update of callers that expect a set, but...
>>>
>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>> ---
>>>>  include/qapi/string-input-visitor.h |   4 +-
>>>>  qapi/string-input-visitor.c         | 410 ++++++++++++++++------------
>>>>  tests/test-string-input-visitor.c   |  18 +-
>>>>  3 files changed, 239 insertions(+), 193 deletions(-)
>>>
>>> ... there's none.
>>>
>>> Let me know if you need help finding them.  I think we tracked them down
>>> during the discussion that led to this series.
>>>
>>
>> Indeed, I missed to document that. So here is the outcome:
>>
>> 1. backends/hostmem.c:host_memory_backend_set_host_nodes()
>>
>> -> calls visit_type_uint16List(via bitmap)
>> -> the code can deal with duplicates/unsorted lists (bitmap_set)
> 
> Yes.
> 
>> Side node: I am not sure if there should be some range checks, but maybe
>> the bitmap is large enough .... hm ...
> 
> Fishy.  MAX_NODES is 128.  Tinker, tinker, ...
> 
>     $ upstream-qemu -nodefaults -object memory-backend-file,id=mem0,mem-path=x,size=4096,host-nodes=12345
>     Segmentation fault (core dumped)
> 
> Igor, Eduardo, this is yours.
> 
> There's another use of visit_type_uint16List() is this file, but it's in
> property getter host_memory_backend_get_host_nodes(), and property
> getters aren't used with the string input visitor.
> 
>> 2. qapi-visit.c::visit_type_Memdev_members()
>>
>> -> calls visit_type_uint16List()
>> -> I think this never used for input, only for output / freeing
> 
> Yes, it's used by query-memdev with the QObject output visitor to build
> the value of @host-nodes.
> 
>> 3. qapi-visit.c::visit_type_NumaNodeOptions_members()
>>
>> -> calls visit_type_uint16List()
>> -> I think this never used for input, only for output / freeing
> 
> It's used for input, but with the opts visitor, see parse_numa().
> 
>> 4. qapi-visit.c::visit_type_RockerOfDpaGroup_members
>>
>> -> calls visit_type_uint32List()
>> -> I think this never used for input, only for output / freeing
> 
> Yes, it's used by query-rocker-of-dpa-groups with the QObject output
> visitor to build the value of @group-ids.
> 
>> 5. qapi-visit.c::visit_type_RxFilterInfo_members()
>>
>> -> calls visit_type_intList()
>> -> I think this never used for input, only for output / freeing
> 
> Yes, it's used by query-rx-filter with the QObject output visitor to
> build the value of @vlan-table.
> 
>> 6. numa.c::query_memdev()
>>
>> -> calls object_property_get_uint16List()
>> --> String parsed via visit_type_uint16List() into list
> 
> QOM, hard to understand.
> 
> The value of struct HostMemoryBackend member @host-nodes (a bitmap) is
> first converted to a list (sorted, no duplicates) with
> host_memory_backend_get_host_nodes() via object_property_get(), then
> converted to a string with the string output visitor.  The resulting
> string is then converted back to a list with the string input visitor.
> 
> Despite the shenanigans going on in the string output visitor, I'd
> expect the resulting list to also be sorted and without duplicates.
> 
>> -> qmp_query_memdev() uses this list
>> --> Not relevant if unique or sorted
> 
> Depends on the contract of QMP command query-memdev.  Here's the
> relevant part.
> 
>     # @host-nodes: host nodes for its memory policy
> 
> Useless.
> 
> "Sorted, no duplicates" might have become de facto ABI.  Not sure.
> However, I believe your patch won't affect it, as per the argument I
> just made.
> 
>> -> hmp_info_memdev() uses this list
>> --> List converted again to a string using string output visitor
>>
>> -> I don't think unique/sorted is relevant here.
> 
> HMP is not a stable interface.
> 
>> Am I missing anything / is any of my statements wrong?
> 
> Searching the QAPI schema for lists of integers coughs up block latency
> histogram stuff, but that's unrelated, as far as I can tell.
> 
> Looks like we're good.  I didn't expect that :)
> 

Haha, me too. Will add a short description to the patch message and
maybe resend tomorrow!


-- 

Thanks,

David / dhildenb

[Qemu-devel] [PATCH v1 7/9] test-string-input-visitor: use virtual walk

[David Hildenbrand]

We now support virtual walks, so use that instead.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 tests/test-string-input-visitor.c | 36 +++++++++++--------------------
 1 file changed, 12 insertions(+), 24 deletions(-)

diff --git a/tests/test-string-input-visitor.c b/tests/test-string-input-visitor.c
index a198aedfce..4ad4cc0b56 100644
--- a/tests/test-string-input-visitor.c
+++ b/tests/test-string-input-visitor.c
@@ -115,7 +115,6 @@ static void test_visitor_in_intList(TestInputVisitorData *data,
     uint64_t expect5[] = { UINT64_MAX };
     Error *err = NULL;
     int64List *res = NULL;
-    int64List *tail;
     Visitor *v;
     int64_t val;
 
@@ -169,39 +168,28 @@ static void test_visitor_in_intList(TestInputVisitorData *data,
 
     v = visitor_input_test_init(data, "0,2-3");
 
-    /* Would be simpler if the visitor genuinely supported virtual walks */
-    visit_start_list(v, NULL, (GenericList **)&res, sizeof(*res),
-                     &error_abort);
-    tail = res;
-    visit_type_int64(v, NULL, &tail->value, &error_abort);
-    g_assert_cmpint(tail->value, ==, 0);
-    tail = (int64List *)visit_next_list(v, (GenericList *)tail, sizeof(*res));
-    g_assert(tail);
-    visit_type_int64(v, NULL, &tail->value, &error_abort);
-    g_assert_cmpint(tail->value, ==, 2);
-    tail = (int64List *)visit_next_list(v, (GenericList *)tail, sizeof(*res));
-    g_assert(tail);
+    visit_start_list(v, NULL, NULL, 0, &error_abort);
+    visit_type_int64(v, NULL, &val, &error_abort);
+    g_assert_cmpint(val, ==, 0);
+    visit_type_int64(v, NULL, &val, &error_abort);
+    g_assert_cmpint(val, ==, 2);
 
     visit_check_list(v, &err);
     error_free_or_abort(&err);
-    visit_end_list(v, (void **)&res);
-
-    qapi_free_int64List(res);
+    visit_end_list(v, NULL);
 
     /* Visit beyond end of list */
+
     v = visitor_input_test_init(data, "0");
 
-    visit_start_list(v, NULL, (GenericList **)&res, sizeof(*res),
-                     &error_abort);
-    tail = res;
-    visit_type_int64(v, NULL, &tail->value, &err);
-    g_assert_cmpint(tail->value, ==, 0);
+    visit_start_list(v, NULL, NULL, 0, &error_abort);
+    visit_type_int64(v, NULL, &val, &err);
+    g_assert_cmpint(val, ==, 0);
     visit_type_int64(v, NULL, &val, &err);
     error_free_or_abort(&err);
-    visit_check_list(v, &error_abort);
-    visit_end_list(v, (void **)&res);
 
-    qapi_free_int64List(res);
+    visit_check_list(v, &error_abort);
+    visit_end_list(v, NULL);
 }
 
 static void test_visitor_in_bool(TestInputVisitorData *data,
-- 
2.17.2

Re: [Qemu-devel] [PATCH v1 7/9] test-string-input-visitor: use virtual walk

[Markus Armbruster]

David Hildenbrand <david@redhat.com> writes:

> We now support virtual walks, so use that instead.
>
> Signed-off-by: David Hildenbrand <david@redhat.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

[Qemu-devel] [PATCH v1 8/9] test-string-input-visitor: split off uint64 list tests

[David Hildenbrand]

Basically copy all int64 list tests but adapt them to work on uint64
instead. The values for very big/very small values have to be adapted.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 tests/test-string-input-visitor.c | 94 +++++++++++++++++++++++++++++--
 1 file changed, 90 insertions(+), 4 deletions(-)

diff --git a/tests/test-string-input-visitor.c b/tests/test-string-input-visitor.c
index 4ad4cc0b56..d2ec453252 100644
--- a/tests/test-string-input-visitor.c
+++ b/tests/test-string-input-visitor.c
@@ -112,7 +112,6 @@ static void test_visitor_in_intList(TestInputVisitorData *data,
     int64_t expect2[] = { 32767, -32768, -32767 };
     int64_t expect3[] = { INT64_MIN, INT64_MAX };
     int64_t expect4[] = { 1 };
-    uint64_t expect5[] = { UINT64_MAX };
     Error *err = NULL;
     int64List *res = NULL;
     Visitor *v;
@@ -133,9 +132,6 @@ static void test_visitor_in_intList(TestInputVisitorData *data,
     v = visitor_input_test_init(data, "1-1");
     check_ilist(v, expect4, ARRAY_SIZE(expect4));
 
-    v = visitor_input_test_init(data, "18446744073709551615");
-    check_ulist(v, expect5, ARRAY_SIZE(expect5));
-
     /* Value too large */
 
     v = visitor_input_test_init(data, "9223372036854775808");
@@ -192,6 +188,94 @@ static void test_visitor_in_intList(TestInputVisitorData *data,
     visit_end_list(v, NULL);
 }
 
+static void test_visitor_in_uintList(TestInputVisitorData *data,
+                                     const void *unused)
+{
+    uint64_t expect1[] = { 1, 2, 0, 2, 3, 4, 20, 5, 6, 7,
+                           8, 9, 1, 2, 3, 4, 5, 6, 7, 8 };
+    uint64_t expect2[] = { 32767, -32768, -32767 };
+    uint64_t expect3[] = { INT64_MIN, INT64_MAX };
+    uint64_t expect4[] = { 1 };
+    uint64_t expect5[] = { UINT64_MAX };
+    Error *err = NULL;
+    uint64List *res = NULL;
+    Visitor *v;
+    uint64_t val;
+
+    /* Valid lists */
+
+    v = visitor_input_test_init(data, "1,2,0,2-4,20,5-9,1-8");
+    check_ulist(v, expect1, ARRAY_SIZE(expect1));
+
+    v = visitor_input_test_init(data, "32767,-32768--32767");
+    check_ulist(v, expect2, ARRAY_SIZE(expect2));
+
+    v = visitor_input_test_init(data,
+                                "-9223372036854775808,9223372036854775807");
+    check_ulist(v, expect3, ARRAY_SIZE(expect3));
+
+    v = visitor_input_test_init(data, "1-1");
+    check_ulist(v, expect4, ARRAY_SIZE(expect4));
+
+    v = visitor_input_test_init(data, "18446744073709551615");
+    check_ulist(v, expect5, ARRAY_SIZE(expect5));
+
+    /* Value too large */
+
+    v = visitor_input_test_init(data, "18446744073709551616");
+    visit_type_uint64List(v, NULL, &res, &err);
+    error_free_or_abort(&err);
+    g_assert(!res);
+
+    /* Value too small */
+
+    v = visitor_input_test_init(data, "-18446744073709551616");
+    visit_type_uint64List(v, NULL, &res, &err);
+    error_free_or_abort(&err);
+    g_assert(!res);
+
+    /* Empty list */
+
+    v = visitor_input_test_init(data, "");
+    visit_type_uint64List(v, NULL, &res, &error_abort);
+    g_assert(!res);
+
+    /* Not a list */
+
+    v = visitor_input_test_init(data, "not an uint list");
+
+    visit_type_uint64List(v, NULL, &res, &err);
+    error_free_or_abort(&err);
+    g_assert(!res);
+
+    /* Unvisited list tail */
+
+    v = visitor_input_test_init(data, "0,2-3");
+
+    visit_start_list(v, NULL, NULL, 0, &error_abort);
+    visit_type_uint64(v, NULL, &val, &error_abort);
+    g_assert_cmpuint(val, ==, 0);
+    visit_type_uint64(v, NULL, &val, &error_abort);
+    g_assert_cmpuint(val, ==, 2);
+
+    visit_check_list(v, &err);
+    error_free_or_abort(&err);
+    visit_end_list(v, NULL);
+
+    /* Visit beyond end of list */
+
+    v = visitor_input_test_init(data, "0");
+
+    visit_start_list(v, NULL, NULL, 0, &error_abort);
+    visit_type_uint64(v, NULL, &val, &err);
+    g_assert_cmpuint(val, ==, 0);
+    visit_type_uint64(v, NULL, &val, &err);
+    error_free_or_abort(&err);
+
+    visit_check_list(v, &error_abort);
+    visit_end_list(v, NULL);
+}
+
 static void test_visitor_in_bool(TestInputVisitorData *data,
                                  const void *unused)
 {
@@ -352,6 +436,8 @@ int main(int argc, char **argv)
                            &in_visitor_data, test_visitor_in_int);
     input_visitor_test_add("/string-visitor/input/intList",
                            &in_visitor_data, test_visitor_in_intList);
+    input_visitor_test_add("/string-visitor/input/uintList",
+                           &in_visitor_data, test_visitor_in_uintList);
     input_visitor_test_add("/string-visitor/input/bool",
                            &in_visitor_data, test_visitor_in_bool);
     input_visitor_test_add("/string-visitor/input/number",
-- 
2.17.2

Re: [Qemu-devel] [PATCH v1 8/9] test-string-input-visitor: split off uint64 list tests

[Markus Armbruster]

David Hildenbrand <david@redhat.com> writes:

> Basically copy all int64 list tests but adapt them to work on uint64
> instead. The values for very big/very small values have to be adapted.
>
> Signed-off-by: David Hildenbrand <david@redhat.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

[Qemu-devel] [PATCH v1 9/9] test-string-input-visitor: add range overflow tests

[David Hildenbrand]

Let's make sure that the range handling code can properly deal with
ranges that end at the biggest possible number.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 tests/test-string-input-visitor.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/tests/test-string-input-visitor.c b/tests/test-string-input-visitor.c
index d2ec453252..86b1f9d593 100644
--- a/tests/test-string-input-visitor.c
+++ b/tests/test-string-input-visitor.c
@@ -112,6 +112,7 @@ static void test_visitor_in_intList(TestInputVisitorData *data,
     int64_t expect2[] = { 32767, -32768, -32767 };
     int64_t expect3[] = { INT64_MIN, INT64_MAX };
     int64_t expect4[] = { 1 };
+    int64_t expect5[] = { INT64_MAX - 2,  INT64_MAX - 1, INT64_MAX };
     Error *err = NULL;
     int64List *res = NULL;
     Visitor *v;
@@ -132,6 +133,10 @@ static void test_visitor_in_intList(TestInputVisitorData *data,
     v = visitor_input_test_init(data, "1-1");
     check_ilist(v, expect4, ARRAY_SIZE(expect4));
 
+    v = visitor_input_test_init(data,
+                                "9223372036854775805-9223372036854775807");
+    check_ilist(v, expect5, ARRAY_SIZE(expect5));
+
     /* Value too large */
 
     v = visitor_input_test_init(data, "9223372036854775808");
@@ -197,6 +202,7 @@ static void test_visitor_in_uintList(TestInputVisitorData *data,
     uint64_t expect3[] = { INT64_MIN, INT64_MAX };
     uint64_t expect4[] = { 1 };
     uint64_t expect5[] = { UINT64_MAX };
+    uint64_t expect6[] = { UINT64_MAX - 2,  UINT64_MAX - 1, UINT64_MAX };
     Error *err = NULL;
     uint64List *res = NULL;
     Visitor *v;
@@ -220,6 +226,10 @@ static void test_visitor_in_uintList(TestInputVisitorData *data,
     v = visitor_input_test_init(data, "18446744073709551615");
     check_ulist(v, expect5, ARRAY_SIZE(expect5));
 
+    v = visitor_input_test_init(data,
+                                "18446744073709551613-18446744073709551615");
+    check_ulist(v, expect6, ARRAY_SIZE(expect6));
+
     /* Value too large */
 
     v = visitor_input_test_init(data, "18446744073709551616");
-- 
2.17.2

Re: [Qemu-devel] [PATCH v1 9/9] test-string-input-visitor: add range overflow tests

[Markus Armbruster]

David Hildenbrand <david@redhat.com> writes:

> Let's make sure that the range handling code can properly deal with
> ranges that end at the biggest possible number.
>
> Signed-off-by: David Hildenbrand <david@redhat.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>