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

[David Hildenbrand <david@redhat.com>]

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?

(It made sense under the old patch subject ;) )

> 
>> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> With the commit message improved once more:
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> 
Thanks!

-- 

Thanks,

David / dhildenb