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

This code used elog where it really ought to use ereport, mainly so that
it can report a SQLSTATE different from ERRCODE_INTERNAL_ERROR.  There
were some other random deviations from typical error report practice too.

In addition, we can make some cleanups that were impractical six months
ago:

* Use one variadic macro, instead of several with different numbers
of arguments, reducing the temptation to force-fit messages into
particular numbers of arguments;

* Use %m, even in the frontend case, simplifying the code.

Discussion: https://postgr.es/m/6025.1527351693@sss.pgh.pa.us

diff --git a/src/common/exec.c b/src/common/exec.c
index c207c0289fd0f423111867419c328d4daecc3a62..410dc2df451d81c182a02f8f1f0285a88faeeba8 100644 (file)
--- a/src/common/exec.c
+++ b/src/common/exec.c
@@ -25,14 +25,24 @@
 #include <sys/wait.h>
 #include <unistd.h>
 
+/*
+ * Hacky solution to allow expressing both frontend and backend error reports
+ * in one macro call.  First argument of log_error is an errcode() call of
+ * some sort (ignored if FRONTEND); the rest are errmsg_internal() arguments,
+ * i.e. message string and any parameters for it.
+ *
+ * Caller must provide the gettext wrapper around the message string, if
+ * appropriate, so that it gets translated in the FRONTEND case; this
+ * motivates using errmsg_internal() not errmsg().  We handle appending a
+ * newline, if needed, inside the macro, so that there's only one translatable
+ * string per call not two.
+ */
 #ifndef FRONTEND
-/* We use only 3- and 4-parameter elog calls in this file, for simplicity */
-/* NOTE: caller must provide gettext call around str! */
-#define log_error(str, param)  elog(LOG, str, param)
-#define log_error4(str, param, arg1)   elog(LOG, str, param, arg1)
+#define log_error(errcodefn, ...) \
+       ereport(LOG, (errcodefn, errmsg_internal(__VA_ARGS__)))
 #else
-#define log_error(str, param)  (fprintf(stderr, str, param), fputc('\n', stderr))
-#define log_error4(str, param, arg1)   (fprintf(stderr, str, param, arg1), fputc('\n', stderr))
+#define log_error(errcodefn, ...) \
+       (fprintf(stderr, __VA_ARGS__), fputc('\n', stderr))
 #endif
 
 #ifdef _MSC_VER
@@ -124,10 +134,8 @@ find_my_exec(const char *argv0, char *retpath)
 
        if (!getcwd(cwd, MAXPGPATH))
        {
-               int                     save_errno = errno;
-
-               log_error(_("could not identify current directory: %s"),
-                                 strerror(save_errno));
+               log_error(errcode_for_file_access(),
+                                 _("could not identify current directory: %m"));
                return -1;
        }
 
@@ -145,7 +153,8 @@ find_my_exec(const char *argv0, char *retpath)
                if (validate_exec(retpath) == 0)
                        return resolve_symlinks(retpath);
 
-               log_error(_("invalid binary \"%s\""), retpath);
+               log_error(errcode(ERRCODE_WRONG_OBJECT_TYPE),
+                                 _("invalid binary \"%s\""), retpath);
                return -1;
        }
 
@@ -194,14 +203,16 @@ find_my_exec(const char *argv0, char *retpath)
                                case -1:                /* wasn't even a candidate, keep looking */
                                        break;
                                case -2:                /* found but disqualified */
-                                       log_error(_("could not read binary \"%s\""),
+                                       log_error(errcode(ERRCODE_WRONG_OBJECT_TYPE),
+                                                         _("could not read binary \"%s\""),
                                                          retpath);
                                        break;
                        }
                } while (*endp);
        }
 
-       log_error(_("could not find a \"%s\" to execute"), argv0);
+       log_error(errcode(ERRCODE_UNDEFINED_FILE),
+                         _("could not find a \"%s\" to execute"), argv0);
        return -1;
 }
 
@@ -240,10 +251,8 @@ resolve_symlinks(char *path)
         */
        if (!getcwd(orig_wd, MAXPGPATH))
        {
-               int                     save_errno = errno;
-
-               log_error(_("could not identify current directory: %s"),
-                                 strerror(save_errno));
+               log_error(errcode_for_file_access(),
+                                 _("could not identify current directory: %m"));
                return -1;
        }
 
@@ -258,10 +267,8 @@ resolve_symlinks(char *path)
                        *lsep = '\0';
                        if (chdir(path) == -1)
                        {
-                               int                     save_errno = errno;
-
-                               log_error4(_("could not change directory to \"%s\": %s"),
-                                                  path, strerror(save_errno));
+                               log_error(errcode_for_file_access(),
+                                                 _("could not change directory to \"%s\": %m"), path);
                                return -1;
                        }
                        fname = lsep + 1;
@@ -273,10 +280,12 @@ resolve_symlinks(char *path)
                        !S_ISLNK(buf.st_mode))
                        break;
 
+               errno = 0;
                rllen = readlink(fname, link_buf, sizeof(link_buf));
                if (rllen < 0 || rllen >= sizeof(link_buf))
                {
-                       log_error(_("could not read symbolic link \"%s\""), fname);
+                       log_error(errcode_for_file_access(),
+                                         _("could not read symbolic link \"%s\": %m"), fname);
                        return -1;
                }
                link_buf[rllen] = '\0';
@@ -288,10 +297,8 @@ resolve_symlinks(char *path)
 
        if (!getcwd(path, MAXPGPATH))
        {
-               int                     save_errno = errno;
-
-               log_error(_("could not identify current directory: %s"),
-                                 strerror(save_errno));
+               log_error(errcode_for_file_access(),
+                                 _("could not identify current directory: %m"));
                return -1;
        }
        join_path_components(path, path, link_buf);
@@ -299,10 +306,8 @@ resolve_symlinks(char *path)
 
        if (chdir(orig_wd) == -1)
        {
-               int                     save_errno = errno;
-
-               log_error4(_("could not change directory to \"%s\": %s"),
-                                  orig_wd, strerror(save_errno));
+               log_error(errcode_for_file_access(),
+                                 _("could not change directory to \"%s\": %m"), orig_wd);
                return -1;
        }
 #endif                                                 /* HAVE_READLINK */
@@ -532,20 +537,15 @@ pclose_check(FILE *stream)
        if (exitstatus == -1)
        {
                /* pclose() itself failed, and hopefully set errno */
-               int                     save_errno = errno;
-
-               log_error(_("pclose failed: %s"),
-                                 strerror(save_errno));
+               log_error(errcode(ERRCODE_SYSTEM_ERROR),
+                                 _("pclose failed: %m"));
        }
        else
        {
                reason = wait_result_to_str(exitstatus);
-               log_error("%s", reason);
-#ifdef FRONTEND
-               free(reason);
-#else
+               log_error(errcode(ERRCODE_SYSTEM_ERROR),
+                                 "%s", reason);
                pfree(reason);
-#endif
        }
        return exitstatus;
 }
@@ -666,19 +666,24 @@ AddUserToTokenDacl(HANDLE hToken)
                        ptdd = (TOKEN_DEFAULT_DACL *) LocalAlloc(LPTR, dwSize);
                        if (ptdd == NULL)
                        {
-                               log_error("could not allocate %lu bytes of memory", dwSize);
+                               log_error(errcode(ERRCODE_OUT_OF_MEMORY),
+                                                 _("out of memory"));
                                goto cleanup;
                        }
 
                        if (!GetTokenInformation(hToken, tic, (LPVOID) ptdd, dwSize, &dwSize))
                        {
-                               log_error("could not get token information: error code %lu", GetLastError());
+                               log_error(errcode(ERRCODE_SYSTEM_ERROR),
+                                                 "could not get token information: error code %lu",
+                                                 GetLastError());
                                goto cleanup;
                        }
                }
                else
                {
-                       log_error("could not get token information buffer size: error code %lu", GetLastError());
+                       log_error(errcode(ERRCODE_SYSTEM_ERROR),
+                                         "could not get token information buffer size: error code %lu",
+                                         GetLastError());
                        goto cleanup;
                }
        }
@@ -688,7 +693,9 @@ AddUserToTokenDacl(HANDLE hToken)
                                                   (DWORD) sizeof(ACL_SIZE_INFORMATION),
                                                   AclSizeInformation))
        {
-               log_error("could not get ACL information: error code %lu", GetLastError());
+               log_error(errcode(ERRCODE_SYSTEM_ERROR),
+                                 "could not get ACL information: error code %lu",
+                                 GetLastError());
                goto cleanup;
        }
 
@@ -704,13 +711,15 @@ AddUserToTokenDacl(HANDLE hToken)
        pacl = (PACL) LocalAlloc(LPTR, dwNewAclSize);
        if (pacl == NULL)
        {
-               log_error("could not allocate %lu bytes of memory", dwNewAclSize);
+               log_error(errcode(ERRCODE_OUT_OF_MEMORY),
+                                 _("out of memory"));
                goto cleanup;
        }
 
        if (!InitializeAcl(pacl, dwNewAclSize, ACL_REVISION))
        {
-               log_error("could not initialize ACL: error code %lu", GetLastError());
+               log_error(errcode(ERRCODE_SYSTEM_ERROR),
+                                 "could not initialize ACL: error code %lu", GetLastError());
                goto cleanup;
        }
 
@@ -719,13 +728,15 @@ AddUserToTokenDacl(HANDLE hToken)
        {
                if (!GetAce(ptdd->DefaultDacl, i, (LPVOID *) &pace))
                {
-                       log_error("could not get ACE: error code %lu", GetLastError());
+                       log_error(errcode(ERRCODE_SYSTEM_ERROR),
+                                         "could not get ACE: error code %lu", GetLastError());
                        goto cleanup;
                }
 
                if (!AddAce(pacl, ACL_REVISION, MAXDWORD, pace, ((PACE_HEADER) pace)->AceSize))
                {
-                       log_error("could not add ACE: error code %lu", GetLastError());
+                       log_error(errcode(ERRCODE_SYSTEM_ERROR),
+                                         "could not add ACE: error code %lu", GetLastError());
                        goto cleanup;
                }
        }
@@ -733,7 +744,9 @@ AddUserToTokenDacl(HANDLE hToken)
        /* Add the new ACE for the current user */
        if (!AddAccessAllowedAceEx(pacl, ACL_REVISION, OBJECT_INHERIT_ACE, GENERIC_ALL, pTokenUser->User.Sid))
        {
-               log_error("could not add access allowed ACE: error code %lu", GetLastError());
+               log_error(errcode(ERRCODE_SYSTEM_ERROR),
+                                 "could not add access allowed ACE: error code %lu",
+                                 GetLastError());
                goto cleanup;
        }
 
@@ -742,7 +755,9 @@ AddUserToTokenDacl(HANDLE hToken)
 
        if (!SetTokenInformation(hToken, tic, (LPVOID) &tddNew, dwNewAclSize))
        {
-               log_error("could not set token information: error code %lu", GetLastError());
+               log_error(errcode(ERRCODE_SYSTEM_ERROR),
+                                 "could not set token information: error code %lu",
+                                 GetLastError());
                goto cleanup;
        }
 
@@ -788,13 +803,16 @@ GetTokenUser(HANDLE hToken, PTOKEN_USER *ppTokenUser)
 
                        if (*ppTokenUser == NULL)
                        {
-                               log_error("could not allocate %lu bytes of memory", dwLength);
+                               log_error(errcode(ERRCODE_OUT_OF_MEMORY),
+                                                 _("out of memory"));
                                return FALSE;
                        }
                }
                else
                {
-                       log_error("could not get token information buffer size: error code %lu", GetLastError());
+                       log_error(errcode(ERRCODE_SYSTEM_ERROR),
+                                         "could not get token information buffer size: error code %lu",
+                                         GetLastError());
                        return FALSE;
                }
        }
@@ -808,7 +826,9 @@ GetTokenUser(HANDLE hToken, PTOKEN_USER *ppTokenUser)
                LocalFree(*ppTokenUser);
                *ppTokenUser = NULL;
 
-               log_error("could not get token information: error code %lu", GetLastError());
+               log_error(errcode(ERRCODE_SYSTEM_ERROR),
+                                 "could not get token information: error code %lu",
+                                 GetLastError());
                return FALSE;
        }