Using the %m printf format more
Hi hackers,
I just noticed that commit d93627bc added a bunch of pg_fatal() calls
using %s and strerror(errno), which could be written more concisely as
%m. I'm assuming this was done because the surrounding code also uses
this pattern, and hadn't been changed to use %m when support for that
was added to snprintf.c to avoid backporting hazards. However, that
support was in v12, which is now the oldest still-supported back branch,
so we can safely make that change.
The attached patch does so everywhere appropriate. One place where it's
not appropriate is the TAP-emitting functions in pg_regress, since those
call fprintf() and other potentially errno-modifying functions before
evaluating the format string.
- ilmari
>From 17ad27665821decf9116723fe8873bd256e9d75d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?=
Date: Wed, 6 Mar 2024 16:58:33 +
Subject: [PATCH] Use %m printf format instead of strerror(errno) where
appropriate
Now that all live branches (12-) support %m in printf formats, let's
use it everywhere appropriate.
Particularly, we're _not_ converting the TAP output calls in
pg_regress, since those functions call fprintf() and friends before
evaluating the format string, which can clobber errno.
---
src/backend/postmaster/postmaster.c | 21 ++--
src/backend/postmaster/syslogger.c| 2 +-
src/backend/utils/misc/guc.c | 9 +-
src/bin/initdb/findtimezone.c | 4 +-
src/bin/pg_ctl/pg_ctl.c | 74 +++---
src/bin/pg_dump/compress_gzip.c | 2 +-
src/bin/pg_dump/compress_none.c | 5 +-
src/bin/pg_upgrade/check.c| 41 +++-
src/bin/pg_upgrade/controldata.c | 6 +-
src/bin/pg_upgrade/exec.c | 14 ++-
src/bin/pg_upgrade/file.c | 98 +--
src/bin/pg_upgrade/function.c | 3 +-
src/bin/pg_upgrade/option.c | 10 +-
src/bin/pg_upgrade/parallel.c | 12 +--
src/bin/pg_upgrade/pg_upgrade.c | 4 +-
src/bin/pg_upgrade/relfilenumber.c| 5 +-
src/bin/pg_upgrade/tablespace.c | 4 +-
src/bin/pg_upgrade/version.c | 9 +-
src/common/psprintf.c | 4 +-
src/interfaces/ecpg/preproc/ecpg.c| 12 +--
src/test/isolation/isolationtester.c | 2 +-
.../modules/libpq_pipeline/libpq_pipeline.c | 4 +-
src/tools/ifaddrs/test_ifaddrs.c | 2 +-
23 files changed, 157 insertions(+), 190 deletions(-)
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index f3c09e8dc0..af8a1efe66 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -1375,12 +1375,12 @@ PostmasterMain(int argc, char *argv[])
/* Make PID file world readable */
if (chmod(external_pid_file, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH) != 0)
-write_stderr("%s: could not change permissions of external PID file \"%s\": %s\n",
- progname, external_pid_file, strerror(errno));
+write_stderr("%s: could not change permissions of external PID file \"%s\": %m\n",
+ progname, external_pid_file);
}
else
- write_stderr("%s: could not write external PID file \"%s\": %s\n",
- progname, external_pid_file, strerror(errno));
+ write_stderr("%s: could not write external PID file \"%s\": %m\n",
+ progname, external_pid_file);
on_proc_exit(unlink_external_pid_file, 0);
}
@@ -1589,8 +1589,8 @@ checkControlFile(void)
{
write_stderr("%s: could not find the database system\n"
"Expected to find it in the directory \"%s\",\n"
- "but could not open file \"%s\": %s\n",
- progname, DataDir, path, strerror(errno));
+ "but could not open file \"%s\": %m\n",
+ progname, DataDir, path);
ExitPostmaster(2);
}
FreeFile(fp);
@@ -6277,15 +6277,13 @@ read_backend_variables(char *id, Port **port, BackgroundWorker **worker)
fp = AllocateFile(id, PG_BINARY_R);
if (!fp)
{
- write_stderr("could not open backend variables file \"%s\": %s\n",
- id, strerror(errno));
+ write_stderr("could not open backend variables file \"%s\": %m\n", id);
exit(1);
}
if (fread(¶m, sizeof(param), 1, fp) != 1)
{
- write_stderr("could not read from backend variables file \"%s\": %s\n",
- id, strerror(errno));
+ write_stderr("could not read from backend variables file \"%s\": %m\n", id);
exit(1);
}
@@ -6293,8 +6291,7 @@ read_backend_variables(char *id, Port **port, BackgroundWorker **worker)
FreeFile(fp);
if (unlink(id) != 0)
{
- write_stderr("could not remove file \"%s\": %s\n",
- id, strerror(errno));
+ write_stderr("could not remove file \"%s\": %m\n", id);
exit(1);
}
#else
diff --git a/src/backend/postmaster/syslogger.c b/src/backend/postmaster/syslogger.c
index c2a6a226e7..d9d042f562 100644
--- a/src/back
Re: Using the %m printf format more
On Wed, Mar 06, 2024 at 07:11:19PM +, Dagfinn Ilmari Mannsåker wrote: > I just noticed that commit d93627bc added a bunch of pg_fatal() calls > using %s and strerror(errno), which could be written more concisely as > %m. I'm assuming this was done because the surrounding code also uses > this pattern, and hadn't been changed to use %m when support for that > was added to snprintf.c to avoid backporting hazards. However, that > support was in v12, which is now the oldest still-supported back branch, > so we can safely make that change. Right. This may still create some spurious conflicts, but that's manageable for error strings. The changes in your patch look OK. > The attached patch does so everywhere appropriate. One place where it's > not appropriate is the TAP-emitting functions in pg_regress, since those > call fprintf() I am not really following your argument with pg_regress.c and fprintf(). d6c55de1f99a should make that possible even in the case of emit_tap_output_v(), no? > and other potentially errno-modifying functions before > evaluating the format string. Sure. -- Michael signature.asc Description: PGP signature
Re: Using the %m printf format more
Michael Paquier writes:
> On Wed, Mar 06, 2024 at 07:11:19PM +, Dagfinn Ilmari Mannsåker wrote:
>
>> The attached patch does so everywhere appropriate. One place where it's
>> not appropriate is the TAP-emitting functions in pg_regress, since those
>> call fprintf()
>
> I am not really following your argument with pg_regress.c and
> fprintf(). d6c55de1f99a should make that possible even in the case of
> emit_tap_output_v(), no?
The problem isn't that emit_tap_output_v() doesn't support %m, which it
does, but that it potentially calls fprintf() to output TAP protocol
elements such as "\n" and "# " before it calls vprintf(…, fmt, …), and
those calls might clobber errno. An option is to make it save errno at
the start and restore it before the vprintf() calls, as in the second
attached patch.
>> and other potentially errno-modifying functions before
>> evaluating the format string.
>
> Sure.
On closer look, fprintf() is actually the only errno-clobbering function
it calls, I was just hedging my bets in that statement.
- ilmari
>From 3727341aad84fbd275acb24f92591cc734fdd6a7 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?=
Date: Wed, 6 Mar 2024 16:58:33 +
Subject: [PATCH v2 1/2] Use %m printf format instead of strerror(errno) where
appropriate
Now that all live branches (12-) support %m in printf formats, let's
use it everywhere appropriate.
Particularly, we're _not_ converting the TAP output calls in
pg_regress, since those functions call fprintf() and friends before
evaluating the format string, which can clobber errno.
---
src/backend/postmaster/postmaster.c | 21 ++--
src/backend/postmaster/syslogger.c| 2 +-
src/backend/utils/misc/guc.c | 9 +-
src/bin/initdb/findtimezone.c | 4 +-
src/bin/pg_ctl/pg_ctl.c | 74 +++---
src/bin/pg_dump/compress_gzip.c | 2 +-
src/bin/pg_dump/compress_none.c | 5 +-
src/bin/pg_upgrade/check.c| 41 +++-
src/bin/pg_upgrade/controldata.c | 6 +-
src/bin/pg_upgrade/exec.c | 14 ++-
src/bin/pg_upgrade/file.c | 98 +--
src/bin/pg_upgrade/function.c | 3 +-
src/bin/pg_upgrade/option.c | 10 +-
src/bin/pg_upgrade/parallel.c | 12 +--
src/bin/pg_upgrade/pg_upgrade.c | 4 +-
src/bin/pg_upgrade/relfilenumber.c| 5 +-
src/bin/pg_upgrade/tablespace.c | 4 +-
src/bin/pg_upgrade/version.c | 9 +-
src/common/psprintf.c | 4 +-
src/interfaces/ecpg/preproc/ecpg.c| 12 +--
src/port/path.c | 3 +-
src/test/isolation/isolationtester.c | 2 +-
.../modules/libpq_pipeline/libpq_pipeline.c | 4 +-
src/tools/ifaddrs/test_ifaddrs.c | 2 +-
24 files changed, 158 insertions(+), 192 deletions(-)
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index f3c09e8dc0..af8a1efe66 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -1375,12 +1375,12 @@ PostmasterMain(int argc, char *argv[])
/* Make PID file world readable */
if (chmod(external_pid_file, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH) != 0)
-write_stderr("%s: could not change permissions of external PID file \"%s\": %s\n",
- progname, external_pid_file, strerror(errno));
+write_stderr("%s: could not change permissions of external PID file \"%s\": %m\n",
+ progname, external_pid_file);
}
else
- write_stderr("%s: could not write external PID file \"%s\": %s\n",
- progname, external_pid_file, strerror(errno));
+ write_stderr("%s: could not write external PID file \"%s\": %m\n",
+ progname, external_pid_file);
on_proc_exit(unlink_external_pid_file, 0);
}
@@ -1589,8 +1589,8 @@ checkControlFile(void)
{
write_stderr("%s: could not find the database system\n"
"Expected to find it in the directory \"%s\",\n"
- "but could not open file \"%s\": %s\n",
- progname, DataDir, path, strerror(errno));
+ "but could not open file \"%s\": %m\n",
+ progname, DataDir, path);
ExitPostmaster(2);
}
FreeFile(fp);
@@ -6277,15 +6277,13 @@ read_backend_variables(char *id, Port **port, BackgroundWorker **worker)
fp = AllocateFile(id, PG_BINARY_R);
if (!fp)
{
- write_stderr("could not open backend variables file \"%s\": %s\n",
- id, strerror(errno));
+ write_stderr("could not open backend variables file \"%s\": %m\n", id);
exit(1);
}
if (fread(¶m, sizeof(param), 1, fp) != 1)
{
- write_stderr("could not read from backend variables file \"%s\": %s\n",
- id, strerror(errno));
+ write_stderr("could not read from backend variables file \"%s\": %m\n", id);
exit(1);
}
@@ -6293,8 +6291,7 @@ read_backend_variables(char *
Re: Using the %m printf format more
On Mon, Mar 11, 2024 at 11:19:16AM +, Dagfinn Ilmari Mannsåker wrote: > On closer look, fprintf() is actually the only errno-clobbering function > it calls, I was just hedging my bets in that statement. This makes the whole simpler, so I'd be OK with that. I am wondering if others have opinions to offer about that. I've applied v2-0001 for now, as it is worth on its own and it shaves a bit of code. -- Michael signature.asc Description: PGP signature
Re: Using the %m printf format more
Michael Paquier writes: > On Mon, Mar 11, 2024 at 11:19:16AM +, Dagfinn Ilmari Mannsåker wrote: >> On closer look, fprintf() is actually the only errno-clobbering function >> it calls, I was just hedging my bets in that statement. > > This makes the whole simpler, so I'd be OK with that. I am wondering > if others have opinions to offer about that. If no one chimes in in the next couple of days I'll add it to the commitfest so it doesn't get lost. > I've applied v2-0001 for now, as it is worth on its own and it shaves > a bit of code. Thanks! - ilmari
Re: Using the %m printf format more
On 12.03.24 02:22, Michael Paquier wrote: On Mon, Mar 11, 2024 at 11:19:16AM +, Dagfinn Ilmari Mannsåker wrote: On closer look, fprintf() is actually the only errno-clobbering function it calls, I was just hedging my bets in that statement. This makes the whole simpler, so I'd be OK with that. I am wondering if others have opinions to offer about that. The 0002 patch looks sensible. It would be good to fix that, otherwise it could have some confusing outcomes in the future.
Re: Using the %m printf format more
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. -- Michael signature.asc Description: PGP signature
Re: Using the %m printf format more
Michael Paquier 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. - ilmari
Re: Using the %m printf format more
Dagfinn Ilmari Mannsåker writes:
> Michael Paquier 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?=
Date: Mon, 11 Mar 2024 11:08:14 +
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
Re: Using the %m printf format more
On Fri, Mar 22, 2024 at 01:58:24PM +, Dagfinn Ilmari Mannsåker wrote: > 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. I see no problem to do that now rather than later. So, done to make pg_regress able to use %m. -- Michael signature.asc Description: PGP signature
Re: Using the %m printf format more
On Thu, 4 Apr 2024, at 03:35, Michael Paquier wrote: > On Fri, Mar 22, 2024 at 01:58:24PM +, Dagfinn Ilmari Mannsåker wrote: >> 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. > > I see no problem to do that now rather than later. So, done to make > pg_regress able to use %m. Thanks! -- - ilmari
