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

1 files changed, 34 insertions, 50 deletions

diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c
index 76f01c73f56..06f6775fc65 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 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",