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
Attachments:
0001-Use-m-printf-format-instead-of-strerror-errno-where-.patchtext/x-diffDownload+157-191
On Wed, Mar 06, 2024 at 07:11:19PM +0000, 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
Michael Paquier <michael@paquier.xyz> writes:
On Wed, Mar 06, 2024 at 07:11:19PM +0000, 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
On Mon, Mar 11, 2024 at 11:19:16AM +0000, 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
Michael Paquier <michael@paquier.xyz> writes:
On Mon, Mar 11, 2024 at 11:19:16AM +0000, 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
On 12.03.24 02:22, Michael Paquier wrote:
On Mon, Mar 11, 2024 at 11:19:16AM +0000, 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.
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
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.
- ilmari
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.
Show quoted text
- ilmari
Attachments:
v2-0001-Save-errno-in-emit_tap_output_v-and-use-m-in-call.patchtext/x-diff; charset=utf-8Download+34-51
On Fri, Mar 22, 2024 at 01:58:24PM +0000, 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
On Thu, 4 Apr 2024, at 03:35, Michael Paquier wrote:
On Fri, Mar 22, 2024 at 01:58:24PM +0000, 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