David Hildenbrand <da...@redhat.com> writes:
> On 20.11.18 21:58, Markus Armbruster wrote:
>> I think the title should be something like
>>
>> qapi: Rewrite string-input-visitor's integer and list parsing
>>
>> because you don't actually rewrite all of it.
>>
>> Eric Blake <ebl...@redhat.com> writes:
>>
>>> On 11/20/18 3:25 AM, David Hildenbrand wrote:
>>>> 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
>>>> - visiting beyond the end of a list is not handled properly
>>>> - we don't actually parse lists, we parse *sets*: members are sorted,
>>>> and duplicates eliminated
>>>>
>>>> So let's rewrite it by getting rid of usage of the type "Range" and
>>>> properly supporting lists of int64_t and uint64_t (including ranges of
>>>> both types), fixing the above mentioned issues.
>>>>
>>>> Lists of other types are not supported and will properly report an
>>>> error. Virtual walks are now supported.
>>>>
>>>> Tests have to be fixed up:
>>>> - Two BUGs were hardcoded that are fixed now
>>>> - The string-input-visitor now actually returns a parsed list and not
>>>> an ordered set.
>>>>
>>>> Please note that no users/callers have to be fixed up. Candiates using
>>>
>>> s/Candiates/Candidates/
>>>
>>>> visit_type_uint16List() and friends are:
>>>> - backends/hostmem.c:host_memory_backend_set_host_nodes()
>>>> -- Code can deal with dupilcates/unsorted lists
>>>
>>> s/dupilcates/duplicates/
>
> Thanks, both fixed.
>
>>
>>>> @@ -330,9 +381,10 @@ static void parse_type_null(Visitor *v, const char
>>>> *name, QNull **obj,
>>>> {
>>>> StringInputVisitor *siv = to_siv(v);
>>>> + assert(siv->lm == LM_NONE);
>>>> *obj = NULL;
>>>> - if (!siv->string || siv->string[0]) {
>>>> + if (siv->string[0]) {
>>>
>>> Why did this condition change?
>>
>> As far as I can tell, siv->string can't ever be null. Sticking the
>> change into this patch is perhaps debatable. I'm okay with it.
>
> Yes, we have an assertion when creating the visitor. Do you want me to
> pull this into a separate patch?
I'm okay with it as is. Up to you.
> (It made sense under the old patch subject ;) )
It did :)
>>> Reviewed-by: Eric Blake <ebl...@redhat.com>
>>
>> With the commit message improved once more:
>> Reviewed-by: Markus Armbruster <arm...@redhat.com>
>>
> Thanks!
