[Toybox] [PATCH] grep: add --line-buffered and fix regular buffering.

[Rob Landley]

On 2/24/19 4:27 PM, enh via Toybox wrote:
> This is slightly more involved than just calling setlinebuf(3) because
> the code was already implicitly flushing more than once per line thanks
> to all the calls to the 'x' wrappers around stdio.

Most of them can go, I tend to err on the side of error checking (which requires
a flush to see if it worked) and then pull back later. (The xexit() code checks
at the end, but you want to catch things like "disk full" or a broken pipe
earlier rather than sitting there spinning...)

> I've un-'x'ed putsl and putsn, given that the only caller doesn't want
> to flush that often, but I left them in lib/ for now.
> 
> As usual I haven't added a bogus short option to correspond to
> --line-buffered because I fear that one day (when it's many years
> of existing practice too late to fix either side) we'll end up with
> a collision.

Many moons ago I taught busybox mount to autodetect when it needed to do
loopback mounts so you didn't need to say -o loop, and util-linux eventually
caught up and did that too. They do sometimes accept sufficiently widespread
external practice. :)

But yeah, syntax for --longopt without short opt exists for a reason...

> Tested by looking at `strace -e write` output when running
> `grep fpu_ /proc/cpuinfo > /dev/null` with and without --line-buffered
> (the redirection is necessary because stdout is line buffered by default
> if it's a terminal).
> ---
>  lib/lib.c         | 17 +++++++++++++++++
>  lib/lib.h         |  4 ++--
>  lib/xwrap.c       | 19 -------------------
>  toys/posix/grep.c | 31 ++++++++++++++++---------------
>  4 files changed, 35 insertions(+), 36 deletions(-)

The "x" prefix means "can error_exit()" (patient zero of which was xmalloc()),
the flush was just necessary to do said checking reliably. Your new ones can
still error_exit(), so technically they'd still be x without the flush.

Of course this raises the "some flush and some don't spectre..." Hmmm.  How about:

diff --git a/lib/xwrap.c b/lib/xwrap.c
index 12016a2..ea3e9f1 100644
--- a/lib/xwrap.c
+++ b/lib/xwrap.c
@@ -143,7 +143,7 @@ char *xmprintf(char *format, ...)

 void xflush(void)
 {
-  if (fflush(0) || ferror(stdout)) perror_exit("write");
+  if ((!toys.noflush && fflush(0)) || ferror(stdout)) perror_exit("write");
 }

 void xprintf(char *format, ...)
diff --git a/toys.h b/toys.h
index 9e49900..40eca86 100644
--- a/toys.h
+++ b/toys.h
@@ -106,6 +106,7 @@ extern struct toy_context {
   int signalfd;            // and writes signal to this fd, if set
   char exitval;            // Value error_exit feeds to exit()
   char wasroot;            // dropped setuid
+  char noflush;            // tell xflush() not to (for performance).

   // This is at the end so toy_init() doesn't zero it.
   sigjmp_buf *rebound;     // siglongjmp here instead of exit when do_rebound

If that looks like a reasonable approach, I can try to chop this patch down to
what it actually does if you don't want to resubmit. (Note: xexit() already does
the flush itself because error_msg() not error_exit()...)

Rob