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

[David Hildenbrand <david@redhat.com>]

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".


>> 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." ?
[...]

> 
>> What about "Less list elements expected"? That at least matches the
>> other error.
> 
> Good enough.  I'd say "fewer", though.

Fine with me!
[...]
>>>> +        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");
>>>> +        }
>>>
>>> I figure I'd make try_parse_int64_list_entry() just parse, and on
>>> success fall through to case LM_INT64_RANGE.  But your solution works,
>>> too.
>>
>> Then we would have to represent even single values as ranges, which is
>> something I'd like to avoid.
> 
> Your artistic license applies.

It actually looks nicer your way (and seems to be less error prone).
Stay tuned!

-- 

Thanks,

David / dhildenb