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

[Rob Landley]

On 2/24/19 10:56 PM, enh wrote:
> 
> 
> On Sun, Feb 24, 2019, 15:32 Rob Landley <rob at landley.net
> <mailto:rob at landley.net>> wrote:
> 
>     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 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.)
> 
> 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. 

Hmmm... You have a point.

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

I've been moving xprintf() to printf() in several places myself.

I don't want a toybox command to produce endless output without checking for
error ("sed blah bigfile.img | head" shouldn't run a long time, for example).
And debian keeps breaking "tee" so:

  (while true; do echo -n .; sleep 1; done) | tee /dev/null

Won't produce output for 4k and then suddenly produces multiple screens full.
(They fix it, they break it, they fix it again...)

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

I think what I usually want to do is check for errors but not flush. (There
isn't a standard write() buffer size, an officially policy for _when_ it gets
flushed out, and nothing like a timeout, which is why I was flushing it in the
first place, but that turned out to be worse...)

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

To be honest I was calling xflush() from so many places because it was an easy
way to get the if (ferror) perror_exit("write"); Possibly what I want is an
xferror() that xflush() can call...

> Personally I'm not 100% convinced that putsn and putsnl pull their weight at
> all...

Neither am I, but it's annoying there _isn't_ one in the standard library. (And
puts() adding a newline but fputs() _not_ doing so is just irritating.)

That said, you can fwrite() a gigabyte piped through cat and it all goes out in
one gulp even with glibc. (Yes, I wrote a test program.) This is why I have a
writeall() for filehandles but not an fwriteall(): because I _think_ libc can be
trusted to retry short writes? (Something's doing it. When I worry about data
integrity I do it myself with filehandles and writeall() anyway, although
get_line() in lib.c is a pre-posix-2008 relic I need to clean out. It's on the
todo list...)

And trusting fwrite() is only for squishy values of "trust" because posix won't
come out and SAY it retries short writes. In fact posix says that being
interrupted by a signal makes it return a short write which I believe is _not_
what Linux does with things like SIGSTOP/CONT and SIGCHLD, those the OS will
retry internally, and yes I know this because I broke it:

  http://lkml.iu.edu/hypermail/linux/kernel/0503.3/1756.html

And then the implementation changed (splice went in circa
https://lwn.net/Articles/178199/ and the pipe plumbing got a complete rewrite)
which had subtle behavior diferences... *shrug* The usual.

(If you're wondering why I get a bit paranoid about this stuff in places, it's
scar tissue and old trauma. I have another todo item to investigate the
sigaction(SA_RESTART) behavior's impact on this stuff and what the defaults can
be trusted to _be_ these days, but again: todo list runneth over and this ain't
my $DAYJOB...)

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

Except too many times it's _not_ composed...

$ grep '("%s",' toys/*/*.c | wc
     52     252    3250

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

Sigh, we should probably make the helpful ones explicit and remove the rest.

I'll add a cleaup pass to the todo heap...

Rob