Horton

Re: Allowing printf("%m") only where it actually works

Tom Lane <tgl@sss.pgh.pa.us>

View thread View on postgr.es
From: Tom Lane <tgl@sss.pgh.pa.us>
To: Thomas Munro <thomas.munro@enterprisedb.com>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Date: 2018-08-19T19:12:00Z
Lists: pgsql-hackers

Attachments

I wrote:
>> Consider the following approach:
>> 1. Teach src/port/snprintf.c about %m.  While I've not written a patch
>> for this, it looks pretty trivial.
>> 2. Teach configure to test for %m and if it's not there, use the
>> replacement snprintf.  (Note: we're already forcing snprintf replacement
>> in cross-compiles, so the added run-time test isn't losing anything.)
>> 3. Get rid of elog.c's hand-made substitution of %m strings, and instead
>> just let it pass the correct errno value down.  (We'd likely need to do
>> some fooling in appendStringInfoVA and related functions to preserve
>> call-time errno, but that's not complicated, nor expensive.)
>> 4. (Optional) Get rid of strerror(errno) calls in favor of %m, even in
>> frontend code.

> So I started to hack on this, and soon noticed that actually, what elog.c
> replaces %m with is *not* the result of strerror(), it's the result of
> useful_strerror().

After further thought, I realized that the best way to handle that is to
turn useful_strerror() into a globally available wrapper pg_strerror()
that replaces strerror() everyplace.  That way we get its protections in
frontend code as well as backend, and we ensure that the results of
printing strerror(errno) match what %m would do (so that step 4 above is
just cosmetic code simplification and doesn't change behavior).  We'd
speculated about doing that back when 8e68816cc went in, but not actually
pulled the trigger.

So the first attached patch does that, and then the second one implements
%m in snprintf.c and causes it to be used all the time.  I've not touched
step 4 yet, that could be done later/piecemeal.

Although the attached causes strerror.c to be included in libpq, I think
it's actually dead code at the moment, because on any reasonably modern
platform (including *all* of the buildfarm) libpq does not depend on
strerror but strerror_r, cf pqStrerror().  It's tempting to expand
strerror.c to include a similar wrapper for strerror_r, so that the
extra functionality exists for libpq's usage too.  (Also, it'd likely
be better for snprintf.c to depend on strerror_r not strerror, to avoid
unnecessary thread-unsafety.)  But I've left that for later.

A couple of additional notes for review:

* The 0002 patch will conflict with my snprintf-speedup patch, but
resolving that is simple (just need to move one of the %m hunks around).

* src/port/strerror.c already exists, but as far as I can see it's been
dead code for decades; no ANSI-C-compliant platform lacks strerror()
altogether.  Moreover, ecpg never got taught to include it, so obviously
we've not built on a platform with that problem anytime recently.
So I just removed the former contents of that file.

* The most nervous-making aspect of this patch, IMO, is that there's an
addition to the API spec for appendStringInfoVA and pvsnprintf: callers
have to preserve errno when looping.  Fortunately there are very few
direct callers of those, but I'm slightly worried that extensions might
do so.  I don't see any way to avoid that change though.

* I dropped configure's checks for existence/declaration of snprintf
and vsnprintf, since (a) we no longer care, and (b) those are pretty
much useless anyway; no active buildfarm member fails those checks.

* The Windows aspects of this are untested.  It seems like importing
pgwin32_socket_strerror's behavior into the frontend ought to be a
bug fix, though: win32_port.h redefines socket error symbols whether
FRONTEND is set or not, so aren't we printing bogus info for socket
errors in frontend right now?

			regards, tom lane

Commits

  1. In pg_log_generic(), be more paranoid about preserving errno.

  2. Make src/common/exec.c's error logging less ugly.

  3. Select appropriate PG_PRINTF_ATTRIBUTE for recent NetBSD.

  4. Fix detection of the result type of strerror_r().

  5. Try another way to detect the result type of strerror_r().

  6. Clean up *printf macros to avoid conflict with format archetypes.

  7. Fix link failures due to snprintf/strerror changes.

  8. Implement %m in src/port/snprintf.c, and teach elog.c to rely on that.

  9. Always use our own versions of *printf().

  10. Incorporate strerror_r() into src/port/snprintf.c, too.

  11. Convert elog.c's useful_strerror() into a globally-used strerror wrapper.

  12. Revert "Distinguish printf-like functions that support %m from those that don't."

  13. Produce compiler errors if errno is referenced inside elog/ereport calls.

  14. Distinguish printf-like functions that support %m from those that don't.

  15. Fix unportable usage of printf("%m").

  16. Be more robust when strerror() doesn't give a useful result.