On 15.11.18 10:48, Markus Armbruster wrote:
> David Hildenbrand <da...@redhat.com> writes:
>
>> On 14.11.18 18:38, Markus Armbruster wrote:
>>> David Hildenbrand <da...@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
