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