On Thu, Apr 11, 2019 at 4:44 PM Rob Landley <[email protected]> wrote:
>
> On 4/11/19 12:18 PM, enh wrote:
> > Ping for direction here? I'm happy to implement whichever way you want to
> > go.
> >
> > I think we had:
> >
> > 1. Don't implicitly flush in all the x* routines, add any missing explicit
> > flushes (of which I expect there will be very few).
> >
> > 2. Don't use the x* routines as much.
>
> I recently did:
>
> commit 8f882370be150d80969a1910c20b5d223d084b76
> Author: Rob Landley <[email protected]>
> Date: Tue Apr 2 15:03:32 2019 -0500
>
> Have xflush() only flush stdout (that's all it checks errors on),
> and tweak a couple comments.
>
> but removing even that much flushing unless explicitly called is probably
> fine.
cool... i've attached a new patch that applies cleanly on ToT.
> The point of xprintf() and friends is if you have
>
> yes | head
>
> then "yes" should end when stdout closes. It doesn't have to notice
> _imediately_, but generating potentially unbounded data into a pipe shouldn't
> keep going an arbitrarily long time, and we disable sigpipe because that's
> bugs
> waiting to happen.
>
> FILE * output flushes itself eventually, and then we'll notice the error. I
> think musl's FILE buffer is like 256 bytes and glibc was what, 4k? We don't
> need
> to flush that.
(glibc is 8KiB, fwiw, but i've never seen a libc use a bigger buffer than that.)
> The stdio code flushes when the output includes \n so xputc('\n') is already a
> flush inside libc anyway. I should grep or xprintf/xputc/xputs uses and see if
> there's an obvious one missing. (The password: prompt is the only one that
> comes
> to mind, toysh will need one too...)
>
> Oh, the other thing I have in this bucket is eliminating get_line(), which
> can't
> be done sanely without input blocking (and is largely why FILE * exists; if
> you
> can't push data back into a filehandle then you either read a byte at a time
> or
> remember the overshoot; get_line() reads a byte at a time and performance
> sucks
> and it makes strace output unreadable).
>
> I initially wrote it because posix-2001 didn't have getline() yet (it was a
> glibc extension I didn't want to rely on), but posix-2008 added it, so...
yeah, i've been able to clean up a *lot* of code thanks to getline().
(though anyone who thinks fgets() is evil really needs to look at its
truly evil twin fgetln()...)
> Rob
From 3d6cc6ab83fa7f078a43d72c6d71fbc047b97c37 Mon Sep 17 00:00:00 2001
From: Elliott Hughes <[email protected]>
Date: Thu, 11 Apr 2019 17:24:50 -0700
Subject: [PATCH] grep: add --line-buffered and fix regular buffering.
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.
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.
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(-)
diff --git a/lib/lib.c b/lib/lib.c
index c4d871c2..9e1347cc 100644
--- a/lib/lib.c
+++ b/lib/lib.c
@@ -135,6 +135,23 @@ ssize_t writeall(int fd, void *buf, size_t len)
return count;
}
+// Put string with length (does not append newline)
+void putsl(char *s, int len)
+{
+ int out;
+
+ while (len != (out = fwrite(s, 1, len, stdout))) {
+ if (out<1) perror_exit("write");
+ len -= out;
+ s += out;
+ }
+}
+
+void putsn(char *s)
+{
+ putsl(s, strlen(s));
+}
+
// skip this many bytes of input. Return 0 for success, >0 means this much
// left after input skipped.
off_t lskip(int fd, off_t offset)
diff --git a/lib/lib.h b/lib/lib.h
index 0f15484e..4eef025a 100644
--- a/lib/lib.h
+++ b/lib/lib.h
@@ -125,8 +125,6 @@ char *xstrdup(char *s);
void *xmemdup(void *s, long len);
char *xmprintf(char *format, ...) printf_format;
void xprintf(char *format, ...) printf_format;
-void xputsl(char *s, int len);
-void xputsn(char *s);
void xputs(char *s);
void xputc(char c);
void xflush(void);
@@ -195,6 +193,8 @@ void error_msg_raw(char *msg);
void perror_msg_raw(char *msg);
void error_exit_raw(char *msg);
void perror_exit_raw(char *msg);
+void putsl(char *s, int len);
+void putsn(char *s);
ssize_t readall(int fd, void *buf, size_t len);
ssize_t writeall(int fd, void *buf, size_t len);
off_t lskip(int fd, off_t offset);
diff --git a/lib/xwrap.c b/lib/xwrap.c
index 12170ada..f056c1e1 100644
--- a/lib/xwrap.c
+++ b/lib/xwrap.c
@@ -154,25 +154,6 @@ void xprintf(char *format, ...)
xflush();
}
-// Put string with length (does not append newline)
-void xputsl(char *s, int len)
-{
- int out;
-
- while (len != (out = fwrite(s, 1, len, stdout))) {
- if (out<1) perror_exit("write");
- len -= out;
- s += out;
- }
- xflush();
-}
-
-// xputs with no newline
-void xputsn(char *s)
-{
- xputsl(s, strlen(s));
-}
-
// Write string to stdout with newline, flushing and checking for errors
void xputs(char *s)
{
diff --git a/toys/posix/grep.c b/toys/posix/grep.c
index 9239614f..264aae22 100644
--- a/toys/posix/grep.c
+++ b/toys/posix/grep.c
@@ -10,7 +10,7 @@
* echo hello | grep -f </dev/null
*
-USE_GREP(NEWTOY(grep, "(color):;S(exclude)*M(include)*ZzEFHIab(byte-offset)h(no-filename)ino(only-matching)rsvwcl(files-with-matches)q(quiet)(silent)e*f*C#B#A#m#x[!wx][!EFw]", TOYFLAG_BIN|TOYFLAG_ARGFAIL(2)))
+USE_GREP(NEWTOY(grep, "(color):;(line-buffered);S(exclude)*M(include)*ZzEFHIab(byte-offset)h(no-filename)ino(only-matching)rsvwcl(files-with-matches)q(quiet)(silent)e*f*C#B#A#m#x[!wx][!EFw]", TOYFLAG_BIN|TOYFLAG_ARGFAIL(2)))
USE_EGREP(OLDTOY(egrep, grep, TOYFLAG_BIN|TOYFLAG_ARGFAIL(2)))
USE_FGREP(OLDTOY(fgrep, grep, TOYFLAG_BIN|TOYFLAG_ARGFAIL(2)))
@@ -95,15 +95,14 @@ static void outline(char *line, char dash, char *name, long lcount, long bcount,
if (!trim && FLAG(o)) return;
if (name && FLAG(H)) printf("%s%s%s%c", TT.purple, name, TT.cyan, dash);
if (FLAG(c)) {
- printf("%s%ld", TT.grey, lcount);
- xputc(TT.outdelim);
+ printf("%s%ld%c", TT.grey, lcount, TT.outdelim);
} else if (lcount && FLAG(n)) numdash(lcount, dash);
if (bcount && FLAG(b)) numdash(bcount-1, dash);
if (line) {
- if (FLAG(color)) xputsn(FLAG(o) ? TT.red : TT.grey);
+ if (FLAG(color)) putsn(FLAG(o) ? TT.red : TT.grey);
// support embedded NUL bytes in output
- xputsl(line, trim);
- xputc(TT.outdelim);
+ putsl(line, trim);
+ putchar(TT.outdelim);
}
}
@@ -255,7 +254,7 @@ static void do_grep(int fd, char *name)
// At least one line we didn't print since match while -ABC active
if (bars) {
- xputs(bars);
+ puts(bars);
bars = 0;
}
matched++;
@@ -265,7 +264,7 @@ static void do_grep(int fd, char *name)
xexit();
}
if (FLAG(l)) {
- xprintf("%s%c", name, TT.outdelim);
+ printf("%s%c", name, TT.outdelim);
free(line);
fclose(file);
return;
@@ -292,10 +291,10 @@ static void do_grep(int fd, char *name)
if (matched==1)
outline(FLAG(color) ? 0 : line, ':', name, lcount, bcount, ulen);
if (FLAG(color)) {
- xputsn(TT.grey);
- if (mm->rm_so) xputsl(line, mm->rm_so);
- xputsn(TT.red);
- xputsl(line+mm->rm_so, mm->rm_eo-mm->rm_so);
+ putsn(TT.grey);
+ if (mm->rm_so) putsl(line, mm->rm_so);
+ putsn(TT.red);
+ putsl(line+mm->rm_so, mm->rm_eo-mm->rm_so);
}
if (TT.A) after = TT.A+1;
@@ -311,9 +310,9 @@ static void do_grep(int fd, char *name)
if (matched) {
// Finish off pending line color fragment.
if (FLAG(color) && !FLAG(o)) {
- xputsn(TT.grey);
- if (ulen > start-line) xputsl(start, ulen-(start-line));
- xputc(TT.outdelim);
+ putsn(TT.grey);
+ if (ulen > start-line) putsl(start, ulen-(start-line));
+ putchar(TT.outdelim);
}
mcount++;
} else {
@@ -485,6 +484,8 @@ void grep_main(void)
if (!FLAG(h) && toys.optc>1) toys.optflags |= FLAG_H;
+ if (FLAG(line_buffered)) setlinebuf(stdout);
+
if (FLAG(s)) {
close(2);
xopen_stdio("/dev/null", O_RDWR);
--
2.21.0.392.gf8f6787159e-goog
_______________________________________________
Toybox mailing list
[email protected]
http://lists.landley.net/listinfo.cgi/toybox-landley.net
