Re: [Qemu-devel] [PATCH RFC 3/6] qapi: rewrite string-input-visitor

[Markus Armbruster <armbru@redhat.com>]

David Hildenbrand <david@redhat.com> writes:

> On 15.11.18 10:48, Markus Armbruster wrote:
>> David Hildenbrand <david@redhat.com> writes:
>> 
>>> On 14.11.18 18:38, Markus Armbruster wrote:
>>>> David Hildenbrand <david@redhat.com> writes:
>>>>
>>>>> The input visitor has some problems right now, especially
>>>>> - unsigned type "Range" is used to process signed ranges, resulting in
>>>>>   inconsistent behavior and ugly/magical code
>>>>> - uint64_t are parsed like int64_t, so big uint64_t values are not
>>>>>   supported and error messages are misleading
>>>>> - lists/ranges of int64_t are accepted although no list is parsed and
>>>>>   we should rather report an error
>>>>> - lists/ranges are preparsed using int64_t, making it hard to
>>>>>   implement uint64_t values or uint64_t lists
>>>>> - types that don't support lists don't bail out
>>>>
>>>> Known weirdness: empty list is invalid (test-string-input-visitor.c
>>>> demonstates).  Your patch doesn't change that (or else it would update
>>>> the test).  Should it be changed?
>>>>
>>>
>>> I don't change the test, so the old behavior still works.
>>> (empty string -> error)
>> 
>> Understand.  Design question: should it remain an error?  Feel free to
>> declare the question out of scope for this patch.
>
> I think I was confused, let me retry to explain.
>
> Empty lists actually don't result in an error. Calling start_list() on
> an empty string works just fine.
>
> However
> - check_list() will result in "Fewer list elements expected"
> - visit_type_.*int64() will result in "Fewer list elements expected"
> - next_list() will result in NULL
>
> I guess that is the intended behavior. E.g. the test does
>
> v = visitor_input_test_init(data, "");
> visit_type_uint64List(v, NULL, &res, &error_abort);
> g_assert(!res);
>
> So there won't be any error as the first "visit_next_list()" will
> properly indicate "NULL".

You know, I was confused, too :)  I looked at commit 3d089cea0d3, which
added the test case:

+    /* Empty list is invalid (weird) */
+
+    v = visitor_input_test_init(data, "");
+    visit_type_int64List(v, NULL, &res, &err);
+    error_free_or_abort(&err);

I missed regression fix commit d2788227c61:

-    /* Empty list is invalid (weird) */
+    /* Empty list */
 
     v = visitor_input_test_init(data, "");
-    visit_type_int64List(v, NULL, &res, &err);
-    error_free_or_abort(&err);
+    visit_type_int64List(v, NULL, &res, &error_abort);
+    g_assert(!res);

So the test actually demonstrates empty lists work fine before and after
your patch.

>>> Added "Only flat lists of integers (int64/uint64) are supported."
>> 
>> Hmm, do lists of narrower integer types also work?  I guess they do: the
>> narrower visit_type_*int*() call v->type_*int64() via
>> visit_type_*intN().
>> 
>> Lists of type size are expressly excluded, in parse_type_size() below.
>> That's okay, we can lift the restriction when it gets in the way.
>
> Right, we can make that clearer
>
> "Only flat lists of integers (except type "size") are supported." ?
> [...]

Sold!

[...]