On 9/12/23 20:25, Oliver Webb via Toybox wrote: > This patch fixes a memory leak in ts by making sure it frees the pointers > given by xgetline()
Blah, conventional getline() does a realloc() recycling the same buffer, xgetline returns a string to be freed. I knew that. :P Local variable declarations go at the start of blocks, documented coding style thing for the project: https://landley.net/toybox/design.html#:~:text=Variable%20declarations You can put them at the start of existing { local blocks } if you prefer, but it doesn't help with modern optimizers doing liveness analysis, and I personally tend to group them up top where I can see them. > I knew about this leak for a few days and was planning to submit a patch for > it some time in the future, > but now that ts got promoted I feel like I should submit it sooner rather > then later > > The other fix is that if it's doing -i or -s it uses gmtime() instead of > localtime() rel = !!(toys.optflags&~FLAG_m) is a fancy way of saying (FLAG(i) || FLAG(s)) but now that I look at it I should just change it to be the explicit version and let the optimizer sort it out. (Last I checked it doesn't even when I use | instead of || but eh, more readable beats saving a couple bytes...) Anyway: the "is either of these two flags set" test is already cached in a local "relative time" variable, I'll fix it up... > so the timestamp starts at 00:00:00 instead of something like 20:00:00 for > PDT or 18:00:00 for CST. Sigh, and thus the subtraction does NOT zero it out. (Elliott can confirm how tired all the timezone plumbing makes me.) Also, I have no idea how to test this command. I've caught the busybox ts -i producing "0" offsets with ts -i and sleep 1 in the loop, because a 2ms delay between the left side of the pipe emitting the string and the right side of the pipe getting scheduled to process the string means the _next_ string seems to show up 998 milliseconds later, which rounds down. Which is why I wanted -m in there, but that shows _more_ jitter and thus a less reproducible test. (Unless I do $((math)) on the result, but... what does that prove? What am I _testing_? Interpreting the results on a loaded system is an AI-complete problem, unless I'm just testing "number of digits emitted and where the punctuation goes"?) Applied, with my two tweaks on top of it in a second commit. No regression tests to run, but: $ for i in one two three four five; do echo $i; sleep 1; done | ./ts -im 00:00:00.000 one 00:00:01.004 two 00:00:01.001 three 00:00:01.001 four 00:00:01.002 five Looks ok to me I guess? Thanks, Rob _______________________________________________ Toybox mailing list Toybox@lists.landley.net http://lists.landley.net/listinfo.cgi/toybox-landley.net