GNU bug report logs - #64540
[PATCH] od: exit out on failure to write to stdout

Previous Next

Full log

Message #31 received at 64540-done <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Bernhard Voelker <mail <at> bernhard-voelker.de>,
 Paul Eggert <eggert <at> cs.ucla.edu>, Josef Bacik <josef <at> toxicpanda.com>,
 64540-done <at> debbugs.gnu.org
Subject: Re: bug#64540: [PATCH] od: exit out on failure to write to stdout
Date: Mon, 17 Jul 2023 11:31:39 +0100

On 17/07/2023 07:35, Bernhard Voelker wrote:
> On 7/15/23 23:08, Pádraig Brady wrote:
>> The attached patch-set addresses two classes of issue:
>> 1. Doubled error messages upon write errors
>> 2. Continued processing upon write errors  (the orig problem reported).
> 
> Nice work!
> 
> Patch 1:
> 
>   > --- /dev/null
>   > +++ b/tests/misc/write-errors.sh
>   > @@ -0,0 +1,70 @@
> 
>   > +if ! test -w /dev/full || ! test -c /dev/full; then
>   > +  skip_ 'Need /dev/full'
> 
> Other tests use a slightly different skip message:
>     skip_ '/dev/full is required'
> 
> I'd favor the latter, so that we don't miss it once we'd like
> to factor it out to a require_dev_full in init.cfg.
> 
> Re. patch 002:
> 
>   >     all: avoid repeated diagnostic upon write error
>   >
>   >     * cfg.mk (sc_some_programs_must_avoid_exit_failure): Adjust to
>   >     avoid false positive.
>   >     (sc_prohibit_exit_write_error): A new syntax check to prohibit
>   >     open coding error(..., "write error"); instead directiing to use...
> 
> s/ii/i/
> 
>   > --- a/src/system.h
>   > +++ b/src/system.h
>   > @@ -762,6 +762,14 @@ The following directory is part of the cycle:\n  %s\n"), \
>   >      }					\
>   >    while (0)
>   >
>   > +static inline void
>   > +write_error (void)
>   > +{
>   > +  fflush (stdout);    /* Ensure nothing buffered that might induce an error. */
>   > +  clearerr (stdout);  /* To avoid extraneous diagnostic from close_stdout.  */
>   > +  error (EXIT_FAILURE, errno, _("write error"));
>   > +}
> 
> Hmm, fflush could theoretically change errno, couldn't it?
> In that case, error() would give a wrong diagnostic.
> FWIW: `man clearerr` states that this function doesn't touch errno.

Yes good call.
Current version is:

/* exit with a _single_ "write error" diagnostic.  */
static inline void
write_error (void)
{
  int saved_errno = errno;
  fflush (stdout);    /* Ensure nothing buffered that might induce an error. */
  clearerr (stdout);  /* To avoid extraneous diagnostic from close_stdout.  */
  error (EXIT_FAILURE, saved_errno, _("write error"));
}

I'll push later.

Marking this as done.

Thanks for the review.

Pádraig

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.