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

[David Hildenbrand <david@redhat.com>]

On 05.11.18 16:37, Markus Armbruster wrote:
> David Hildenbrand <david@redhat.com> writes:
> 
>> On 31.10.18 18:55, Markus Armbruster wrote:
>>> David Hildenbrand <david@redhat.com> writes:
>>>
>>>> On 31.10.18 15:40, Markus Armbruster wrote:
>>>>> David Hildenbrand <david@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.

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?)

-- 

Thanks,

David / dhildenb