On 07.11.18 16:29, Markus Armbruster wrote:
> David Hildenbrand <da...@redhat.com> writes:
>
>> On 05.11.18 21:43, Markus Armbruster wrote:
>>> David Hildenbrand <da...@redhat.com> writes:
>>>
>>>> On 05.11.18 16:37, Markus Armbruster wrote:
>>>>> David Hildenbrand <da...@redhat.com> writes:
>>>>>
>>>>>> On 31.10.18 18:55, Markus Armbruster wrote:
>>>>>>> David Hildenbrand <da...@redhat.com> writes:
>>>>>>>
>>>>>>>> On 31.10.18 15:40, Markus Armbruster wrote:
>>>>>>>>> David Hildenbrand <da...@redhat.com> writes:
>>>>>>>>>
>>>>>>>>>> The qemu api claims to be easier to use, and the resulting code
>>>>>>>>>> seems to
>>>>>>>>>> agree.
>>>>>>> [...]
>>>>>>>>>> @@ -60,9 +61,7 @@ static int parse_str(StringInputVisitor *siv,
>>>>>>>>>> const char *name, Error **errp)
>>>>>>>>>> }
>>>>>>>>>>
>>>>>>>>>> do {
>>>>>>>>>> - errno = 0;
>>>>>>>>>> - start = strtoll(str, &endptr, 0);
>>>>>>>>>> - if (errno == 0 && endptr > str) {
>>>>>>>>>> + if (!qemu_strtoi64(str, &endptr, 0, &start)) {
>>>>>>>>>> if (*endptr == '\0') {
>>>>>>>>>> cur = g_malloc0(sizeof(*cur));
>>>>>>>>>> range_set_bounds(cur, start, start);
>>>>>>>>>> @@ -71,11 +70,7 @@ static int parse_str(StringInputVisitor *siv,
>>>>>>>>>> const char *name, Error **errp)
>>>>>>>>>> str = NULL;
>>>>>>>>>> } else if (*endptr == '-') {
>>>>>>>>>> str = endptr + 1;
>>>>>>>>>> - errno = 0;
>>>>>>>>>> - end = strtoll(str, &endptr, 0);
>>>>>>>>>> - if (errno == 0 && endptr > str && start <= end &&
>>>>>>>>>> - (start > INT64_MAX - 65536 ||
>>>>>>>>>> - end < start + 65536)) {
>>>>>>>>>> + if (!qemu_strtoi64(str, &endptr, 0, &end) && start
>>>>>>>>>> < end) {
>>>>>>>>>
>>>>>>>>> You deleted (start > INT64_MAX - 65536 || end < start + 65536). Can
>>>>>>>>> you
>>>>>>>>> explain that to me? I'm feeling particularly dense today...
>>>>>>>>
>>>>>>>> qemu_strtoi64 performs all different kinds of error handling completely
>>>>>>>> internally. This old code here was an attempt to filter out -EWHATEVER
>>>>>>>> from the response. No longer needed as errors and the actual value are
>>>>>>>> reported via different ways.
>>>>>>>
>>>>>>> I understand why errno == 0 && endptr > str go away. They also do in
>>>>>>> the previous hunk.
>>>>>>>
>>>>>>> The deletion of (start > INT64_MAX - 65536 || end < start + 65536) is
>>>>>>> unobvious. What does it do before the patch?
>>>>>>>
>>>>>>> The condition goes back to commit 659268ffbff, which predates my watch
>>>>>>> as maintainer. Its commit message is of no particular help. Its code
>>>>>>> is... allright, the less I say about that, the better.
>>>>>>>
>>>>>>> We're parsing a range here. We already parsed its lower bound into
>>>>>>> @start (and guarded against errors), and its upper bound into @end (and
>>>>>>> guarded against errors).
>>>>>>>
>>>>>>> If the condition you delete is false, we goto error. So the condition
>>>>>>> is about range validity. I figure it's an attempt to require valid
>>>>>>> ranges to be no "wider" than 65535. The second part end < start + 65536
>>>>>>> checks exactly that, except shit happens when start + 65536 overflows.
>>>>>>> The first part attempts to guard against that, but
>>>>>>>
>>>>>>> (1) INT64_MAX is *wrong*, because we compute in long long, and
>>>>>>>
>>>>>>> (2) it rejects even small ranges like INT64_MAX - 2 .. INT64_MAX - 1.
>>>>>>>
>>>>>>> WTF?!?
>>>>>>>
>>>>>>> Unless I'm mistaken, the condition is not about handling any of the
>>>>>>> errors that qemu_strtoi64() handles for us.
>>>>>>>
>>>>>>> The easiest way for you out of this morass is probably to keep the
>>>>>>> condition exactly as it was, then use the "my patch doesn't make things
>>>>>>> any worse" get-out-of-jail-free card.
>>>>>>>
>>>>>>
>>>>>> Looking at the code in qapi/string-output-visitor.c related to range and
>>>>>> list handling I feel like using the get-out-of-jail-free card to get out
>>>>>> of qapi code now :) Too much magic in that code and too little time for
>>>>>> me to understand it all.
>>>>>>
>>>>>> Thanks for your time and review anyway. My time is better invested in
>>>>>> other parts of QEMU. I will drop both patches from this series.
>>>>>
>>>>> Understand.
>>>>>
>>>>> When I first looked at the ranges stuff in the string input visitor, I
>>>>> felt the urge to clean it up, then sat on my hands until it passed.
>>>>>
>>>>> The rest is reasonable once you understand how it works. The learning
>>>>> curve is less than pleasant, though.
>>>>>
>>>>
>>>> Maybe I'll pick this up again when I have more time to invest.
>>>>
>>>> The general concept
>>>>
>>>> 1. of having an input visitor that is able to parse different types
>>>> (expected by e.g. a property) sounds sane to me.
>>>>
>>>> 2. of having a list of *something*, assuming it is int64_t, and assuming
>>>> it is to be parsed into a list of ranges sounds completely broken to me.
>>>
>>> Starting point: the string visitors can only do scalars. We have a need
>>> for lists of integers (see below). The general solution would be
>>> generalizing these visitors to lists (and maybe objects while we're at
>>> it). YAGNI. So we put in a quick hack that can do just lists of
>>> integers.
>>>
>>> Except applying YAGNI to stable interfaces is *bonkers*.
>>>
>>>> I was not even able to find an example QEMU comand line for 2. Is this
>>>> maybe some very old code that nobody actually uses anymore? (who uses
>>>> list of ranges?)
>>>
>>> The one I remember offhand is -numa node,cpus=..., but that one's
>>> actually parsed with the options visitor. Which is even hairier, but at
>>> least competently coded.
>>>
>>> To find uses, we need to follow the uses of the string visitors.
>>>
>>> Of the callers of string_input_visitor_new(),
>>> object_property_get_uint16List() is the only one that deals with lists.
>>> It's used by query_memdev() for property host-nodes.
>>>
>>> The callers of string_output_visitor_new() lead to MigrationInfo member
>>> postcopy-vcpu-blocktime, and Memdev member host-nodes again.
>>>
>>> Searching the QAPI schema for lists of integers coughs up a few more
>>> candidates: NumaNodeOptions member cpus (covered above), RxFilterInfo
>>> member vlan-table (unrelated, as far as I can tell), RockerOfDpaGroup
>>> (likewise), block latency histogram stuff (likewise).
>>>
>>
>> As Eric pointed out, tests/test-string-input-visitor.c actually tests
>> for range support in test_visitor_in_intList.
>>
>> I might be completely wrong, but actually the string input visitor
>> should not pre-parse stuff into a list of ranges, but instead parse on
>> request (parse_type_...) and advance in the logical list on "next_list".
>> And we should parse ranges *only* if we are expecting a list. Because a
>> range is simply a short variant of a list. A straight parse_type_uint64
>> should bail out if we haven't started a list.
>
> Yes, parse_type_int64() & friends should simply parse the appropriate
> integer, *except* when we're working on a list. Then they should return
> the next integer, which may or may not require parsing.
>
> Say, input is "1-3,5", and the visitor is called like
>
> visit_start_list()
> visit_next_list() more input, returns "there's more"
> visit_type_int() parses "1-3,", buffers 2-3, returns 1
> visit_next_list() buffer not empty, returns "there's more"
> visit_type_int() unbuffers and returns 2
> visit_next_list() buffer not empty, returns "there's more"
> visit_type_int() unbuffers and returns 3
> visit_next_list() more input, returns "there's more"
> visit_type_int() parses "5", returns 5
> visit_next_list() buffer empty and no more input, returns "done"
> visit_end_list()
>
Would it be valid to do something like this (skipping elements without a
proper visit_type_int)
visit_start_list();
visit_next_list(); more input, returns "there's more"
visit_next_list(); parses "1-3,", buffers 2-3, skips over 1
visit_type_int(); returns 2
...
Or mixing types
visit_start_list();
visit_next_list();
visit_type_int64();
visit_next_list();
visit_type_uint64();
IOW, can I assume that after every visit_next_list(), visit_type_X is
called, and that X remains the same for one pass over the list?
Also, I assume it is supposed to work like this
visit_start_list();
visit_next_list(); more input, returns "there's more"
visit_type_int(); returns 1 (parses 1-3, buffers 1-3)
visit_type_int(); returns 1
visit_type_int(); returns 1
visit_next_list(); more input, unbuffers 1
visit_type_int(); returns 2
So unbuffering actually happens on visit_next_list()?
--
Thanks,
David / dhildenb
