qapi: rewrite string-input-visitor

Let's use the new function.

Signed-off-by David Hildenbrand <[email protected]>

---
 qapi/string-input-visitor.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

qapi/string-input-visitor.c +1 -0

index b3fdd0827d..dee708d384 100644
@@ -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);

Quoted 26 lines · David Hildenbrand

Let's use the new function.

Signed-off-by David Hildenbrand <[email protected]>

---
 qapi/string-input-visitor.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

qapi/string-input-visitor.c +1 -1

index b3fdd0827d..dee708d384 100644
@@ -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;
 

qapi/string-input-visitor.c static void parse_type_number(Visitor *v, const char *name, double *obj, · line 322

Quoted 7 lines

qapi/string-input-visitor.c +1 -3

-    errno = 0;
-    val = strtod(siv->string, &endp);
-    if (errno || endp == siv->string || *endp) {
+    if (qemu_strtod(siv->string, NULL, &val)) {
         error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
                    "number");
         return;

Three more: in qobject-input-visitor.c's
qobject_input_type_number_keyval(),

json-parser.c's parse_literal().

The latter doesn't check for errors since the lexer ensures the input is sane. Overflow can still happen, and is silently ignored. Feel free not to convert this one.

Quoted 2 lines

Three more: in qobject-input-visitor.c's
qobject_input_type_number_keyval(),

This one is interesting, as it properly bails out when parsing "inf" (via isFinite()). - should we do the same for the string input visitor?

Especially, should we forbid "inf" and "NaN" in both scenarios?

Quoted 7 lines · David Hildenbrand

Quoted 2 lines

Three more: in qobject-input-visitor.c's
qobject_input_type_number_keyval(),

This one is interesting, as it properly bails out when parsing "inf" (via isFinite()). - should we do the same for the string input visitor?

Especially, should we forbid "inf" and "NaN" in both scenarios?

JSON can't represent non-finite doubles. Internally, we might be able to use them, but you have a point that consistently rejecting non-finite in all of our QAPI parsers makes it easier to reason about the code base (the command line can't be used to inject a value not possible via QMP). So that makes sense to me. qemu_strtod() shouldn't reject non-finite numbers (because it is useful for more than just qapi), but we could add a new qemu_strtod_finite() if that would help avoid duplication.

Quoted 6 lines · David Hildenbrand

Quoted 2 lines

Three more: in qobject-input-visitor.c's
qobject_input_type_number_keyval(),

This one is interesting, as it properly bails out when parsing "inf" (via isFinite()). - should we do the same for the string input visitor?

Quoted 1 line

Especially, should we forbid "inf" and "NaN" in both scenarios?

JSON can't represent non-finite doubles. Internally, we might be able to use them, but you have a point that consistently rejecting non-finite in all of our QAPI parsers makes it easier to reason about the code base (the command line can't be used to inject a value not possible via QMP). So that makes sense to me.

qemu_strtod() shouldn't
reject non-finite numbers (because it is useful for more than just qapi),

but we could add a new qemu_strtod_finite() if that would help
avoid duplication.

Quoted 7 lines

cutil.c's do_strtosz(), and
json-parser.c's parse_literal().

The latter doesn't check for errors since the lexer ensures the input is sane. Overflow can still happen, and is silently ignored. Feel free not to convert this one.

I'll do the conversion of all (allowing -ERANGE where it used to be allow), we can then discuss with the patches at hand if it makes sense.

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

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.

While at it, do some smaller style changes (e.g. use g_assert).

Signed-off-by David Hildenbrand <[email protected]>

---
 include/qapi/string-input-visitor.h |   3 +-
 qapi/string-input-visitor.c         | 436 +++++++++++++++++-----------
 tests/test-string-input-visitor.c   |  18 +-
 3 files changed, 264 insertions(+), 193 deletions(-)

include/qapi/string-input-visitor.h +0 -0

index 33551340e3..2c40f7f5a6 100644

include/qapi/string-input-visitor.h +0 -0

  */
 Visitor *string_input_visitor_new(const char *str);
 

qapi/string-input-visitor.c +17 -3

index dee708d384..16da47409e 100644
@@ -4,10 +4,10 @@
  * Copyright Red Hat, Inc. 2012-2016
  *
  * Author: Paolo Bonzini <[email protected]>
+ *         David Hildenbrand <[email protected]>
  *
  * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
  * See the COPYING.LIB file in the top-level directory.
- *
  */
 
 #include "qemu/osdep.h"

@@ -18,21 +18,39 @@
 #include "qapi/qmp/qerror.h"
 #include "qapi/qmp/qnull.h"
 #include "qemu/option.h"
-#include "qemu/queue.h"
-#include "qemu/range.h"
 #include "qemu/cutils.h"
 
+typedef enum ListMode {
+    /* no list parsing active / no list expected */
+    LM_NONE,
+    /* we have an unparsed string remaining */
+    LM_UNPARSED,
+    /* we have an unfinished int64 range */
+    LM_INT64_RANGE,
+    /* we have an unfinished uint64 range */
+    LM_UINT64_RANGE,
+    /* we have parsed the string completely and no range is remaining */
+    LM_END,
+} ListMode;
+
+typedef union RangeLimit {
+    int64_t i64;
+    uint64_t u64;

qapi/string-input-visitor.c +1 -69

+    /* the original string to parse */
     const char *string;
-    void *list; /* Only needed for sanity checking the caller */
 };
 
 static StringInputVisitor *to_siv(Visitor *v)

@@ -40,136 +58,48 @@ 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);

qapi/string-input-visitor.c +2 -4

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

qapi/string-input-visitor.c +5 -5

         return;
     }
+    siv->list = list;
+    siv->unparsed_string = siv->string;
 
-    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;
         }

qapi/string-input-visitor.c +0 -3

 static GenericList *next_list(Visitor *v, GenericList *tail, size_t size)
 {
     StringInputVisitor *siv = to_siv(v);
-    Range *r;
-
-    if (!siv->ranges || !siv->cur_range) {

qapi/string-input-visitor.c +0 -9

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

qapi/string-input-visitor.c +0 -0

     }
 
     tail->next = g_malloc0(size);

qapi/string-input-visitor.c +3 -14

         return;
-    }
-
-    r = siv->cur_range->data;
-    if (!r) {
+    case LM_END:
         return;
+    default:
+        g_assert_not_reached();
     }
-
-    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;
-        }

qapi/string-input-visitor.c +63 -16

+    g_assert(siv->list == obj);
+    siv->list = NULL;
+    siv->unparsed_string = NULL;
+    siv->lm = LM_NONE;
 }
 
-static void parse_type_int64(Visitor *v, const char *name, int64_t *obj,
-                             Error **errp)
+static int try_parse_int64_list_entry(StringInputVisitor *siv, int64_t *obj)
 {
-    StringInputVisitor *siv = to_siv(v);
+    const char *endptr;
+    int64_t start, end;
 
-    if (parse_str(siv, name, errp) < 0) {
-        return;
+    if (qemu_strtoi64(siv->unparsed_string, &endptr, 0, &start)) {
+        return -EINVAL;
     }
 
-    if (!siv->ranges) {
-        goto error;
+    switch (endptr[0]) {
+    case '\0':
+        siv->lm = LM_END;
+        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;
+        }
+        /* we require at least two elements in a range */
+        if (start >= end) {
+            return -EINVAL;
+        }
+        switch (endptr[0]) {
+        case '\0':
+            siv->unparsed_string = endptr;
+            break;
+        case ',':
+            siv->unparsed_string = endptr + 1;
+            break;
+        default:
+            return -EINVAL;
+        }
+        /* we have a proper range */
+        siv->lm = LM_INT64_RANGE;
+        siv->rangeNext.i64 = start + 1;
+        siv->rangeEnd.i64 = end;
+        break;
+    default:
+        return -EINVAL;
     }
 
-    if (!siv->cur_range) {
-        Range *r;
+    *obj = start;
+    return 0;
+}
 
-        siv->cur_range = g_list_first(siv->ranges);
-        if (!siv->cur_range) {
-            goto error;
+static void parse_type_int64(Visitor *v, const char *name, int64_t *obj,
+                             Error **errp)
+{
+    StringInputVisitor *siv = to_siv(v);
+    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;
         }
-
-        r = siv->cur_range->data;
-        if (!r) {
-            goto error;
+        *obj = val;
+        return;
+    case LM_INT64_RANGE:
+        /* return the next element in the range */
+        g_assert(siv->rangeNext.i64 <= siv->rangeEnd.i64);

qapi/string-input-visitor.c +63 -15

+        return;
+    }
+}
+
+static int try_parse_uint64_list_entry(StringInputVisitor *siv, uint64_t *obj)
+{
+    const char *endptr;
+    uint64_t start, end;
 
-        siv->cur = range_lob(r);
+    /* parse a simple uint64 or range */
+    if (qemu_strtou64(siv->unparsed_string, &endptr, 0, &start)) {
+        return -EINVAL;
     }
 
-    *obj = siv->cur;
-    siv->cur++;
-    return;
+    switch (endptr[0]) {
+    case '\0':
+        siv->lm = LM_END;
+        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;
+        }
+        /* we require at least two elements in a range */
+        if (start >= end) {
+            return -EINVAL;
+        }
+        switch (endptr[0]) {
+        case '\0':
+            siv->unparsed_string = endptr;
+            break;
+        case ',':
+            siv->unparsed_string = endptr + 1;
+            break;
+        default:
+            return -EINVAL;
+        }
+        /* we have a proper range */
+        siv->lm = LM_UINT64_RANGE;
+        siv->rangeNext.u64 = start + 1;
+        siv->rangeEnd.u64 = end;
+        break;
+    default:
+        return -EINVAL;
+    }
 
-error:
-    error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null",
-               "an int64 value or range");
+    *obj = start;
+    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_UINT64_RANGE:
+        /* return the next element in the range */
+        g_assert(siv->rangeNext.u64 <= siv->rangeEnd.u64);

qapi/string-input-visitor.c +13 -0

+        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;
+    case LM_END:
+        error_setg(errp, "No more elements in the list.");
+        return;
+    default:
+        error_setg(errp, "Lists don't support mixed types.");
+        return;
     }
 }
 

qapi/string-input-visitor.c +23 -4

     parse_option_size(name, siv->string, &val, &err);
     if (err) {
         error_propagate(errp, err);

@@ -285,6 +348,11 @@ static void parse_type_bool(Visitor *v, const char *name, bool *obj,
 {
     StringInputVisitor *siv = to_siv(v);
 
+    if (siv->lm != LM_NONE) {
+        error_setg(errp, "Lists not supported for type \"boolean\"");
+        return;
+    }
+
     if (!strcasecmp(siv->string, "on") ||
         !strcasecmp(siv->string, "yes") ||
         !strcasecmp(siv->string, "true")) {

@@ -307,6 +375,11 @@ static void parse_type_str(Visitor *v, const char *name, char **obj,
 {
     StringInputVisitor *siv = to_siv(v);
 
+    if (siv->lm != LM_NONE) {
+        error_setg(errp, "Lists not supported for type \"string\"");
+        return;
+    }
+
     *obj = g_strdup(siv->string);
 }
 

@@ -316,6 +389,11 @@ static void parse_type_number(Visitor *v, const char *name, double *obj,
     StringInputVisitor *siv = to_siv(v);
     double val;
 
+    if (siv->lm != LM_NONE) {
+        error_setg(errp, "Lists not supported for type \"number\"");
+        return;
+    }
+
     if (qemu_strtod(siv->string, NULL, &val)) {
         error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
                    "number");

@@ -332,7 +410,12 @@ static void parse_type_null(Visitor *v, const char *name, QNull **obj,
 
     *obj = NULL;
 
-    if (!siv->string || siv->string[0]) {
+    if (siv->lm != LM_NONE) {
+        error_setg(errp, "Lists not supported for type \"null\"");
+        return;
+    }
+
+    if (siv->string[0]) {
         error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
                    "null");
         return;

@@ -345,8 +428,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);
 }
 

@@ -354,7 +435,7 @@ Visitor *string_input_visitor_new(const char *str)
 {
     StringInputVisitor *v;
 
-    assert(str);
+    g_assert(str);
     v = g_malloc0(sizeof(*v));
 
     v->visitor.type = VISITOR_INPUT;

@@ -372,5 +453,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;
 }

tests/test-string-input-visitor.c +0 -10

index 88e0e1aa9a..0a4152f79d 100644
@@ -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++) {

Quoted 10 lines · 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

Known weirdness: empty list is invalid (test-string-input-visitor.c demonstates). Your patch doesn't change that (or else it would update the test). Should it be changed?

Quoted 13 lines

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.

Quoted 1 line

While at it, do some smaller style changes (e.g. use g_assert).

Please don't replace assert() by g_assert() in files that consistently use assert() now.

In my opinion, g_assert() is one of the cases where GLib pointlessly reinvents wheels that have been carrying load since forever.

Quoted 13 lines

Signed-off-by David Hildenbrand <[email protected]>

---
 include/qapi/string-input-visitor.h |   3 +-
 qapi/string-input-visitor.c         | 436 +++++++++++++++++-----------
 tests/test-string-input-visitor.c   |  18 +-
 3 files changed, 264 insertions(+), 193 deletions(-)

include/qapi/string-input-visitor.h +0 -0

index 33551340e3..2c40f7f5a6 100644
@@ -19,8 +19,7 @@ typedef struct StringInputVisitor StringInputVisitor;
 

include/qapi/string-input-visitor.h typedef struct StringInputVisitor StringInputVisitor; · line 22

Quoted 5 lines

include/qapi/string-input-visitor.h +1 -2

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

Preexisting: neglects to spell out the list limitations, i.e. can do only flat lists of integers. Care do fix that, too?

Quoted 44 lines

include/qapi/string-input-visitor.h +0 -0

  */
 Visitor *string_input_visitor_new(const char *str);
 

qapi/string-input-visitor.c +17 -3

index dee708d384..16da47409e 100644
@@ -4,10 +4,10 @@
  * Copyright Red Hat, Inc. 2012-2016
  *
  * Author: Paolo Bonzini <[email protected]>
+ *         David Hildenbrand <[email protected]>
  *
  * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
  * See the COPYING.LIB file in the top-level directory.
- *
  */
 
 #include "qemu/osdep.h"

@@ -18,21 +18,39 @@
 #include "qapi/qmp/qerror.h"
 #include "qapi/qmp/qnull.h"
 #include "qemu/option.h"
-#include "qemu/queue.h"
-#include "qemu/range.h"
 #include "qemu/cutils.h"
 
+typedef enum ListMode {
+    /* no list parsing active / no list expected */
+    LM_NONE,
+    /* we have an unparsed string remaining */
+    LM_UNPARSED,
+    /* we have an unfinished int64 range */
+    LM_INT64_RANGE,
+    /* we have an unfinished uint64 range */
+    LM_UINT64_RANGE,
+    /* we have parsed the string completely and no range is remaining */
+    LM_END,
+} ListMode;
+
+typedef union RangeLimit {
+    int64_t i64;
+    uint64_t u64;

qapi/string-input-visitor.c line 45

Quoted 10 lines

qapi/string-input-visitor.c +2 -3

+} RangeLimit;
 
 struct StringInputVisitor
 {
     Visitor visitor;
 
-    GList *ranges;
-    GList *cur_range;
-    int64_t cur;
+    /* Porperties related to list processing */

"Properties"

Suggest

       /* List parsing state */

qapi/string-input-visitor.c line 48

Quoted 4 lines

qapi/string-input-visitor.c +3 -0

+    ListMode lm;
+    RangeLimit rangeNext;
+    RangeLimit rangeEnd;

RangeLimit is a good name for rangeEnd, but not so hot for rangeNext. Naming is hard.

qapi/string-input-visitor.c line 50

Quoted 3 lines

qapi/string-input-visitor.c +2 -0

+    const char *unparsed_string;
+    void *list;

You drop /* Only needed for sanity checking the caller */.
Double-checking: is that not true anymore?

qapi/string-input-visitor.c static StringInputVisitor *to_siv(Visitor *v) · line 68

Quoted 1 line

qapi/string-input-visitor.c +1 -0

+        error_setg(errp, "Already processing a list.");

Drop the period, please. error_setg()'s contract stipulates:

* The resulting message should be a single phrase, with no newline or
* trailing punctuation.

More of the same below.

Quoted 6 lines

qapi/string-input-visitor.c +0 -3

 static GenericList *next_list(Visitor *v, GenericList *tail, size_t size)
 {
     StringInputVisitor *siv = to_siv(v);
-    Range *r;
-
-    if (!siv->ranges || !siv->cur_range) {

qapi/string-input-visitor.c static StringInputVisitor *to_siv(Visitor *v) · line 95

Quoted 10 lines

qapi/string-input-visitor.c +4 -4

-        return NULL;
-    }
 
-    r = siv->cur_range->data;
-    if (!r) {
+    switch (siv->lm) {
+    case LM_NONE:
+    case LM_END:
+        /* we have reached the end of the list already or have no list */
         return NULL;

Hmm. Is there a way to reach case LM_NONE other than a programming error? If no, we should abort then. To do so, fold LM_NONE into the default case below.

Quoted 10 lines

qapi/string-input-visitor.c +0 -9

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

qapi/string-input-visitor.c static StringInputVisitor *to_siv(Visitor *v) · line 102

Quoted 10 lines

qapi/string-input-visitor.c +7 -3

-            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:
+        g_assert_not_reached();

abort() suffices, and is more consistent with the rest of qapi/.

Quoted 11 lines

     }
 
     tail->next = g_malloc0(size);

qapi/string-input-visitor.c +0 -2

@@ -179,88 +109,216 @@ 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;
 

qapi/string-input-visitor.c static GenericList *next_list(Visitor *v, GenericList *tail, size_t size) · line 115

Quoted 4 lines

qapi/string-input-visitor.c +3 -1

-    if (!siv->ranges || !siv->cur_range) {
+    switch (siv->lm) {
+    case LM_NONE:
+        error_setg(errp, "Not processing a list.");

Same question as for next_list().

qapi/string-input-visitor.c static GenericList *next_list(Visitor *v, GenericList *tail, size_t size) · line 119

Quoted 5 lines

qapi/string-input-visitor.c +4 -0

+    case LM_INT64_RANGE:
+    case LM_UINT64_RANGE:
+    case LM_UNPARSED:
+        error_setg(errp, "There are elements remaining in the list.");

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.

Quoted 26 lines

qapi/string-input-visitor.c +3 -17

         return;
-    }
-
-    r = siv->cur_range->data;
-    if (!r) {
+    case LM_END:
         return;
+    default:
+        g_assert_not_reached();
     }
-
-    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");
 }
 

qapi/string-input-visitor.c static GenericList *next_list(Visitor *v, GenericList *tail, size_t size) · line 212

Quoted 5 lines

qapi/string-input-visitor.c +0 -1

 static void end_list(Visitor *v, void **obj)
 {
     StringInputVisitor *siv = to_siv(v);
 
-    assert(siv->list == obj);

I suspect state LM_NONE is also a programming error here. If that's correct, let's assert(siv->lm != LM_NONE).

qapi/string-input-visitor.c static GenericList *next_list(Visitor *v, GenericList *tail, size_t size) · line 214

Quoted 8 lines

qapi/string-input-visitor.c +7 -0

+        if (siv->rangeNext.i64 > siv->rangeEnd.i64 || *obj == INT64_MAX) {
+            /* end of range, check if there is more to parse */
+            if (siv->unparsed_string[0]) {
+                siv->lm = LM_UNPARSED;
+            } else {
+                siv->lm = LM_END;
+            }
         }

Are you sure this is kosher?

if siv->rangeNext.i64 and siv->rangeEnd are both initially INT64_MAX, siv->rangeNext++ wraps around to INT64_MIN. Except when the compiler exploits undefined behavior.

qapi/string-input-visitor.c static GenericList *next_list(Visitor *v, GenericList *tail, size_t size) · line 220

Quoted 6 lines

qapi/string-input-visitor.c +6 -0

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

I figure I'd make try_parse_int64_list_entry() just parse, and on success fall through to case LM_INT64_RANGE. But your solution works, too.

qapi/string-input-visitor.c static GenericList *next_list(Visitor *v, GenericList *tail, size_t size) · line 223

Quoted 4 lines

qapi/string-input-visitor.c +3 -0

+        return;
+    case LM_END:
+        error_setg(errp, "No more elements in the list.");

This is the buddy of check_list() case LM_UNPARSED etc. There, input provides more list members than we expect. Here, input provides less. Again, the error is less clear than the one the QObject input visitor
reports: "Parameter '%s' is missing".

Same for the unsigned copy below.

qapi/string-input-visitor.c static GenericList *next_list(Visitor *v, GenericList *tail, size_t size) · line 226

Quoted 4 lines

qapi/string-input-visitor.c +3 -0

+        return;
+    default:
+        error_setg(errp, "Lists don't support mixed types.");

Is this a programming error?

Same for the unsigned copy below.

qapi/string-input-visitor.c static GenericList *next_list(Visitor *v, GenericList *tail, size_t size) · line 308

Quoted 9 lines

qapi/string-input-visitor.c +9 -0

+
+        if (siv->rangeNext.u64 > siv->rangeEnd.u64 || *obj == UINT64_MAX) {
+            /* end of range, check if there is more to parse */
+            if (siv->unparsed_string[0]) {
+                siv->lm = LM_UNPARSED;
+            } else {
+                siv->lm = LM_END;
+            }
+        }

Unsigned wraps around fine. But when you fix the signed code, you should probably keep this unsigned code as similar as practical.

Quoted 21 lines

qapi/string-input-visitor.c +13 -0

+        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;
+    case LM_END:
+        error_setg(errp, "No more elements in the list.");
+        return;
+    default:
+        error_setg(errp, "Lists don't support mixed types.");
+        return;
     }
 }
 

@@ -271,6 +329,11 @@ static void parse_type_size(Visitor *v, const char *name, uint64_t *obj,
     Error *err = NULL;
     uint64_t val;
 

qapi/string-input-visitor.c static void parse_type_size(Visitor *v, const char *name, uint64_t *obj, · line 336

Quoted 5 lines

qapi/string-input-visitor.c +5 -0

+    if (siv->lm != LM_NONE) {
+        error_setg(errp, "Lists not supported for type \"size\"");
+        return;
+    }
+

Programming error? More of the same below.

Quoted 4 lines

qapi/string-input-visitor.c +0 -0

     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)

qapi/string-input-visitor.c static void check_ulist(Visitor *v, uint64_t *expected, size_t n) · line 110

Quoted 6 lines

qapi/string-input-visitor.c +1 -2

 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,

Please wrap this line a bit earlier.

qapi/string-input-visitor.c static void check_ulist(Visitor *v, uint64_t *expected, size_t n) · line 113

Quoted 5 lines

qapi/string-input-visitor.c +2 -1

+                          6, 7, 8 };
     int64_t expect2[] = { 32767, -32768, -32767 };
-    int64_t expect3[] = { INT64_MAX, INT64_MIN };
+    int64_t expect3[] = { INT64_MIN, INT64_MAX };

Did you mean to change this line?

qapi/string-input-visitor.c static void test_visitor_in_intList(TestInputVisitorData *data, · line 182

Quoted 9 lines

qapi/string-input-visitor.c +1 -1

     uint64_t expect4[] = { UINT64_MAX };
     Error *err = NULL;
     int64List *res = NULL;

@@ -189,7 +179,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);

Which of the problems listed in commit message is this?

qapi/string-input-visitor.c static void test_visitor_in_intList(TestInputVisitorData *data, · line 184

Quoted 3 lines

qapi/string-input-visitor.c +0 -0

     visit_check_list(v, &error_abort);
     visit_end_list(v, (void **)&res);

Please don't let my review comments discourage you! This is so much better than the code we have now, and feels pretty close to ready. Thanks a lot!

Quoted 13 lines · Markus Armbruster

Quoted 10 lines · 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

Quoted 4 lines

Known weirdness: empty list is invalid (test-string-input-visitor.c demonstates). Your patch doesn't change that (or else it would update the test). Should it be changed?

I don't change the test, so the old behavior still works. (empty string -> error)

Quoted 12 lines

Quoted 12 lines

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.

Quoted 4 lines

Quoted 1 line

While at it, do some smaller style changes (e.g. use g_assert).

Please don't replace assert() by g_assert() in files that consistently use assert() now.

Alright, will use assert() and don't replace. (I thought g_assert() was the new fancy thing to do it in QEMU).

Quoted 16 lines

In my opinion, g_assert() is one of the cases where GLib pointlessly reinvents wheels that have been carrying load since forever.

Quoted 13 lines

Signed-off-by David Hildenbrand <[email protected]>

---
 include/qapi/string-input-visitor.h |   3 +-
 qapi/string-input-visitor.c         | 436 +++++++++++++++++-----------
 tests/test-string-input-visitor.c   |  18 +-
 3 files changed, 264 insertions(+), 193 deletions(-)

include/qapi/string-input-visitor.h +0 -0

index 33551340e3..2c40f7f5a6 100644
@@ -19,8 +19,7 @@ typedef struct StringInputVisitor StringInputVisitor;
 

Quoted 8 lines

include/qapi/string-input-visitor.h typedef struct StringInputVisitor StringInputVisitor; · line 22

Quoted 5 lines

include/qapi/string-input-visitor.h +1 -2

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

Preexisting: neglects to spell out the list limitations, i.e. can do only flat lists of integers. Care do fix that, too?

Added "Only flat lists of integers (int64/uint64) are supported."

Quoted 56 lines

Quoted 43 lines

include/qapi/string-input-visitor.h +0 -0

  */
 Visitor *string_input_visitor_new(const char *str);
 

qapi/string-input-visitor.c +17 -3

index dee708d384..16da47409e 100644
@@ -4,10 +4,10 @@
  * Copyright Red Hat, Inc. 2012-2016
  *
  * Author: Paolo Bonzini <[email protected]>
+ *         David Hildenbrand <[email protected]>
  *
  * This work is licensed under the terms of the GNU LGPL, version 2.1 or later.
  * See the COPYING.LIB file in the top-level directory.
- *
  */
 
 #include "qemu/osdep.h"

@@ -18,21 +18,39 @@
 #include "qapi/qmp/qerror.h"
 #include "qapi/qmp/qnull.h"
 #include "qemu/option.h"
-#include "qemu/queue.h"
-#include "qemu/range.h"
 #include "qemu/cutils.h"
 
+typedef enum ListMode {
+    /* no list parsing active / no list expected */
+    LM_NONE,
+    /* we have an unparsed string remaining */
+    LM_UNPARSED,
+    /* we have an unfinished int64 range */
+    LM_INT64_RANGE,
+    /* we have an unfinished uint64 range */
+    LM_UINT64_RANGE,
+    /* we have parsed the string completely and no range is remaining */
+    LM_END,
+} ListMode;
+
+typedef union RangeLimit {
+    int64_t i64;
+    uint64_t u64;

qapi/string-input-visitor.c line 45

Quoted 10 lines

qapi/string-input-visitor.c +2 -3

+} RangeLimit;
 
 struct StringInputVisitor
 {
     Visitor visitor;
 
-    GList *ranges;
-    GList *cur_range;
-    int64_t cur;
+    /* Porperties related to list processing */

"Properties"

Quoted 3 lines

Suggest

       /* List parsing state */

Yes, will use that.

Quoted 5 lines

Quoted 4 lines

qapi/string-input-visitor.c +3 -0

+    ListMode lm;
+    RangeLimit rangeNext;
+    RangeLimit rangeEnd;

Quoted 2 lines

RangeLimit is a good name for rangeEnd, but not so hot for rangeNext. Naming is hard.

Initially I had two unnamed unions to not have to give a name ;)

RangeElement ? Will use that for now.

Quoted 6 lines

qapi/string-input-visitor.c line 50

Quoted 3 lines

qapi/string-input-visitor.c +2 -0

+    const char *unparsed_string;
+    void *list;

You drop /* Only needed for sanity checking the caller */.
Double-checking: is that not true anymore?

I consider such comments bad as they can easily become outdated (and IMHO don't help anybody). But I can leave it here, don't really care.

[...]

Quoted 8 lines

Quoted 1 line

+ error_setg(errp, "Already processing a list.");

Drop the period, please. error_setg()'s contract stipulates:

* The resulting message should be a single phrase, with no newline or
* trailing punctuation.

More of the same below.

All dots dropped :)

[...]

Quoted 17 lines

Quoted 16 lines

static GenericList *next_list(Visitor *v, GenericList *tail, size_t size)

 {
     StringInputVisitor *siv = to_siv(v);

qapi/string-input-visitor.c +4 -7

-    Range *r;
-
-    if (!siv->ranges || !siv->cur_range) {
-        return NULL;
-    }
 
-    r = siv->cur_range->data;
-    if (!r) {
+    switch (siv->lm) {
+    case LM_NONE:
+    case LM_END:
+        /* we have reached the end of the list already or have no list */
         return NULL;

Quoted 3 lines

Hmm. Is there a way to reach case LM_NONE other than a programming error? If no, we should abort then. To do so, fold LM_NONE into the default case below.

Alright, all programming errors will be encoded as asserts.

Quoted 21 lines

Quoted 20 lines

qapi/string-input-visitor.c +7 -12

-    }
-
-    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:
+        g_assert_not_reached();

Quoted 1 line

abort() suffices, and is more consistent with the rest of qapi/.

Right, qapi is special ;)

Will use s/g_assert/assert/
s/g_assert_not_reached/abort/

Quoted 11 lines

Quoted 11 lines

     }
 
     tail->next = g_malloc0(size);

qapi/string-input-visitor.c +0 -2

@@ -179,88 +109,216 @@ 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;
 

Quoted 6 lines

qapi/string-input-visitor.c static GenericList *next_list(Visitor *v, GenericList *tail, size_t size) · line 115

Quoted 4 lines

qapi/string-input-visitor.c +3 -1

-    if (!siv->ranges || !siv->cur_range) {
+    switch (siv->lm) {
+    case LM_NONE:
+        error_setg(errp, "Not processing a list.");

Same question as for next_list().

abort() it is :) -> default case

Quoted 6 lines

qapi/string-input-visitor.c static GenericList *next_list(Visitor *v, GenericList *tail, size_t size) · line 119

Quoted 5 lines

qapi/string-input-visitor.c +4 -0

+    case LM_INT64_RANGE:
+    case LM_UINT64_RANGE:
+    case LM_UNPARSED:
+        error_setg(errp, "There are elements remaining in the list.");

Quoted 7 lines

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.

We don't keep track of element count (and I don't like adding it just to print a error message).

What about "Less list elements expected"? That at least matches the other error.

Quoted 26 lines

Quoted 26 lines

qapi/string-input-visitor.c +3 -17

         return;
-    }
-
-    r = siv->cur_range->data;
-    if (!r) {
+    case LM_END:
         return;
+    default:
+        g_assert_not_reached();
     }
-
-    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");
 }
 

Quoted 8 lines

qapi/string-input-visitor.c static GenericList *next_list(Visitor *v, GenericList *tail, size_t size) · line 212

Quoted 5 lines

qapi/string-input-visitor.c +0 -1

 static void end_list(Visitor *v, void **obj)
 {
     StringInputVisitor *siv = to_siv(v);
 
-    assert(siv->list == obj);

I suspect state LM_NONE is also a programming error here. If that's correct, let's assert(siv->lm != LM_NONE).

Indeed, added.

[...]

Quoted 11 lines

qapi/string-input-visitor.c static GenericList *next_list(Visitor *v, GenericList *tail, size_t size) · line 214

Quoted 8 lines

qapi/string-input-visitor.c +7 -0

+        if (siv->rangeNext.i64 > siv->rangeEnd.i64 || *obj == INT64_MAX) {
+            /* end of range, check if there is more to parse */
+            if (siv->unparsed_string[0]) {
+                siv->lm = LM_UNPARSED;
+            } else {
+                siv->lm = LM_END;
+            }
         }

Are you sure this is kosher?

Quoted 3 lines

if siv->rangeNext.i64 and siv->rangeEnd are both initially INT64_MAX, siv->rangeNext++ wraps around to INT64_MIN. Except when the compiler exploits undefined behavior.

*obj == INT64_MAX checks exactly that

(it is the old value of siv->rangeNext.i64 before a possible wraparound).

BTW I added a test case for exactly that as I had the BUG you described in the code ;)

[...]

Quoted 7 lines

Quoted 6 lines

qapi/string-input-visitor.c +6 -0

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

Quoted 3 lines

I figure I'd make try_parse_int64_list_entry() just parse, and on success fall through to case LM_INT64_RANGE. But your solution works, too.

Then we would have to represent even single values as ranges, which is something I'd like to avoid.

Quoted 4 lines

Quoted 3 lines

qapi/string-input-visitor.c +3 -0

+        return;
+    case LM_END:
+        error_setg(errp, "No more elements in the list.");

Quoted 6 lines

This is the buddy of check_list() case LM_UNPARSED etc. There, input provides more list members than we expect. Here, input provides less. Again, the error is less clear than the one the QObject input visitor
reports: "Parameter '%s' is missing".

Same for the unsigned copy below.

Will also use "Less list elements expected".

"Parameter '%s' is missing" does not really apply to list elements without a name. We can discuss when I resend.

Quoted 8 lines

qapi/string-input-visitor.c static GenericList *next_list(Visitor *v, GenericList *tail, size_t size) · line 226

Quoted 4 lines

qapi/string-input-visitor.c +3 -0

+        return;
+    default:
+        error_setg(errp, "Lists don't support mixed types.");

Is this a programming error?

Same for the unsigned copy below.

abort() it is.

[...]

Quoted 10 lines

qapi/string-input-visitor.c static GenericList *next_list(Visitor *v, GenericList *tail, size_t size) · line 308

Quoted 9 lines

qapi/string-input-visitor.c +9 -0

+
+        if (siv->rangeNext.u64 > siv->rangeEnd.u64 || *obj == UINT64_MAX) {
+            /* end of range, check if there is more to parse */
+            if (siv->unparsed_string[0]) {
+                siv->lm = LM_UNPARSED;
+            } else {
+                siv->lm = LM_END;
+            }
+        }

Quoted 2 lines

Unsigned wraps around fine. But when you fix the signed code, you should probably keep this unsigned code as similar as practical.

Again, *obj == UINT64_MAX checks this.

Quoted 21 lines

Quoted 21 lines

qapi/string-input-visitor.c +13 -0

+        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;
+    case LM_END:
+        error_setg(errp, "No more elements in the list.");
+        return;
+    default:
+        error_setg(errp, "Lists don't support mixed types.");
+        return;
     }
 }
 

@@ -271,6 +329,11 @@ static void parse_type_size(Visitor *v, const char *name, uint64_t *obj,
     Error *err = NULL;
     uint64_t val;
 

Quoted 7 lines

qapi/string-input-visitor.c static void parse_type_size(Visitor *v, const char *name, uint64_t *obj, · line 336

Quoted 5 lines

qapi/string-input-visitor.c +5 -0

+    if (siv->lm != LM_NONE) {
+        error_setg(errp, "Lists not supported for type \"size\"");
+        return;
+    }
+

Programming error? More of the same below.

assert() it is.

[...]

Quoted 4 lines

Quoted 4 lines

qapi/string-input-visitor.c +0 -0

     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)

Quoted 8 lines

qapi/string-input-visitor.c static void check_ulist(Visitor *v, uint64_t *expected, size_t n) · line 110

Quoted 6 lines

qapi/string-input-visitor.c +1 -2

 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,

Please wrap this line a bit earlier.

Will do.

Quoted 6 lines

Quoted 5 lines

qapi/string-input-visitor.c +2 -1

+                          6, 7, 8 };
     int64_t expect2[] = { 32767, -32768, -32767 };
-    int64_t expect3[] = { INT64_MAX, INT64_MIN };
+    int64_t expect3[] = { INT64_MIN, INT64_MAX };

Quoted 2 lines

Did you mean to change this line?

Yes, we don't sort the list anymore.

Quoted 11 lines

qapi/string-input-visitor.c static void test_visitor_in_intList(TestInputVisitorData *data, · line 182

Quoted 9 lines

qapi/string-input-visitor.c +1 -1

     uint64_t expect4[] = { UINT64_MAX };
     Error *err = NULL;
     int64List *res = NULL;

@@ -189,7 +179,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);

Which of the problems listed in commit message is this?

I guess the not mentioned "visiting beyond the end of a list is not handled properly" bug (most probably due to old range handling). Will add it to the description,

Quoted 8 lines

qapi/string-input-visitor.c static void test_visitor_in_intList(TestInputVisitorData *data, · line 184

Quoted 3 lines

qapi/string-input-visitor.c +0 -0

     visit_check_list(v, &error_abort);
     visit_end_list(v, (void **)&res);

Please don't let my review comments discourage you! This is so much better than the code we have now, and feels pretty close to ready. Thanks a lot!

I appreciate people reviewing my code :) So don't worry, I'll do requested changes and resend, then we'll see what we are still missing.

BTW: I'll add support for single-element ranges (1-1) as the previous code seemed to accepted that.

Quoted 19 lines · David Hildenbrand

Quoted 17 lines · Markus Armbruster

Quoted 10 lines · 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

Known weirdness: empty list is invalid (test-string-input-visitor.c demonstates). Your patch doesn't change that (or else it would update the test). Should it be changed?

Quoted 2 lines

I don't change the test, so the old behavior still works. (empty string -> error)

Understand. Design question: should it remain an error? Feel free to declare the question out of scope for this patch.

Quoted 1 line

Added "Only flat lists of integers (int64/uint64) are supported."

Hmm, do lists of narrower integer types also work? I guess they do: the narrower visit_type_*int*() call v->type_*int64() via visit_type_*intN().

Lists of type size are expressly excluded, in parse_type_size() below. That's okay, we can lift the restriction when it gets in the way.

Quoted 3 lines

What about "Less list elements expected"? That at least matches the other error.

Good enough. I'd say "fewer", though.

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

qapi/string-input-visitor.c +6 -0

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

Quoted 6 lines

Quoted 3 lines

I figure I'd make try_parse_int64_list_entry() just parse, and on success fall through to case LM_INT64_RANGE. But your solution works, too.

Then we would have to represent even single values as ranges, which is something I'd like to avoid.

Your artistic license applies.

Quoted 21 lines · Markus Armbruster

Quoted 19 lines · David Hildenbrand

Quoted 17 lines · Markus Armbruster

Quoted 10 lines · 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

Known weirdness: empty list is invalid (test-string-input-visitor.c demonstates). Your patch doesn't change that (or else it would update the test). Should it be changed?

Quoted 5 lines

Quoted 2 lines

I don't change the test, so the old behavior still works. (empty string -> error)

Understand. Design question: should it remain an error? Feel free to declare the question out of scope for this patch.

I think I was confused, let me retry to explain.

Empty lists actually don't result in an error. Calling start_list() on an empty string works just fine.

However
- check_list() will result in "Fewer list elements expected"
- visit_type_.*int64() will result in "Fewer list elements expected"
- next_list() will result in NULL

I guess that is the intended behavior. E.g. the test does

v = visitor_input_test_init(data, ""); visit_type_uint64List(v, NULL, &res, &error_abort); g_assert(!res);

So there won't be any error as the first "visit_next_list()" will properly indicate "NULL".

Quoted 6 lines

Quoted 1 line

Added "Only flat lists of integers (int64/uint64) are supported."

Hmm, do lists of narrower integer types also work? I guess they do: the narrower visit_type_*int*() call v->type_*int64() via visit_type_*intN().

Quoted 2 lines

Lists of type size are expressly excluded, in parse_type_size() below. That's okay, we can lift the restriction when it gets in the way.

Right, we can make that clearer

"Only flat lists of integers (except type "size") are supported." ? [...]

Basically copy all int64 list tests but adapt them to work on uint64 instead.

Signed-off-by David Hildenbrand <[email protected]>

---
 tests/test-string-input-visitor.c | 71 +++++++++++++++++++++++++++++--
 1 file changed, 67 insertions(+), 4 deletions(-)

tests/test-string-input-visitor.c +0 -1

index 2f6360e9ca..731094f789 100644
@@ -111,7 +111,6 @@ static void test_visitor_in_intList(TestInputVisitorData *data,
                           6, 7, 8 };
     int64_t expect2[] = { 32767, -32768, -32767 };
     int64_t expect3[] = { INT64_MIN, INT64_MAX };
-    uint64_t expect4[] = { UINT64_MAX };
     Error *err = NULL;
     int64List *res = NULL;
     Visitor *v;

tests/test-string-input-visitor.c +8 -0

+                           6, 7, 8 };
+    uint64_t expect2[] = { 32767, -32768, -32767 };
+    uint64_t expect3[] = { UINT64_MAX };
+    Error *err = NULL;
+    uint64List *res = NULL;
+    Visitor *v;
+    uint64_t val;
+

tests/test-string-input-visitor.c +45 -0

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

@@ -334,6 +395,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",

Quoted 24 lines · David Hildenbrand

Basically copy all int64 list tests but adapt them to work on uint64 instead.

Signed-off-by David Hildenbrand <[email protected]>

---
 tests/test-string-input-visitor.c | 71 +++++++++++++++++++++++++++++--
 1 file changed, 67 insertions(+), 4 deletions(-)

tests/test-string-input-visitor.c +0 -1

index 2f6360e9ca..731094f789 100644
@@ -111,7 +111,6 @@ static void test_visitor_in_intList(TestInputVisitorData *data,
                           6, 7, 8 };
     int64_t expect2[] = { 32767, -32768, -32767 };
     int64_t expect3[] = { INT64_MIN, INT64_MAX };
-    uint64_t expect4[] = { UINT64_MAX };
     Error *err = NULL;
     int64List *res = NULL;
     Visitor *v;

@@ -129,9 +128,6 @@ static void test_visitor_in_intList(TestInputVisitorData *data,
                                 "-9223372036854775808,9223372036854775807");
     check_ilist(v, expect3, ARRAY_SIZE(expect3));
 

tests/test-string-input-visitor.c static void test_visitor_in_intList(TestInputVisitorData *data, · line 134

Quoted 3 lines

tests/test-string-input-visitor.c +0 -3

-    v = visitor_input_test_init(data, "18446744073709551615");
-    check_ulist(v, expect4, ARRAY_SIZE(expect4));
-

Hmm. Testing behavior for this input is still worthwhile, isn't it?

The use of check_ulist() here is admittedly unclean.

Quoted 8 lines

tests/test-string-input-visitor.c +0 -0

     /* Empty list */
 
     v = visitor_input_test_init(data, "");

@@ -174,6 +170,71 @@ static void test_visitor_in_intList(TestInputVisitorData *data,
     visit_end_list(v, NULL);
 }
 

tests/test-string-input-visitor.c static void test_visitor_in_intList(TestInputVisitorData *data, · line 176

Quoted 4 lines

tests/test-string-input-visitor.c +4 -0

+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,

Please wrap this line a bit earlier.

Quoted 13 lines

tests/test-string-input-visitor.c +12 -0

+                           6, 7, 8 };
+    uint64_t expect2[] = { 32767, -32768, -32767 };
+    uint64_t expect3[] = { 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));

tests/test-string-input-visitor.c static void test_visitor_in_intList(TestInputVisitorData *data, · line 194

Quoted 6 lines

tests/test-string-input-visitor.c +6 -0

+
+    v = visitor_input_test_init(data, "32767,-32768--32767");
+    check_ulist(v, expect2, ARRAY_SIZE(expect2));
+
+    v = visitor_input_test_init(data, "18446744073709551615");
+    check_ulist(v, expect3, ARRAY_SIZE(expect3));

Test behavior for large negative numbers?

Quoted 32 lines · Markus Armbruster

Quoted 21 lines · David Hildenbrand

Basically copy all int64 list tests but adapt them to work on uint64 instead.

Signed-off-by David Hildenbrand <[email protected]>

---
 tests/test-string-input-visitor.c | 71 +++++++++++++++++++++++++++++--
 1 file changed, 67 insertions(+), 4 deletions(-)

tests/test-string-input-visitor.c +0 -1

index 2f6360e9ca..731094f789 100644
@@ -111,7 +111,6 @@ static void test_visitor_in_intList(TestInputVisitorData *data,
                           6, 7, 8 };
     int64_t expect2[] = { 32767, -32768, -32767 };
     int64_t expect3[] = { INT64_MIN, INT64_MAX };
-    uint64_t expect4[] = { UINT64_MAX };
     Error *err = NULL;
     int64List *res = NULL;
     Visitor *v;

tests/test-string-input-visitor.c static void test_visitor_in_intList(TestInputVisitorData *data, · line 134

Quoted 6 lines

tests/test-string-input-visitor.c +0 -3

@@ -129,9 +128,6 @@ static void test_visitor_in_intList(TestInputVisitorData *data,
                                 "-9223372036854775808,9223372036854775807");
     check_ilist(v, expect3, ARRAY_SIZE(expect3));
 
-    v = visitor_input_test_init(data, "18446744073709551615");
-    check_ulist(v, expect4, ARRAY_SIZE(expect4));
-

Hmm. Testing behavior for this input is still worthwhile, isn't it?

Quoted 1 line

The use of check_ulist() here is admittedly unclean.

This check has been moved to the other function where we test ulists.

Or do you want this check here to test again ilist and see that an error gets reported as the value is too big? Will add such range checks.

Quoted 8 lines

Quoted 8 lines

tests/test-string-input-visitor.c +0 -0

     /* Empty list */
 
     v = visitor_input_test_init(data, "");

@@ -174,6 +170,71 @@ static void test_visitor_in_intList(TestInputVisitorData *data,
     visit_end_list(v, NULL);
 }
 

Quoted 6 lines

tests/test-string-input-visitor.c static void test_visitor_in_intList(TestInputVisitorData *data, · line 176

Quoted 4 lines

tests/test-string-input-visitor.c +4 -0

+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,

Please wrap this line a bit earlier.

Yes.

Quoted 13 lines

Quoted 13 lines

tests/test-string-input-visitor.c +12 -0

+                           6, 7, 8 };
+    uint64_t expect2[] = { 32767, -32768, -32767 };
+    uint64_t expect3[] = { 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));

Quoted 8 lines

tests/test-string-input-visitor.c static void test_visitor_in_intList(TestInputVisitorData *data, · line 194

Quoted 6 lines

tests/test-string-input-visitor.c +6 -0

+
+    v = visitor_input_test_init(data, "32767,-32768--32767");
+    check_ulist(v, expect2, ARRAY_SIZE(expect2));
+
+    v = visitor_input_test_init(data, "18446744073709551615");
+    check_ulist(v, expect3, ARRAY_SIZE(expect3));

Test behavior for large negative numbers?

Yes, will add.