qapi: rewrite string-input-visitor
v2 cover qapi: rewrite string-input-visitor
Rewrite string-input-visitor to be (hopefully) less ugly. Support int and uint lists (including ranges, but not implemented via type "Range").
Virtual walks are now supported and more errors are cought (and some bugs fixed). Fix and extend the tests. Parsing of uint64_t is now properly supported.
Importantly, when parsing a list we now return the list and not an ordered set (we are not an ordered set parser after all). Whoever needs that can add it on top. As far as I can see, current code can deal with it but I'll have to look at the details.
While at it, introduce and use qemu_strtod() and qemu_strtod_finite(). However don't convert monitor.c as that seems to be too broken for me (it ignores both conversion errors and range errors, and there is an actual unit test in which conversion is expected to fail).
v1 -> v2:
- "cutils: Add qemu_strtod() and qemu_strtod_finite()" -- Add a proper documentation
-- Make -EINVAL perform "no conversion" in qemu_strtod_finite().
- "cutils: Fix qemu_strtosz() & friends to reject non-finite sizes" -- New subject/description
-- Added test case
-- Forward -ERANGE
- "qapi: Fix string-input-visitor to reject NaN and infinities" -- New subject/description
-- Added test case
- "test-string-input-visitor: Add more tests"
-- Add even more tests (descending range, overflow, range limit)
- "qapi: Rewrite string-input-visitor"
-- Accept ranges with one element (e.g. 1-1)
-- Bail out on big ranges (just like before)
-- Minor style fixed + description updates
- "test-string-input-visitor: split off uint64 list tests" -- ALso copy + adapt newly added tests for int64 lists
RFC -> v1:
- Too much to name it all, just some highlights
- Pull some tests before the rework
- Introduce qemu_strtod_finite() and use it in more places
- More tests
- "qapi: rewrite string-input-visitor"
-- programming error -> assert
-- g_assert() -> assert(), g_assert_not_reached() -> abort() -- Accept single-element ranges "1-1"
-- Split parsing and returning the next range element -- Minor style fixes
David Hildenbrand (9):
cutils: Add qemu_strtod() and qemu_strtod_finite()
cutils: Fix qemu_strtosz() & friends to reject non-finite sizes
qapi: Fix string-input-visitor to reject NaN and infinities
qapi: Use qemu_strtod_finite() in qobject-input-visitor
test-string-input-visitor: Add more tests
qapi: Rewrite string-input-visitor
test-string-input-visitor: Use virtual walk
test-string-input-visitor: Split off uint64 list tests
test-string-input-visitor: Add range overflow tests
include/qapi/string-input-visitor.h | 4 +- include/qemu/cutils.h | 8 +- monitor.c | 2 +- qapi/qobject-input-visitor.c | 9 +- qapi/string-input-visitor.c | 413 ++++++++++++++++------------ tests/test-cutils.c | 24 +- tests/test-string-input-visitor.c | 223 ++++++++++++--- util/cutils.c | 81 +++++- 8 files changed, 516 insertions(+), 248 deletions(-)
Signature · 1 line
2.17.2
PATCH v2 1/9 cutils: Add qemu_strtod() and qemu_strtod_finite()
Let's provide a wrapper for strtod().
--- include/qemu/cutils.h | 2 ++ util/cutils.c | 65 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 67 insertions(+)
include/qemu/cutils.h +2 -0
@@ -146,6 +146,8 @@ int qemu_strtoi64(const char *nptr, const char **endptr, int base, int64_t *result);int qemu_strtou64 (const char *nptr,const char **endptr,int base,uint64_t *result); +int qemu_strtod (const char *nptr,const char **endptr,double *result); +int qemu_strtod_finite (const char *nptr,const char **endptr,double *result);int parse_uint (const char *s,unsigned long long *value,char **endptr,int base);
util/cutils.c +65 -0
71 lines not on this page · raw message
Signature · 1 line
2.17.2
Re: [Qemu-devel] [PATCH v2 1/9] cutils: Add qemu_strtod() and qemu_strtod_finite()
Quoted 3 lines · David Hildenbrand
Let's provide a wrapper for
strtod().Reviewed-by Eric Blake <[email protected]>
This changed enough from v1 that I would have dropped R-b to ensure that reviewers notice the differences.
Quoted 7 lines
Signed-off-by David Hildenbrand <[email protected]>--- include/qemu/cutils.h | 2 ++ util/cutils.c | 65 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 67 insertions(+)
Quoted 5 lines
+ * If the conversion overflows, store +/-HUGE_VAL in @result, depending
+ * on the sign, and return-ERANGE.
+ *
+ * If the conversion underflows, store ±0.0 in @result, depending on the
+ * sign, and return-ERANGE.
The use of UTF-8 ± in one place but not both is odd. I think we're at the point where UTF-8 comments are acceptable these days, rather than trying to keep our codebase ASCII-clean, so I don't care which way you resolve the inconsistency.
Quoted 4 lines
util/cutils.c +4
-0+/** +* Convert string @nptr to a finite double. +* +* Works like qemu_strtod(), except that "NaN" and "inf" are rejected
util/cutils.c int qemu_strtou64(const char *nptr, const char **endptr, int base, · line 600
Quoted 10 lines
util/cutils.c +10
-0+* with -EINVAL and no conversion is performed. +*/ +int qemu_strtod_finite (const char *nptr,const char **endptr,double *result) +{ +double tmp; +int ret; + + ret =qemu_strtod (nptr, endptr, &tmp); +if (ret) { +return ret;
So, if we overflow, we are returning -ERANGE but with nothing stored into *result. This is different from qemu_strtod(), where a return of -ERANGE guarantees that *result is one of 4 values (+/- 0.0/inf). That seems awkward.
util/cutils.c int qemu_strtou64(const char *nptr, const char **endptr, int base, · line 605
Quoted 5 lines
util/cutils.c +5
-0+ }else if (!isfinite (tmp)) { +if (endptr) { + *endptr = nptr; + } +return -EINVAL ;
Rewinding back to the start of "inf" is interesting, but matches your documentation.
util/cutils.c int qemu_strtou64(const char *nptr, const char **endptr, int base, · line 611
Quoted 6 lines
util/cutils.c +6
-0+ } + + *result = tmp; +return ret; +} +
I think you still need to fix -ERANGE handling before I can give R-b.
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH v2 1/9] cutils: Add qemu_strtod() and qemu_strtod_finite()
Quoted 21 lines · Eric Blake
Quoted 3 lines · David Hildenbrand
Let's provide a wrapper for
strtod().Reviewed-by Eric Blake <[email protected]>This changed enough from v1 that I would have dropped R-b to ensure that reviewers notice the differences.
Quoted 7 lines
Signed-off-by David Hildenbrand <[email protected]>--- include/qemu/cutils.h | 2 ++ util/cutils.c | 65 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 67 insertions(+)Quoted 5 lines
+ * If the conversion overflows, store +/-HUGE_VAL in @result, depending
+ * on the sign, and return-ERANGE.
+ *
+ * If the conversion underflows, store ±0.0 in @result, depending on the
+ * sign, and return-ERANGE.
Quoted 4 lines
The use of UTF-8 ± in one place but not both is odd. I think we're at the point where UTF-8 comments are acceptable these days, rather than trying to keep our codebase ASCII-clean, so I don't care which way you resolve the inconsistency.
217 out of 6455 git-controlled files contain non-ASCII characters. 53 of them are binary, and don't count. In most text files, it's for spelling names of authors properly in comments. Ample precedence for UTF-8 in comments, I'd say.
That said, I second Eric's call for consistency, with the slightest of preferrences for plain ASCII.
I spotted UTF-8 in two error messages, which might still be unadvisable:
hw/misc/tmp105.c: error_setg(errp, "value %" PRId64 ".%03" PRIu64 " °C is out of range", hw/misc/tmp421.c: error_setg(errp, "value %" PRId64 ".%03" PRIu64 " °C is out of range",
Quoted 11 lines
Quoted 11 lines
util/cutils.c +11
-0+/** +* Convert string @nptr to a finite double. +* +* Works like qemu_strtod(), except that "NaN" and "inf" are rejected +* with -EINVAL and no conversion is performed. +*/ +int qemu_strtod_finite (const char *nptr,const char **endptr,double *result) +{ +double tmp; +int ret; +
Quoted 8 lines
util/cutils.c int qemu_strtou64(const char *nptr, const char **endptr, int base, · line 600
Quoted 3 lines
util/cutils.c +3
-0+ ret =qemu_strtod (nptr, endptr, &tmp); +if (ret) { +return ret;So, if we overflow, we are returning
-ERANGEbut with nothing stored into *result. This is different fromqemu_strtod(), where a return of-ERANGEguarantees that *result is one of 4 values (+/- 0.0/inf). That seems awkward.
Violates the contract's "like qemu_strtod()".
Quoted 8 lines
util/cutils.c int qemu_strtou64(const char *nptr, const char **endptr, int base, · line 605
Quoted 5 lines
util/cutils.c +5
-0+ }else if (!isfinite (tmp)) { +if (endptr) { + *endptr = nptr; + } +return -EINVAL ;Rewinding back to the start of "inf" is interesting, but matches your documentation.
Yes. I like it.
Quoted 8 lines
util/cutils.c int qemu_strtou64(const char *nptr, const char **endptr, int base, · line 611
Quoted 6 lines
util/cutils.c +6
-0+ } + + *result = tmp; +return ret; +} +I think you still need to fix
-ERANGEhandling before I can give R-b.
Re: [Qemu-devel] [PATCH v2 1/9] cutils: Add qemu_strtod() and qemu_strtod_finite()
Quoted 9 lines · Markus Armbruster
Quoted 7 lines · Eric Blake
Quoted 3 lines · David Hildenbrand
Let's provide a wrapper for
strtod().Reviewed-by Eric Blake <[email protected]>This changed enough from v1 that I would have dropped R-b to ensure that reviewers notice the differences.
Indeed, dropping it now ;)
Quoted 19 lines
Quoted 14 lines
Quoted 8 lines
Signed-off-by David Hildenbrand <[email protected]>--- include/qemu/cutils.h | 2 ++ util/cutils.c | 65 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 67 insertions(+)Quoted 5 lines
+ * If the conversion overflows, store +/-HUGE_VAL in @result, depending
+ * on the sign, and return-ERANGE.
+ *
+ * If the conversion underflows, store ±0.0 in @result, depending on the
+ * sign, and return-ERANGE.Quoted 4 lines
The use of UTF-8 ± in one place but not both is odd. I think we're at the point where UTF-8 comments are acceptable these days, rather than trying to keep our codebase ASCII-clean, so I don't care which way you resolve the inconsistency.
Quoted 7 lines
217 out of 6455 git-controlled files contain non-ASCII characters. 53 of them are binary, and don't count. In most text files, it's for spelling names of authors properly in comments. Ample precedence for UTF-8 in comments, I'd say.
That said, I second Eric's call for consistency, with the slightest of preferrences for plain ASCII.
I'll just go with +/-. Thanks.
Quoted 26 lines
I spotted UTF-8 in two error messages, which might still be unadvisable:
hw/misc/tmp105.c: error_setg(errp, "value %" PRId64 ".%03" PRIu64 " °C is out of range", hw/misc/tmp421.c: error_setg(errp, "value %" PRId64 ".%03" PRIu64 " °C is out of range",Quoted 11 lines
Quoted 11 lines
util/cutils.c +11
-0+/** +* Convert string @nptr to a finite double. +* +* Works like qemu_strtod(), except that "NaN" and "inf" are rejected +* with -EINVAL and no conversion is performed. +*/ +int qemu_strtod_finite (const char *nptr,const char **endptr,double *result) +{ +double tmp; +int ret; +Quoted 8 lines
util/cutils.c int qemu_strtou64(const char *nptr, const char **endptr, int base, · line 600
Quoted 3 lines
util/cutils.c +3
-0+ ret =qemu_strtod (nptr, endptr, &tmp); +if (ret) { +return ret;So, if we overflow, we are returning
-ERANGEbut with nothing stored into *result. This is different fromqemu_strtod(), where a return of-ERANGEguarantees that *result is one of 4 values (+/- 0.0/inf). That seems awkward.
Quoted 1 line
Violates the contract's "like
qemu_strtod()".
Right, I missed that. What about something like this:
int qemu_strtod_finite(const char *nptr, const char **endptr, double *result)
{
double tmp;
int ret;
ret = qemu_strtod(nptr, endptr, &tmp);
if (!ret && !isfinite(tmp)) {
if (endptr) {
*endptr = nptr;
}
ret = -EINVAL;
}
if (ret != -EINVAL) {
*result = tmp;
}
return ret;
}Signature · 3 lines
Thanks, David / dhildenb
Markus Armbruster
Reviewed-byRe: [Qemu-devel] [PATCH v2 1/9] cutils: Add qemu_strtod() and qemu_strtod_finite()
Quoted 83 lines · David Hildenbrand
Quoted 9 lines · Markus Armbruster
Quoted 7 lines · Eric Blake
Quoted 3 lines · David Hildenbrand
Let's provide a wrapper for
strtod().Reviewed-by Eric Blake <[email protected]>This changed enough from v1 that I would have dropped R-b to ensure that reviewers notice the differences.
Indeed, dropping it now ;)
Quoted 19 lines
Quoted 14 lines
Quoted 8 lines
Signed-off-by David Hildenbrand <[email protected]>--- include/qemu/cutils.h | 2 ++ util/cutils.c | 65 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 67 insertions(+)Quoted 5 lines
+ * If the conversion overflows, store +/-HUGE_VAL in @result, depending
+ * on the sign, and return-ERANGE.
+ *
+ * If the conversion underflows, store ±0.0 in @result, depending on the
+ * sign, and return-ERANGE.Quoted 4 lines
The use of UTF-8 ± in one place but not both is odd. I think we're at the point where UTF-8 comments are acceptable these days, rather than trying to keep our codebase ASCII-clean, so I don't care which way you resolve the inconsistency.
Quoted 7 lines
217 out of 6455 git-controlled files contain non-ASCII characters. 53 of them are binary, and don't count. In most text files, it's for spelling names of authors properly in comments. Ample precedence for UTF-8 in comments, I'd say.
That said, I second Eric's call for consistency, with the slightest of preferrences for plain ASCII.
I'll just go with +/-. Thanks.
Quoted 26 lines
I spotted UTF-8 in two error messages, which might still be unadvisable:
hw/misc/tmp105.c: error_setg(errp, "value %" PRId64 ".%03" PRIu64 " °C is out of range", hw/misc/tmp421.c: error_setg(errp, "value %" PRId64 ".%03" PRIu64 " °C is out of range",Quoted 11 lines
Quoted 11 lines
util/cutils.c +11
-0+/** +* Convert string @nptr to a finite double. +* +* Works like qemu_strtod(), except that "NaN" and "inf" are rejected +* with -EINVAL and no conversion is performed. +*/ +int qemu_strtod_finite (const char *nptr,const char **endptr,double *result) +{ +double tmp; +int ret; +Quoted 8 lines
util/cutils.c int qemu_strtou64(const char *nptr, const char **endptr, int base, · line 600
Quoted 3 lines
util/cutils.c +3
-0+ ret =qemu_strtod (nptr, endptr, &tmp); +if (ret) { +return ret;So, if we overflow, we are returning
-ERANGEbut with nothing stored into *result. This is different fromqemu_strtod(), where a return of-ERANGEguarantees that *result is one of 4 values (+/- 0.0/inf). That seems awkward.Quoted 1 line
Violates the contract's "like
qemu_strtod()".Right, I missed that. What about something like this:
int
qemu_strtod_finite(const char *nptr, const char **endptr, double *result){ double tmp; int ret; ret = qemu_strtod(nptr, endptr, &tmp); if (!ret && !isfinite(tmp)) { if (endptr) { *endptr = nptr; }
Quoted 8 lines
ret = -EINVAL;
}
if (ret != -EINVAL) {
*result = tmp;
}
return ret;
}With these changes:
Eric Blake
Reviewed-byRe: [Qemu-devel] [PATCH v2 1/9] cutils: Add qemu_strtod() and qemu_strtod_finite()
In reply to David HildenbrandQuoted 29 lines · David Hildenbrand
Quoted 14 lines
Quoted 5 lines
Quoted 5 lines
util/cutils.c +5
-0+int qemu_strtod_finite (const char *nptr,const char **endptr,double *result) +{ +double tmp; +int ret; +Quoted 8 lines
util/cutils.c int qemu_strtou64(const char *nptr, const char **endptr, int base, · line 600
Quoted 3 lines
util/cutils.c +3
-0+ ret =qemu_strtod (nptr, endptr, &tmp); +if (ret) { +return ret;So, if we overflow, we are returning
-ERANGEbut with nothing stored into *result. This is different fromqemu_strtod(), where a return of-ERANGEguarantees that *result is one of 4 values (+/- 0.0/inf). That seems awkward.Quoted 1 line
Violates the contract's "like
qemu_strtod()".Right, I missed that. What about something like this:
int
qemu_strtod_finite(const char *nptr, const char **endptr, double *result){ double tmp; int ret; ret = qemu_strtod(nptr, endptr, &tmp); if (!ret && !isfinite(tmp)) { if (endptr) { *endptr = nptr; }
Quoted 8 lines
ret = -EINVAL;
}
if (ret != -EINVAL) {
*result = tmp;
}
return ret;
}With that algorithm, v3 can have:
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
PATCH v2 2/9 cutils: Fix qemu_strtosz() & friends to reject non-finite sizes
In reply to David Hildenbrandqemu_strtosz() & friends reject NaNs, but happily accept inifities. They shouldn't. Fix that.
The fix makes use of qemu_strtod_finite(). To avoid ugly casts, change the @end parameter of qemu_strtosz() & friends from char ** to const char **.
Also, add two test cases, testing that "inf" and "NaN" are properly rejected.
--- include/qemu/cutils.h | 6 +++--- monitor.c | 2 +- tests/test-cutils.c | 24 +++++++++++++++++------- util/cutils.c | 16 +++++++--------- 4 files changed, 28 insertions(+), 20 deletions(-)
include/qemu/cutils.h +3 -3
@@ -153,9 +153,9 @@ int parse_uint(const char *s, unsigned long long *value, char **endptr, int base);int parse_uint_full (const char *s,unsigned long long *value,int base); -int qemu_strtosz (const char *nptr,char **end,uint64_t *result); -int qemu_strtosz_MiB (const char *nptr,char **end,uint64_t *result); -int qemu_strtosz_metric (const char *nptr,char **end,uint64_t *result); +int qemu_strtosz (const char *nptr,const char **end,uint64_t *result); +int qemu_strtosz_MiB (const char *nptr,const char **end,uint64_t *result); +int qemu_strtosz_metric (const char *nptr,const char **end,uint64_t *result);/* used to print char* safely */ #define STR_OR_NULL (str) ((str) ? (str) : "null")
monitor.c +1 -1
@@ -3231,7 +3231,7 @@ static QDict *monitor_parse_arguments(Monitor *mon, {int ret;uint64_t val; -char *end; +const char *end;while (qemu_isspace (*p)) { p++;
tests/test-cutils.c +17 -7
@@ -1950,7 +1950,7 @@ static void test_qemu_strtou64_full_max(void)static void test_qemu_strtosz_simple (void ) {const char *str; -char *endptr =NULL ; +const char *endptr;int err;uint64_t res =0xbaadf00d ;
@@ -2017,7 +2017,7 @@ static void test_qemu_strtosz_units(void)const char *p ="1P" ;const char *e ="1E" ;int err; -char *endptr =NULL ; +const char *endptr;uint64_t res =0xbaadf00d ;/* default is M */
@@ -2066,7 +2066,7 @@ static void test_qemu_strtosz_float(void) {const char *str ="12.345M" ;int err; -char *endptr =NULL ; +const char *endptr;uint64_t res =0xbaadf00d ; err =qemu_strtosz (str, &endptr, &res);
@@ -2078,7 +2078,7 @@ static void test_qemu_strtosz_float(void)static void test_qemu_strtosz_invalid (void ) {const char *str; -char *endptr =NULL ; +const char *endptr;int err;uint64_t res =0xbaadf00d ;
@@ -2096,12 +2096,22 @@ static void test_qemu_strtosz_invalid(void) err =qemu_strtosz (str, &endptr, &res); g_assert_cmpint(err, ==, -EINVAL);g_assert (endptr == str); + + str ="inf" ; + err =qemu_strtosz (str, &endptr, &res); + g_assert_cmpint(err, ==, -EINVAL); +g_assert (endptr == str); + + str ="NaN" ; + err =qemu_strtosz (str, &endptr, &res); + g_assert_cmpint(err, ==, -EINVAL); +g_assert (endptr == str); }static void test_qemu_strtosz_trailing (void ) {const char *str; -char *endptr =NULL ; +const char *endptr;int err;uint64_t res =0xbaadf00d ;
@@ -2126,7 +2136,7 @@ static void test_qemu_strtosz_trailing(void)static void test_qemu_strtosz_erange (void ) {const char *str; -char *endptr =NULL ; +const char *endptr;int err;uint64_t res =0xbaadf00d ;
@@ -2160,7 +2170,7 @@ static void test_qemu_strtosz_metric(void) {const char *str ="12345k" ;int err; -char *endptr =NULL ; +const char *endptr;uint64_t res =0xbaadf00d ; err =qemu_strtosz_metric (str, &endptr, &res);
util/cutils.c +7 -9
@@ -206,20 +206,18 @@ static int64_t suffix_mul(char suffix, int64_t unit)* in *end, if not NULL. Return -ERANGE on overflow, Return -EINVAL on * other error. */ -static int do_strtosz (const char *nptr,char **end, +static int do_strtosz (const char *nptr,const char **end,const char default_suffix,int64_t unit,uint64_t *result) {int retval; -char *endptr; +const char *endptr;unsigned char c;int mul_required =0 ;double val, mul, integral, fraction; - errno =0 ; - val =strtod (nptr, &endptr); -if (isnan (val) || endptr == nptr || errno !=0 ) { - retval = -EINVAL ; + retval =qemu_strtod_finite (nptr, &endptr, &val); +if (retval) { goto out; } fraction =modf (val, &integral);
@@ -259,17 +257,17 @@ out:return retval; } -int qemu_strtosz (const char *nptr,char **end,uint64_t *result) +int qemu_strtosz (const char *nptr,const char **end,uint64_t *result) {return do_strtosz (nptr, end,'B' ,1024 , result); } -int qemu_strtosz_MiB (const char *nptr,char **end,uint64_t *result) +int qemu_strtosz_MiB (const char *nptr,const char **end,uint64_t *result) {return do_strtosz (nptr, end,'M' ,1024 , result); } -int qemu_strtosz_metric (const char *nptr,char **end,uint64_t *result) +int qemu_strtosz_metric (const char *nptr,const char **end,uint64_t *result) {return do_strtosz (nptr, end,'B' ,1000 , result); }
Signature · 1 line
2.17.2
Re: [Qemu-devel] [PATCH v2 2/9] cutils: Fix qemu_strtosz() & friends to reject non-finite sizes
Quoted 1 line · David Hildenbrand
qemu_strtosz()& friends reject NaNs, but happily accept inifities.
s/inifities/infinities/
Quoted 20 lines
They shouldn't. Fix that.
The fix makes use of
qemu_strtod_finite(). To avoid ugly casts, change the @end parameter ofqemu_strtosz()& friends from char ** to const char **.Also, add two test cases, testing that "inf" and "NaN" are properly rejected.
Signed-off-by David Hildenbrand <[email protected]>--- include/qemu/cutils.h | 6 +++--- monitor.c | 2 +- tests/test-cutils.c | 24 +++++++++++++++++------- util/cutils.c | 16 +++++++--------- 4 files changed, 28 insertions(+), 20 deletions(-)+++
b/util/cutils.c
util/cutils.c static int64_t suffix_mul(char suffix, int64_t unit) · line 206
Quoted 1 line
util/cutils.c +0
-0@@ -206,20 +206,18 @@ static int64_t suffix_mul(char suffix, int64_t unit)* in *end, if not NULL. Return -ERANGE on overflow, Return -EINVAL on
Pre-existing, but since you're touching this area: the second 'Return' is unusual capitalization for being mid-sentence. You could even s/Return/of/
Quoted 11 lines
util/cutils.c +2
-2* other error. */ -static int do_strtosz (const char *nptr,char **end, +static int do_strtosz (const char *nptr,const char **end,const char default_suffix,int64_t unit,uint64_t *result) {int retval; -char *endptr; +const char *endptr;unsigned char c;
util/cutils.c static int64_t suffix_mul(char suffix, int64_t unit) · line 221
Quoted 10 lines
util/cutils.c +2
-4int mul_required =0 ;double val, mul, integral, fraction; - errno =0 ; - val =strtod (nptr, &endptr); -if (isnan (val) || endptr == nptr || errno !=0 ) { - retval = -EINVAL ; + retval =qemu_strtod_finite (nptr, &endptr, &val); +if (retval) { goto out;
Here, retval can be -EINVAL (for failure to parse, or encountering "inf" or "NaN") or -ERANGE (overflow, underflow)...
util/cutils.c out:
Quoted 3 lines
util/cutils.c +0
-0} fraction =modf (val, &integral);@@ -259,17 +257,17 @@ out:
out:
if (end) {
*end = endptr;
} else if (*endptr) {
retval = -EINVAL;
}util/cutils.c out: · line 257
Quoted 1 line
util/cutils.c +0
-0return retval;
...if the failure was -EINVAL due to trailing garbage or empty string, nothing changes. If the failure was -EINVAL due to "inf", and the user passed in 'end', then 'end' now points to the beginning of "inf" instead of the end (probably okay). If the failure was -EINVAL due to "inf" and the user gave NULL for 'end', then we slam retval back to -EINVAL (no change). If the failure was -ERANGE, then there is no trailing garbage, so *endptr had better be NULL, and we still fail with -ERANGE. Any other way to reach the out label is unchanged from earlier logic.
It's some hairy code to think about, but I can't find anything wrong with it. Typo fixes are minor, so
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Markus Armbruster
Reviewed-byRe: [Qemu-devel] [PATCH v2 2/9] cutils: Fix qemu_strtosz() & friends to reject non-finite sizes
Quoted 23 lines · Eric Blake
Quoted 1 line · David Hildenbrand
qemu_strtosz()& friends reject NaNs, but happily accept inifities.s/inifities/infinities/
Quoted 17 lines
They shouldn't. Fix that.
The fix makes use of
qemu_strtod_finite(). To avoid ugly casts, change the @end parameter ofqemu_strtosz()& friends from char ** to const char **.Also, add two test cases, testing that "inf" and "NaN" are properly rejected.
Signed-off-by David Hildenbrand <[email protected]>--- include/qemu/cutils.h | 6 +++--- monitor.c | 2 +- tests/test-cutils.c | 24 +++++++++++++++++------- util/cutils.c | 16 +++++++--------- 4 files changed, 28 insertions(+), 20 deletions(-)
Quoted 7 lines
util/cutils.c static int64_t suffix_mul(char suffix, int64_t unit) · line 206
Quoted 3 lines
+++
b/util/cutils.cutil/cutils.c +0
-0@@ -206,20 +206,18 @@ static int64_t suffix_mul(char suffix, int64_t unit)* in *end, if not NULL. Return -ERANGE on overflow, Return -EINVAL on Pre-existing, but since you're touching this area: the second 'Return' is unusual capitalization for being mid-sentence. You could even s/Return/of/
"of"?
Quoted 48 lines
Quoted 11 lines
util/cutils.c +2
-2* other error. */ -static int do_strtosz (const char *nptr,char **end, +static int do_strtosz (const char *nptr,const char **end,const char default_suffix,int64_t unit,uint64_t *result) {int retval; -char *endptr; +const char *endptr;util/cutils.c static int64_t suffix_mul(char suffix, int64_t unit) · line 221
Quoted 10 lines
util/cutils.c +2
-3unsigned char c;int mul_required =0 ;double val, mul, integral, fraction; - errno =0 ; - val =strtod (nptr, &endptr); -if (isnan (val) || endptr == nptr || errno !=0 ) { - retval = -EINVAL ; + retval =qemu_strtod_finite (nptr, &endptr, &val); +if (retval) { goto out;Here, retval can be
-EINVAL(for failure to parse, or encountering "inf" or "NaN") or-ERANGE(overflow, underflow)...util/cutils.c out:
Quoted 3 lines
util/cutils.c +0
-0} fraction =modf (val, &integral);@@ -259,17 +257,17 @@ out:out:
if (end) { *end = endptr; } else if (*endptr) { retval = -EINVAL; }util/cutils.c out: · line 257
Quoted 1 line
util/cutils.c +0
-0return retval;...if the failure was
-EINVALdue to trailing garbage or empty string, nothing changes. If the failure was-EINVALdue to "inf", and the user passed in 'end', then 'end' now points to the beginning of "inf" instead of the end (probably okay). If the failure was-EINVALdue to "inf" and the user gave NULL for 'end', then we slam retval back to-EINVAL(no change). If the failure was-ERANGE, then there is no trailing garbage, so *endptr had better be NULL, and we still fail with-ERANGE. Any other way to reach the out label is unchanged from earlier logic.
Quoted 4 lines
It's some hairy code to think about, but I can't find anything wrong with it. Typo fixes are minor, so
Reviewed-by Eric Blake <[email protected]>
Thanks for your analysis, Eric.
With the typo fixes:
Re: [Qemu-devel] [PATCH v2 2/9] cutils: Fix qemu_strtosz() & friends to reject non-finite sizes
Quoted 33 lines · Markus Armbruster
Quoted 23 lines · Eric Blake
Quoted 1 line · David Hildenbrand
qemu_strtosz()& friends reject NaNs, but happily accept inifities.s/inifities/infinities/
Quoted 17 lines
They shouldn't. Fix that.
The fix makes use of
qemu_strtod_finite(). To avoid ugly casts, change the @end parameter ofqemu_strtosz()& friends from char ** to const char **.Also, add two test cases, testing that "inf" and "NaN" are properly rejected.
Signed-off-by David Hildenbrand <[email protected]>--- include/qemu/cutils.h | 6 +++--- monitor.c | 2 +- tests/test-cutils.c | 24 +++++++++++++++++------- util/cutils.c | 16 +++++++--------- 4 files changed, 28 insertions(+), 20 deletions(-)Quoted 7 lines
util/cutils.c static int64_t suffix_mul(char suffix, int64_t unit) · line 206
Quoted 3 lines
+++
b/util/cutils.cutil/cutils.c +0
-0@@ -206,20 +206,18 @@ static int64_t suffix_mul(char suffix, int64_t unit)* in *end, if not NULL. Return -ERANGE on overflow, Return -EINVAL on Pre-existing, but since you're touching this area: the second 'Return' is unusual capitalization for being mid-sentence. You could even s/Return/of/
Quoted 1 line
"of"?
"or" (ouch - wrong time for my fingers to be slipping on the keyboard)
Quoted 8 lines
Quoted 4 lines
It's some hairy code to think about, but I can't find anything wrong with it. Typo fixes are minor, so
Reviewed-by Eric Blake <[email protected]>Thanks for your analysis, Eric.
With the typo fixes:
Including the fix of my attempt at a typo fix :)
Quoted 2 lines
Reviewed-by Markus Armbruster <[email protected]>
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH v2 2/9] cutils: Fix qemu_strtosz() & friends to reject non-finite sizes
Quoted 34 lines · Eric Blake
Quoted 33 lines · Markus Armbruster
Quoted 30 lines · Eric Blake
Quoted 1 line · David Hildenbrand
qemu_strtosz()& friends reject NaNs, but happily accept inifities.s/inifities/infinities/
Quoted 20 lines
They shouldn't. Fix that.
The fix makes use of
qemu_strtod_finite(). To avoid ugly casts, change the @end parameter ofqemu_strtosz()& friends from char ** to const char **.Also, add two test cases, testing that "inf" and "NaN" are properly rejected.
Signed-off-by David Hildenbrand <[email protected]>--- include/qemu/cutils.h | 6 +++--- monitor.c | 2 +- tests/test-cutils.c | 24 +++++++++++++++++------- util/cutils.c | 16 +++++++--------- 4 files changed, 28 insertions(+), 20 deletions(-)+++
b/util/cutils.cutil/cutils.c static int64_t suffix_mul(char suffix, int64_t unit) · line 206
Quoted 1 line
util/cutils.c +0
-0@@ -206,20 +206,18 @@ static int64_t suffix_mul(char suffix, int64_t unit)* in *end, if not NULL. Return -ERANGE on overflow, Return -EINVAL on Pre-existing, but since you're touching this area: the second 'Return' is unusual capitalization for being mid-sentence. You could even s/Return/of/
Quoted 3 lines
Quoted 1 line
"of"?
"or" (ouch - wrong time for my fingers to be slipping on the keyboard)
Shouldn't that be "and" and s/Return/Returns/
"Returns -ERANGE on overflow and -EINVAL on other errors".
I can include that fixup (whetever version you guys prefer)
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
It's some hairy code to think about, but I can't find anything wrong with it. Typo fixes are minor, so
Quoted 9 lines
Quoted 5 lines
Quoted 1 line
Reviewed-by Eric Blake <[email protected]>Thanks for your analysis, Eric.
With the typo fixes:
Including the fix of my attempt at a typo fix :)
Quoted 1 line
Reviewed-by Markus Armbruster <[email protected]>
Thanks!
Signature · 3 lines
Thanks, David / dhildenb
Re: [Qemu-devel] [PATCH v2 2/9] cutils: Fix qemu_strtosz() & friends to reject non-finite sizes
Quoted 35 lines · David Hildenbrand
Quoted 34 lines · Eric Blake
Quoted 33 lines · Markus Armbruster
Quoted 30 lines · Eric Blake
Quoted 1 line · David Hildenbrand
qemu_strtosz()& friends reject NaNs, but happily accept inifities.s/inifities/infinities/
Quoted 20 lines
They shouldn't. Fix that.
The fix makes use of
qemu_strtod_finite(). To avoid ugly casts, change the @end parameter ofqemu_strtosz()& friends from char ** to const char **.Also, add two test cases, testing that "inf" and "NaN" are properly rejected.
Signed-off-by David Hildenbrand <[email protected]>--- include/qemu/cutils.h | 6 +++--- monitor.c | 2 +- tests/test-cutils.c | 24 +++++++++++++++++------- util/cutils.c | 16 +++++++--------- 4 files changed, 28 insertions(+), 20 deletions(-)+++
b/util/cutils.cutil/cutils.c static int64_t suffix_mul(char suffix, int64_t unit) · line 206
Quoted 1 line
util/cutils.c +0
-0@@ -206,20 +206,18 @@ static int64_t suffix_mul(char suffix, int64_t unit)* in *end, if not NULL. Return -ERANGE on overflow, Return -EINVAL on Pre-existing, but since you're touching this area: the second 'Return' is unusual capitalization for being mid-sentence. You could even s/Return/of/
Quoted 8 lines
Quoted 3 lines
Quoted 1 line
"of"?
"or" (ouch - wrong time for my fingers to be slipping on the keyboard)
Shouldn't that be "and" and s/Return/Returns/
"Returns
-ERANGEon overflow and-EINVALon other errors".
I prefer imperative mood for function contracts: "Convert string to bytes", "Return -ERANGE on overflow", and so forth.
Other than that, I like your phrasing. I'd put a comma before "and", though.
Quoted 18 lines
I can include that fixup (whetever version you guys prefer)
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
It's some hairy code to think about, but I can't find anything wrong with it. Typo fixes are minor, so
Quoted 9 lines
Quoted 5 lines
Quoted 1 line
Reviewed-by Eric Blake <[email protected]>Thanks for your analysis, Eric.
With the typo fixes:
Including the fix of my attempt at a typo fix :)
Quoted 1 line
Reviewed-by Markus Armbruster <[email protected]>Thanks!
Re: [Qemu-devel] [PATCH v2 2/9] cutils: Fix qemu_strtosz() & friends to reject non-finite sizes
In reply to David HildenbrandQuoted 9 lines · David Hildenbrand
Quoted 9 lines
Quoted 8 lines
Quoted 6 lines
util/cutils.c static int64_t suffix_mul(char suffix, int64_t unit) · line 206
Quoted 2 lines
util/cutils.c +0
-0@@ -206,20 +206,18 @@ static int64_t suffix_mul(char suffix, int64_t unit)* in *end, if not NULL. Return -ERANGE on overflow, Return -EINVAL on Pre-existing, but since you're touching this area: the second 'Return' is unusual capitalization for being mid-sentence. You could even s/Return/of/
"of"?
Quoted 8 lines
Quoted 1 line
"or" (ouch - wrong time for my fingers to be slipping on the keyboard)
Shouldn't that be "and" and s/Return/Returns/
"Returns
-ERANGEon overflow and-EINVALon other errors".I can include that fixup (whetever version you guys prefer)
I was thinking:
Return -ERANGE on overflow, or -EINVAL on other errors.
'Return', not 'Returns', because of imperative mood. The choice of 'and' vs. 'or' is less of a sticking point; both sound fine to my native ear, especially since the word 'other' makes it apparent that you won't have both overflow and a conversion error at the same time (my initial choice of 'or' rather than 'and' was solely because you can't have two return values at once; but using 'and' seems okay at implying a sense of prioritization where overflow trumps other detected errors).
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
PATCH v2 3/9 qapi: Fix string-input-visitor to reject NaN and infinities
In reply to David HildenbrandThe string-input-visitor happily accepts NaN and inifities when parsing numbers (doubles). They shouldn't. Fix that.
Also, add two test cases, testing if "NaN" and "inf" is properly rejected.
--- qapi/string-input-visitor.c | 6 ++---- tests/test-string-input-visitor.c | 13 +++++++++++++ 2 files changed, 15 insertions(+), 4 deletions(-)
qapi/string-input-visitor.c +2 -4
@@ -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 ;
tests/test-string-input-visitor.c +13 -0
@@ -252,6 +252,19 @@ static void test_visitor_in_number(TestInputVisitorData *data,visit_type_number (v,NULL , &res, &err);g_assert (!err); g_assert_cmpfloat(res, ==, value); + +/* NaN and infinity has to be rejected */ + + v =visitor_input_test_init (data,"NaN" ); + +visit_type_number (v,NULL , &res, &err); +error_free_or_abort (&err); + + v =visitor_input_test_init (data,"inf" ); + +visit_type_number (v,NULL , &res, &err); +error_free_or_abort (&err); + } static void test_visitor_in_string(TestInputVisitorData *data,
Signature · 1 line
2.17.2
Re: [Qemu-devel] [PATCH v2 3/9] qapi: Fix string-input-visitor to reject NaN and infinities
Quoted 1 line · David Hildenbrand
The string-input-visitor happily accepts NaN and inifities when parsing
Make that "infinities".
Quoted 7 lines
numbers (doubles). They shouldn't. Fix that.
Also, add two test cases, testing if "NaN" and "inf" is properly rejected.
Reviewed-by Eric Blake <[email protected]>Signed-off-by David Hildenbrand <[email protected]>
PS: I still owe you an analysis of backward compatibility impact.
PATCH v2 4/9 qapi: Use qemu_strtod_finite() in qobject-input-visitor
In reply to David HildenbrandLet'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).
--- qapi/qobject-input-visitor.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
qapi/qobject-input-visitor.c +5 -4
@@ -562,19 +562,20 @@ static void qobject_input_type_number_keyval(Visitor *v, const char *name, {QObjectInputVisitor *qiv =to_qiv (v);const char *str =qobject_input_get_keyval (qiv, name, errp); -char *endp; +double val;if (!str) {return ; } - errno =0 ; - *obj =strtod (str, &endp); -if (errno || endp == str || *endp || !isfinite (*obj)) { +if (qemu_strtod_finite (str,NULL , &val)) {/* TODO report -ERANGE more nicely */ error_setg (errp,QERR_INVALID_PARAMETER_TYPE ,full_name (qiv, name),"number" ); +return ; } + + *obj = val; } static void qobject_input_type_any(Visitor *v, const char *name, QObject **obj,
Signature · 1 line
2.17.2
PATCH v2 5/9 test-string-input-visitor: Add more tests
In reply to David HildenbrandTest that very big/small values are not accepted and that ranges with only one element work. Also test that ranges are ascending and cannot have more than 65536 elements.
Rename expect4 to expect5, as we will be moving that to a separate ulist test after the rework.
--- tests/test-string-input-visitor.c | 41 +++++++++++++++++++++++++++++-- 1 file changed, 39 insertions(+), 2 deletions(-)
tests/test-string-input-visitor.c +39 -2
@@ -121,7 +121,8 @@ static void test_visitor_in_intList(TestInputVisitorData *data,int64_t expect1[] = {0 ,1 ,2 ,3 ,4 ,5 ,6 ,7 ,8 ,9 ,20 };int64_t expect2[] = {32767 ,-32768 ,-32767 };int64_t expect3[] = {INT64_MAX ,INT64_MIN }; -uint64_t expect4[] = {UINT64_MAX }; +int64_t expect4[] = {1 }; +uint64_t expect5[] = {UINT64_MAX };Error *err =NULL ;int64List *res =NULL ;int64List *tail;
@@ -140,8 +141,44 @@ static void test_visitor_in_intList(TestInputVisitorData *data, "-9223372036854775808,9223372036854775807");check_ilist (v, expect3,ARRAY_SIZE (expect3)); + v =visitor_input_test_init (data,"1-1" ); +check_ilist (v, expect4,ARRAY_SIZE (expect4)); + v =visitor_input_test_init (data,"18446744073709551615" ); -check_ulist (v, expect4,ARRAY_SIZE (expect4)); +check_ulist (v, expect5,ARRAY_SIZE (expect5)); + +/* Value too large */ + + v =visitor_input_test_init (data,"9223372036854775808" ); +visit_type_int64List (v,NULL , &res, &err); +error_free_or_abort (&err); +g_assert (!res); + +/* Value too small */ + + v =visitor_input_test_init (data,"-9223372036854775809" ); +visit_type_int64List (v,NULL , &res, &err); +error_free_or_abort (&err); +g_assert (!res); + +/* Range not ascending */ + + v =visitor_input_test_init (data,"3-1" ); +visit_type_int64List (v,NULL , &res, &err); +error_free_or_abort (&err); +g_assert (!res); + + v =visitor_input_test_init (data,"9223372036854775807-0" ); +visit_type_int64List (v,NULL , &res, &err); +error_free_or_abort (&err); +g_assert (!res); + +/* Range too big (65536 is the limit against DOS attacks) */ + + v =visitor_input_test_init (data,"0-65536" ); +visit_type_int64List (v,NULL , &res, &err); +error_free_or_abort (&err); +g_assert (!res);/* Empty list */
Signature · 1 line
2.17.2
Re: [Qemu-devel] [PATCH v2 5/9] test-string-input-visitor: Add more tests
Quoted 4 lines · David Hildenbrand
Test that very big/small values are not accepted and that ranges with only one element work. Also test that ranges are ascending and cannot have more than 65536 elements.
Quoted 8 lines
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 | 41 +++++++++++++++++++++++++++++-- 1 file changed, 39 insertions(+), 2 deletions(-)
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH v2 5/9] test-string-input-visitor: Add more tests
Quoted 8 lines · Eric Blake
Quoted 7 lines · David Hildenbrand
Test that very big/small values are not accepted and that ranges with only one element work. Also test that ranges are ascending and cannot have more than 65536 elements.
Rename expect4 to expect5, as we will be moving that to a separate ulist test after the rework.
Quoted 7 lines
Quoted 5 lines
Signed-off-by David Hildenbrand <[email protected]>---
tests/test-string-input-visitor.c| 41 +++++++++++++++++++++++++++++--1 file changed, 39 insertions(+), 2 deletions(-)Reviewed-by Eric Blake <[email protected]>
Do we also want to test garbage strings like:
"1-" (incomplete range)
"1X-2" (garbage suffix on first element)
"1-2X" (garbage suffix on second element)
and/or
"-2--1" (valid range of signed integers)
"-1--2" (questionable whether this is valid for the two largest unsigned integers)
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH v2 5/9] test-string-input-visitor: Add more tests
Quoted 19 lines · Eric Blake
Quoted 8 lines · Eric Blake
Quoted 7 lines · David Hildenbrand
Test that very big/small values are not accepted and that ranges with only one element work. Also test that ranges are ascending and cannot have more than 65536 elements.
Rename expect4 to expect5, as we will be moving that to a separate ulist test after the rework.
Quoted 7 lines
Quoted 5 lines
Signed-off-by David Hildenbrand <[email protected]>---
tests/test-string-input-visitor.c| 41 +++++++++++++++++++++++++++++--1 file changed, 39 insertions(+), 2 deletions(-)Reviewed-by Eric Blake <[email protected]>Do we also want to test garbage strings like:
Quoted 8 lines
"1-" (incomplete range)
"1X-2" (garbage suffix on first element)
"1-2X" (garbage suffix on second element)and/or
"-2--1" (valid range of signed integers)
"-1--2" (questionable whether this is valid for the two largest unsigned integers)
Or even " 1- 2" (we permit leading whitespace for plain integers - do we also permit it in both sides of a range)? Also, if we permit whitespace after the range '-', should we permit it beforehand?
These sorts of questions may be fine in followup patches.
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Markus Armbruster
Reviewed-byRe: [Qemu-devel] [PATCH v2 5/9] test-string-input-visitor: Add more tests
Quoted 26 lines · Eric Blake
Quoted 25 lines · Eric Blake
Quoted 8 lines · Eric Blake
Quoted 7 lines · David Hildenbrand
Test that very big/small values are not accepted and that ranges with only one element work. Also test that ranges are ascending and cannot have more than 65536 elements.
Rename expect4 to expect5, as we will be moving that to a separate ulist test after the rework.
Quoted 7 lines
Quoted 5 lines
Signed-off-by David Hildenbrand <[email protected]>---
tests/test-string-input-visitor.c| 41 +++++++++++++++++++++++++++++--1 file changed, 39 insertions(+), 2 deletions(-)Reviewed-by Eric Blake <[email protected]>Do we also want to test garbage strings like:
"1-" (incomplete range)
"1X-2" (garbage suffix on first element)
"1-2X" (garbage suffix on second element)and/or
Quoted 7 lines
Quoted 3 lines
"-2--1" (valid range of signed integers)
"-1--2" (questionable whether this is valid for the two largest unsigned integers)Or even " 1- 2" (we permit leading whitespace for plain integers - do we also permit it in both sides of a range)? Also, if we permit whitespace after the range '-', should we permit it beforehand?
For what it's worth, the tests for integers (as opposed to ranges) aren't that thorough, either. test-cutils.c already provides cover for them, but not for ranges.
Quoted 1 line
These sorts of questions may be fine in followup patches.
David is doing me a favor with these patches. The tests feel good enough, I don't want to hold these patches hostage to extort more.
Re: [Qemu-devel] [PATCH v2 5/9] test-string-input-visitor: Add more tests
In reply to Eric BlakeQuoted 30 lines · Eric Blake
Quoted 23 lines · Eric Blake
Quoted 8 lines · Eric Blake
Quoted 7 lines · David Hildenbrand
Test that very big/small values are not accepted and that ranges with only one element work. Also test that ranges are ascending and cannot have more than 65536 elements.
Rename expect4 to expect5, as we will be moving that to a separate ulist test after the rework.
Quoted 7 lines
Quoted 5 lines
Signed-off-by David Hildenbrand <[email protected]>---
tests/test-string-input-visitor.c| 41 +++++++++++++++++++++++++++++--1 file changed, 39 insertions(+), 2 deletions(-)Reviewed-by Eric Blake <[email protected]>Do we also want to test garbage strings like:
"1-" (incomplete range)
"1X-2" (garbage suffix on first element)
"1-2X" (garbage suffix on second element)Quoted 5 lines
and/or
"-2--1" (valid range of signed integers)
"-1--2" (questionable whether this is valid for the two largest unsigned integers)
Quoted 6 lines
Or even " 1- 2" (we permit leading whitespace for plain integers - do we also permit it in both sides of a range)? Also, if we permit whitespace after the range '-', should we permit it beforehand?
These sorts of questions may be fine in followup patches.
As always, you can never cover all cases during tests :)
While these things surely make sense, I will not add them right now. As you said, we can do that later.
Taking about spaces:
I think spaces before the "-" are not supported before/after the rewrite (I remember that strto.* does not skip over them). Spaces after the space should be covered by strto* automatically (strto.* skips over leading spaces).
Thanks!
Signature · 3 lines
Thanks, David / dhildenb
Re: [Qemu-devel] [PATCH v2 5/9] test-string-input-visitor: Add more tests
Quoted 44 lines · David Hildenbrand
Quoted 30 lines · Eric Blake
Quoted 23 lines · Eric Blake
Quoted 8 lines · Eric Blake
Quoted 7 lines · David Hildenbrand
Test that very big/small values are not accepted and that ranges with only one element work. Also test that ranges are ascending and cannot have more than 65536 elements.
Rename expect4 to expect5, as we will be moving that to a separate ulist test after the rework.
Quoted 7 lines
Quoted 5 lines
Signed-off-by David Hildenbrand <[email protected]>---
tests/test-string-input-visitor.c| 41 +++++++++++++++++++++++++++++--1 file changed, 39 insertions(+), 2 deletions(-)Reviewed-by Eric Blake <[email protected]>Do we also want to test garbage strings like:
"1-" (incomplete range)
"1X-2" (garbage suffix on first element)
"1-2X" (garbage suffix on second element)Quoted 5 lines
and/or
"-2--1" (valid range of signed integers)
"-1--2" (questionable whether this is valid for the two largest unsigned integers)Quoted 6 lines
Or even " 1- 2" (we permit leading whitespace for plain integers - do we also permit it in both sides of a range)? Also, if we permit whitespace after the range '-', should we permit it beforehand?
These sorts of questions may be fine in followup patches.
As always, you can never cover all cases during tests :)
While these things surely make sense, I will not add them right now. As you said, we can do that later.
Quoted 6 lines
Taking about spaces:
I think spaces before the "-" are not supported before/after the rewrite (I remember that strto.* does not skip over them). Spaces after the space should be covered by strto* automatically (strto.* skips over leading spaces).
qemu_strtol() & friends ignore leading whitespace, just like strtol() does. Trailing whitespace is handled like any other trailing
characters: hard error when endptr is null, else make *endptr point to it.
I'd expect whitespace to be accepted before either number of a range, but not between the first number and the '-'.
Perhaps rejecting whitespace there would be the cleaner interface, but then we get to discuss backward compatibility, and I go "thanks, but no, thanks."
PATCH v2 6/9 qapi: Rewrite string-input-visitor
In reply to David HildenbrandThe input visitor has some problems right now, especially
- unsigned type "Range" is used to process signed ranges, resulting in inconsistent behavior and ugly/magical code
- uint64_t are parsed like int64_t, so big uint64_t values are not supported and error messages are misleading
- lists/ranges of int64_t are accepted although no list is parsed and we should rather report an error
- lists/ranges are preparsed using int64_t, making it hard to implement uint64_t values or uint64_t lists
- types that don't support lists don't bail out
- visiting beyond the end of a list is not handled properly
- we don't actually parse lists, we parse *sets*: members are sorted, and duplicates eliminated
So let's rewrite it by getting rid of usage of the type "Range" and properly supporting lists of int64_t and uint64_t (including ranges of both types), fixing the above mentioned issues.
Lists of other types are not supported and will properly report an error. Virtual walks are now supported.
Tests have to be fixed up:
- Two BUGs were hardcoded that are fixed now
- The string-input-visitor now actually returns a parsed list and not an ordered set.
Please note that no users/callers have to be fixed up. Candiates using visit_type_uint16List() and friends are:
- backends/hostmem.c:host_memory_backend_set_host_nodes() -- Code can deal with dupilcates/unsorted lists
- numa.c::query_memdev()
-- via object_property_get_uint16List(), the list will still be sorted
and without duplicates (via host_memory_backend_get_host_nodes())
- qapi-visit.c::visit_type_Memdev_members()
- qapi-visit.c::visit_type_NumaNodeOptions_members()
- qapi-visit.c::visit_type_RockerOfDpaGroup_members
- qapi-visit.c::visit_type_RxFilterInfo_members()
-- Not used with string-input-visitor.
--- include/qapi/string-input-visitor.h | 4 +- qapi/string-input-visitor.c | 405 ++++++++++++++++------------ tests/test-string-input-visitor.c | 18 +- 3 files changed, 234 insertions(+), 193 deletions(-)
include/qapi/string-input-visitor.h +2 -2
@@ -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 +228 -177
529 lines not on this page · raw message
tests/test-string-input-visitor.c +4 -14
@@ -92,16 +92,6 @@ static void check_ulist(Visitor *v, uint64_t *expected, size_t n)uint64List *tail;int i; -/* BUG: unsigned numbers above INT64_MAX don't work */ -for (i =0 ; i < n; i++) { -if (expected[i] >INT64_MAX ) { -Error *err =NULL ; -visit_type_uint64List (v,NULL , &res, &err); -error_free_or_abort (&err); -return ; - } - } -visit_type_uint64List (v,NULL , &res, &error_abort); tail = res;for (i =0 ; i < n; i++) {
@@ -117,10 +107,10 @@ static void check_ulist(Visitor *v, uint64_t *expected, size_t n)static void test_visitor_in_intList (TestInputVisitorData *data,const void *unused) { -/* Note: the visitor *sorts* ranges *unsigned* */ -int64_t expect1[] = {0 ,1 ,2 ,3 ,4 ,5 ,6 ,7 ,8 ,9 ,20 }; +int64_t expect1[] = {1 ,2 ,0 ,2 ,3 ,4 ,20 ,5 ,6 ,7 , +8 ,9 ,1 ,2 ,3 ,4 ,5 ,6 ,7 ,8 };int64_t expect2[] = {32767 ,-32768 ,-32767 }; -int64_t expect3[] = {INT64_MAX ,INT64_MIN }; +int64_t expect3[] = {INT64_MIN ,INT64_MAX };int64_t expect4[] = {1 };uint64_t expect5[] = {UINT64_MAX };Error *err =NULL ;
@@ -226,7 +216,7 @@ static void test_visitor_in_intList(TestInputVisitorData *data,visit_type_int64 (v,NULL , &tail->value, &err); g_assert_cmpint(tail->value, ==, 0);visit_type_int64 (v,NULL , &val, &err); - g_assert_cmpint(val, ==, 1); /* BUG */ +error_free_or_abort (&err);visit_check_list (v, &error_abort);visit_end_list (v, (void **)&res);
Signature · 1 line
2.17.2
Re: [Qemu-devel] [PATCH v2 6/9] qapi: Rewrite string-input-visitor
Quoted 21 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_tare parsed likeint64_t, so biguint64_tvalues are not supported and error messages are misleading
- lists/ranges ofint64_tare accepted although no list is parsed and we should rather report an error
- lists/ranges are preparsed usingint64_t, making it hard to implementuint64_tvalues oruint64_tlists
- types that don't support lists don't bail out
- visiting beyond the end of a list is not handled properly
- we don't actually parse lists, we parse *sets*: members are sorted, and duplicates eliminatedSo let's rewrite it by getting rid of usage of the type "Range" and properly supporting lists of
int64_tanduint64_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.
Quoted 6 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.Please note that no users/callers have to be fixed up. Candiates using
s/Candiates/Candidates/
Quoted 3 lines
visit_type_uint16List()and friends are:
- backends/hostmem.c:host_memory_backend_set_host_nodes() -- Code can deal with dupilcates/unsorted lists
s/dupilcates/duplicates/
Quoted 26 lines
-
numa.c::query_memdev()
-- viaobject_property_get_uint16List(), the list will still be sorted
and without duplicates (viahost_memory_backend_get_host_nodes())
- qapi-visit.c::visit_type_Memdev_members()
- qapi-visit.c::visit_type_NumaNodeOptions_members()
- qapi-visit.c::visit_type_RockerOfDpaGroup_members
- qapi-visit.c::visit_type_RxFilterInfo_members()
-- Not used with string-input-visitor.Signed-off-by David Hildenbrand <[email protected]>--- include/qapi/string-input-visitor.h | 4 +- qapi/string-input-visitor.c | 405 ++++++++++++++++------------ tests/test-string-input-visitor.c | 18 +- 3 files changed, 234 insertions(+), 193 deletions(-)struct StringInputVisitor { Visitor visitor;qapi/string-input-visitor.c +1
-3-GList *ranges; -GList *cur_range; -int64_t cur; +/* List parsing state */
qapi/string-input-visitor.c line 57
Quoted 10 lines
qapi/string-input-visitor.c +6
-1+ListMode lm; +RangeElement rangeNext; +RangeElement rangeEnd; +const char *unparsed_string; +void *list; +/* The original string to parse */ const char *string; -void *list;/* Only needed for sanity checking the caller */ };
Makes sense.
qapi/string-input-visitor.c static GenericList *next_list(Visitor *v, GenericList *tail, size_t size) · line 114
Quoted 13 lines
qapi/string-input-visitor.c +5
-3@@ -179,88 +106,208 @@ static GenericList *next_list(Visitor *v, GenericList *tail, size_t size)static void check_list (Visitor *v,Error **errp) {const StringInputVisitor *siv =to_siv (v); -Range *r; -GList *cur_range; -if (!siv->ranges || !siv->cur_range) { +switch (siv->lm) { +case LM_INT64_RANGE : +case LM_UINT64_RANGE : +case LM_UNPARSED : +error_setg (errp,"Fewer list elements expected" );
Bike-shedding - I don't know if "Too many list elements supplied" would make the error any more legible.
Quoted 13 lines
qapi/string-input-visitor.c +6
-2static 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)) {
qapi/string-input-visitor.c static GenericList *next_list(Visitor *v, GenericList *tail, size_t size) · line 201
Quoted 10 lines
qapi/string-input-visitor.c +9
-0+error_setg (errp,QERR_INVALID_PARAMETER_VALUE , + name ? name :"null" ,"int64" ); +return ; + } + *obj = val;return ; +case LM_UNPARSED : +if (try_parse_int64_list_entry (siv, obj)) { +error_setg (errp,QERR_INVALID_PARAMETER_VALUE , name ? name :"null" , +"list of int64 values or ranges" );
The error message might be a bit misleading for a range larger than 64k, but that's not too bad.
qapi/string-input-visitor.c static GenericList *next_list(Visitor *v, GenericList *tail, size_t size) · line 211
Quoted 10 lines
qapi/string-input-visitor.c +10
-0+return ; + } +assert (siv->lm ==LM_INT64_RANGE ); +/* fall through */ +case LM_INT64_RANGE : +/* return the next element in the range */ +assert (siv->rangeNext.i64 <= siv->rangeEnd.i64); + *obj = siv->rangeNext.i64++; + +if (siv->rangeNext.i64 > siv->rangeEnd.i64 || *obj ==INT64_MAX ) {
I think our compiler options guarantee that we have sane signed wraparound and thus this is a safe comparison on overflow; but if you were to swap it so that the *obj == INT64_MAX check is performed first, you wouldn't even have to debate about whether undefined C semantics are being invoked.
qapi/string-input-visitor.c static GenericList *next_list(Visitor *v, GenericList *tail, size_t size) · line 217
Quoted 6 lines
qapi/string-input-visitor.c +6
-0+/* end of range, check if there is more to parse */ + siv->lm = siv->unparsed_string[0 ] ?LM_UNPARSED :LM_END ; + } +return ; +case LM_END : +error_setg (errp,"Fewer list elements expected" );
Again, bikeshedding if "too many list elements supplied" would make any more sense.
qapi/string-input-visitor.c static GenericList *next_list(Visitor *v, GenericList *tail, size_t size) · line 230
Quoted 10 lines
qapi/string-input-visitor.c +6
-3+static int try_parse_uint64_list_entry (StringInputVisitor *siv,uint64_t *obj) +{ +const char *endptr; +uint64_t start, end; - siv->cur_range =g_list_first (siv->ranges); -if (!siv->cur_range) { - goto error; +/* parse a simple uint64 or range */ +if (qemu_strtou64 (siv->unparsed_string, &endptr,0 , &start)) {
Lots of duplication between the signed and unsigned variants. But I don't see any easy way to factor it out into a common helper, as there are just too many places where signed vs. unsigned does not easily lend itself to common code.
qapi/string-input-visitor.c static void parse_type_null(Visitor *v, const char *name, QNull **obj, · line 387
Quoted 9 lines
qapi/string-input-visitor.c +2
-1@@ -330,9 +381,10 @@ static void parse_type_null(Visitor *v, const char *name, QNull **obj, {StringInputVisitor *siv =to_siv (v); +assert (siv->lm ==LM_NONE ); *obj =NULL ; -if (!siv->string || siv->string[0 ]) { +if (siv->string[0 ]) {
Why did this condition change?
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Markus Armbruster
Reviewed-byRe: [Qemu-devel] [PATCH v2 6/9] qapi: Rewrite string-input-visitor
I think the title should be something like
qapi: Rewrite string-input-visitor's integer and list parsing
because you don't actually rewrite all of it.
Quoted 74 lines · Eric Blake
Quoted 21 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_tare parsed likeint64_t, so biguint64_tvalues are not supported and error messages are misleading
- lists/ranges ofint64_tare accepted although no list is parsed and we should rather report an error
- lists/ranges are preparsed usingint64_t, making it hard to implementuint64_tvalues oruint64_tlists
- types that don't support lists don't bail out
- visiting beyond the end of a list is not handled properly
- we don't actually parse lists, we parse *sets*: members are sorted, and duplicates eliminatedSo let's rewrite it by getting rid of usage of the type "Range" and properly supporting lists of
int64_tanduint64_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.
Quoted 6 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.Please note that no users/callers have to be fixed up. Candiates using
s/Candiates/Candidates/
Quoted 3 lines
visit_type_uint16List()and friends are:
- backends/hostmem.c:host_memory_backend_set_host_nodes() -- Code can deal with dupilcates/unsorted listss/dupilcates/duplicates/
Quoted 24 lines
-
numa.c::query_memdev()
-- viaobject_property_get_uint16List(), the list will still be sorted
and without duplicates (viahost_memory_backend_get_host_nodes())
- qapi-visit.c::visit_type_Memdev_members()
- qapi-visit.c::visit_type_NumaNodeOptions_members()
- qapi-visit.c::visit_type_RockerOfDpaGroup_members
- qapi-visit.c::visit_type_RxFilterInfo_members()
-- Not used with string-input-visitor.Signed-off-by David Hildenbrand <[email protected]>--- include/qapi/string-input-visitor.h | 4 +- qapi/string-input-visitor.c | 405 ++++++++++++++++------------ tests/test-string-input-visitor.c | 18 +- 3 files changed, 234 insertions(+), 193 deletions(-)struct StringInputVisitor
{ Visitor visitor;- GList *ranges;
qapi/string-input-visitor.c +0
-2-GList *cur_range; -int64_t cur;qapi/string-input-visitor.c line 57
Quoted 10 lines
qapi/string-input-visitor.c +6
-1+/* List parsing state */ +ListMode lm; +RangeElement rangeNext; +RangeElement rangeEnd; +const char *unparsed_string; +void *list; +/* The original string to parse */ const char *string; -void *list;/* Only needed for sanity checking the caller */ };Makes sense.
Quoted 15 lines
qapi/string-input-visitor.c static GenericList *next_list(Visitor *v, GenericList *tail, size_t size) · line 114
Quoted 12 lines
qapi/string-input-visitor.c +5
-2@@ -179,88 +106,208 @@ static GenericList *next_list(Visitor *v, GenericList *tail, size_t size)static void check_list (Visitor *v,Error **errp) {const StringInputVisitor *siv =to_siv (v); -Range *r; -GList *cur_range; - if (!siv->ranges || !siv->cur_range) { +switch (siv->lm) { +case LM_INT64_RANGE : +case LM_UINT64_RANGE : +case LM_UNPARSED : +error_setg (errp,"Fewer list elements expected" );Bike-shedding - I don't know if "Too many list elements supplied" would make the error any more legible.
See my review of PATCH RFC 3/6:
Hmm. qobject_input_check_list() reports "Only %u list elements expected in %s" here. Looks unintuitive, until you remember how it's normally
used: to parse user input. The error gets reported when the parsing didn't consume the whole list. Can only happen with a virtual walk. And when it happens, the issue to report is "you provided a longer list than I can accept". qobject_input_check_list() does exactly that. Your message is less clear.
David didn't feel counting elements just for that was worthwhile here, and I agreed.
Quoted 51 lines
Quoted 13 lines
qapi/string-input-visitor.c +6
-2static 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)) {qapi/string-input-visitor.c static GenericList *next_list(Visitor *v, GenericList *tail, size_t size) · line 201
Quoted 10 lines
qapi/string-input-visitor.c +9
-0+error_setg (errp,QERR_INVALID_PARAMETER_VALUE , + name ? name :"null" ,"int64" ); +return ; + } + *obj = val;return ; +case LM_UNPARSED : +if (try_parse_int64_list_entry (siv, obj)) { +error_setg (errp,QERR_INVALID_PARAMETER_VALUE , name ? name :"null" , +"list of int64 values or ranges" );The error message might be a bit misleading for a range larger than 64k, but that's not too bad.
qapi/string-input-visitor.c static GenericList *next_list(Visitor *v, GenericList *tail, size_t size) · line 211
Quoted 10 lines
qapi/string-input-visitor.c +10
-0+return ; + } +assert (siv->lm ==LM_INT64_RANGE ); +/* fall through */ +case LM_INT64_RANGE : +/* return the next element in the range */ +assert (siv->rangeNext.i64 <= siv->rangeEnd.i64); + *obj = siv->rangeNext.i64++; + +if (siv->rangeNext.i64 > siv->rangeEnd.i64 || *obj ==INT64_MAX ) {I think our compiler options guarantee that we have sane signed wraparound and thus this is a safe comparison on overflow; but if you were to swap it so that the *obj ==
INT64_MAXcheck is performed first, you wouldn't even have to debate about whether undefined C semantics are being invoked.qapi/string-input-visitor.c static GenericList *next_list(Visitor *v, GenericList *tail, size_t size) · line 217
Quoted 6 lines
qapi/string-input-visitor.c +6
-0+/* end of range, check if there is more to parse */ + siv->lm = siv->unparsed_string[0 ] ?LM_UNPARSED :LM_END ; + } +return ; +case LM_END : +error_setg (errp,"Fewer list elements expected" );
Quoted 2 lines
Again, bikeshedding if "too many list elements supplied" would make any more sense.
Same argument.
Quoted 6 lines
Quoted 6 lines
qapi/string-input-visitor.c +4
-1+static int try_parse_uint64_list_entry (StringInputVisitor *siv,uint64_t *obj) +{ +const char *endptr; +uint64_t start, end; - siv->cur_range =g_list_first (siv->ranges); -if (!siv->cur_range) {
Quoted 8 lines
qapi/string-input-visitor.c static GenericList *next_list(Visitor *v, GenericList *tail, size_t size) · line 230
Quoted 3 lines
qapi/string-input-visitor.c +2
-1- goto error; +/* parse a simple uint64 or range */ +if (qemu_strtou64 (siv->unparsed_string, &endptr,0 , &start)) {Lots of duplication between the signed and unsigned variants. But I don't see any easy way to factor it out into a common helper, as there are just too many places where signed vs. unsigned does not easily lend itself to common code.
Yes.
Quoted 9 lines
qapi/string-input-visitor.c static void parse_type_null(Visitor *v, const char *name, QNull **obj, · line 386
Quoted 7 lines
qapi/string-input-visitor.c +1
-0@@ -330,9 +381,10 @@ static void parse_type_null(Visitor *v, const char *name, QNull **obj, {StringInputVisitor *siv =to_siv (v); +assert (siv->lm ==LM_NONE ); *obj =NULL ; - if (!siv->string || siv->string[0]) { +if (siv->string[0 ]) {Why did this condition change?
As far as I can tell, siv->string can't ever be null. Sticking the change into this patch is perhaps debatable. I'm okay with it.
Quoted 1 line
Reviewed-by Eric Blake <[email protected]>
With the commit message improved once more:
Re: [Qemu-devel] [PATCH v2 6/9] qapi: Rewrite string-input-visitor
Quoted 44 lines · Markus Armbruster
I think the title should be something like
qapi: Rewrite string-input-visitor's integer and list parsing
because you don't actually rewrite all of it.
Quoted 36 lines · Eric Blake
Quoted 21 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_tare parsed likeint64_t, so biguint64_tvalues are not supported and error messages are misleading
- lists/ranges ofint64_tare accepted although no list is parsed and we should rather report an error
- lists/ranges are preparsed usingint64_t, making it hard to implementuint64_tvalues oruint64_tlists
- types that don't support lists don't bail out
- visiting beyond the end of a list is not handled properly
- we don't actually parse lists, we parse *sets*: members are sorted, and duplicates eliminatedSo let's rewrite it by getting rid of usage of the type "Range" and properly supporting lists of
int64_tanduint64_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.
Quoted 6 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.Please note that no users/callers have to be fixed up. Candiates using
s/Candiates/Candidates/
Quoted 3 lines
visit_type_uint16List()and friends are:
- backends/hostmem.c:host_memory_backend_set_host_nodes() -- Code can deal with dupilcates/unsorted listss/dupilcates/duplicates/
Thanks, both fixed.
Quoted 11 lines
Quoted 10 lines
qapi/string-input-visitor.c static void parse_type_null(Visitor *v, const char *name, QNull **obj, · line 386
Quoted 8 lines
qapi/string-input-visitor.c +1
-0@@ -330,9 +381,10 @@ static void parse_type_null(Visitor *v, const char *name, QNull **obj, {StringInputVisitor *siv =to_siv (v); +assert (siv->lm ==LM_NONE ); *obj =NULL ; - if (!siv->string || siv->string[0]) { +if (siv->string[0 ]) {Why did this condition change?
Quoted 2 lines
As far as I can tell,
siv->stringcan't ever be null. Sticking the change into this patch is perhaps debatable. I'm okay with it.
Yes, we have an assertion when creating the visitor. Do you want me to pull this into a separate patch?
(It made sense under the old patch subject ;) )
Quoted 6 lines
Quoted 2 lines
Reviewed-by Eric Blake <[email protected]>With the commit message improved once more:
Reviewed-by Markus Armbruster <[email protected]>
Thanks!
Signature · 3 lines
Thanks, David / dhildenb
Re: [Qemu-devel] [PATCH v2 6/9] qapi: Rewrite string-input-visitor
Quoted 59 lines · David Hildenbrand
Quoted 44 lines · Markus Armbruster
I think the title should be something like
qapi: Rewrite string-input-visitor's integer and list parsing
because you don't actually rewrite all of it.
Quoted 36 lines · Eric Blake
Quoted 21 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_tare parsed likeint64_t, so biguint64_tvalues are not supported and error messages are misleading
- lists/ranges ofint64_tare accepted although no list is parsed and we should rather report an error
- lists/ranges are preparsed usingint64_t, making it hard to implementuint64_tvalues oruint64_tlists
- types that don't support lists don't bail out
- visiting beyond the end of a list is not handled properly
- we don't actually parse lists, we parse *sets*: members are sorted, and duplicates eliminatedSo let's rewrite it by getting rid of usage of the type "Range" and properly supporting lists of
int64_tanduint64_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.
Quoted 6 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.Please note that no users/callers have to be fixed up. Candiates using
s/Candiates/Candidates/
Quoted 3 lines
visit_type_uint16List()and friends are:
- backends/hostmem.c:host_memory_backend_set_host_nodes() -- Code can deal with dupilcates/unsorted listss/dupilcates/duplicates/
Thanks, both fixed.
Quoted 11 lines
Quoted 10 lines
qapi/string-input-visitor.c static void parse_type_null(Visitor *v, const char *name, QNull **obj, · line 386
Quoted 8 lines
qapi/string-input-visitor.c +1
-0@@ -330,9 +381,10 @@ static void parse_type_null(Visitor *v, const char *name, QNull **obj, {StringInputVisitor *siv =to_siv (v); +assert (siv->lm ==LM_NONE ); *obj =NULL ; - if (!siv->string || siv->string[0]) { +if (siv->string[0 ]) {Why did this condition change?
Quoted 5 lines
Quoted 2 lines
As far as I can tell,
siv->stringcan't ever be null. Sticking the change into this patch is perhaps debatable. I'm okay with it.Yes, we have an assertion when creating the visitor. Do you want me to pull this into a separate patch?
I'm okay with it as is. Up to you.
Quoted 1 line
(It made sense under the old patch subject ;) )
It did :)
Quoted 6 lines
Quoted 5 lines
Quoted 1 line
Reviewed-by Eric Blake <[email protected]>With the commit message improved once more:
Reviewed-by Markus Armbruster <[email protected]>Thanks!
PATCH v2 7/9 test-string-input-visitor: Use virtual walk
In reply to David HildenbrandWe now support virtual walks, so use that instead.
--- tests/test-string-input-visitor.c | 36 +++++++++++-------------------- 1 file changed, 12 insertions(+), 24 deletions(-)
tests/test-string-input-visitor.c +12 -24
@@ -115,7 +115,6 @@ static void test_visitor_in_intList(TestInputVisitorData *data,uint64_t expect5[] = {UINT64_MAX };Error *err =NULL ;int64List *res =NULL ; -int64List *tail;Visitor *v;int64_t val;
@@ -188,39 +187,28 @@ static void test_visitor_in_intList(TestInputVisitorData *data, v =visitor_input_test_init (data,"0,2-3" ); -/* Would be simpler if the visitor genuinely supported virtual walks */ -visit_start_list (v,NULL , (GenericList **)&res,sizeof (*res), - &error_abort); - tail = res; -visit_type_int64 (v,NULL , &tail->value, &error_abort); - g_assert_cmpint(tail->value, ==, 0); - tail = (int64List *)visit_next_list (v, (GenericList *)tail,sizeof (*res)); -g_assert (tail); -visit_type_int64 (v,NULL , &tail->value, &error_abort); - g_assert_cmpint(tail->value, ==, 2); - tail = (int64List *)visit_next_list (v, (GenericList *)tail,sizeof (*res)); -g_assert (tail); +visit_start_list (v,NULL ,NULL ,0 , &error_abort); +visit_type_int64 (v,NULL , &val, &error_abort); + g_assert_cmpint(val, ==, 0); +visit_type_int64 (v,NULL , &val, &error_abort); + g_assert_cmpint(val, ==, 2);visit_check_list (v, &err);error_free_or_abort (&err); -visit_end_list (v, (void **)&res); - -qapi_free_int64List (res); +visit_end_list (v,NULL );/* Visit beyond end of list */ + v =visitor_input_test_init (data,"0" ); -visit_start_list (v,NULL , (GenericList **)&res,sizeof (*res), - &error_abort); - tail = res; -visit_type_int64 (v,NULL , &tail->value, &err); - g_assert_cmpint(tail->value, ==, 0); +visit_start_list (v,NULL ,NULL ,0 , &error_abort); +visit_type_int64 (v,NULL , &val, &err); + g_assert_cmpint(val, ==, 0);visit_type_int64 (v,NULL , &val, &err);error_free_or_abort (&err); -visit_check_list (v, &error_abort); -visit_end_list (v, (void **)&res); -qapi_free_int64List (res); +visit_check_list (v, &error_abort); +visit_end_list (v,NULL ); } static void test_visitor_in_bool(TestInputVisitorData *data,
Signature · 1 line
2.17.2
Re: [Qemu-devel] [PATCH v2 7/9] test-string-input-visitor: Use virtual walk
Quoted 7 lines · David Hildenbrand
We now support virtual walks, so use that instead.
Reviewed-by Markus Armbruster <[email protected]>Signed-off-by David Hildenbrand <[email protected]>--- tests/test-string-input-visitor.c | 36 +++++++++++-------------------- 1 file changed, 12 insertions(+), 24 deletions(-)
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
PATCH v2 8/9 test-string-input-visitor: Split off uint64 list tests
In reply to David HildenbrandBasically 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.
--- tests/test-string-input-visitor.c | 113 ++++++++++++++++++++++++++++-- 1 file changed, 109 insertions(+), 4 deletions(-)
tests/test-string-input-visitor.c +109 -4
@@ -112,7 +112,6 @@ static void test_visitor_in_intList(TestInputVisitorData *data,int64_t expect2[] = {32767 ,-32768 ,-32767 };int64_t expect3[] = {INT64_MIN ,INT64_MAX };int64_t expect4[] = {1 }; -uint64_t expect5[] = {UINT64_MAX };Error *err =NULL ;int64List *res =NULL ;Visitor *v;
@@ -133,9 +132,6 @@ static void test_visitor_in_intList(TestInputVisitorData *data, v =visitor_input_test_init (data,"1-1" );check_ilist (v, expect4,ARRAY_SIZE (expect4)); - v =visitor_input_test_init (data,"18446744073709551615" ); -check_ulist (v, expect5,ARRAY_SIZE (expect5)); -/* Value too large */ v =visitor_input_test_init (data,"9223372036854775808" );
@@ -211,6 +207,113 @@ static void test_visitor_in_intList(TestInputVisitorData *data,visit_end_list (v,NULL ); } +static void test_visitor_in_uintList (TestInputVisitorData *data, +const void *unused) +{ +uint64_t expect1[] = {1 ,2 ,0 ,2 ,3 ,4 ,20 ,5 ,6 ,7 , +8 ,9 ,1 ,2 ,3 ,4 ,5 ,6 ,7 ,8 }; +uint64_t expect2[] = {32767 ,-32768 ,-32767 }; +uint64_t expect3[] = {INT64_MIN ,INT64_MAX }; +uint64_t expect4[] = {1 }; +uint64_t expect5[] = {UINT64_MAX }; +Error *err =NULL ; +uint64List *res =NULL ; +Visitor *v; +uint64_t val; + +/* Valid lists */ + + v =visitor_input_test_init (data,"1,2,0,2-4,20,5-9,1-8" ); +check_ulist (v, expect1,ARRAY_SIZE (expect1)); + + v =visitor_input_test_init (data,"32767,-32768--32767" ); +check_ulist (v, expect2,ARRAY_SIZE (expect2)); + + v =visitor_input_test_init (data, +"-9223372036854775808,9223372036854775807" ); +check_ulist (v, expect3,ARRAY_SIZE (expect3)); + + v =visitor_input_test_init (data,"1-1" ); +check_ulist (v, expect4,ARRAY_SIZE (expect4)); + + v =visitor_input_test_init (data,"18446744073709551615" ); +check_ulist (v, expect5,ARRAY_SIZE (expect5)); + +/* Value too large */ + + v =visitor_input_test_init (data,"18446744073709551616" ); +visit_type_uint64List (v,NULL , &res, &err); +error_free_or_abort (&err); +g_assert (!res); + +/* Value too small */ + + v =visitor_input_test_init (data,"-18446744073709551616" ); +visit_type_uint64List (v,NULL , &res, &err); +error_free_or_abort (&err); +g_assert (!res); + +/* Range not ascending */ + + v =visitor_input_test_init (data,"3-1" ); +visit_type_uint64List (v,NULL , &res, &err); +error_free_or_abort (&err); +g_assert (!res); + + v =visitor_input_test_init (data,"18446744073709551615-0" ); +visit_type_uint64List (v,NULL , &res, &err); +error_free_or_abort (&err); +g_assert (!res); + +/* Range too big (65536 is the limit against DOS attacks) */ + + v =visitor_input_test_init (data,"0-65536" ); +visit_type_uint64List (v,NULL , &res, &err); +error_free_or_abort (&err); +g_assert (!res); + +/* Empty list */ + + v =visitor_input_test_init (data,"" ); +visit_type_uint64List (v,NULL , &res, &error_abort); +g_assert (!res); + +/* Not a list */ + + v =visitor_input_test_init (data,"not an uint list" ); + +visit_type_uint64List (v,NULL , &res, &err); +error_free_or_abort (&err); +g_assert (!res); + +/* Unvisited list tail */ + + v =visitor_input_test_init (data,"0,2-3" ); + +visit_start_list (v,NULL ,NULL ,0 , &error_abort); +visit_type_uint64 (v,NULL , &val, &error_abort); + g_assert_cmpuint(val, ==, 0); +visit_type_uint64 (v,NULL , &val, &error_abort); + g_assert_cmpuint(val, ==, 2); + +visit_check_list (v, &err); +error_free_or_abort (&err); +visit_end_list (v,NULL ); + +/* Visit beyond end of list */ + + v =visitor_input_test_init (data,"0" ); + +visit_start_list (v,NULL ,NULL ,0 , &error_abort); +visit_type_uint64 (v,NULL , &val, &err); + g_assert_cmpuint(val, ==, 0); +visit_type_uint64 (v,NULL , &val, &err); +error_free_or_abort (&err); + +visit_check_list (v, &error_abort); +visit_end_list (v,NULL ); +} +static void test_visitor_in_bool (TestInputVisitorData *data,const void *unused) {
@@ -384,6 +487,8 @@ int main(int argc, char **argv) &in_visitor_data, test_visitor_in_int);input_visitor_test_add ("/string-visitor/input/intList" , &in_visitor_data, test_visitor_in_intList); +input_visitor_test_add ("/string-visitor/input/uintList" , + &in_visitor_data, test_visitor_in_uintList);input_visitor_test_add ("/string-visitor/input/bool" , &in_visitor_data, test_visitor_in_bool); input_visitor_test_add("/string-visitor/input/number",
Signature · 1 line
2.17.2
PATCH v2 9/9 test-string-input-visitor: Add range overflow tests
In reply to David HildenbrandLet's make sure that the range handling code can properly deal with ranges that end at the biggest possible number.
--- tests/test-string-input-visitor.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
tests/test-string-input-visitor.c +10 -0
@@ -112,6 +112,7 @@ static void test_visitor_in_intList(TestInputVisitorData *data,int64_t expect2[] = {32767 ,-32768 ,-32767 };int64_t expect3[] = {INT64_MIN ,INT64_MAX };int64_t expect4[] = {1 }; +int64_t expect5[] = {INT64_MAX -2 ,INT64_MAX -1 ,INT64_MAX };Error *err =NULL ;int64List *res =NULL ;Visitor *v;
@@ -132,6 +133,10 @@ static void test_visitor_in_intList(TestInputVisitorData *data, v =visitor_input_test_init (data,"1-1" );check_ilist (v, expect4,ARRAY_SIZE (expect4)); + v =visitor_input_test_init (data, +"9223372036854775805-9223372036854775807" ); +check_ilist (v, expect5,ARRAY_SIZE (expect5)); +/* Value too large */ v =visitor_input_test_init (data,"9223372036854775808" );
@@ -216,6 +221,7 @@ static void test_visitor_in_uintList(TestInputVisitorData *data,uint64_t expect3[] = {INT64_MIN ,INT64_MAX };uint64_t expect4[] = {1 };uint64_t expect5[] = {UINT64_MAX }; +uint64_t expect6[] = {UINT64_MAX -2 ,UINT64_MAX -1 ,UINT64_MAX };Error *err =NULL ;uint64List *res =NULL ;Visitor *v;
@@ -239,6 +245,10 @@ static void test_visitor_in_uintList(TestInputVisitorData *data, v =visitor_input_test_init (data,"18446744073709551615" );check_ulist (v, expect5,ARRAY_SIZE (expect5)); + v =visitor_input_test_init (data, +"18446744073709551613-18446744073709551615" ); +check_ulist (v, expect6,ARRAY_SIZE (expect6)); +/* Value too large */ v =visitor_input_test_init (data,"18446744073709551616" );
Signature · 1 line
2.17.2