qapi: rewrite string-input-visitor

Let's provide a wrapper for strtod().

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

---
  include/qemu/cutils.h |  2 ++
  util/cutils.c         | 38 ++++++++++++++++++++++++++++++++++++++
  2 files changed, 40 insertions(+)

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

We will now also reject "inf" properly.

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

---
  include/qemu/cutils.h |  6 +++---
  monitor.c             |  2 +-
  tests/test-cutils.c   | 14 +++++++-------
  util/cutils.c         | 14 ++++++--------
  4 files changed, 17 insertions(+), 19 deletions(-)

+++ b/tests/test-cutils.c

tests/test-cutils.c +1 -1

@@ -1950,7 +1950,7 @@ static void test_qemu_strtou64_full_max(void)
  static void test_qemu_strtosz_simple(void)
  {
      const char *str;
-    char *endptr = NULL;
+    const char *endptr = NULL;

index 7868a683e8..dd61fb4558 100644

util/cutils.c +1 -2

@@ -206,19 +206,17 @@ static int64_t suffix_mul(char suffix, int64_t unit)
   * in *end, if not NULL. Return -ERANGE on overflow, Return -EINVAL on
   * other error.
   */
-static int do_strtosz(const char *nptr, char **end,
+static int do_strtosz(const char *nptr, const char **end,
                        const char default_suffix, int64_t unit,
                        uint64_t *result)
  {
      int retval;
-    char *endptr;

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

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

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

Quoted 6 lines · David Hildenbrand

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

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

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

Reviewed-by Eric Blake <[email protected]>

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

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

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

---
  qapi/qobject-input-visitor.c | 9 +++++----
  1 file changed, 5 insertions(+), 4 deletions(-)

Quoted 8 lines · David Hildenbrand

Let's provide a wrapper for strtod().

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

---
  include/qemu/cutils.h |  2 ++
  util/cutils.c         | 38 ++++++++++++++++++++++++++++++++++++++
  2 files changed, 40 insertions(+)

util/cutils.c int qemu_strtou64(const char *nptr, const char **endptr, int base, · line 573

Quoted 6 lines

util/cutils.c +6 -0

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

s/rejcted/rejected/

util/cutils.c int qemu_strtou64(const char *nptr, const char **endptr, int base, · line 577

Quoted 4 lines

+ */

util/cutils.c +3 -0

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

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

util/cutils.c int qemu_strtou64(const char *nptr, const char **endptr, int base, · line 581

Quoted 4 lines

util/cutils.c +4 -0

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

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

With the typo fix,

Reviewed-by Eric Blake <[email protected]>

Quoted 2 lines · David Hildenbrand

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

Quoted 3 lines

We will now also reject "inf" properly.

Quoted 16 lines

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

---
  include/qemu/cutils.h |  6 +++---
  monitor.c             |  2 +-
  tests/test-cutils.c   | 14 +++++++-------
  util/cutils.c         | 14 ++++++--------
  4 files changed, 17 insertions(+), 19 deletions(-)

+++ b/tests/test-cutils.c

tests/test-cutils.c +1 -1

@@ -1950,7 +1950,7 @@ static void test_qemu_strtou64_full_max(void)
  static void test_qemu_strtosz_simple(void)
  {
      const char *str;
-    char *endptr = NULL;
+    const char *endptr = NULL;

...

util/cutils.c

Quoted 1 line

util/cutils.c +0 -0

Conversion of call sites is good (in fact, you could drop the ' = NULL' initialization, since do_strtosz() guarantees it is set to something sane even on error).

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

Quoted 14 lines

index 7868a683e8..dd61fb4558 100644

util/cutils.c +1 -1

@@ -206,19 +206,17 @@ static int64_t suffix_mul(char suffix, int64_t unit)
   * in *end, if not NULL. Return -ERANGE on overflow, Return -EINVAL on
   * other error.
   */
-static int do_strtosz(const char *nptr, char **end,
+static int do_strtosz(const char *nptr, const char **end,
                        const char default_suffix, int64_t unit,
                        uint64_t *result)
  {
      int retval;

util/cutils.c static int64_t suffix_mul(char suffix, int64_t unit) · line 220

Quoted 10 lines

util/cutils.c +2 -3

-    char *endptr;
+    const char *endptr;
      unsigned char c;
      int mul_required = 0;
      double val, mul, integral, fraction;
  -    errno = 0;
-    val = strtod(nptr, &endptr);
-    if (isnan(val) || endptr == nptr || errno != 0) {
+    if (qemu_strtod_finite(nptr, &endptr, &val)) {
          retval = -EINVAL;

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

retval = qemu_strtod_finite(...);

if (retval) {
    goto out;

util/cutils.c out:

Quoted 3 lines

util/cutils.c +0 -0

          goto out;
      }

@@ -259,17 +257,17 @@ out:

More context:

out:

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

util/cutils.c out: · line 258

Quoted 2 lines

util/cutils.c +0 -0

      return retval;
  }

Everything else looks okay.

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

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

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 +2 -4

index b3fdd0827d..b89c6c4e06 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;
 
-    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;

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

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

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

---
  tests/test-string-input-visitor.c | 22 ++++++++++++++++++++--
  1 file changed, 20 insertions(+), 2 deletions(-)

Quoted 11 lines · Eric Blake

Quoted 8 lines · David Hildenbrand

Let's provide a wrapper for strtod().

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

---
  include/qemu/cutils.h |  2 ++
  util/cutils.c         | 38 ++++++++++++++++++++++++++++++++++++++
  2 files changed, 40 insertions(+)

Quoted 8 lines

util/cutils.c int qemu_strtou64(const char *nptr, const char **endptr, int base, · line 573

Quoted 6 lines

util/cutils.c +6 -0

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

s/rejcted/rejected/

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

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

[...] either

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

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

where b^emin is the smallest normal number.

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

Quoted 11 lines

util/cutils.c int qemu_strtou64(const char *nptr, const char **endptr, int base, · line 577

Quoted 5 lines

+ */

util/cutils.c +3 -0

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

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

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

Quoted 4 lines

util/cutils.c int qemu_strtou64(const char *nptr, const char **endptr, int base, · line 581

Quoted 4 lines

util/cutils.c +4 -0

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

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

Quoted 6 lines

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

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

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

Quoted 4 lines

With the typo fix,

Reviewed-by Eric Blake <[email protected]>

Quoted 11 lines · David Hildenbrand

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

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

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

---
  tests/test-string-input-visitor.c | 22 ++++++++++++++++++++--
  1 file changed, 20 insertions(+), 2 deletions(-)

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

Quoted 3 lines · Eric Blake

Quoted 2 lines · David Hildenbrand

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

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

Quoted 3 lines

Quoted 3 lines

We will now also reject "inf" properly.

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

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

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

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

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

Quoted 18 lines

Quoted 17 lines

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

---
  include/qemu/cutils.h |  6 +++---
  monitor.c             |  2 +-
  tests/test-cutils.c   | 14 +++++++-------
  util/cutils.c         | 14 ++++++--------
  4 files changed, 17 insertions(+), 19 deletions(-)

+++ b/tests/test-cutils.c

tests/test-cutils.c +1 -1

@@ -1950,7 +1950,7 @@ static void test_qemu_strtou64_full_max(void)
  static void test_qemu_strtosz_simple(void)
  {
      const char *str;
-    char *endptr = NULL;
+    const char *endptr = NULL;

...

Quoted 5 lines

util/cutils.c

Quoted 1 line

util/cutils.c +0 -0

Conversion of call sites is good (in fact, you could drop the ' = NULL' initialization, since do_strtosz() guarantees it is set to something sane even on error).

Yes.

Quoted 2 lines

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

Let's stick it into test_qemu_strtosz_invalid().

Quoted 29 lines

Quoted 15 lines

index 7868a683e8..dd61fb4558 100644

util/cutils.c +1 -1

@@ -206,19 +206,17 @@ static int64_t suffix_mul(char suffix, int64_t unit)
   * in *end, if not NULL. Return -ERANGE on overflow, Return -EINVAL on
   * other error.
   */
-static int do_strtosz(const char *nptr, char **end,
+static int do_strtosz(const char *nptr, const char **end,
                        const char default_suffix, int64_t unit,
                        uint64_t *result)
  {
      int retval;

util/cutils.c static int64_t suffix_mul(char suffix, int64_t unit) · line 220

Quoted 10 lines

util/cutils.c +2 -3

-    char *endptr;
+    const char *endptr;
      unsigned char c;
      int mul_required = 0;
      double val, mul, integral, fraction;
  -    errno = 0;
-    val = strtod(nptr, &endptr);
-    if (isnan(val) || endptr == nptr || errno != 0) {
+    if (qemu_strtod_finite(nptr, &endptr, &val)) {
          retval = -EINVAL;

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

Quoted 3 lines

retval = qemu_strtod_finite(...);

if (retval) {
    goto out;

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

Quoted 17 lines

util/cutils.c out:

Quoted 4 lines

util/cutils.c +0 -0

          goto out;
      }

@@ -259,17 +257,17 @@ out:

More context:

out:

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

Quoted 2 lines

util/cutils.c +0 -0

      return retval;
  }

Quoted 4 lines

Everything else looks okay.

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

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

Quoted 7 lines

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

works like qemu_strtoul(). I'd prefer longhand for qemu_strtod(). It costs us a few lines, but it results in a clearer contract.

/**
* Convert string @nptr to a double.
*
* This is a wrapper around strtod() that is harder to misuse.
* Semantics of @nptr and @endptr match strtod() with differences
* noted below.
*
* @nptr may be null, and no conversion is performed then. *
* If no conversion is performed, store @nptr in *@endptr and return

* -EINVAL.
*
* If @endptr is null, and the string isn't fully converted, return
* -EINVAL. This is the case when the pointer that would be stored in
* a non-null @endptr points to a character other than '\0'. *
* If the conversion overflows @result, store +/-HUGE_VAL, depending on
* the sign, in @result and return -ERANGE.

Quoted 13 lines · Eric Blake

Quoted 11 lines · David Hildenbrand

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

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

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

---
  tests/test-string-input-visitor.c | 22 ++++++++++++++++++++--
  1 file changed, 20 insertions(+), 2 deletions(-)

Quoted 9 lines

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

Ranges have to be ascending and old code enforced that. New code still enforces it. Wrapping ranges are AFAIC also not supported - not ascending. I can add a test.

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

Quoted 3 lines · David Hildenbrand

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

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

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

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

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

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

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

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

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

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

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

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

index 33551340e3..921f3875b9 100644
@@ -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);
 

qapi/string-input-visitor.c +25 -87

index b89c6c4e06..4113be01fb 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 RangeElement {
+    int64_t i64;
+    uint64_t u64;
+} RangeElement;
 
 struct StringInputVisitor
 {
     Visitor visitor;
 
-    GList *ranges;
-    GList *cur_range;
-    int64_t cur;
+    /* List parsing state */
+    ListMode lm;
+    RangeElement rangeNext;
+    RangeElement rangeEnd;
+    const char *unparsed_string;
+    void *list;
 
+    /* The original string to parse */
     const char *string;
-    void *list; /* Only needed for sanity checking the caller */
 };
 
 static StringInputVisitor *to_siv(Visitor *v)

@@ -40,136 +58,43 @@ static StringInputVisitor *to_siv(Visitor *v)
     return container_of(v, StringInputVisitor, visitor);
 }
 
-static void free_range(void *range, void *dummy)
-{
-    g_free(range);
-}
-
-static int parse_str(StringInputVisitor *siv, const char *name, Error **errp)
-{
-    char *str = (char *) siv->string;
-    long long start, end;
-    Range *cur;
-    char *endptr;
-
-    if (siv->ranges) {
-        return 0;
-    }
-
-    if (!*str) {
-        return 0;
-    }
-
-    do {
-        errno = 0;
-        start = strtoll(str, &endptr, 0);
-        if (errno == 0 && endptr > str) {
-            if (*endptr == '\0') {
-                cur = g_malloc0(sizeof(*cur));
-                range_set_bounds(cur, start, start);
-                siv->ranges = range_list_insert(siv->ranges, cur);
-                cur = NULL;
-                str = NULL;
-            } else if (*endptr == '-') {
-                str = endptr + 1;
-                errno = 0;
-                end = strtoll(str, &endptr, 0);
-                if (errno == 0 && endptr > str && start <= end &&
-                    (start > INT64_MAX - 65536 ||
-                     end < start + 65536)) {
-                    if (*endptr == '\0') {
-                        cur = g_malloc0(sizeof(*cur));
-                        range_set_bounds(cur, start, end);
-                        siv->ranges = range_list_insert(siv->ranges, cur);
-                        cur = NULL;
-                        str = NULL;
-                    } else if (*endptr == ',') {
-                        str = endptr + 1;
-                        cur = g_malloc0(sizeof(*cur));
-                        range_set_bounds(cur, start, end);
-                        siv->ranges = range_list_insert(siv->ranges, cur);
-                        cur = NULL;
-                    } else {
-                        goto error;
-                    }
-                } else {
-                    goto error;
-                }
-            } else if (*endptr == ',') {
-                str = endptr + 1;
-                cur = g_malloc0(sizeof(*cur));
-                range_set_bounds(cur, start, start);
-                siv->ranges = range_list_insert(siv->ranges, cur);
-                cur = NULL;
-            } else {
-                goto error;
-            }
-        } else {
-            goto error;
-        }
-    } while (str);
-
-    return 0;
-error:
-    g_list_foreach(siv->ranges, free_range, NULL);
-    g_list_free(siv->ranges);
-    siv->ranges = NULL;
-    error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null",
-               "an int64 value or range");
-    return -1;
-}
-
-static void

qapi/string-input-visitor.c +88 -53

+    assert(siv->lm == LM_NONE);
     siv->list = list;
+    siv->unparsed_string = siv->string;
 
-    if (parse_str(siv, name, errp) < 0) {
-        *list = NULL;
-        return;
-    }
-
-    siv->cur_range = g_list_first(siv->ranges);
-    if (siv->cur_range) {
-        Range *r = siv->cur_range->data;
-        if (r) {
-            siv->cur = range_lob(r);
+    if (!siv->string[0]) {
+        if (list) {
+            *list = NULL;
         }
-        *list = g_malloc0(size);
+        siv->lm = LM_END;
     } else {
-        *list = NULL;
+        if (list) {
+            *list = g_malloc0(size);
+        }
+        siv->lm = LM_UNPARSED;
     }
 }
 
 static GenericList *next_list(Visitor *v, GenericList *tail, size_t size)
 {
     StringInputVisitor *siv = to_siv(v);
-    Range *r;
 
-    if (!siv->ranges || !siv->cur_range) {
+    switch (siv->lm) {
+    case LM_END:
         return NULL;
-    }
-
-    r = siv->cur_range->data;
-    if (!r) {
-        return NULL;
-    }
-
-    if (!range_contains(r, siv->cur)) {
-        siv->cur_range = g_list_next(siv->cur_range);
-        if (!siv->cur_range) {
-            return NULL;
-        }
-        r = siv->cur_range->data;
-        if (!r) {
-            return NULL;
-        }
-        siv->cur = range_lob(r);
+    case LM_INT64_RANGE:
+    case LM_UINT64_RANGE:
+    case LM_UNPARSED:
+        /* we have an unparsed string or something left in a range */
+        break;
+    default:
+        abort();
     }
 
     tail->next = g_malloc0(size);

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

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

+        }
+        return;
+    case LM_END:
+        error_setg(errp, "Fewer list elements expected");
+        return;
+    default:
+        abort();
     }
+}
 
-    if (!siv->ranges) {
-        goto error;
-    }
-
-    if (!siv->cur_range) {

qapi/string-input-visitor.c +83 -24

+    if (qemu_strtou64(siv->unparsed_string, &endptr, 0, &start)) {
+        return -EINVAL;
+    }
+    end = start;
+
+    switch (endptr[0]) {
+    case '\0':
+        siv->unparsed_string = endptr;
+        break;
+    case ',':
+        siv->unparsed_string = endptr + 1;
+        break;
+    case '-':
+        /* parse the end of the range */
+        if (qemu_strtou64(endptr + 1, &endptr, 0, &end)) {
+            return -EINVAL;
         }
-
-        r = siv->cur_range->data;
-        if (!r) {
-            goto error;
+        if (start > end) {
+            return -EINVAL;
         }
-
-        siv->cur = range_lob(r);
+        switch (endptr[0]) {
+        case '\0':
+            siv->unparsed_string = endptr;
+            break;
+        case ',':
+            siv->unparsed_string = endptr + 1;
+            break;
+        default:
+            return -EINVAL;
+        }
+        break;
+    default:
+        return -EINVAL;
     }
 
-    *obj = siv->cur;
-    siv->cur++;
-    return;
-
-error:
-    error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null",
-               "an int64 value or range");
+    /* we have a proper range (with maybe only one element) */
+    siv->lm = LM_UINT64_RANGE;
+    siv->rangeNext.u64 = start;
+    siv->rangeEnd.u64 = end;
+    return 0;
 }
 
 static void parse_type_uint64(Visitor *v, const char *name, uint64_t *obj,
                               Error **errp)
 {
-    /* FIXME: parse_type_int64 mishandles values over INT64_MAX */
-    int64_t i;
-    Error *err = NULL;
-    parse_type_int64(v, name, &i, &err);
-    if (err) {
-        error_propagate(errp, err);
-    } else {
-        *obj = i;
+    StringInputVisitor *siv = to_siv(v);
+    uint64_t val;
+
+    switch (siv->lm) {
+    case LM_NONE:
+        /* just parse a simple uint64, bail out if not completely consumed */
+        if (qemu_strtou64(siv->string, NULL, 0, &val)) {
+            error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null",
+                       "uint64");
+            return;
+        }
+        *obj = val;
+        return;
+    case LM_UNPARSED:
+        if (try_parse_uint64_list_entry(siv, obj)) {
+            error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null",
+                       "list of uint64 values or ranges");
+            return;
+        }
+        assert(siv->lm == LM_UINT64_RANGE);
+        /* FALLTHROUGH */
+    case LM_UINT64_RANGE:
+        /* return the next element in the range */
+        assert(siv->rangeNext.u64 <= siv->rangeEnd.u64);
+        *obj = siv->rangeNext.u64++;
+
+        if (siv->rangeNext.u64 > siv->rangeEnd.u64 || *obj == UINT64_MAX) {
+            /* end of range, check if there is more to parse */
+            if (siv->unparsed_string[0]) {
+                siv->lm = LM_UNPARSED;
+            } else {
+                siv->lm = LM_END;
+            }
+        }
+        return;
+    case LM_END:
+        error_setg(errp, "Fewer list elements expected");
+        return;
+    default:
+        abort();
     }
 }
 

@@ -271,6 +323,7 @@ static void parse_type_size(Visitor *v, const char *name, uint64_t *obj,
     Error *err = NULL;
     uint64_t val;
 
+    assert(siv->lm == LM_NONE);
     parse_option_size(name, siv->string, &val, &err);
     if (err) {
         error_propagate(errp, err);

@@ -285,6 +338,7 @@ static void parse_type_bool(Visitor *v, const char *name, bool *obj,
 {
     StringInputVisitor *siv = to_siv(v);
 
+    assert(siv->lm == LM_NONE);
     if (!strcasecmp(siv->string, "on") ||
         !strcasecmp(siv->string, "yes") ||
         !strcasecmp(siv->string, "true")) {

@@ -307,6 +361,7 @@ static void parse_type_str(Visitor *v, const char *name, char **obj,
 {
     StringInputVisitor *siv = to_siv(v);
 
+    assert(siv->lm == LM_NONE);
     *obj = g_strdup(siv->string);
 }
 

@@ -316,6 +371,7 @@ static void parse_type_number(Visitor *v, const char *name, double *obj,
     StringInputVisitor *siv = to_siv(v);
     double val;
 
+    assert(siv->lm == LM_NONE);
     if (qemu_strtod_finite(siv->string, NULL, &val)) {
         error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
                    "number");

@@ -330,9 +386,10 @@ static void parse_type_null(Visitor *v, const char *name, QNull **obj,
 {
     StringInputVisitor *siv = to_siv(v);
 
+    assert(siv->lm == LM_NONE);
     *obj = NULL;
 
-    if (!siv->string || siv->string[0]) {
+    if (siv->string[0]) {
         error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
                    "null");
         return;

@@ -345,8 +402,6 @@ static void string_input_free(Visitor *v)
 {
     StringInputVisitor *siv = to_siv(v);
 
-    g_list_foreach(siv->ranges, free_range, NULL);
-    g_list_free(siv->ranges);
     g_free(siv);
 }
 

@@ -372,5 +427,6 @@ Visitor *string_input_visitor_new(const char *str)
     v->visitor.free = string_input_free;
 
     v->string = str;
+    v->lm = LM_NONE;
     return &v->visitor;
 }

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

index f55e0696c0..a198aedfce 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++) {

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

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

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

We now support virtual walks, so use that instead.

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

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

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

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

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

Quoted 5 lines

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

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

Quoted 6 lines

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

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

... there's none.

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

Quoted 40 lines

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

index 33551340e3..921f3875b9 100644
@@ -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);
 

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

index b89c6c4e06..4113be01fb 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"
 

Quoted 16 lines

static void parse_type_int64(Visitor *v, const char *name, int64_t *obj,
Error **errp)

 {
     StringInputVisitor *siv = to_siv(v);

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

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

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

Quoted 10 lines

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

+        *obj = val;
         return;
+    case LM_UNPARSED:
+        if (try_parse_int64_list_entry(siv, obj)) {
+            error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name ? name : "null",
+                       "list of int64 values or ranges");
+            return;
+        }
+        assert(siv->lm == LM_INT64_RANGE);
+        /* FALLTHROUGH */

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

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

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

Quoted 13 lines

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

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

I'd do

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

Matter of taste, entirely up to you.

Quoted 22 lines

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

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

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

Quoted 4 lines

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

-        siv->cur_range = g_list_first(siv->ranges);
-        if (!siv->cur_range) {
-            goto error;
+    /* parse a simple uint64 or range */

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

Quoted 15 lines

Quoted 5 lines

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

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

Quoted 6 lines

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

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

Quoted 5 lines

... there's none.

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

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

1. backends/hostmem.c:host_memory_backend_set_host_nodes()

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

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

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

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

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

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

4. qapi-visit.c::visit_type_RockerOfDpaGroup_members

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

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

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

6. numa.c::query_memdev()

-> calls object_property_get_uint16List()
--> String parsed via visit_type_uint16List() into list

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

-> hmp_info_memdev() uses this list
--> List converted again to a string using string output visitor

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

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

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

Quoted 21 lines · David Hildenbrand

Quoted 15 lines

Quoted 5 lines

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

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

Quoted 6 lines

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

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

Quoted 5 lines

... there's none.

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

Quoted 6 lines

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

1. backends/hostmem.c:host_memory_backend_set_host_nodes()

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

Yes.

Quoted 2 lines

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

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

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

Igor, Eduardo, this is yours.

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

Quoted 4 lines

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

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

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

Quoted 4 lines

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

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

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

Quoted 4 lines

4. qapi-visit.c::visit_type_RockerOfDpaGroup_members

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

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

Quoted 4 lines

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

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

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

Quoted 4 lines

6. numa.c::query_memdev()

-> calls object_property_get_uint16List()
--> String parsed via visit_type_uint16List() into list

QOM, hard to understand.

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

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

Quoted 2 lines

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

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

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

Useless.

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

Quoted 4 lines

-> hmp_info_memdev() uses this list
--> List converted again to a string using string output visitor

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

HMP is not a stable interface.

Quoted 1 line

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

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

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