qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v3 1/7] qapi: use qemu_strtoi64() in parse_str


From: David Hildenbrand
Subject: Re: [Qemu-devel] [PATCH v3 1/7] qapi: use qemu_strtoi64() in parse_str
Date: Thu, 8 Nov 2018 15:49:07 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1

On 08.11.18 15:42, Eric Blake wrote:
> On 11/8/18 7:05 AM, David Hildenbrand wrote:
> 
>>>> Thanks for the excessive answer! I think that should be enough to come
>>>> up with a RFC of a *rewrite* of the string input visitor :)
>>>
>>> You're welcome!  I love great questions, they make me *think*.
>>>
>>> Besides, if something's worth doing, it's probably worth overdoing ;)
>>>
>>
>> I found some more ugliness, looking at the tests. I am not sure the test
>> is correct here.
>>
>> test_visitor_in_intList():
>>
>> v = visitor_input_test_init(data, "1,2,0,2-4,20,5-9,1-8");
>>
>> -> we expect { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 20 }, implying that the
>> visitor actually does sorting + duplicate elimination. Now I consider
>> this is wrong. We are parsing a list of integers, not an ordered set.
>>
>> What's your take on this?
> 
> The test matches the current reality (yes, the string input visitor 
> currently sorts and deduplicates) - but you are also right that in 
> general JSON [] lists are supposed to preserve order. I suspect our use 
> of -numa parsing relies on the result being sorted - but I would not be 
> opposed to a rewrite that dumbs down the string visitor to provide an 
> unsorted list, and move the sorting into a post-pass belonging to the 
> -numa code.  Updating the tests to match as part of a rewrite is thus 
> okay, if we have the justification that we audited all other users to 
> see that they either don't care or that we can move the sorting out of 
> the list walker and into the callers that do care.
> 

-numa is parsed via the object parser and not the string input parser as
far as I understood Markus. I think only "host-nodes" for memdev would
be relevant right now, and as far as I can see, that shouldn't break
(have to take a closer look).

-- 

Thanks,

David / dhildenb



reply via email to

[Prev in Thread] Current Thread [Next in Thread]