From d6c55de1f99a9028540516316b95321a7b12a540 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Wed, 26 Sep 2018 13:31:56 -0400 Subject: [PATCH] Implement %m in src/port/snprintf.c, and teach elog.c to rely on that. I started out with the idea that we needed to detect use of %m format specs in contexts other than elog/ereport calls, because we couldn't rely on that working in *printf calls. But a better answer is to fix things so that it does work. Now that we're using snprintf.c all the time, we can implement %m in that and we've fixed the problem. This requires also adjusting our various printf-wrapping functions so that they ensure "errno" is preserved when they call snprintf.c. Remove elog.c's handmade implementation of %m, and let it rely on snprintf to support the feature. That should provide some performance gain, though I've not attempted to measure it. There are a lot of places where we could now simplify 'printf("%s", strerror(errno))' into 'printf("%m")', but I'm not in any big hurry to make that happen. Patch by me, reviewed by Michael Paquier Discussion: https://postgr.es/m/2975.1526862605@sss.pgh.pa.us --- src/backend/lib/stringinfo.c | 6 +++ src/backend/utils/error/elog.c | 70 ++-------------------------- src/bin/pg_dump/pg_backup_archiver.c | 4 ++ src/bin/pg_dump/pg_backup_tar.c | 2 + src/common/psprintf.c | 5 ++ src/interfaces/libpq/pqexpbuffer.c | 7 +++ src/pl/plpython/plpy_elog.c | 2 + src/port/snprintf.c | 28 ++++++++++- 8 files changed, 57 insertions(+), 67 deletions(-) diff --git a/src/backend/lib/stringinfo.c b/src/backend/lib/stringinfo.c index 798a823ac9f..df7e01f76d9 100644 --- a/src/backend/lib/stringinfo.c +++ b/src/backend/lib/stringinfo.c @@ -77,12 +77,15 @@ resetStringInfo(StringInfo str) void appendStringInfo(StringInfo str, const char *fmt,...) { + int save_errno = errno; + for (;;) { va_list args; int needed; /* Try to format the data. */ + errno = save_errno; va_start(args, fmt); needed = appendStringInfoVA(str, fmt, args); va_end(args); @@ -105,6 +108,9 @@ appendStringInfo(StringInfo str, const char *fmt,...) * pass the return value to enlargeStringInfo() before trying again; see * appendStringInfo for standard usage pattern. * + * Caution: callers must be sure to preserve their entry-time errno + * when looping, in case the fmt contains "%m". + * * XXX This API is ugly, but there seems no alternative given the C spec's * restrictions on what can portably be done with va_list arguments: you have * to redo va_start before you can rescan the argument list, and we can't do diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c index 22e5d876181..b9c11301ca2 100644 --- a/src/backend/utils/error/elog.c +++ b/src/backend/utils/error/elog.c @@ -177,7 +177,6 @@ static void write_csvlog(ErrorData *edata); static void send_message_to_server_log(ErrorData *edata); static void write_pipe_chunks(char *data, int len, int dest); static void send_message_to_frontend(ErrorData *edata); -static char *expand_fmt_string(const char *fmt, ErrorData *edata); static const char *error_severity(int elevel); static void append_with_tabs(StringInfo buf, const char *str); static bool is_log_level_output(int elevel, int log_min_level); @@ -705,13 +704,10 @@ errcode_for_socket_access(void) */ #define EVALUATE_MESSAGE(domain, targetfield, appendval, translateit) \ { \ - char *fmtbuf; \ StringInfoData buf; \ /* Internationalize the error format string */ \ if ((translateit) && !in_error_recursion_trouble()) \ fmt = dgettext((domain), fmt); \ - /* Expand %m in format string */ \ - fmtbuf = expand_fmt_string(fmt, edata); \ initStringInfo(&buf); \ if ((appendval) && edata->targetfield) { \ appendStringInfoString(&buf, edata->targetfield); \ @@ -722,15 +718,14 @@ errcode_for_socket_access(void) { \ va_list args; \ int needed; \ + errno = edata->saved_errno; \ va_start(args, fmt); \ - needed = appendStringInfoVA(&buf, fmtbuf, args); \ + needed = appendStringInfoVA(&buf, fmt, args); \ va_end(args); \ if (needed == 0) \ break; \ enlargeStringInfo(&buf, needed); \ } \ - /* Done with expanded fmt */ \ - pfree(fmtbuf); \ /* Save the completed message into the stack item */ \ if (edata->targetfield) \ pfree(edata->targetfield); \ @@ -746,15 +741,12 @@ errcode_for_socket_access(void) #define EVALUATE_MESSAGE_PLURAL(domain, targetfield, appendval) \ { \ const char *fmt; \ - char *fmtbuf; \ StringInfoData buf; \ /* Internationalize the error format string */ \ if (!in_error_recursion_trouble()) \ fmt = dngettext((domain), fmt_singular, fmt_plural, n); \ else \ fmt = (n == 1 ? fmt_singular : fmt_plural); \ - /* Expand %m in format string */ \ - fmtbuf = expand_fmt_string(fmt, edata); \ initStringInfo(&buf); \ if ((appendval) && edata->targetfield) { \ appendStringInfoString(&buf, edata->targetfield); \ @@ -765,15 +757,14 @@ errcode_for_socket_access(void) { \ va_list args; \ int needed; \ + errno = edata->saved_errno; \ va_start(args, n); \ - needed = appendStringInfoVA(&buf, fmtbuf, args); \ + needed = appendStringInfoVA(&buf, fmt, args); \ va_end(args); \ if (needed == 0) \ break; \ enlargeStringInfo(&buf, needed); \ } \ - /* Done with expanded fmt */ \ - pfree(fmtbuf); \ /* Save the completed message into the stack item */ \ if (edata->targetfield) \ pfree(edata->targetfield); \ @@ -3328,59 +3319,6 @@ send_message_to_frontend(ErrorData *edata) */ -/* - * expand_fmt_string --- process special format codes in a format string - * - * We must replace %m with the appropriate strerror string, since vsnprintf - * won't know what to do with it. - * - * The result is a palloc'd string. - */ -static char * -expand_fmt_string(const char *fmt, ErrorData *edata) -{ - StringInfoData buf; - const char *cp; - - initStringInfo(&buf); - - for (cp = fmt; *cp; cp++) - { - if (cp[0] == '%' && cp[1] != '\0') - { - cp++; - if (*cp == 'm') - { - /* - * Replace %m by system error string. If there are any %'s in - * the string, we'd better double them so that vsnprintf won't - * misinterpret. - */ - const char *cp2; - - cp2 = strerror(edata->saved_errno); - for (; *cp2; cp2++) - { - if (*cp2 == '%') - appendStringInfoCharMacro(&buf, '%'); - appendStringInfoCharMacro(&buf, *cp2); - } - } - else - { - /* copy % and next char --- this avoids trouble with %%m */ - appendStringInfoCharMacro(&buf, '%'); - appendStringInfoCharMacro(&buf, *cp); - } - } - else - appendStringInfoCharMacro(&buf, *cp); - } - - return buf.data; -} - - /* * error_severity --- get string representing elevel * diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c index d1faa70d78d..e976def42aa 100644 --- a/src/bin/pg_dump/pg_backup_archiver.c +++ b/src/bin/pg_dump/pg_backup_archiver.c @@ -1507,6 +1507,7 @@ archputs(const char *s, Archive *AH) int archprintf(Archive *AH, const char *fmt,...) { + int save_errno = errno; char *p; size_t len = 128; /* initial assumption about buffer size */ size_t cnt; @@ -1519,6 +1520,7 @@ archprintf(Archive *AH, const char *fmt,...) p = (char *) pg_malloc(len); /* Try to format the data. */ + errno = save_errno; va_start(args, fmt); cnt = pvsnprintf(p, len, fmt, args); va_end(args); @@ -1640,6 +1642,7 @@ RestoreOutput(ArchiveHandle *AH, OutputContext savedContext) int ahprintf(ArchiveHandle *AH, const char *fmt,...) { + int save_errno = errno; char *p; size_t len = 128; /* initial assumption about buffer size */ size_t cnt; @@ -1652,6 +1655,7 @@ ahprintf(ArchiveHandle *AH, const char *fmt,...) p = (char *) pg_malloc(len); /* Try to format the data. */ + errno = save_errno; va_start(args, fmt); cnt = pvsnprintf(p, len, fmt, args); va_end(args); diff --git a/src/bin/pg_dump/pg_backup_tar.c b/src/bin/pg_dump/pg_backup_tar.c index 007be1298fb..407a56d8d4f 100644 --- a/src/bin/pg_dump/pg_backup_tar.c +++ b/src/bin/pg_dump/pg_backup_tar.c @@ -1026,6 +1026,7 @@ _EndBlobs(ArchiveHandle *AH, TocEntry *te) static int tarPrintf(ArchiveHandle *AH, TAR_MEMBER *th, const char *fmt,...) { + int save_errno = errno; char *p; size_t len = 128; /* initial assumption about buffer size */ size_t cnt; @@ -1038,6 +1039,7 @@ tarPrintf(ArchiveHandle *AH, TAR_MEMBER *th, const char *fmt,...) p = (char *) pg_malloc(len); /* Try to format the data. */ + errno = save_errno; va_start(args, fmt); cnt = pvsnprintf(p, len, fmt, args); va_end(args); diff --git a/src/common/psprintf.c b/src/common/psprintf.c index 04465a18e69..2cf100f0954 100644 --- a/src/common/psprintf.c +++ b/src/common/psprintf.c @@ -45,6 +45,7 @@ char * psprintf(const char *fmt,...) { + int save_errno = errno; size_t len = 128; /* initial assumption about buffer size */ for (;;) @@ -60,6 +61,7 @@ psprintf(const char *fmt,...) result = (char *) palloc(len); /* Try to format the data. */ + errno = save_errno; va_start(args, fmt); newlen = pvsnprintf(result, len, fmt, args); va_end(args); @@ -89,6 +91,9 @@ psprintf(const char *fmt,...) * Other error cases do not return, but exit via elog(ERROR) or exit(). * Hence, this shouldn't be used inside libpq. * + * Caution: callers must be sure to preserve their entry-time errno + * when looping, in case the fmt contains "%m". + * * Note that the semantics of the return value are not exactly C99's. * First, we don't promise that the estimated buffer size is exactly right; * callers must be prepared to loop multiple times to get the right size. diff --git a/src/interfaces/libpq/pqexpbuffer.c b/src/interfaces/libpq/pqexpbuffer.c index 0814ec6dbe0..43c36c3bff8 100644 --- a/src/interfaces/libpq/pqexpbuffer.c +++ b/src/interfaces/libpq/pqexpbuffer.c @@ -233,6 +233,7 @@ enlargePQExpBuffer(PQExpBuffer str, size_t needed) void printfPQExpBuffer(PQExpBuffer str, const char *fmt,...) { + int save_errno = errno; va_list args; bool done; @@ -244,6 +245,7 @@ printfPQExpBuffer(PQExpBuffer str, const char *fmt,...) /* Loop in case we have to retry after enlarging the buffer. */ do { + errno = save_errno; va_start(args, fmt); done = appendPQExpBufferVA(str, fmt, args); va_end(args); @@ -261,6 +263,7 @@ printfPQExpBuffer(PQExpBuffer str, const char *fmt,...) void appendPQExpBuffer(PQExpBuffer str, const char *fmt,...) { + int save_errno = errno; va_list args; bool done; @@ -270,6 +273,7 @@ appendPQExpBuffer(PQExpBuffer str, const char *fmt,...) /* Loop in case we have to retry after enlarging the buffer. */ do { + errno = save_errno; va_start(args, fmt); done = appendPQExpBufferVA(str, fmt, args); va_end(args); @@ -281,6 +285,9 @@ appendPQExpBuffer(PQExpBuffer str, const char *fmt,...) * Shared guts of printfPQExpBuffer/appendPQExpBuffer. * Attempt to format data and append it to str. Returns true if done * (either successful or hard failure), false if need to retry. + * + * Caution: callers must be sure to preserve their entry-time errno + * when looping, in case the fmt contains "%m". */ static bool appendPQExpBufferVA(PQExpBuffer str, const char *fmt, va_list args) diff --git a/src/pl/plpython/plpy_elog.c b/src/pl/plpython/plpy_elog.c index e244104fed7..3814a6c32df 100644 --- a/src/pl/plpython/plpy_elog.c +++ b/src/pl/plpython/plpy_elog.c @@ -46,6 +46,7 @@ static bool set_string_attr(PyObject *obj, char *attrname, char *str); void PLy_elog_impl(int elevel, const char *fmt,...) { + int save_errno = errno; char *xmsg; char *tbmsg; int tb_depth; @@ -96,6 +97,7 @@ PLy_elog_impl(int elevel, const char *fmt,...) va_list ap; int needed; + errno = save_errno; va_start(ap, fmt); needed = appendStringInfoVA(&emsg, dgettext(TEXTDOMAIN, fmt), ap); va_end(ap); diff --git a/src/port/snprintf.c b/src/port/snprintf.c index 851e2ae330a..2c77eec1c6b 100644 --- a/src/port/snprintf.c +++ b/src/port/snprintf.c @@ -64,6 +64,14 @@ * * 5. Space and '#' flags are not implemented. * + * In addition, we support some extensions over C99: + * + * 1. Argument order control through "%n$" and "*n$", as required by POSIX. + * + * 2. "%m" expands to the value of strerror(errno), where errno is the + * value that variable had at the start of the call. This is a glibc + * extension, but a very useful one. + * * * Historically the result values of sprintf/snprintf varied across platforms. * This implementation now follows the C99 standard: @@ -155,6 +163,13 @@ static void flushbuffer(PrintfTarget *target); static void dopr(PrintfTarget *target, const char *format, va_list args); +/* + * Externally visible entry points. + * + * All of these are just wrappers around dopr(). Note it's essential that + * they not change the value of "errno" before reaching dopr(). + */ + int pg_vsnprintf(char *str, size_t count, const char *fmt, va_list args) { @@ -315,11 +330,12 @@ static void trailing_pad(int *padlen, PrintfTarget *target); /* - * dopr(): poor man's version of doprintf + * dopr(): the guts of *printf for all cases. */ static void dopr(PrintfTarget *target, const char *format, va_list args) { + int save_errno = errno; const char *format_start = format; int ch; bool have_dollar; @@ -497,6 +513,7 @@ nextch1: else have_non_dollar = true; break; + case 'm': case '%': break; } @@ -802,6 +819,15 @@ nextch2: precision, pointflag, target); break; + case 'm': + { + char errbuf[PG_STRERROR_R_BUFLEN]; + const char *errm = strerror_r(save_errno, + errbuf, sizeof(errbuf)); + + dostr(errm, strlen(errm), target); + } + break; case '%': dopr_outch('%', target); break;