| Message ID | 20170812120510.28750-52-phil@nwl.cc |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | stephen hemminger |
| Headers | show |
On 08/12/2017 02:05 PM, Phil Sutter wrote: > This is merely to silence the compiler warning. If write to stderr > failed, assume that printing an error message will fail as well so don't > even try. > > Signed-off-by: Phil Sutter <phil@nwl.cc> > --- > lib/bpf.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/lib/bpf.c b/lib/bpf.c > index 1dcb261dc915f..825e071cea572 100644 > --- a/lib/bpf.c > +++ b/lib/bpf.c > @@ -591,7 +591,8 @@ int bpf_trace_pipe(void) > > ret = read(fd, buff, sizeof(buff) - 1); > if (ret > 0) { > - write(2, buff, ret); > + if (write(STDERR_FILENO, buff, ret) != ret) > + return -1; Quite unlikely to fail, but we should probably bark loudly here instead of just returning -1. Perhaps assert() would suit better. > fflush(stderr); > } > } >
On Mon, Aug 14, 2017 at 11:17:39AM +0200, Daniel Borkmann wrote: > On 08/12/2017 02:05 PM, Phil Sutter wrote: > > This is merely to silence the compiler warning. If write to stderr > > failed, assume that printing an error message will fail as well so don't > > even try. > > > > Signed-off-by: Phil Sutter <phil@nwl.cc> > > --- > > lib/bpf.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/lib/bpf.c b/lib/bpf.c > > index 1dcb261dc915f..825e071cea572 100644 > > --- a/lib/bpf.c > > +++ b/lib/bpf.c > > @@ -591,7 +591,8 @@ int bpf_trace_pipe(void) > > > > ret = read(fd, buff, sizeof(buff) - 1); > > if (ret > 0) { > > - write(2, buff, ret); > > + if (write(STDERR_FILENO, buff, ret) != ret) > > + return -1; > > Quite unlikely to fail, but we should probably bark loudly > here instead of just returning -1. Perhaps assert() would > suit better. Well, according to assert(3), it will print to stderr before aborting the program. So if writing STDERR_FILENO failed, I guess it won't provide much more detail to the user, either. If bpf_trace_pipe() returns non-zero, parse_bpf() prints an error message to stderr and returns -1. Ultimately tc will return non-zero. With stderr unfit for writing into, I doubt there's anything left we could do besides that. But I really think we shouldn't make such a fuss about it - writing to stderr either always works or we're in trouble everywhere. This patch was merely to shut gcc up, so no need to waste much energy on a scenario which won't happen anyway. Thanks, Phil
On 08/14/2017 07:25 PM, Phil Sutter wrote: [...] > But I really think we shouldn't make such a fuss about it - writing to > stderr either always works or we're in trouble everywhere. This patch > was merely to shut gcc up, so no need to waste much energy on a scenario > which won't happen anyway. Yup, fair enough, makes sense. Acked-by: Daniel Borkmann <daniel@iogearbox.net>
From: Phil Sutter > Sent: 12 August 2017 13:05 > This is merely to silence the compiler warning. If write to stderr > failed, assume that printing an error message will fail as well so don't > even try. > > Signed-off-by: Phil Sutter <phil@nwl.cc> > --- > lib/bpf.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/lib/bpf.c b/lib/bpf.c > index 1dcb261dc915f..825e071cea572 100644 > --- a/lib/bpf.c > +++ b/lib/bpf.c > @@ -591,7 +591,8 @@ int bpf_trace_pipe(void) > > ret = read(fd, buff, sizeof(buff) - 1); > if (ret > 0) { > - write(2, buff, ret); > + if (write(STDERR_FILENO, buff, ret) != ret) > + return -1; > fflush(stderr); > } WTF is this code doing anyway? write() is a system call, fflush() writes out any data buffered in the stdio stream. If there was anything buffered you'd want to output it earlier. Otherwise if it is going to use fflush() it should be using fwrite(). I presume the function is allowed to write to stderr - since in general library functions shouldn't assume fd 0/1/2 or stdin/out/err are valid. There is a lot of code out there that does close(0); close(1); close(2); but leaves stdout/err valid. Call printf() instead of sprint() and eventually 10k of data gets written somewhere rather unexpected. If it is a copy loop, what is wrong with the last byte of buff[]. It is valid for write() to return a partial length - the code should probably loop until all the data is accepted (or error). David
On 08/15/2017 02:31 PM, David Laight wrote: [...] > WTF is this code doing anyway? > write() is a system call, fflush() writes out any data buffered in the > stdio stream. > If there was anything buffered you'd want to output it earlier. > Otherwise if it is going to use fflush() it should be using fwrite(). > > I presume the function is allowed to write to stderr - since in general > library functions shouldn't assume fd 0/1/2 or stdin/out/err are valid. > There is a lot of code out there that does close(0); close(1); close(2); > but leaves stdout/err valid. Call printf() instead of sprint() and eventually > 10k of data gets written somewhere rather unexpected. > > If it is a copy loop, what is wrong with the last byte of buff[]. > It is valid for write() to return a partial length - the code should > probably loop until all the data is accepted (or error). Just send a patch if you really care; would have probably been faster than typing up your email. ;) Thank you!
diff --git a/lib/bpf.c b/lib/bpf.c index 1dcb261dc915f..825e071cea572 100644 --- a/lib/bpf.c +++ b/lib/bpf.c @@ -591,7 +591,8 @@ int bpf_trace_pipe(void) ret = read(fd, buff, sizeof(buff) - 1); if (ret > 0) { - write(2, buff, ret); + if (write(STDERR_FILENO, buff, ret) != ret) + return -1; fflush(stderr); } }
This is merely to silence the compiler warning. If write to stderr failed, assume that printing an error message will fail as well so don't even try. Signed-off-by: Phil Sutter <phil@nwl.cc> --- lib/bpf.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)