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