Re: Undetected fatal errors from redirected print

[arnold]

Hi.

Thanks for the proposed patch. I'd rather see this done cleanly
and go the FSF paperwork route.  I can't apply this as is.

Thanks,

Arnold

"Miguel Pineiro Jr." <mpj@pineiro.cc> wrote:

> On Thu, Nov 25, 2021, at 3:51 AM, arnold@skeeve.com wrote:
> > On Tue, Nov 23, 2021, at 6:24 PM, Andrew J. Schorr wrote:
> >> > The fclose has to do an implicit fflush, of course, before closing the
> >> > file. Are you suggesting we should call fflush prior to fclose and then
> >> > issue an error if fflush fails (but not if fclose fails)?
> >
> > "Miguel Pineiro Jr." <mpj@pineiro.cc> wrote:
> >> Yes, to distinguish a fatal error in the print statement's output
> >> from subsequent, unrelated stream closing errors. Perhaps something
> >> like do_fflush (which understands when a redirection has been declared
> >> nonfatal).
> >
> > I suppose this isn't unreasonable. I will look at it.
> >
> > Thanks,
> >
> > Arnold
>
> It occurred to me, why not allow efwrite to finish what it started. At
> stream close, it can be called to flush its data and handle its errors.
>
> Presently, print statement data may be written by efwrite, close_rp,
> and close_io (stdout), each with a different notion of what should
> be done when things don't go according to plan. It would be ideal
> if all print statement output were handled by a single code path,
> yielding consistent error handling and messaging.
>
> Below you'll find a crude, minimal, proof-of-concept diff. The
> changes compiled cleanly, `make check` passed all tests, and my
> initial report's test script produced the expected output.
>
> I overload efwrite's 'from' parameter. If the name begins with an
> asterisk, it signals that the caller is closing the stream; there's
> no new data to print; and that efwrite must unconditionally flush
> the stream and handle any errors.
>
> Though I appreciate its succinctness, I realize it's a hack. If
> the idea of using efwrite sounds good, but this approach does not,
> there are other options. Perhaps efwrite could gain a parameter. Or,
> efwrite's flushing and error handling code could be extracted into
> a function for use in efwrite, close_redir, and close_io.
>
> To improve this patch, I'd delete the stdout handling code in
> close_io which the call to efwrite makes redundant. I'd also add
> commentary. I did neither to keep the patch as small as possible,
> in hopes of staying below the copyright assignment threshold. If
> someone wants to run with this, I'm sure they can whip it into shape.
>
> This patch is based on the last version before the autotools bump in
> September: a08cc63b
>
> Take care,
> Miguel
>
>
> diff --git a/awk.h b/awk.h
> index cb7c4990..caca5d4c 100644
> --- a/awk.h
> +++ b/awk.h
> @@ -1464,6 +1464,7 @@ extern bool is_identchar(int c);
>  extern NODE *make_regnode(NODETYPE type, NODE *exp);
>  extern bool validate_qualified_name(char *token);
>  /* builtin.c */
> +extern void efwrite(const void *ptr, size_t size, size_t count, FILE *fp, 
> const char *from, struct redirect *rp, bool flush);
>  extern double double_to_int(double d);
>  extern NODE *do_exp(int nargs);
>  extern NODE *do_fflush(int nargs);
> diff --git a/builtin.c b/builtin.c
> index 73a404e6..fe0e5ac4 100644
> --- a/builtin.c
> +++ b/builtin.c
> @@ -98,7 +98,7 @@ fatal(_("attempt to use array `%s' in a scalar context"), 
> array_vname(s1)); \
>  
>  /* efwrite --- like fwrite, but with error checking */
>  
> -static void
> +void
>  efwrite(const void *ptr,
>       size_t size,
>       size_t count,
> @@ -108,6 +108,8 @@ efwrite(const void *ptr,
>       bool flush)
>  {
>       errno = 0;
> +     if (*from == '*')
> +             goto flush;
>       if (rp != NULL) {
>               if (rp->output.gawk_fwrite(ptr, size, count, fp, 
> rp->output.opaque) != count)
>                       goto wrerror;
> @@ -116,6 +118,7 @@ efwrite(const void *ptr,
>       if (flush
>         && ((fp == stdout && output_is_tty)
>             || (rp != NULL && (rp->flag & RED_NOBUF) != 0))) {
> +flush:
>               if (rp != NULL) {
>                       rp->output.gawk_fflush(fp, rp->output.opaque);
>                       if (rp->output.gawk_ferror(fp, rp->output.opaque))
> diff --git a/io.c b/io.c
> index 91c94d9b..eebe8163 100644
> --- a/io.c
> +++ b/io.c
> @@ -1375,6 +1375,8 @@ close_redir(struct redirect *rp, bool exitwarn, 
> two_way_close_type how)
>  
>       if (rp == NULL)
>               return 0;
> +     if ((rp->flag & RED_WRITE) != 0 && rp->output.fp != NULL)
> +             efwrite(NULL, 0, 0, rp->output.fp, "*close_redir", rp, true);
>       if (rp->output.fp == stdout || rp->output.fp == stderr)
>               goto checkwarn;         /* bypass closing, remove from list */
>  
> @@ -1558,6 +1560,7 @@ close_io(bool *stdio_problem, bool *got_EPIPE)
>        * them, we just flush them, and do that across the board.
>        */
>       *stdio_problem = false;
> +     efwrite(NULL, 0, 0, stdout, "*close_io", NULL, true);
>       /* we don't warn about stdout/stderr if EPIPE, but we do error exit */
>       if (fflush(stdout) != 0) {
>  #ifdef __MINGW32__