Re: [PATCH] head,tail: consistently diagnose write errors

[Bernhard Voelker]

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.

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

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

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.

Have a nice day,
Berny

BTW: /usr/bin/head above is v8.21.