Re: [Qemu-devel] [PATCH v2 6/9] qapi: Rewrite string-input-visitor

[Markus Armbruster <armbru@redhat.com>]

David Hildenbrand <david@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 <eblake@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 <eblake@redhat.com>
>> 
>> With the commit message improved once more:
>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>> 
> Thanks!