[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: |
Tue, 6 Nov 2018 10:19:52 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 |
On 05.11.18 21:43, Markus Armbruster wrote:
> David Hildenbrand <address@hidden> writes:
>
>> On 05.11.18 16:37, Markus Armbruster wrote:
>>> David Hildenbrand <address@hidden> writes:
>>>
>>>> On 31.10.18 18:55, Markus Armbruster wrote:
>>>>> David Hildenbrand <address@hidden> writes:
>>>>>
>>>>>> On 31.10.18 15:40, Markus Armbruster wrote:
>>>>>>> David Hildenbrand <address@hidden> 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.
I guess I am starting to understand how this magic is supposed to work.
Always parsing and processing one list token at a time
("size"/"uint64_t" or "range of such") should be the way to go. And if
nobody requested to parse a list (start_list()), also ranges should not
be allowed. This pre-parsing of the whole list and unconditional use of
ranges should go.
Ranges are still ugly but needed as far as I can understand (as a
shortcut for lengthy lists).
Am I on the right track?
--
Thanks,
David / dhildenb
- Re: [Qemu-devel] [PATCH v3 1/7] qapi: use qemu_strtoi64() in parse_str, Markus Armbruster, 2018/11/05
- Re: [Qemu-devel] [PATCH v3 1/7] qapi: use qemu_strtoi64() in parse_str, David Hildenbrand, 2018/11/05
- Re: [Qemu-devel] [PATCH v3 1/7] qapi: use qemu_strtoi64() in parse_str, Eric Blake, 2018/11/05
- Re: [Qemu-devel] [PATCH v3 1/7] qapi: use qemu_strtoi64() in parse_str, Markus Armbruster, 2018/11/05
- Re: [Qemu-devel] [PATCH v3 1/7] qapi: use qemu_strtoi64() in parse_str,
David Hildenbrand <=
- Re: [Qemu-devel] [PATCH v3 1/7] qapi: use qemu_strtoi64() in parse_str, Markus Armbruster, 2018/11/07
- Re: [Qemu-devel] [PATCH v3 1/7] qapi: use qemu_strtoi64() in parse_str, David Hildenbrand, 2018/11/07
- Re: [Qemu-devel] [PATCH v3 1/7] qapi: use qemu_strtoi64() in parse_str, Markus Armbruster, 2018/11/08
- Re: [Qemu-devel] [PATCH v3 1/7] qapi: use qemu_strtoi64() in parse_str, David Hildenbrand, 2018/11/08
- Re: [Qemu-devel] [PATCH v3 1/7] qapi: use qemu_strtoi64() in parse_str, Markus Armbruster, 2018/11/08
- Re: [Qemu-devel] [PATCH v3 1/7] qapi: use qemu_strtoi64() in parse_str, David Hildenbrand, 2018/11/08
- Re: [Qemu-devel] [PATCH v3 1/7] qapi: use qemu_strtoi64() in parse_str, Markus Armbruster, 2018/11/08
- Re: [Qemu-devel] [PATCH v3 1/7] qapi: use qemu_strtoi64() in parse_str, David Hildenbrand, 2018/11/08
- Re: [Qemu-devel] [PATCH v3 1/7] qapi: use qemu_strtoi64() in parse_str, Eric Blake, 2018/11/08
- Re: [Qemu-devel] [PATCH v3 1/7] qapi: use qemu_strtoi64() in parse_str, David Hildenbrand, 2018/11/08