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

[Pádraig Brady]

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.