| Message ID | 20181120092542.13102-6-david@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qapi: rewrite string-input-visitor | expand |
On 11/20/18 3:25 AM, David Hildenbrand wrote: > Test that very big/small values are not accepted and that ranges with > only one element work. Also test that ranges are ascending and cannot > have more than 65536 elements. > > Rename expect4 to expect5, as we will be moving that to a separate ulist > test after the rework. > > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > tests/test-string-input-visitor.c | 41 +++++++++++++++++++++++++++++-- > 1 file changed, 39 insertions(+), 2 deletions(-) > Reviewed-by: Eric Blake <eblake@redhat.com>
On 11/20/18 11:06 AM, Eric Blake wrote: > On 11/20/18 3:25 AM, David Hildenbrand wrote: >> Test that very big/small values are not accepted and that ranges with >> only one element work. Also test that ranges are ascending and cannot >> have more than 65536 elements. >> >> Rename expect4 to expect5, as we will be moving that to a separate ulist >> test after the rework. >> >> Signed-off-by: David Hildenbrand <david@redhat.com> >> --- >> tests/test-string-input-visitor.c | 41 +++++++++++++++++++++++++++++-- >> 1 file changed, 39 insertions(+), 2 deletions(-) >> > > Reviewed-by: Eric Blake <eblake@redhat.com> Do we also want to test garbage strings like: "1-" (incomplete range) "1X-2" (garbage suffix on first element) "1-2X" (garbage suffix on second element) and/or "-2--1" (valid range of signed integers) "-1--2" (questionable whether this is valid for the two largest unsigned integers)
On 11/20/18 11:20 AM, Eric Blake wrote: > On 11/20/18 11:06 AM, Eric Blake wrote: >> On 11/20/18 3:25 AM, David Hildenbrand wrote: >>> Test that very big/small values are not accepted and that ranges with >>> only one element work. Also test that ranges are ascending and cannot >>> have more than 65536 elements. >>> >>> Rename expect4 to expect5, as we will be moving that to a separate ulist >>> test after the rework. >>> >>> Signed-off-by: David Hildenbrand <david@redhat.com> >>> --- >>> tests/test-string-input-visitor.c | 41 +++++++++++++++++++++++++++++-- >>> 1 file changed, 39 insertions(+), 2 deletions(-) >>> >> >> Reviewed-by: Eric Blake <eblake@redhat.com> > > Do we also want to test garbage strings like: > > "1-" (incomplete range) > "1X-2" (garbage suffix on first element) > "1-2X" (garbage suffix on second element) > > and/or > > "-2--1" (valid range of signed integers) > "-1--2" (questionable whether this is valid for the two largest unsigned > integers) Or even " 1- 2" (we permit leading whitespace for plain integers - do we also permit it in both sides of a range)? Also, if we permit whitespace after the range '-', should we permit it beforehand? These sorts of questions may be fine in followup patches.
Eric Blake <eblake@redhat.com> writes: > On 11/20/18 11:20 AM, Eric Blake wrote: >> On 11/20/18 11:06 AM, Eric Blake wrote: >>> On 11/20/18 3:25 AM, David Hildenbrand wrote: >>>> Test that very big/small values are not accepted and that ranges with >>>> only one element work. Also test that ranges are ascending and cannot >>>> have more than 65536 elements. >>>> >>>> Rename expect4 to expect5, as we will be moving that to a separate ulist >>>> test after the rework. >>>> >>>> Signed-off-by: David Hildenbrand <david@redhat.com> >>>> --- >>>> tests/test-string-input-visitor.c | 41 +++++++++++++++++++++++++++++-- >>>> 1 file changed, 39 insertions(+), 2 deletions(-) >>>> >>> >>> Reviewed-by: Eric Blake <eblake@redhat.com> >> >> Do we also want to test garbage strings like: >> >> "1-" (incomplete range) >> "1X-2" (garbage suffix on first element) >> "1-2X" (garbage suffix on second element) >> >> and/or >> >> "-2--1" (valid range of signed integers) >> "-1--2" (questionable whether this is valid for the two largest >> unsigned integers) > > Or even " 1- 2" (we permit leading whitespace for plain integers - do > we also permit it in both sides of a range)? Also, if we permit > whitespace after the range '-', should we permit it beforehand? For what it's worth, the tests for integers (as opposed to ranges) aren't that thorough, either. test-cutils.c already provides cover for them, but not for ranges. > These sorts of questions may be fine in followup patches. David is doing me a favor with these patches. The tests feel good enough, I don't want to hold these patches hostage to extort more. Reviewed-by: Markus Armbruster <armbru@redhat.com>
On 20.11.18 18:26, Eric Blake wrote: > On 11/20/18 11:20 AM, Eric Blake wrote: >> On 11/20/18 11:06 AM, Eric Blake wrote: >>> On 11/20/18 3:25 AM, David Hildenbrand wrote: >>>> Test that very big/small values are not accepted and that ranges with >>>> only one element work. Also test that ranges are ascending and cannot >>>> have more than 65536 elements. >>>> >>>> Rename expect4 to expect5, as we will be moving that to a separate ulist >>>> test after the rework. >>>> >>>> Signed-off-by: David Hildenbrand <david@redhat.com> >>>> --- >>>> tests/test-string-input-visitor.c | 41 +++++++++++++++++++++++++++++-- >>>> 1 file changed, 39 insertions(+), 2 deletions(-) >>>> >>> >>> Reviewed-by: Eric Blake <eblake@redhat.com> >> >> Do we also want to test garbage strings like: >> >> "1-" (incomplete range) >> "1X-2" (garbage suffix on first element) >> "1-2X" (garbage suffix on second element) >> >> and/or >> >> "-2--1" (valid range of signed integers) >> "-1--2" (questionable whether this is valid for the two largest unsigned >> integers) > > Or even " 1- 2" (we permit leading whitespace for plain integers - do we > also permit it in both sides of a range)? Also, if we permit whitespace > after the range '-', should we permit it beforehand? > > These sorts of questions may be fine in followup patches. > As always, you can never cover all cases during tests :) While these things surely make sense, I will not add them right now. As you said, we can do that later. Taking about spaces: I think spaces before the "-" are not supported before/after the rewrite (I remember that strto.* does not skip over them). Spaces after the space should be covered by strto* automatically (strto.* skips over leading spaces). Thanks!
David Hildenbrand <david@redhat.com> writes: > On 20.11.18 18:26, Eric Blake wrote: >> On 11/20/18 11:20 AM, Eric Blake wrote: >>> On 11/20/18 11:06 AM, Eric Blake wrote: >>>> On 11/20/18 3:25 AM, David Hildenbrand wrote: >>>>> Test that very big/small values are not accepted and that ranges with >>>>> only one element work. Also test that ranges are ascending and cannot >>>>> have more than 65536 elements. >>>>> >>>>> Rename expect4 to expect5, as we will be moving that to a separate ulist >>>>> test after the rework. >>>>> >>>>> Signed-off-by: David Hildenbrand <david@redhat.com> >>>>> --- >>>>> tests/test-string-input-visitor.c | 41 +++++++++++++++++++++++++++++-- >>>>> 1 file changed, 39 insertions(+), 2 deletions(-) >>>>> >>>> >>>> Reviewed-by: Eric Blake <eblake@redhat.com> >>> >>> Do we also want to test garbage strings like: >>> >>> "1-" (incomplete range) >>> "1X-2" (garbage suffix on first element) >>> "1-2X" (garbage suffix on second element) >>> >>> and/or >>> >>> "-2--1" (valid range of signed integers) >>> "-1--2" (questionable whether this is valid for the two largest unsigned >>> integers) >> >> Or even " 1- 2" (we permit leading whitespace for plain integers - do we >> also permit it in both sides of a range)? Also, if we permit whitespace >> after the range '-', should we permit it beforehand? >> >> These sorts of questions may be fine in followup patches. >> > > As always, you can never cover all cases during tests :) > > While these things surely make sense, I will not add them right now. As > you said, we can do that later. > > > Taking about spaces: > > I think spaces before the "-" are not supported before/after the rewrite > (I remember that strto.* does not skip over them). Spaces after the > space should be covered by strto* automatically (strto.* skips over > leading spaces). qemu_strtol() & friends ignore leading whitespace, just like strtol() does. Trailing whitespace is handled like any other trailing characters: hard error when endptr is null, else make *endptr point to it. I'd expect whitespace to be accepted before either number of a range, but not between the first number and the '-'. Perhaps rejecting whitespace there would be the cleaner interface, but then we get to discuss backward compatibility, and I go "thanks, but no, thanks."
diff --git a/tests/test-string-input-visitor.c b/tests/test-string-input-visitor.c index 1efba06948..8ee0d1b284 100644 --- a/tests/test-string-input-visitor.c +++ b/tests/test-string-input-visitor.c @@ -121,7 +121,8 @@ static void test_visitor_in_intList(TestInputVisitorData *data, int64_t expect1[] = { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 20 }; int64_t expect2[] = { 32767, -32768, -32767 }; int64_t expect3[] = { INT64_MAX, INT64_MIN }; - uint64_t expect4[] = { UINT64_MAX }; + int64_t expect4[] = { 1 }; + uint64_t expect5[] = { UINT64_MAX }; Error *err = NULL; int64List *res = NULL; int64List *tail; @@ -140,8 +141,44 @@ static void test_visitor_in_intList(TestInputVisitorData *data, "-9223372036854775808,9223372036854775807"); check_ilist(v, expect3, ARRAY_SIZE(expect3)); + v = visitor_input_test_init(data, "1-1"); + check_ilist(v, expect4, ARRAY_SIZE(expect4)); + v = visitor_input_test_init(data, "18446744073709551615"); - check_ulist(v, expect4, ARRAY_SIZE(expect4)); + check_ulist(v, expect5, ARRAY_SIZE(expect5)); + + /* Value too large */ + + v = visitor_input_test_init(data, "9223372036854775808"); + visit_type_int64List(v, NULL, &res, &err); + error_free_or_abort(&err); + g_assert(!res); + + /* Value too small */ + + v = visitor_input_test_init(data, "-9223372036854775809"); + visit_type_int64List(v, NULL, &res, &err); + error_free_or_abort(&err); + g_assert(!res); + + /* Range not ascending */ + + v = visitor_input_test_init(data, "3-1"); + visit_type_int64List(v, NULL, &res, &err); + error_free_or_abort(&err); + g_assert(!res); + + v = visitor_input_test_init(data, "9223372036854775807-0"); + visit_type_int64List(v, NULL, &res, &err); + error_free_or_abort(&err); + g_assert(!res); + + /* Range too big (65536 is the limit against DOS attacks) */ + + v = visitor_input_test_init(data, "0-65536"); + visit_type_int64List(v, NULL, &res, &err); + error_free_or_abort(&err); + g_assert(!res); /* Empty list */
Test that very big/small values are not accepted and that ranges with only one element work. Also test that ranges are ascending and cannot have more than 65536 elements. Rename expect4 to expect5, as we will be moving that to a separate ulist test after the rework. Signed-off-by: David Hildenbrand <david@redhat.com> --- tests/test-string-input-visitor.c | 41 +++++++++++++++++++++++++++++-- 1 file changed, 39 insertions(+), 2 deletions(-)