Dagfinn Ilmari Mannsåker <ilm...@ilmari.org> writes:
> Michael Paquier <mich...@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?= <ilm...@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
