From 85230a247c74b92d9676abdf6693ac9d56c373cf Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Thu, 4 Apr 2024 11:33:07 +0900 Subject: [PATCH] pg_regress: Save errno in emit_tap_output_v() and switch to %m MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- 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 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",