Re: Undetected fatal errors from redirected print

[Miguel Pineiro Jr.]

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__