Re: [PATCH] head,tail: consistently diagnose write errors (original) (raw)

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

On 01/31/2014 12:30 AM, Bernhard Voelker wrote:

On 01/30/2014 01:23 AM, Pádraig Brady wrote: >> Unfortunately, the atexit code now produces a second error diagnostic. >> It doesn't hurt, but it looks a bit ugly. > > We discussed that foible previously, and thought it not worth > adding the extra complexity to avoid in general. > > http://lists.gnu.org/archive/html/coreutils/2011-05/msg00061.html > > Though in this specific case we're using wrapper functions > instead of fwrite() so we can easily add the clearerr() there. > > Updated patch attached.

Thanks ... also for the link to the other thread.

The changes in the sources look rather good to me - a minor nit follows below (regarding the diagnostic message).

I'm not sure about the test:

> diff --git a/tests/misc/head-write-error.sh b/tests/misc/head-write-error.sh > new file mode 100755 > index 0000000..b749760 > --- /dev/null > +++ b/tests/misc/head-write-error.sh > @@ -0,0 +1,47 @@ > +#!/bin/sh > +# Ensure we diagnose and not continue writing to > +# the output if we get a write error. > +

If I read it correctly, the test claims to verify that head exits early on write errors ...

> +# Memory is bounded in these cases > +for item in lines bytes; do > + for N in 0 1; do > + # pipe case > + yes | timeout 10s head --$item=-$N > /dev/full 2> err && fail=1 > + test $? = 124 && fail=1 > + test -s err || fail=1 > + rm err > + > + # seekable case > + timeout 10s head --$item=-$N bigseek > /dev/full 2> err && fail=1 > + test $? = 124 && fail=1 > + test -s err || fail=1 > + done > +done

... while the above just seems to verify that head exits non-Zero on a write error - well, head already did before.

Right, the pipe case above does check for the early exit, but substituting /usr/bin/head into the seek case above doesn't induce a failure as it should.

The difference with the patch is that for the pipe case head now prints the detailed diagnostic

- /usr/bin/head: write error + src/head: write error: No space left on device

and in the seekable case only outputs 1 line instead of 2:

- /usr/bin/head: error writing ‘standard input’: No space left on device - /usr/bin/head: write error + src/head: write error: No space left on device

(Interestingly, in the latter case there was an error in the old message: we don't write standard input.)

Cool another problem fixed with this patch.

So to say, the test doesn't do exactly what it says. It would kind of do if it would verify that the output only contains the message from xwritestdout and not that from the atexit code, something like:

echo 'head: write error: No space left on device' > exp compare exp err || fail=1

OK I've added that (less the check on the possibly varying "No space left on device" portion).

Finally, looking at the changes in the error messages above: I'm missing the file name in the new error message, i.e. stdout. The user might be confused and wonder where writing failed. I'd add this to the message again.

Done and pushed.

thanks for the very thorough review!

Pádraig.

• Re: [PATCH] head,tail: consistently diagnose write errors,Pádraig Brady <=

• Prev by Date:Re: [PATCH] ls: adding --zero/-z option, including tests
• Next by Date:Suppress filenames in wc output
• Previous by thread:[PATCH] ls: adding --zero/-z option, including tests
• Next by thread:Suppress filenames in wc output
• Index(es):
  • Date
  • Thread