Re: Using the %m printf format more

Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> writes:

> Michael Paquier <michael@paquier.xyz> writes:
>
>> On Wed, Mar 13, 2024 at 02:33:52PM +0100, Peter Eisentraut wrote:
>>> The 0002 patch looks sensible.  It would be good to fix that, otherwise it
>>> could have some confusing outcomes in the future.
>>
>> You mean if we begin to use %m in future callers of
>> emit_tap_output_v(), hypothetically?  That's a fair argument.
>
> Yeah, developers would rightfully expect to be able to use %m with
> anything that takes a printf format string.  Case in point: when I was
> first doing the conversion I did change the bail() and diag() calls in
> pg_regress to %m, and only while I was preparing the patch for
> submission did I think to check the actual implementation to see if it
> was safe to do so.
>
> The alternative would be to document that you can't use %m with these
> functions, which is silly IMO, given how simple the fix is.
>
> One minor improvement I can think of is to add a comment by the
> save_errno declaration noting that it's needed in order to support %m.

Here's an updated patch that adds such a comment.  I'll add it to the
commitfest later today unless someone commits it before then.

> - ilmari

From 4312d746a722582202a013c7199f5c42e88db951 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org>
Date: Mon, 11 Mar 2024 11:08:14 +0000
Subject: [PATCH v2] Save errno in emit_tap_output_v() and use %m in callers
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This defends aganst the fprintf() calls before vprintf(…, fmt, …)
clobbering errno, thus breaking %m.
---
 src/test/regress/pg_regress.c | 84 ++++++++++++++---------------------
 1 file changed, 34 insertions(+), 50 deletions(-)

diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c
index 76f01c73f5..ea94d874b0 100644
--- 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 we need to save it 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",
-- 
2.39.2