Re: [RFC] closeout redundant diagnostic

[Jim Meyering]

Pádraig Brady wrote:
> On 17/05/11 14:48, Eric Blake wrote:
>> On 05/17/2011 05:11 AM, Pádraig Brady wrote:
>>> I was just testing a framebuffer device here and
>>> noticed that there were 2 errors printed by `tr`
>>>
>>> $ tr '\0' $(($RANDOM % 256)) </dev/zero >/dev/full
>>> tr: write error: No space left on device
>>> tr: write error
>>>
>>> I was then wondering it there was some other
>>> error with the device rather than just filling it.
>>> But the latter error is just closeout() noticing
>>> the original write error, and diagnosing just
>>> in case the original writer did not.

Thanks for bring this up.

To get a feel for what's involved, I ran this:

    git grep -A3 -E 'if \((fwrite|f?puts|f?putc(har)?|f?printf) \('

which shows there are at least 24 instances of code like this in coreutils.
Obviously it doesn't find any that are not right after "if (", like
this putchar use from yes.c:

        if (fputs (argv[i], stdout) == EOF
            || putchar (i == argc - 1 ? '\n' : ' ') == EOF)
          {
            error (0, errno, _("standard output"));
            exit (EXIT_FAILURE);

Running this spots a few more:
  git grep -B2 -A2 -E '(fwrite|f?puts|f?putc(har)?|f?printf) \(.*!='
but of course, that won't match things like "if (printf (...))".

>>> So 3 options.
>>> 1. leave as is
>>> 2. assume all writers will diagnose,
>>> so don't diagnose previous errors in closeout()
>>> 3. make it configurable (with close_stdout_set_ignore_previous())
>>
>> 4. any client that diagnoses its own errors before calling closeout()
>> should also call clearerr()
>
> I thought that might be too invasive,
> but it's a definitely possibility.
>
>> 5. fix the clients to quit diagnosing an error when close_stdout will
>> also diagnose it
>
> I thought of that, but then the specific errno would be lost.

(5) is not desirable

(4) would be an improvement if it can be sufficiently non-invasive.
For example, if the target code is like this, from sort.c,
where it's easy to change the die function:

                     if (fputc (wc, fp) == EOF)
                       die (_("write failed"), output_file);

In that case, simply calling clearerr from die would solve
the two cases like this in sort.c (the other uses fwrite).
paste has a similar function named write_error.

However, in tr, the two offenders look like this:

                   if (fwrite (io_buf, 1, nr, stdout) != nr)
                     error (EXIT_FAILURE, errno, _("write error"));

In that case, adding curly braces just to insert the clearerr call
does not seem worthwhile, but writing a wrapper function just for
this purpose would be ok:

                   if (fwrite (io_buf, 1, nr, stdout) != nr)
                     die (EXIT_FAILURE, errno, _("write error"));

Though it'd have to be a varargs function, possibly defined in system.h.
(alternatively a simple one-argument function, and maybe one more that
also takes a file name, for those that print more than "write error")
It would always use errno, and could call exit with the exit_failure global
value from the exitfail module.

difficulties:
  - finding all instances where this new function should be used
  - ensuring that clearing the error indicator is "ok".
      It's almost certainly fine whenever the diagnostic-printing
      code ends up exiting right away, but if it merely warns and
      continues execution, we'd have to be sure nothing depends
      on the error indicator we've just reset.
  - adding test cases for the affected code.  It can be very hard to
      exercise error-path code.

All of this to avoid a second, less-informative diagnostic.

I can see the appeal of "(1): leave as is" ;-)
but if someone is inclined to fix these, I'd welcome the improvements.