On 09.11.22 13:29, Alvaro Herrera wrote:
+/*
+ * Append a formatted string to the given buffer, after translation. A
+ * newline is automatically appended; the format should not end with a
+ * newline.
+ */
I find the "after translation" bit unclear -- does it mean that the
caller should have already translated, or is it the other way around? I
would say "Append a formatted string to the given buffer, after
translating it", which (to me) conveys more clearly that translation
occurs here.
ok
+ /* Loop in case we have to retry after enlarging the buffer. */
+ do
+ {
+ errno = save_errno;
+ va_start(args, fmt);
+ done = appendPQExpBufferVA(errorMessage, libpq_gettext(fmt),
args);
I wonder if it makes sense to do libpq_gettext() just once, instead of
redoing it on each iteration.
I wonder whether that would expose us to potential compiler warnings
about the format string not being constant. As long as the compiler can
trace that the string comes from gettext, it knows what's going on.
Also, most error strings in practice don't need the loop, so maybe it's
not a big issue.
+/*------------------------
+ * appendPQExpBufferVA
+ * 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".
+ */
+extern bool appendPQExpBufferVA(PQExpBuffer str, const char *fmt, va_list
args) pg_attribute_printf(2, 0);
As an API user, I don't care that this is shared guts for something
else, I just care about what it does. I think deleting that line is a
sufficient fix.
ok
@@ -420,7 +418,8 @@ pqsecure_raw_write(PGconn *conn, const void *ptr, size_t
len)
snprintf(msgbuf, sizeof(msgbuf),
libpq_gettext("server closed the
connection unexpectedly\n"
"\tThis
probably means the server terminated abnormally\n"
- "\tbefore
or while processing the request.\n"));
+ "\tbefore
or while processing the request."));
+ strlcat(msgbuf, "\n", sizeof(msgbuf));
conn->write_err_msg = strdup(msgbuf);
/* Now claim the write succeeded */
n = len;
In these two places, we're writing the error message manually to a
separate variable, so the extra \n is necessary. It looks a bit odd to
do it with strlcat() after the fact, but AFAICT it's necessary as it
keeps the \n out of the translation catalog, which is good. This is
nonobvious, so perhaps add a comment about it.
ok
