pg_regress: Save errno in emit_tap_output_v() and switch to %m

emit_tap_output_v() includes some fprintf() calls for some output
related to the TAP protocol, that may clobber errno and break %m.  This
commit makes the logging of pg_regress smarter by saving errno before
restoring it in vfprintf() where the input strings are used, removing
the need for strerror().  All logs are switched to %m rather than
strerror(), shaving some code.

This was not a problem until now as pg_regress.c did not use %m, but the
change is simple enough that we have no reason to not support this
placeholder, and that will avoid future mistakes if new logs that
include %m are added.

Author: Dagfinn Ilmari Mannsåker
Reviewed-by: Peter Eisentraunt, Michael Paquier
Discussion: https://postgr.es/m/87sf13jhuw.fsf@wibble.ilmari.org

diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c
index 76f01c73f566b73272622e144ec978ad3e5c6951..06f6775fc65b082bfd252b3472c8380528add9ab 100644 (file)
--- a/src/test/regress/pg_regress.c
+++ b/src/test/regress/pg_regress.c
@@ -341,6 +341,14 @@ emit_tap_output_v(TAPtype type, const char *fmt, va_list argp)
 {
        va_list         argp_logfile;
        FILE       *fp;
+       int                     save_errno;
+
+       /*
+        * The fprintf() calls used to output TAP-protocol elements might clobber
+        * errno, so save it here and restore it before vfprintf()-ing the user's
+        * format string, in case it contains %m placeholders.
+        */
+       save_errno = errno;
 
        /*
         * Diagnostic output will be hidden by prove unless printed to stderr. The
@@ -379,9 +387,13 @@ emit_tap_output_v(TAPtype type, const char *fmt, va_list argp)
                if (logfile)
                        fprintf(logfile, "# ");
        }
+       errno = save_errno;
        vfprintf(fp, fmt, argp);
        if (logfile)
+       {
+               errno = save_errno;
                vfprintf(logfile, fmt, argp_logfile);
+       }
 
        /*
         * If we are entering into a note with more details to follow, register
@@ -492,10 +504,7 @@ make_temp_sockdir(void)
 
        temp_sockdir = mkdtemp(template);
        if (temp_sockdir == NULL)
-       {
-               bail("could not create directory \"%s\": %s",
-                        template, strerror(errno));
-       }
+               bail("could not create directory \"%s\": %m", template);
 
        /* Stage file names for remove_temp().  Unsafe in a signal handler. */
        UNIXSOCK_PATH(sockself, port, temp_sockdir);
@@ -616,8 +625,7 @@ load_resultmap(void)
                /* OK if it doesn't exist, else complain */
                if (errno == ENOENT)
                        return;
-               bail("could not open file \"%s\" for reading: %s",
-                        buf, strerror(errno));
+               bail("could not open file \"%s\" for reading: %m", buf);
        }
 
        while (fgets(buf, sizeof(buf), f))
@@ -1046,10 +1054,7 @@ config_sspi_auth(const char *pgdata, const char *superuser_name)
 #define CW(cond)       \
        do { \
                if (!(cond)) \
-               { \
-                       bail("could not write to file \"%s\": %s", \
-                                fname, strerror(errno)); \
-               } \
+                       bail("could not write to file \"%s\": %m", fname); \
        } while (0)
 
        res = snprintf(fname, sizeof(fname), "%s/pg_hba.conf", pgdata);
@@ -1064,8 +1069,7 @@ config_sspi_auth(const char *pgdata, const char *superuser_name)
        hba = fopen(fname, "w");
        if (hba == NULL)
        {
-               bail("could not open file \"%s\" for writing: %s",
-                        fname, strerror(errno));
+               bail("could not open file \"%s\" for writing: %m", fname);
        }
        CW(fputs("# Configuration written by config_sspi_auth()\n", hba) >= 0);
        CW(fputs("host all all 127.0.0.1/32  sspi include_realm=1 map=regress\n",
@@ -1079,8 +1083,7 @@ config_sspi_auth(const char *pgdata, const char *superuser_name)
        ident = fopen(fname, "w");
        if (ident == NULL)
        {
-               bail("could not open file \"%s\" for writing: %s",
-                        fname, strerror(errno));
+               bail("could not open file \"%s\" for writing: %m", fname);
        }
        CW(fputs("# Configuration written by config_sspi_auth()\n", ident) >= 0);
 
@@ -1210,7 +1213,7 @@ spawn_process(const char *cmdline)
        pid = fork();
        if (pid == -1)
        {
-               bail("could not fork: %s", strerror(errno));
+               bail("could not fork: %m");
        }
        if (pid == 0)
        {
@@ -1226,7 +1229,7 @@ spawn_process(const char *cmdline)
                cmdline2 = psprintf("exec %s", cmdline);
                execl(shellprog, shellprog, "-c", cmdline2, (char *) NULL);
                /* Not using the normal bail() here as we want _exit */
-               bail_noatexit("could not exec \"%s\": %s", shellprog, strerror(errno));
+               bail_noatexit("could not exec \"%s\": %m", shellprog);
        }
        /* in parent */
        return pid;
@@ -1262,8 +1265,7 @@ file_size(const char *file)
 
        if (!f)
        {
-               diag("could not open file \"%s\" for reading: %s",
-                        file, strerror(errno));
+               diag("could not open file \"%s\" for reading: %m", file);
                return -1;
        }
        fseek(f, 0, SEEK_END);
@@ -1284,8 +1286,7 @@ file_line_count(const char *file)
 
        if (!f)
        {
-               diag("could not open file \"%s\" for reading: %s",
-                        file, strerror(errno));
+               diag("could not open file \"%s\" for reading: %m", file);
                return -1;
        }
        while ((c = fgetc(f)) != EOF)
@@ -1325,9 +1326,7 @@ static void
 make_directory(const char *dir)
 {
        if (mkdir(dir, S_IRWXU | S_IRWXG | S_IRWXO) < 0)
-       {
-               bail("could not create directory \"%s\": %s", dir, strerror(errno));
-       }
+               bail("could not create directory \"%s\": %m", dir);
 }
 
 /*
@@ -1456,10 +1455,7 @@ results_differ(const char *testname, const char *resultsfile, const char *defaul
 
                alt_expectfile = get_alternative_expectfile(expectfile, i);
                if (!alt_expectfile)
-               {
-                       bail("Unable to check secondary comparison files: %s",
-                                strerror(errno));
-               }
+                       bail("Unable to check secondary comparison files: %m");
 
                if (!file_exists(alt_expectfile))
                {
@@ -1572,9 +1568,7 @@ wait_for_tests(PID_TYPE * pids, int *statuses, instr_time *stoptimes,
                p = wait(&exit_status);
 
                if (p == INVALID_PID)
-               {
-                       bail("failed to wait for subprocesses: %s", strerror(errno));
-               }
+                       bail("failed to wait for subprocesses: %m");
 #else
                DWORD           exit_status;
                int                     r;
@@ -1664,10 +1658,7 @@ run_schedule(const char *schedule, test_start_function startfunc,
 
        scf = fopen(schedule, "r");
        if (!scf)
-       {
-               bail("could not open file \"%s\" for reading: %s",
-                        schedule, strerror(errno));
-       }
+               bail("could not open file \"%s\" for reading: %m", schedule);
 
        while (fgets(scbuf, sizeof(scbuf), scf))
        {
@@ -1931,20 +1922,15 @@ open_result_files(void)
        logfilename = pg_strdup(file);
        logfile = fopen(logfilename, "w");
        if (!logfile)
-       {
-               bail("could not open file \"%s\" for writing: %s",
-                        logfilename, strerror(errno));
-       }
+               bail("could not open file \"%s\" for writing: %m", logfilename);
 
        /* create the diffs file as empty */
        snprintf(file, sizeof(file), "%s/regression.diffs", outputdir);
        difffilename = pg_strdup(file);
        difffile = fopen(difffilename, "w");
        if (!difffile)
-       {
-               bail("could not open file \"%s\" for writing: %s",
-                        difffilename, strerror(errno));
-       }
+               bail("could not open file \"%s\" for writing: %m", difffilename);
+
        /* we don't keep the diffs file open continuously */
        fclose(difffile);
 
@@ -2406,10 +2392,8 @@ regression_main(int argc, char *argv[],
                snprintf(buf, sizeof(buf), "%s/data/postgresql.conf", temp_instance);
                pg_conf = fopen(buf, "a");
                if (pg_conf == NULL)
-               {
-                       bail("could not open \"%s\" for adding extra config: %s",
-                                buf, strerror(errno));
-               }
+                       bail("could not open \"%s\" for adding extra config: %m", buf);
+
                fputs("\n# Configuration added by pg_regress\n\n", pg_conf);
                fputs("log_autovacuum_min_duration = 0\n", pg_conf);
                fputs("log_checkpoints = on\n", pg_conf);
@@ -2427,8 +2411,8 @@ regression_main(int argc, char *argv[],
                        extra_conf = fopen(temp_config, "r");
                        if (extra_conf == NULL)
                        {
-                               bail("could not open \"%s\" to read extra config: %s",
-                                        temp_config, strerror(errno));
+                               bail("could not open \"%s\" to read extra config: %m",
+                                        temp_config);
                        }
                        while (fgets(line_buf, sizeof(line_buf), extra_conf) != NULL)
                                fputs(line_buf, pg_conf);
@@ -2503,7 +2487,7 @@ regression_main(int argc, char *argv[],
                                 outputdir);
                postmaster_pid = spawn_process(buf);
                if (postmaster_pid == INVALID_PID)
-                       bail("could not spawn postmaster: %s", strerror(errno));
+                       bail("could not spawn postmaster: %m");
 
                /*
                 * Wait till postmaster is able to accept connections; normally takes
@@ -2566,7 +2550,7 @@ regression_main(int argc, char *argv[],
                         */
 #ifndef WIN32
                        if (kill(postmaster_pid, SIGKILL) != 0 && errno != ESRCH)
-                               bail("could not kill failed postmaster: %s", strerror(errno));
+                               bail("could not kill failed postmaster: %m");
 #else
                        if (TerminateProcess(postmaster_pid, 255) == 0)
                                bail("could not kill failed postmaster: error code %lu",