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",