Re: [Qemu-devel] [PATCH v3 1/7] qapi: use qemu_strtoi64() in parse_str

[David Hildenbrand]

On 08.11.18 15:36, Markus Armbruster wrote:
> David Hildenbrand <address@hidden> writes:
> 
>> I found some more ugliness, looking at the tests. I am not sure the test
>> is correct here.
>>
>> test_visitor_in_intList():
>>
>> v = visitor_input_test_init(data, "1,2,0,2-4,20,5-9,1-8");
>>
>> -> we expect { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 20 }, implying that the
>> visitor actually does sorting + duplicate elimination. Now I consider
>> this is wrong. We are parsing a list of integers, not an ordered set.
>>
>> What's your take on this?
> 
> I think you're right.  Visitors are tied to QAPI, and QAPI does *lists*,
> not sets.  Lists are more general than sets.
> 
> I figure what drove development of this code was a need for sets, so
> sets got implemented.  Review fail.
> 
> If the visitor does lists, whatever needs sets can convert the lists to
> sets.
> 
> I'd advise against evolving the current code towards sanity.  Instead,
> rewrite, and rely on tests to avoid unwanted changes in behavior.
> 

With the current rewrite I have, I can parse uint64 and int64 lists. The
other types will bail out if lists are used right now.

The changes that have to be done to the tests are:

diff --git a/tests/test-string-input-visitor.c
b/tests/test-string-input-visitor.c
index 88e0e1aa9a..5d2d707e80 100644
--- a/tests/test-string-input-visitor.c
+++ b/tests/test-string-input-visitor.c
@@ -92,16 +92,6 @@ static void check_ulist(Visitor *v, uint64_t
*expected, size_t n)
     uint64List *tail;
     int i;

-    /* BUG: unsigned numbers above INT64_MAX don't work */
-    for (i = 0; i < n; i++) {
-        if (expected[i] > INT64_MAX) {
-            Error *err = NULL;
-            visit_type_uint64List(v, NULL, &res, &err);
-            error_free_or_abort(&err);
-            return;
-        }
-    }
-
     visit_type_uint64List(v, NULL, &res, &error_abort);
     tail = res;
     for (i = 0; i < n; i++) {
@@ -118,9 +108,9 @@ 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 };
     uint64_t expect4[] = { UINT64_MAX };
     Error *err = NULL;
     int64List *res = NULL;
@@ -189,7 +179,7 @@ static void
test_visitor_in_intList(TestInputVisitorData *data,
     visit_type_int64(v, NULL, &tail->value, &err);
     g_assert_cmpint(tail->value, ==, 0);
     visit_type_int64(v, NULL, &val, &err);
-    g_assert_cmpint(val, ==, 1); /* BUG */
+    error_free_or_abort(&err);
     visit_check_list(v, &error_abort);
     visit_end_list(v, (void **)&res);


Basically fixing two bugs (yey, let's make our tests pass by hardcoding
BUG behavior, so the actually fixed code will break them)

And converting two assumptions about ordered sets into unordered lists.

(using unsigned ranges for handling signed ranges is completely broken,
as can also be seen via the removed "Note:")

-- 

Thanks,

David / dhildenb