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

[Qemu-devel] [PATCH v2 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).

v1 -> v2:
- "cutils: Add qemu_strtod() and qemu_strtod_finite()"
-- Add a proper documentation
-- Make -EINVAL perform "no conversion" in qemu_strtod_finite().
- "cutils: Fix qemu_strtosz() & friends to reject non-finite sizes"
-- New subject/description
-- Added test case
-- Forward -ERANGE
- "qapi: Fix string-input-visitor to reject NaN and infinities"
-- New subject/description
-- Added test case
- "test-string-input-visitor: Add more tests"
-- Add even more tests (descending range, overflow, range limit)
- "qapi: Rewrite string-input-visitor"
-- Accept ranges with one element (e.g. 1-1)
-- Bail out on big ranges (just like before)
-- Minor style fixed + description updates
- "test-string-input-visitor: split off uint64 list tests"
-- ALso copy + adapt newly added tests for int64 lists

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: Fix qemu_strtosz() & friends to reject non-finite sizes
  qapi: Fix string-input-visitor to reject NaN and infinities
  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         | 413 ++++++++++++++++------------
 tests/test-cutils.c                 |  24 +-
 tests/test-string-input-visitor.c   | 223 ++++++++++++---
 util/cutils.c                       |  81 +++++-
 8 files changed, 516 insertions(+), 248 deletions(-)

-- 
2.17.2

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

[David Hildenbrand]

Let's provide a wrapper for strtod().

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 include/qemu/cutils.h |  2 ++
 util/cutils.c         | 65 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 67 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..c965dbfcad 100644
--- a/util/cutils.c
+++ b/util/cutils.c
@@ -544,6 +544,71 @@ 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.
+  *
+ * 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, store +/-HUGE_VAL in @result, depending
+ * on the sign, and return -ERANGE.
+ *
+ * If the conversion underflows, store ±0.0 in @result, depending on the
+ * sign, and return -ERANGE.
+ *
+ * Else store the converted value in @result, and return zero.
+ */
+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_strtod(), except that "NaN" and "inf" are rejected
+ * with -EINVAL and no conversion is performed.
+ */
+int qemu_strtod_finite(const char *nptr, const char **endptr, double *result)
+{
+    double tmp;
+    int ret;
+
+    ret = qemu_strtod(nptr, endptr, &tmp);
+    if (ret) {
+        return ret;
+    } else if (!isfinite(tmp)) {
+        if (endptr) {
+            *endptr = nptr;
+        }
+        return -EINVAL;
+    }
+
+    *result = tmp;
+    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

[Qemu-devel] [PATCH v2 2/9] cutils: Fix qemu_strtosz() & friends to reject non-finite sizes

[David Hildenbrand]

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 **.

Also, add two test cases, testing that "inf" and "NaN" are properly
rejected.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 include/qemu/cutils.h |  6 +++---
 monitor.c             |  2 +-
 tests/test-cutils.c   | 24 +++++++++++++++++-------
 util/cutils.c         | 16 +++++++---------
 4 files changed, 28 insertions(+), 20 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..1aa8351520 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;
     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;
     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;
     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;
     int err;
     uint64_t res = 0xbaadf00d;
 
@@ -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);
 }
 
 static void test_qemu_strtosz_trailing(void)
 {
     const char *str;
-    char *endptr = NULL;
+    const char *endptr;
     int err;
     uint64_t res = 0xbaadf00d;
 
@@ -2126,7 +2136,7 @@ static void test_qemu_strtosz_trailing(void)
 static void test_qemu_strtosz_erange(void)
 {
     const char *str;
-    char *endptr = NULL;
+    const char *endptr;
     int err;
     uint64_t res = 0xbaadf00d;
 
@@ -2160,7 +2170,7 @@ static void test_qemu_strtosz_metric(void)
 {
     const char *str = "12345k";
     int err;
-    char *endptr = NULL;
+    const char *endptr;
     uint64_t res = 0xbaadf00d;
 
     err = qemu_strtosz_metric(str, &endptr, &res);
diff --git a/util/cutils.c b/util/cutils.c
index c965dbfcad..98732ff5b2 100644
--- a/util/cutils.c
+++ b/util/cutils.c
@@ -206,20 +206,18 @@ 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) {
-        retval = -EINVAL;
+    retval = qemu_strtod_finite(nptr, &endptr, &val);
+    if (retval) {
         goto out;
     }
     fraction = modf(val, &integral);
@@ -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

[Qemu-devel] [PATCH v2 3/9] qapi: Fix string-input-visitor to reject NaN and infinities

[David Hildenbrand]

The string-input-visitor happily accepts NaN and inifities when parsing
numbers (doubles). They shouldn't. Fix that.

Also, add two test cases, testing if "NaN" and "inf" is properly
rejected.

Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 qapi/string-input-visitor.c       |  6 ++----
 tests/test-string-input-visitor.c | 13 +++++++++++++
 2 files changed, 15 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;
diff --git a/tests/test-string-input-visitor.c b/tests/test-string-input-visitor.c
index 88e0e1aa9a..1efba06948 100644
--- a/tests/test-string-input-visitor.c
+++ b/tests/test-string-input-visitor.c
@@ -252,6 +252,19 @@ static void test_visitor_in_number(TestInputVisitorData *data,
     visit_type_number(v, NULL, &res, &err);
     g_assert(!err);
     g_assert_cmpfloat(res, ==, value);
+
+    /* NaN and infinity has to be rejected */
+
+    v = visitor_input_test_init(data, "NaN");
+
+    visit_type_number(v, NULL, &res, &err);
+    error_free_or_abort(&err);
+
+    v = visitor_input_test_init(data, "inf");
+
+    visit_type_number(v, NULL, &res, &err);
+    error_free_or_abort(&err);
+
 }
 
 static void test_visitor_in_string(TestInputVisitorData *data,
-- 
2.17.2

[Qemu-devel] [PATCH v2 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).

Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
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

[Qemu-devel] [PATCH v2 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. Also test that ranges are ascending and cannot
have more than 65536 elements.

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 | 41 +++++++++++++++++++++++++++++--
 1 file changed, 39 insertions(+), 2 deletions(-)

diff --git a/tests/test-string-input-visitor.c b/tests/test-string-input-visitor.c
index 1efba06948..8ee0d1b284 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,44 @@ 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);
+
+    /* Range not ascending */
+
+    v = visitor_input_test_init(data, "3-1");
+    visit_type_int64List(v, NULL, &res, &err);
+    error_free_or_abort(&err);
+    g_assert(!res);
+
+    v = visitor_input_test_init(data, "9223372036854775807-0");
+    visit_type_int64List(v, NULL, &res, &err);
+    error_free_or_abort(&err);
+    g_assert(!res);
+
+    /* Range too big (65536 is the limit against DOS attacks) */
+
+    v = visitor_input_test_init(data, "0-65536");
+    visit_type_int64List(v, NULL, &res, &err);
+    error_free_or_abort(&err);
+    g_assert(!res);
 
     /* Empty list */
 
-- 
2.17.2

[Qemu-devel] [PATCH v2 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
- we don't actually parse lists, we parse *sets*: members are sorted,
  and duplicates eliminated

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.

Please note that no users/callers have to be fixed up. Candiates using
visit_type_uint16List() and friends are:
- backends/hostmem.c:host_memory_backend_set_host_nodes()
-- Code can deal with dupilcates/unsorted lists
- numa.c::query_memdev()
-- via object_property_get_uint16List(), the list will still be sorted
   and without duplicates (via host_memory_backend_get_host_nodes())
- qapi-visit.c::visit_type_Memdev_members()
- qapi-visit.c::visit_type_NumaNodeOptions_members()
- qapi-visit.c::visit_type_RockerOfDpaGroup_members
- qapi-visit.c::visit_type_RxFilterInfo_members()
-- Not used with string-input-visitor.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 include/qapi/string-input-visitor.h |   4 +-
 qapi/string-input-visitor.c         | 405 ++++++++++++++++------------
 tests/test-string-input-visitor.c   |  18 +-
 3 files changed, 234 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..bd92080667 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,42 @@
 #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;
+
+/* protect against DOS attacks, limit the amount of elements per range */
+#define RANGE_MAX_ELEMENTS 65536
+
+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 +61,42 @@ 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);
+    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) {
-        return NULL;
-    }
 
-    r = siv->cur_range->data;
-    if (!r) {
+    switch (siv->lm) {
+    case LM_END:
         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 +106,208 @@ 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;
+
+    /* parse a simple int64 or range */
+    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 || end - start >= RANGE_MAX_ELEMENTS) {
+            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);
+        /* fall through */
+    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 */
+            siv->lm = siv->unparsed_string[0] ? LM_UNPARSED : 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 || end - start >= RANGE_MAX_ELEMENTS) {
+            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);
+        /* fall through */
+    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 */
+            siv->lm = siv->unparsed_string[0] ? LM_UNPARSED : LM_END;
+        }
+        return;
+    case LM_END:
+        error_setg(errp, "Fewer list elements expected");
+        return;
+    default:
+        abort();
     }
 }
 
@@ -271,6 +318,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 +333,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 +356,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 +366,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 +381,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 +397,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 +422,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 8ee0d1b284..c5379365a6 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;
@@ -226,7 +216,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

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

[David Hildenbrand]

We now support virtual walks, so use that instead.

Reviewed-by: Markus Armbruster <armbru@redhat.com>
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 c5379365a6..718d9a03c3 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;
 
@@ -188,39 +187,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

[Qemu-devel] [PATCH v2 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.

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

diff --git a/tests/test-string-input-visitor.c b/tests/test-string-input-visitor.c
index 718d9a03c3..9b1dd44b2d 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");
@@ -211,6 +207,113 @@ 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);
+
+    /* Range not ascending */
+
+    v = visitor_input_test_init(data, "3-1");
+    visit_type_uint64List(v, NULL, &res, &err);
+    error_free_or_abort(&err);
+    g_assert(!res);
+
+    v = visitor_input_test_init(data, "18446744073709551615-0");
+    visit_type_uint64List(v, NULL, &res, &err);
+    error_free_or_abort(&err);
+    g_assert(!res);
+
+    /* Range too big (65536 is the limit against DOS attacks) */
+
+    v = visitor_input_test_init(data, "0-65536");
+    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)
 {
@@ -384,6 +487,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

[Qemu-devel] [PATCH v2 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.

Reviewed-by: Markus Armbruster <armbru@redhat.com>
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 9b1dd44b2d..34b54dfc89 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");
@@ -216,6 +221,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;
@@ -239,6 +245,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 v2 1/9] cutils: Add qemu_strtod() and qemu_strtod_finite()

[Eric Blake]

On 11/20/18 3:25 AM, David Hildenbrand wrote:
> Let's provide a wrapper for strtod().
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>

This changed enough from v1 that I would have dropped R-b to ensure that 
reviewers notice the differences.

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

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

The use of UTF-8 ± in one place but not both is odd.  I think we're at 
the point where UTF-8 comments are acceptable these days, rather than 
trying to keep our codebase ASCII-clean, so I don't care which way you 
resolve the inconsistency.

> +/**
> + * Convert string @nptr to a finite double.
> + *
> + * Works like qemu_strtod(), except that "NaN" and "inf" are rejected
> + * with -EINVAL and no conversion is performed.
> + */
> +int qemu_strtod_finite(const char *nptr, const char **endptr, double *result)
> +{
> +    double tmp;
> +    int ret;
> +
> +    ret = qemu_strtod(nptr, endptr, &tmp);
> +    if (ret) {
> +        return ret;

So, if we overflow, we are returning -ERANGE but with nothing stored 
into *result.  This is different from qemu_strtod(), where a return of 
-ERANGE guarantees that *result is one of 4 values (+/- 0.0/inf).  That 
seems awkward.

> +    } else if (!isfinite(tmp)) {
> +        if (endptr) {
> +            *endptr = nptr;
> +        }
> +        return -EINVAL;

Rewinding back to the start of "inf" is interesting, but matches your 
documentation.

> +    }
> +
> +    *result = tmp;
> +    return ret;
> +}
> +

I think you still need to fix -ERANGE handling before I can give R-b.

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

Re: [Qemu-devel] [PATCH v2 2/9] cutils: Fix qemu_strtosz() & friends to reject non-finite sizes

[Eric Blake]

On 11/20/18 3:25 AM, David Hildenbrand wrote:
> qemu_strtosz() & friends reject NaNs, but happily accept inifities.

s/inifities/infinities/

> 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 **.
> 
> Also, add two test cases, testing that "inf" and "NaN" are properly
> rejected.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>   include/qemu/cutils.h |  6 +++---
>   monitor.c             |  2 +-
>   tests/test-cutils.c   | 24 +++++++++++++++++-------
>   util/cutils.c         | 16 +++++++---------
>   4 files changed, 28 insertions(+), 20 deletions(-)
> 

> +++ b/util/cutils.c
> @@ -206,20 +206,18 @@ static int64_t suffix_mul(char suffix, int64_t unit)
>    * in *end, if not NULL. Return -ERANGE on overflow, Return -EINVAL on

Pre-existing, but since you're touching this area: the second 'Return' 
is unusual capitalization for being mid-sentence.  You could even 
s/Return/of/

>    * 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) {
> -        retval = -EINVAL;
> +    retval = qemu_strtod_finite(nptr, &endptr, &val);
> +    if (retval) {
>           goto out;

Here, retval can be -EINVAL (for failure to parse, or encountering "inf" 
or "NaN") or -ERANGE (overflow, underflow)...

>       }
>       fraction = modf(val, &integral);
> @@ -259,17 +257,17 @@ out:

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

>       return retval;

...if the failure was -EINVAL due to trailing garbage or empty string, 
nothing changes. If the failure was -EINVAL due to "inf", and the user 
passed in 'end', then 'end' now points to the beginning of "inf" instead 
of the end (probably okay). If the failure was -EINVAL due to "inf" and 
the user gave NULL for 'end', then we slam retval back to -EINVAL (no 
change).  If the failure was -ERANGE, then there is no trailing garbage, 
so *endptr had better be NULL, and we still fail with -ERANGE.  Any 
other way to reach the out label is unchanged from earlier logic.

It's some hairy code to think about, but I can't find anything wrong 
with it.  Typo fixes are minor, so

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 v2 5/9] test-string-input-visitor: Add more tests

[Eric Blake]

On 11/20/18 3:25 AM, David Hildenbrand wrote:
> Test that very big/small values are not accepted and that ranges with
> only one element work. Also test that ranges are ascending and cannot
> have more than 65536 elements.
> 
> 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 | 41 +++++++++++++++++++++++++++++--
>   1 file changed, 39 insertions(+), 2 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 v2 5/9] test-string-input-visitor: Add more tests

[Eric Blake]

On 11/20/18 11:06 AM, Eric Blake wrote:
> On 11/20/18 3:25 AM, David Hildenbrand wrote:
>> Test that very big/small values are not accepted and that ranges with
>> only one element work. Also test that ranges are ascending and cannot
>> have more than 65536 elements.
>>
>> 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 | 41 +++++++++++++++++++++++++++++--
>>   1 file changed, 39 insertions(+), 2 deletions(-)
>>
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>

Do we also want to test garbage strings like:

"1-" (incomplete range)
"1X-2" (garbage suffix on first element)
"1-2X" (garbage suffix on second element)

and/or

"-2--1" (valid range of signed integers)
"-1--2" (questionable whether this is valid for the two largest unsigned 
integers)

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

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

[Eric Blake]

On 11/20/18 11:20 AM, Eric Blake wrote:
> On 11/20/18 11:06 AM, Eric Blake wrote:
>> On 11/20/18 3:25 AM, David Hildenbrand wrote:
>>> Test that very big/small values are not accepted and that ranges with
>>> only one element work. Also test that ranges are ascending and cannot
>>> have more than 65536 elements.
>>>
>>> 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 | 41 +++++++++++++++++++++++++++++--
>>>   1 file changed, 39 insertions(+), 2 deletions(-)
>>>
>>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> Do we also want to test garbage strings like:
> 
> "1-" (incomplete range)
> "1X-2" (garbage suffix on first element)
> "1-2X" (garbage suffix on second element)
> 
> and/or
> 
> "-2--1" (valid range of signed integers)
> "-1--2" (questionable whether this is valid for the two largest unsigned 
> integers)

Or even " 1- 2" (we permit leading whitespace for plain integers - do we 
also permit it in both sides of a range)?  Also, if we permit whitespace 
after the range '-', should we permit it beforehand?

These sorts of questions may be fine in followup patches.

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

Re: [Qemu-devel] [PATCH v2 6/9] qapi: Rewrite string-input-visitor

[Eric Blake]

On 11/20/18 3:25 AM, David Hildenbrand wrote:
> 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
> 
> 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.
> 
> Please note that no users/callers have to be fixed up. Candiates using

s/Candiates/Candidates/

> visit_type_uint16List() and friends are:
> - backends/hostmem.c:host_memory_backend_set_host_nodes()
> -- Code can deal with dupilcates/unsorted lists

s/dupilcates/duplicates/

> - numa.c::query_memdev()
> -- via object_property_get_uint16List(), the list will still be sorted
>     and without duplicates (via host_memory_backend_get_host_nodes())
> - qapi-visit.c::visit_type_Memdev_members()
> - qapi-visit.c::visit_type_NumaNodeOptions_members()
> - qapi-visit.c::visit_type_RockerOfDpaGroup_members
> - qapi-visit.c::visit_type_RxFilterInfo_members()
> -- Not used with string-input-visitor.
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>   include/qapi/string-input-visitor.h |   4 +-
>   qapi/string-input-visitor.c         | 405 ++++++++++++++++------------
>   tests/test-string-input-visitor.c   |  18 +-
>   3 files changed, 234 insertions(+), 193 deletions(-)
> 

>   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 */
>   };
>   

Makes sense.

> @@ -179,88 +106,208 @@ 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");

Bike-shedding - I don't know if "Too many list elements supplied" would 
make the error any more legible.

>   
>   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");

The error message might be a bit misleading for a range larger than 64k, 
but that's not too bad.

> +            return;
> +        }
> +        assert(siv->lm == LM_INT64_RANGE);
> +        /* fall through */
> +    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) {

I think our compiler options guarantee that we have sane signed 
wraparound and thus this is a safe comparison on overflow; but if you 
were to swap it so that the *obj == INT64_MAX check is performed first, 
you wouldn't even have to debate about whether undefined C semantics are 
being invoked.

> +            /* end of range, check if there is more to parse */
> +            siv->lm = siv->unparsed_string[0] ? LM_UNPARSED : LM_END;
> +        }
> +        return;
> +    case LM_END:
> +        error_setg(errp, "Fewer list elements expected");

Again, bikeshedding if "too many list elements supplied" would make any 
more sense.

> +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)) {

Lots of duplication between the signed and unsigned variants.  But I 
don't see any easy way to factor it out into a common helper, as there 
are just too many places where signed vs. unsigned does not easily lend 
itself to common code.

> @@ -330,9 +381,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]) {

Why did this condition change?

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 v2 7/9] test-string-input-visitor: Use virtual walk

[Eric Blake]

On 11/20/18 3:25 AM, David Hildenbrand wrote:
> We now support virtual walks, so use that instead.
> 
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>   tests/test-string-input-visitor.c | 36 +++++++++++--------------------
>   1 file changed, 12 insertions(+), 24 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 v2 1/9] cutils: Add qemu_strtod() and qemu_strtod_finite()

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 11/20/18 3:25 AM, David Hildenbrand wrote:
>> Let's provide a wrapper for strtod().
>>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>
> This changed enough from v1 that I would have dropped R-b to ensure
> that reviewers notice the differences.
>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>   include/qemu/cutils.h |  2 ++
>>   util/cutils.c         | 65 +++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 67 insertions(+)
>>
>
>> + * If the conversion overflows, store +/-HUGE_VAL in @result, depending
>> + * on the sign, and return -ERANGE.
>> + *
>> + * If the conversion underflows, store ±0.0 in @result, depending on the
>> + * sign, and return -ERANGE.
>
> The use of UTF-8 ± in one place but not both is odd.  I think we're at
> the point where UTF-8 comments are acceptable these days, rather than
> trying to keep our codebase ASCII-clean, so I don't care which way you
> resolve the inconsistency.

217 out of 6455 git-controlled files contain non-ASCII characters.  53
of them are binary, and don't count.  In most text files, it's for
spelling names of authors properly in comments.  Ample precedence for
UTF-8 in comments, I'd say.

That said, I second Eric's call for consistency, with the slightest of
preferrences for plain ASCII.

I spotted UTF-8 in two error messages, which might still be unadvisable:

hw/misc/tmp105.c:        error_setg(errp, "value %" PRId64 ".%03" PRIu64 " °C is out of range",
hw/misc/tmp421.c:        error_setg(errp, "value %" PRId64 ".%03" PRIu64 " °C is out of range",

>> +/**
>> + * Convert string @nptr to a finite double.
>> + *
>> + * Works like qemu_strtod(), except that "NaN" and "inf" are rejected
>> + * with -EINVAL and no conversion is performed.
>> + */
>> +int qemu_strtod_finite(const char *nptr, const char **endptr, double *result)
>> +{
>> +    double tmp;
>> +    int ret;
>> +
>> +    ret = qemu_strtod(nptr, endptr, &tmp);
>> +    if (ret) {
>> +        return ret;
>
> So, if we overflow, we are returning -ERANGE but with nothing stored
> into *result.  This is different from qemu_strtod(), where a return of
> -ERANGE guarantees that *result is one of 4 values (+/- 0.0/inf).
> That seems awkward.

Violates the contract's "like qemu_strtod()".

>> +    } else if (!isfinite(tmp)) {
>> +        if (endptr) {
>> +            *endptr = nptr;
>> +        }
>> +        return -EINVAL;
>
> Rewinding back to the start of "inf" is interesting, but matches your
> documentation.

Yes.  I like it.

>> +    }
>> +
>> +    *result = tmp;
>> +    return ret;
>> +}
>> +
>
> I think you still need to fix -ERANGE handling before I can give R-b.

Re: [Qemu-devel] [PATCH v2 2/9] cutils: Fix qemu_strtosz() & friends to reject non-finite sizes

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 11/20/18 3:25 AM, David Hildenbrand wrote:
>> qemu_strtosz() & friends reject NaNs, but happily accept inifities.
>
> s/inifities/infinities/
>
>> 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 **.
>>
>> Also, add two test cases, testing that "inf" and "NaN" are properly
>> rejected.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>   include/qemu/cutils.h |  6 +++---
>>   monitor.c             |  2 +-
>>   tests/test-cutils.c   | 24 +++++++++++++++++-------
>>   util/cutils.c         | 16 +++++++---------
>>   4 files changed, 28 insertions(+), 20 deletions(-)
>>
>
>> +++ b/util/cutils.c
>> @@ -206,20 +206,18 @@ static int64_t suffix_mul(char suffix, int64_t unit)
>>    * in *end, if not NULL. Return -ERANGE on overflow, Return -EINVAL on
>
> Pre-existing, but since you're touching this area: the second 'Return'
> is unusual capitalization for being mid-sentence.  You could even
> s/Return/of/

"of"?

>
>>    * 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) {
>> -        retval = -EINVAL;
>> +    retval = qemu_strtod_finite(nptr, &endptr, &val);
>> +    if (retval) {
>>           goto out;
>
> Here, retval can be -EINVAL (for failure to parse, or encountering
> "inf" or "NaN") or -ERANGE (overflow, underflow)...
>
>>       }
>>       fraction = modf(val, &integral);
>> @@ -259,17 +257,17 @@ out:
>
> out:
>     if (end) {
>         *end = endptr;
>     } else if (*endptr) {
>         retval = -EINVAL;
>     }
>
>>       return retval;
>
> ...if the failure was -EINVAL due to trailing garbage or empty string,
> nothing changes. If the failure was -EINVAL due to "inf", and the user
> passed in 'end', then 'end' now points to the beginning of "inf"
> instead of the end (probably okay). If the failure was -EINVAL due to
> "inf" and the user gave NULL for 'end', then we slam retval back to
> -EINVAL (no change).  If the failure was -ERANGE, then there is no
> trailing garbage, so *endptr had better be NULL, and we still fail
> with -ERANGE.  Any other way to reach the out label is unchanged from
> earlier logic.
>
> It's some hairy code to think about, but I can't find anything wrong
> with it.  Typo fixes are minor, so
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks for your analysis, Eric.

With the typo fixes:
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH v2 3/9] qapi: Fix string-input-visitor to reject NaN and infinities

[Markus Armbruster]

David Hildenbrand <david@redhat.com> writes:

> The string-input-visitor happily accepts NaN and inifities when parsing

Make that "infinities".

> numbers (doubles). They shouldn't. Fix that.
>
> Also, add two test cases, testing if "NaN" and "inf" is properly
> rejected.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>

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

PS: I still owe you an analysis of backward compatibility impact.

Re: [Qemu-devel] [PATCH v2 2/9] cutils: Fix qemu_strtosz() & friends to reject non-finite sizes

[Eric Blake]

On 11/20/18 2:31 PM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> On 11/20/18 3:25 AM, David Hildenbrand wrote:
>>> qemu_strtosz() & friends reject NaNs, but happily accept inifities.
>>
>> s/inifities/infinities/
>>
>>> 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 **.
>>>
>>> Also, add two test cases, testing that "inf" and "NaN" are properly
>>> rejected.
>>>
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>> ---
>>>    include/qemu/cutils.h |  6 +++---
>>>    monitor.c             |  2 +-
>>>    tests/test-cutils.c   | 24 +++++++++++++++++-------
>>>    util/cutils.c         | 16 +++++++---------
>>>    4 files changed, 28 insertions(+), 20 deletions(-)
>>>
>>
>>> +++ b/util/cutils.c
>>> @@ -206,20 +206,18 @@ static int64_t suffix_mul(char suffix, int64_t unit)
>>>     * in *end, if not NULL. Return -ERANGE on overflow, Return -EINVAL on
>>
>> Pre-existing, but since you're touching this area: the second 'Return'
>> is unusual capitalization for being mid-sentence.  You could even
>> s/Return/of/
> 
> "of"?

"or" (ouch - wrong time for my fingers to be slipping on the keyboard)


>> It's some hairy code to think about, but I can't find anything wrong
>> with it.  Typo fixes are minor, so
>>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> Thanks for your analysis, Eric.
> 
> With the typo fixes:

Including the fix of my attempt at a typo fix :)

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

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

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

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 11/20/18 11:20 AM, Eric Blake wrote:
>> On 11/20/18 11:06 AM, Eric Blake wrote:
>>> On 11/20/18 3:25 AM, David Hildenbrand wrote:
>>>> Test that very big/small values are not accepted and that ranges with
>>>> only one element work. Also test that ranges are ascending and cannot
>>>> have more than 65536 elements.
>>>>
>>>> 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 | 41 +++++++++++++++++++++++++++++--
>>>>   1 file changed, 39 insertions(+), 2 deletions(-)
>>>>
>>>
>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>
>> Do we also want to test garbage strings like:
>>
>> "1-" (incomplete range)
>> "1X-2" (garbage suffix on first element)
>> "1-2X" (garbage suffix on second element)
>>
>> and/or
>>
>> "-2--1" (valid range of signed integers)
>> "-1--2" (questionable whether this is valid for the two largest
>> unsigned integers)
>
> Or even " 1- 2" (we permit leading whitespace for plain integers - do
> we also permit it in both sides of a range)?  Also, if we permit
> whitespace after the range '-', should we permit it beforehand?

For what it's worth, the tests for integers (as opposed to ranges)
aren't that thorough, either.  test-cutils.c already provides cover for
them, but not for ranges.

> These sorts of questions may be fine in followup patches.

David is doing me a favor with these patches.  The tests feel good
enough, I don't want to hold these patches hostage to extort more.

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

Re: [Qemu-devel] [PATCH v2 6/9] qapi: Rewrite string-input-visitor

[Markus Armbruster]

I think the title should be something like

    qapi: Rewrite string-input-visitor's integer and list parsing

because you don't actually rewrite all of it.

Eric Blake <eblake@redhat.com> writes:

> On 11/20/18 3:25 AM, David Hildenbrand wrote:
>> 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
>>
>> 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.
>>
>> Please note that no users/callers have to be fixed up. Candiates using
>
> s/Candiates/Candidates/
>
>> visit_type_uint16List() and friends are:
>> - backends/hostmem.c:host_memory_backend_set_host_nodes()
>> -- Code can deal with dupilcates/unsorted lists
>
> s/dupilcates/duplicates/
>
>> - numa.c::query_memdev()
>> -- via object_property_get_uint16List(), the list will still be sorted
>>     and without duplicates (via host_memory_backend_get_host_nodes())
>> - qapi-visit.c::visit_type_Memdev_members()
>> - qapi-visit.c::visit_type_NumaNodeOptions_members()
>> - qapi-visit.c::visit_type_RockerOfDpaGroup_members
>> - qapi-visit.c::visit_type_RxFilterInfo_members()
>> -- Not used with string-input-visitor.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>   include/qapi/string-input-visitor.h |   4 +-
>>   qapi/string-input-visitor.c         | 405 ++++++++++++++++------------
>>   tests/test-string-input-visitor.c   |  18 +-
>>   3 files changed, 234 insertions(+), 193 deletions(-)
>>
>
>>   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 */
>>   };
>>   
>
> Makes sense.
>
>> @@ -179,88 +106,208 @@ 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");
>
> Bike-shedding - I don't know if "Too many list elements supplied"
> would make the error any more legible.

See my review of PATCH RFC 3/6:

    Hmm.  qobject_input_check_list() reports "Only %u list elements expected
    in %s" here.  Looks unintuitive, until you remember how it's normally
    used: to parse user input.  The error gets reported when the parsing
    didn't consume the whole list.  Can only happen with a virtual walk.
    And when it happens, the issue to report is "you provided a longer list
    than I can accept".  qobject_input_check_list() does exactly that.  Your
    message is less clear.

David didn't feel counting elements just for that was worthwhile here,
and I agreed.

>>     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");
>
> The error message might be a bit misleading for a range larger than
> 64k, but that's not too bad.
>
>> +            return;
>> +        }
>> +        assert(siv->lm == LM_INT64_RANGE);
>> +        /* fall through */
>> +    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) {
>
> I think our compiler options guarantee that we have sane signed
> wraparound and thus this is a safe comparison on overflow; but if you
> were to swap it so that the *obj == INT64_MAX check is performed
> first, you wouldn't even have to debate about whether undefined C
> semantics are being invoked.
>
>> +            /* end of range, check if there is more to parse */
>> +            siv->lm = siv->unparsed_string[0] ? LM_UNPARSED : LM_END;
>> +        }
>> +        return;
>> +    case LM_END:
>> +        error_setg(errp, "Fewer list elements expected");
>
> Again, bikeshedding if "too many list elements supplied" would make
> any more sense.

Same argument.

>> +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)) {
>
> Lots of duplication between the signed and unsigned variants.  But I
> don't see any easy way to factor it out into a common helper, as there
> are just too many places where signed vs. unsigned does not easily
> lend itself to common code.

Yes.

>> @@ -330,9 +381,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]) {
>
> Why did this condition change?

As far as I can tell, siv->string can't ever be null.  Sticking the
change into this patch is perhaps debatable.  I'm okay with it.

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

With the commit message improved once more:
Reviewed-by: Markus Armbruster <armbru@redhat.com>

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

[David Hildenbrand]

On 20.11.18 21:07, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> On 11/20/18 3:25 AM, David Hildenbrand wrote:
>>> Let's provide a wrapper for strtod().
>>>
>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>
>> This changed enough from v1 that I would have dropped R-b to ensure
>> that reviewers notice the differences.

Indeed, dropping it now ;)

>>
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>> ---
>>>   include/qemu/cutils.h |  2 ++
>>>   util/cutils.c         | 65 +++++++++++++++++++++++++++++++++++++++++++
>>>   2 files changed, 67 insertions(+)
>>>
>>
>>> + * If the conversion overflows, store +/-HUGE_VAL in @result, depending
>>> + * on the sign, and return -ERANGE.
>>> + *
>>> + * If the conversion underflows, store ±0.0 in @result, depending on the
>>> + * sign, and return -ERANGE.
>>
>> The use of UTF-8 ± in one place but not both is odd.  I think we're at
>> the point where UTF-8 comments are acceptable these days, rather than
>> trying to keep our codebase ASCII-clean, so I don't care which way you
>> resolve the inconsistency.
> 
> 217 out of 6455 git-controlled files contain non-ASCII characters.  53
> of them are binary, and don't count.  In most text files, it's for
> spelling names of authors properly in comments.  Ample precedence for
> UTF-8 in comments, I'd say.
> 
> That said, I second Eric's call for consistency, with the slightest of
> preferrences for plain ASCII.

I'll just go with +/-. Thanks.

> 
> I spotted UTF-8 in two error messages, which might still be unadvisable:
> 
> hw/misc/tmp105.c:        error_setg(errp, "value %" PRId64 ".%03" PRIu64 " °C is out of range",
> hw/misc/tmp421.c:        error_setg(errp, "value %" PRId64 ".%03" PRIu64 " °C is out of range",
> 
>>> +/**
>>> + * Convert string @nptr to a finite double.
>>> + *
>>> + * Works like qemu_strtod(), except that "NaN" and "inf" are rejected
>>> + * with -EINVAL and no conversion is performed.
>>> + */
>>> +int qemu_strtod_finite(const char *nptr, const char **endptr, double *result)
>>> +{
>>> +    double tmp;
>>> +    int ret;
>>> +
>>> +    ret = qemu_strtod(nptr, endptr, &tmp);
>>> +    if (ret) {
>>> +        return ret;
>>
>> So, if we overflow, we are returning -ERANGE but with nothing stored
>> into *result.  This is different from qemu_strtod(), where a return of
>> -ERANGE guarantees that *result is one of 4 values (+/- 0.0/inf).
>> That seems awkward.
> 
> Violates the contract's "like qemu_strtod()".

Right, I missed that. What about something like this:

int qemu_strtod_finite(const char *nptr, const char **endptr, double
*result)
{
    double tmp;
    int ret;

    ret = qemu_strtod(nptr, endptr, &tmp);
    if (!ret && !isfinite(tmp)) {
        if (endptr) {
            *endptr = nptr;
        }
        ret = -EINVAL;
    }

    if (ret != -EINVAL) {
        *result = tmp;
    }
    return ret;
}



-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v2 2/9] cutils: Fix qemu_strtosz() & friends to reject non-finite sizes

[David Hildenbrand]

On 20.11.18 21:41, Eric Blake wrote:
> On 11/20/18 2:31 PM, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>>
>>> On 11/20/18 3:25 AM, David Hildenbrand wrote:
>>>> qemu_strtosz() & friends reject NaNs, but happily accept inifities.
>>>
>>> s/inifities/infinities/
>>>
>>>> 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 **.
>>>>
>>>> Also, add two test cases, testing that "inf" and "NaN" are properly
>>>> rejected.
>>>>
>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>> ---
>>>>    include/qemu/cutils.h |  6 +++---
>>>>    monitor.c             |  2 +-
>>>>    tests/test-cutils.c   | 24 +++++++++++++++++-------
>>>>    util/cutils.c         | 16 +++++++---------
>>>>    4 files changed, 28 insertions(+), 20 deletions(-)
>>>>
>>>
>>>> +++ b/util/cutils.c
>>>> @@ -206,20 +206,18 @@ static int64_t suffix_mul(char suffix, int64_t unit)
>>>>     * in *end, if not NULL. Return -ERANGE on overflow, Return -EINVAL on
>>>
>>> Pre-existing, but since you're touching this area: the second 'Return'
>>> is unusual capitalization for being mid-sentence.  You could even
>>> s/Return/of/
>>
>> "of"?
> 
> "or" (ouch - wrong time for my fingers to be slipping on the keyboard)

Shouldn't that be "and" and s/Return/Returns/


"Returns -ERANGE on overflow and -EINVAL on other errors".

I can include that fixup (whetever version you guys prefer)

> 
> 
>>> It's some hairy code to think about, but I can't find anything wrong
>>> with it.  Typo fixes are minor, so
>>>
>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>
>> Thanks for your analysis, Eric.
>>
>> With the typo fixes:
> 
> Including the fix of my attempt at a typo fix :)
> 
>> Reviewed-by: Markus Armbruster <armbru@redhat.com>

Thanks!

-- 

Thanks,

David / dhildenb

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

[David Hildenbrand]

On 20.11.18 18:26, Eric Blake wrote:
> On 11/20/18 11:20 AM, Eric Blake wrote:
>> On 11/20/18 11:06 AM, Eric Blake wrote:
>>> On 11/20/18 3:25 AM, David Hildenbrand wrote:
>>>> Test that very big/small values are not accepted and that ranges with
>>>> only one element work. Also test that ranges are ascending and cannot
>>>> have more than 65536 elements.
>>>>
>>>> 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 | 41 +++++++++++++++++++++++++++++--
>>>>   1 file changed, 39 insertions(+), 2 deletions(-)
>>>>
>>>
>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>
>> Do we also want to test garbage strings like:
>>
>> "1-" (incomplete range)
>> "1X-2" (garbage suffix on first element)
>> "1-2X" (garbage suffix on second element)
>>
>> and/or
>>
>> "-2--1" (valid range of signed integers)
>> "-1--2" (questionable whether this is valid for the two largest unsigned 
>> integers)
> 
> Or even " 1- 2" (we permit leading whitespace for plain integers - do we 
> also permit it in both sides of a range)?  Also, if we permit whitespace 
> after the range '-', should we permit it beforehand?
> 
> These sorts of questions may be fine in followup patches.
> 

As always, you can never cover all cases during tests :)

While these things surely make sense, I will not add them right now. As
you said, we can do that later.


Taking about spaces:

I think spaces before the "-" are not supported before/after the rewrite
(I remember that strto.* does not skip over them). Spaces after the
space should be covered by strto* automatically (strto.* skips over
leading spaces).

Thanks!

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH v2 6/9] qapi: Rewrite string-input-visitor

[David Hildenbrand]

On 20.11.18 21:58, Markus Armbruster wrote:
> I think the title should be something like
> 
>     qapi: Rewrite string-input-visitor's integer and list parsing
> 
> because you don't actually rewrite all of it.
> 
> Eric Blake <eblake@redhat.com> writes:
> 
>> On 11/20/18 3:25 AM, David Hildenbrand wrote:
>>> 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
>>>
>>> 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.
>>>
>>> Please note that no users/callers have to be fixed up. Candiates using
>>
>> s/Candiates/Candidates/
>>
>>> visit_type_uint16List() and friends are:
>>> - backends/hostmem.c:host_memory_backend_set_host_nodes()
>>> -- Code can deal with dupilcates/unsorted lists
>>
>> s/dupilcates/duplicates/

Thanks, both fixed.

> 
>>> @@ -330,9 +381,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]) {
>>
>> Why did this condition change?
> 
> As far as I can tell, siv->string can't ever be null.  Sticking the
> change into this patch is perhaps debatable.  I'm okay with it.

Yes, we have an assertion when creating the visitor. Do you want me to
pull this into a separate patch?

(It made sense under the old patch subject ;) )

> 
>> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> With the commit message improved once more:
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> 
Thanks!

-- 

Thanks,

David / dhildenb

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

[Markus Armbruster]

David Hildenbrand <david@redhat.com> writes:

> On 20.11.18 21:07, Markus Armbruster wrote:
>> Eric Blake <eblake@redhat.com> writes:
>> 
>>> On 11/20/18 3:25 AM, David Hildenbrand wrote:
>>>> Let's provide a wrapper for strtod().
>>>>
>>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>>
>>> This changed enough from v1 that I would have dropped R-b to ensure
>>> that reviewers notice the differences.
>
> Indeed, dropping it now ;)
>
>>>
>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>> ---
>>>>   include/qemu/cutils.h |  2 ++
>>>>   util/cutils.c         | 65 +++++++++++++++++++++++++++++++++++++++++++
>>>>   2 files changed, 67 insertions(+)
>>>>
>>>
>>>> + * If the conversion overflows, store +/-HUGE_VAL in @result, depending
>>>> + * on the sign, and return -ERANGE.
>>>> + *
>>>> + * If the conversion underflows, store ±0.0 in @result, depending on the
>>>> + * sign, and return -ERANGE.
>>>
>>> The use of UTF-8 ± in one place but not both is odd.  I think we're at
>>> the point where UTF-8 comments are acceptable these days, rather than
>>> trying to keep our codebase ASCII-clean, so I don't care which way you
>>> resolve the inconsistency.
>> 
>> 217 out of 6455 git-controlled files contain non-ASCII characters.  53
>> of them are binary, and don't count.  In most text files, it's for
>> spelling names of authors properly in comments.  Ample precedence for
>> UTF-8 in comments, I'd say.
>> 
>> That said, I second Eric's call for consistency, with the slightest of
>> preferrences for plain ASCII.
>
> I'll just go with +/-. Thanks.
>
>> 
>> I spotted UTF-8 in two error messages, which might still be unadvisable:
>> 
>> hw/misc/tmp105.c:        error_setg(errp, "value %" PRId64 ".%03" PRIu64 " °C is out of range",
>> hw/misc/tmp421.c:        error_setg(errp, "value %" PRId64 ".%03" PRIu64 " °C is out of range",
>> 
>>>> +/**
>>>> + * Convert string @nptr to a finite double.
>>>> + *
>>>> + * Works like qemu_strtod(), except that "NaN" and "inf" are rejected
>>>> + * with -EINVAL and no conversion is performed.
>>>> + */
>>>> +int qemu_strtod_finite(const char *nptr, const char **endptr, double *result)
>>>> +{
>>>> +    double tmp;
>>>> +    int ret;
>>>> +
>>>> +    ret = qemu_strtod(nptr, endptr, &tmp);
>>>> +    if (ret) {
>>>> +        return ret;
>>>
>>> So, if we overflow, we are returning -ERANGE but with nothing stored
>>> into *result.  This is different from qemu_strtod(), where a return of
>>> -ERANGE guarantees that *result is one of 4 values (+/- 0.0/inf).
>>> That seems awkward.
>> 
>> Violates the contract's "like qemu_strtod()".
>
> Right, I missed that. What about something like this:
>
> int qemu_strtod_finite(const char *nptr, const char **endptr, double
> *result)
> {
>     double tmp;
>     int ret;
>
>     ret = qemu_strtod(nptr, endptr, &tmp);
>     if (!ret && !isfinite(tmp)) {
>         if (endptr) {
>             *endptr = nptr;
>         }
>         ret = -EINVAL;
>     }
>
>     if (ret != -EINVAL) {
>         *result = tmp;
>     }
>     return ret;
> }

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

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

[Markus Armbruster]

David Hildenbrand <david@redhat.com> writes:

> On 20.11.18 18:26, Eric Blake wrote:
>> On 11/20/18 11:20 AM, Eric Blake wrote:
>>> On 11/20/18 11:06 AM, Eric Blake wrote:
>>>> On 11/20/18 3:25 AM, David Hildenbrand wrote:
>>>>> Test that very big/small values are not accepted and that ranges with
>>>>> only one element work. Also test that ranges are ascending and cannot
>>>>> have more than 65536 elements.
>>>>>
>>>>> 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 | 41 +++++++++++++++++++++++++++++--
>>>>>   1 file changed, 39 insertions(+), 2 deletions(-)
>>>>>
>>>>
>>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>>
>>> Do we also want to test garbage strings like:
>>>
>>> "1-" (incomplete range)
>>> "1X-2" (garbage suffix on first element)
>>> "1-2X" (garbage suffix on second element)
>>>
>>> and/or
>>>
>>> "-2--1" (valid range of signed integers)
>>> "-1--2" (questionable whether this is valid for the two largest unsigned 
>>> integers)
>> 
>> Or even " 1- 2" (we permit leading whitespace for plain integers - do we 
>> also permit it in both sides of a range)?  Also, if we permit whitespace 
>> after the range '-', should we permit it beforehand?
>> 
>> These sorts of questions may be fine in followup patches.
>> 
>
> As always, you can never cover all cases during tests :)
>
> While these things surely make sense, I will not add them right now. As
> you said, we can do that later.
>
>
> Taking about spaces:
>
> I think spaces before the "-" are not supported before/after the rewrite
> (I remember that strto.* does not skip over them). Spaces after the
> space should be covered by strto* automatically (strto.* skips over
> leading spaces).

qemu_strtol() & friends ignore leading whitespace, just like strtol()
does.  Trailing whitespace is handled like any other trailing
characters: hard error when endptr is null, else make *endptr point to
it.

I'd expect whitespace to be accepted before either number of a range,
but not between the first number and the '-'.

Perhaps rejecting whitespace there would be the cleaner interface, but
then we get to discuss backward compatibility, and I go "thanks, but no,
thanks."

Re: [Qemu-devel] [PATCH v2 6/9] qapi: Rewrite string-input-visitor

[Markus Armbruster]

David Hildenbrand <david@redhat.com> writes:

> On 20.11.18 21:58, Markus Armbruster wrote:
>> I think the title should be something like
>> 
>>     qapi: Rewrite string-input-visitor's integer and list parsing
>> 
>> because you don't actually rewrite all of it.
>> 
>> Eric Blake <eblake@redhat.com> writes:
>> 
>>> On 11/20/18 3:25 AM, David Hildenbrand wrote:
>>>> 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
>>>>
>>>> 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.
>>>>
>>>> Please note that no users/callers have to be fixed up. Candiates using
>>>
>>> s/Candiates/Candidates/
>>>
>>>> visit_type_uint16List() and friends are:
>>>> - backends/hostmem.c:host_memory_backend_set_host_nodes()
>>>> -- Code can deal with dupilcates/unsorted lists
>>>
>>> s/dupilcates/duplicates/
>
> Thanks, both fixed.
>
>> 
>>>> @@ -330,9 +381,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]) {
>>>
>>> Why did this condition change?
>> 
>> As far as I can tell, siv->string can't ever be null.  Sticking the
>> change into this patch is perhaps debatable.  I'm okay with it.
>
> Yes, we have an assertion when creating the visitor. Do you want me to
> pull this into a separate patch?

I'm okay with it as is.  Up to you.

> (It made sense under the old patch subject ;) )

It did :)

>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> 
>> With the commit message improved once more:
>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>> 
> Thanks!

Re: [Qemu-devel] [PATCH v2 2/9] cutils: Fix qemu_strtosz() & friends to reject non-finite sizes

[Markus Armbruster]

David Hildenbrand <david@redhat.com> writes:

> On 20.11.18 21:41, Eric Blake wrote:
>> On 11/20/18 2:31 PM, Markus Armbruster wrote:
>>> Eric Blake <eblake@redhat.com> writes:
>>>
>>>> On 11/20/18 3:25 AM, David Hildenbrand wrote:
>>>>> qemu_strtosz() & friends reject NaNs, but happily accept inifities.
>>>>
>>>> s/inifities/infinities/
>>>>
>>>>> 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 **.
>>>>>
>>>>> Also, add two test cases, testing that "inf" and "NaN" are properly
>>>>> rejected.
>>>>>
>>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>>> ---
>>>>>    include/qemu/cutils.h |  6 +++---
>>>>>    monitor.c             |  2 +-
>>>>>    tests/test-cutils.c   | 24 +++++++++++++++++-------
>>>>>    util/cutils.c         | 16 +++++++---------
>>>>>    4 files changed, 28 insertions(+), 20 deletions(-)
>>>>>
>>>>
>>>>> +++ b/util/cutils.c
>>>>> @@ -206,20 +206,18 @@ static int64_t suffix_mul(char suffix, int64_t unit)
>>>>>     * in *end, if not NULL. Return -ERANGE on overflow, Return -EINVAL on
>>>>
>>>> Pre-existing, but since you're touching this area: the second 'Return'
>>>> is unusual capitalization for being mid-sentence.  You could even
>>>> s/Return/of/
>>>
>>> "of"?
>> 
>> "or" (ouch - wrong time for my fingers to be slipping on the keyboard)
>
> Shouldn't that be "and" and s/Return/Returns/
>
>
> "Returns -ERANGE on overflow and -EINVAL on other errors".

I prefer imperative mood for function contracts: "Convert string to
bytes", "Return -ERANGE on overflow", and so forth.

Other than that, I like your phrasing.  I'd put a comma before "and",
though.

> I can include that fixup (whetever version you guys prefer)
>
>> 
>> 
>>>> It's some hairy code to think about, but I can't find anything wrong
>>>> with it.  Typo fixes are minor, so
>>>>
>>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>>
>>> Thanks for your analysis, Eric.
>>>
>>> With the typo fixes:
>> 
>> Including the fix of my attempt at a typo fix :)
>> 
>>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>
> Thanks!

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

[Eric Blake]

On 11/21/18 4:35 AM, David Hildenbrand wrote:

>>>> +int qemu_strtod_finite(const char *nptr, const char **endptr, double *result)
>>>> +{
>>>> +    double tmp;
>>>> +    int ret;
>>>> +
>>>> +    ret = qemu_strtod(nptr, endptr, &tmp);
>>>> +    if (ret) {
>>>> +        return ret;
>>>
>>> So, if we overflow, we are returning -ERANGE but with nothing stored
>>> into *result.  This is different from qemu_strtod(), where a return of
>>> -ERANGE guarantees that *result is one of 4 values (+/- 0.0/inf).
>>> That seems awkward.
>>
>> Violates the contract's "like qemu_strtod()".
> 
> Right, I missed that. What about something like this:
> 
> int qemu_strtod_finite(const char *nptr, const char **endptr, double
> *result)
> {
>      double tmp;
>      int ret;
> 
>      ret = qemu_strtod(nptr, endptr, &tmp);
>      if (!ret && !isfinite(tmp)) {
>          if (endptr) {
>              *endptr = nptr;
>          }
>          ret = -EINVAL;
>      }
> 
>      if (ret != -EINVAL) {
>          *result = tmp;
>      }
>      return ret;
> }

With that algorithm, v3 can have:
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 v2 2/9] cutils: Fix qemu_strtosz() & friends to reject non-finite sizes

[Eric Blake]

On 11/21/18 4:44 AM, David Hildenbrand wrote:

>>>>> @@ -206,20 +206,18 @@ static int64_t suffix_mul(char suffix, int64_t unit)
>>>>>      * in *end, if not NULL. Return -ERANGE on overflow, Return -EINVAL on
>>>>
>>>> Pre-existing, but since you're touching this area: the second 'Return'
>>>> is unusual capitalization for being mid-sentence.  You could even
>>>> s/Return/of/
>>>
>>> "of"?
>>
>> "or" (ouch - wrong time for my fingers to be slipping on the keyboard)
> 
> Shouldn't that be "and" and s/Return/Returns/
> 
> 
> "Returns -ERANGE on overflow and -EINVAL on other errors".
> 
> I can include that fixup (whetever version you guys prefer)

I was thinking:

Return -ERANGE on overflow, or -EINVAL on other errors.

'Return', not 'Returns', because of imperative mood.  The choice of 
'and' vs. 'or' is less of a sticking point; both sound fine to my native 
ear, especially since the word 'other' makes it apparent that you won't 
have both overflow and a conversion error at the same time (my initial 
choice of 'or' rather than 'and' was solely because you can't have two 
return values at once; but using 'and' seems okay at implying a sense of 
prioritization where overflow trumps other detected errors).

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