Re: Undetected fatal errors from redirected print

[Andrew J. Schorr]

Hi,

On Tue, Nov 30, 2021 at 05:25:15AM -0700, arnold@skeeve.com wrote:
> I like the patch.

Thanks. I have to admit that I have never before tried returning
the value of a function that returns void, but it seems to work as hoped.

> One small request:
> +             /* flush before closing to take advantage of the error
> +              * handling */
> Please do that as one line, or else as two lines using // comments.

I changed it to:

        if ((rp->flag & RED_WRITE) && rp->output.fp)
                /* flush before closing to leverage special error handling */
                efflush(rp->output.fp, "close_redir", rp);

OK? Or do you prefer // for one-liners like that?

> Please add a ChangeLog entry and push to gawk-5.1-stable.

We still need to consider the special fflush handling of stdout and stderr
in io.c:close_io.

Currently, we've got this logic at the end of the function:

        /*
         * Some of the non-Unix os's have problems doing an fclose()
         * on stdout and stderr.  Since we don't really need to close
         * them, we just flush them, and do that across the board.
         */
        *stdio_problem = false;
        /* we don't warn about stdout/stderr if EPIPE, but we do error exit */
        if (fflush(stdout) != 0) {
#ifdef __MINGW32__
                if (errno == 0 || errno == EINVAL)
                        w32_maybe_set_errno();
#endif
                if (errno != EPIPE)
                        warning(_("error writing standard output: %s"), 
strerror(errno));
                else
                        *got_EPIPE = true;

                status++;
                *stdio_problem = true;
        }
        if (fflush(stderr) != 0) {
#ifdef __MINGW32__
                if (errno == 0 || errno == EINVAL)
                        w32_maybe_set_errno();
#endif
                if (errno != EPIPE)
                        warning(_("error writing standard error: %s"), 
strerror(errno));
                else
                        *got_EPIPE = true;

                status++;
                *stdio_problem = true;
        }
        return status;

Miguel proposed adding the equivalent of
        efflush(stdout, "close_io", NULL);
at the top of that section. So that raises a few issues.

1. In efflush, we ignore the return code from efflush, but then check the
status of ferror. Whereas here, we trust the fflush return code. Is there
a reason for the different approach?

2. The w32_maybe_set_errno() logic is the same in wrerror as here, but things
diverge after that. If we call efflush and get an error, it may be fatal,
whereas here, we just set stdio_problem and status and maybe got_EPIPE.

If one looks at the call to close_io in interpret.h, one sees this:

                case Op_atexit:
                {
                        bool stdio_problem = false;
                        bool got_EPIPE = false;

                        /* avoid false source indications */
                        source = NULL;
                        sourceline = 0;
                        (void) nextfile(& curfile, true);       /* close input 
data file */
                        /*
                         * This used to be:
                         *
                         * if (close_io() != 0 && ! exiting && exit_val == 0)
                         *      exit_val = 1;
                         *
                         * Other awks don't care about problems closing open 
files
                         * and pipes, in that it doesn't affect their exit 
status.
                         * So we no longer do either.
                         */
                        (void) close_io(& stdio_problem, & got_EPIPE);
                        /*
                         * However, we do want to exit non-zero if there was a 
problem
                         * with stdout/stderr, so we reinstate a slightly 
different
                         * version of the above:
                         */
                        if (stdio_problem && ! exiting && exit_val == 0)
                                exit_val = 1;

                        close_extensions();

                        if (got_EPIPE)
                                die_via_sigpipe();
                }
                        break;


So I'm not sure what to make of this. It seems there were some specific
policies established for how to handle stdout/stderr flush problems on exit. 
Thoughts?

> > P.S. I'm getting a "make check" ordering error for the iolint test in the
> > master branch, but I imagine that's unrelated:
> 
> It is unrelated.  Upgrade to Ubuntu and it'll go away. :-)

Good. :-)

Regards,
Andy