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. 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