<div dir="auto"><div><br><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sun, Feb 24, 2019, 15:32 Rob Landley <<a href="mailto:rob@landley.net">rob@landley.net</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On 2/24/19 4:27 PM, enh via Toybox wrote:<br>
> This is slightly more involved than just calling setlinebuf(3) because<br>
> the code was already implicitly flushing more than once per line thanks<br>
> to all the calls to the 'x' wrappers around stdio.<br>
<br>
Most of them can go, I tend to err on the side of error checking (which requires<br>
a flush to see if it worked) and then pull back later. (The xexit() code checks<br>
at the end, but you want to catch things like "disk full" or a broken pipe<br>
earlier rather than sitting there spinning...)<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">I think all of them should go. That would match GNU and BSD. Remember that you'll get your disk full or broken pipe every time you fill the stdio buffer *or* automatically after each line if you're outputting to a terminal anyway. (And in the uncommon case you really do care, that's exactly what --line-buffered is for.)</div><div dir="auto"><br></div><div dir="auto">Optimizing for the exceptional case where there are large gaps in the output *and* you don't want all the output anyway, at the expense of performance in successful cases seems wrong. </div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> I've un-'x'ed putsl and putsn, given that the only caller doesn't want<br>
> to flush that often, but I left them in lib/ for now.<br>
> <br>
> As usual I haven't added a bogus short option to correspond to<br>
> --line-buffered because I fear that one day (when it's many years<br>
> of existing practice too late to fix either side) we'll end up with<br>
> a collision.<br>
<br>
Many moons ago I taught busybox mount to autodetect when it needed to do<br>
loopback mounts so you didn't need to say -o loop, and util-linux eventually<br>
caught up and did that too. They do sometimes accept sufficiently widespread<br>
external practice. :)<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">(Yeah, but having to deal with building on Linux and macOS, it's the cases where two different implementations choose the same letter for two different things that causes me the most pain. On a fairly regular basis, which is why I'd like to switch the Mac over at some point. But Linux first!)</div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
But yeah, syntax for --longopt without short opt exists for a reason...<br>
<br>
> Tested by looking at `strace -e write` output when running<br>
> `grep fpu_ /proc/cpuinfo > /dev/null` with and without --line-buffered<br>
> (the redirection is necessary because stdout is line buffered by default<br>
> if it's a terminal).<br>
> ---<br>
> lib/lib.c | 17 +++++++++++++++++<br>
> lib/lib.h | 4 ++--<br>
> lib/xwrap.c | 19 -------------------<br>
> toys/posix/grep.c | 31 ++++++++++++++++---------------<br>
> 4 files changed, 35 insertions(+), 36 deletions(-)<br>
<br>
The "x" prefix means "can error_exit()" (patient zero of which was xmalloc()),<br>
the flush was just necessary to do said checking reliably. Your new ones can<br>
still error_exit(), so technically they'd still be x without the flush.<br>
<br>
Of course this raises the "some flush and some don't spectre..." Hmmm. How about:<br>
<br>
diff --git a/lib/xwrap.c b/lib/xwrap.c<br>
index 12016a2..ea3e9f1 100644<br>
--- a/lib/xwrap.c<br>
+++ b/lib/xwrap.c<br>
@@ -143,7 +143,7 @@ char *xmprintf(char *format, ...)<br>
<br>
void xflush(void)<br>
{<br>
- if (fflush(0) || ferror(stdout)) perror_exit("write");<br>
+ if ((!toys.noflush && fflush(0)) || ferror(stdout)) perror_exit("write");<br>
}<br>
<br>
void xprintf(char *format, ...)<br>
diff --git a/toys.h b/toys.h<br>
index 9e49900..40eca86 100644<br>
--- a/toys.h<br>
+++ b/toys.h<br>
@@ -106,6 +106,7 @@ extern struct toy_context {<br>
int signalfd; // and writes signal to this fd, if set<br>
char exitval; // Value error_exit feeds to exit()<br>
char wasroot; // dropped setuid<br>
+ char noflush; // tell xflush() not to (for performance).<br>
<br>
// This is at the end so toy_init() doesn't zero it.<br>
sigjmp_buf *rebound; // siglongjmp here instead of exit when do_rebound<br>
<br>
If that looks like a reasonable approach, I can try to chop this patch down to<br>
what it actually does if you don't want to resubmit. (Note: xexit() already does<br>
the flush itself because error_msg() not error_exit()...)<br></blockquote></div></div><div dir="auto"><br></div><div dir="auto">To me this is an even worse idea than the current x* behavior. At least I've learned that all the x* output functions cause an unnecessary flush because we've fixed this over-zealous flushing in several places already. (And those are just the ones we've caught.)</div><div dir="auto"><br></div><div dir="auto">I think part of the reason we've had trouble with this is because it's implicit. Adding *another* piece of magic seems like step in the wrong direction, especially for something (early flushing) that seems like it's usually a mistake anyway.</div><div dir="auto"><br></div><div dir="auto">(This specific suggestion seems doubly dangerous because it *breaks* xflush --- the one *explicit* way to cause a flush. I'm all in favor of xflush. It's how we fixed things like top to reduce flicker, for example.)</div><div dir="auto"><br></div><div dir="auto">Personally I'm not 100% convinced that putsn and putsnl pull their weight at all... I'd happily use printf("%s", s) for the former given that neither reader nor writer needs to think about what that means, and it composes better with any neighboring printf or putchar or whatever. Likewise "%*.*s", which is widely used and a generally useful thing for the reader to learn. (But this is because I think implicit early flushes are a bug, not a feature, so I need to persuade you of that first 😀 I wonder if the numbers are amenable to going through and trying to work out which if any are genuinely helpful...)</div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Rob<br>
_______________________________________________<br>
Toybox mailing list<br>
<a href="mailto:Toybox@lists.landley.net" target="_blank" rel="noreferrer">Toybox@lists.landley.net</a><br>
<a href="http://lists.landley.net/listinfo.cgi/toybox-landley.net" rel="noreferrer noreferrer" target="_blank">http://lists.landley.net/listinfo.cgi/toybox-landley.net</a><br>
</blockquote></div></div></div>