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?
(It made sense under the old patch subject ;) )
>
>> Reviewed-by: Eric Blake <ebl...@redhat.com>
>
> With the commit message improved once more:
> Reviewed-by: Markus Armbruster <arm...@redhat.com>
>
Thanks!
--
Thanks,
David / dhildenb
