Re: Allowing printf("%m") only where it actually works - Mailing list pgsql-hackers

I wrote:
> Thomas Munro <thomas.munro@enterprisedb.com> writes:
>> Here's an experimental way to do that, if you don't mind depending on
>> gory details of libc implementations (ie knowledge of what it expands
>> too).  Not sure how to avoid that since it's a macro on all modern
>> systems, and we don't have a way to temporarily redefine a macro.  If
>> you enable it for just ereport(), it compiles cleanly after 81256cd
>> (but fails on earlier commits).  If you enable it for elog() too then
>> it finds problems with exec.c.

> Hmm ... that's pretty duff code in exec.c, isn't it.  Aside from the
> question of errno unsafety, it's using elog where it really ought to be
> using ereport, it's not taking any thought for the reported SQLSTATE,
> etc.  I'm hesitant to mess with it mere hours before the beta wrap,
> but we really oughta improve that.

I wrote up a patch that makes src/common/exec.c do error reporting more
like other frontend/backend-common files (attached).  Now that I've done
so, though, I'm having second thoughts.  The thing that I don't like
about this is that it doubles the number of translatable strings created
by this file.  While there's not *that* many of them, translators have to
deal with each one several times because this file is included by several
different frontend programs.  So that seems like a rather high price to
pay to deal with what, at present, is a purely hypothetical hazard.
(Basically what this would protect against is elog_start changing errno,
which it doesn't.)  Improving the errcode situation is somewhat useful,
but still maybe it's not worth the cost.

Another approach we could consider is keeping exec.c's one-off approach
to error handling and letting it redefine pg_prevent_errno_in_scope() as
empty.  But that's ugly.

Or we could make the affected call sites work like this:

        int save_errno = errno;

        log_error(_("could not identify current directory: %s"),
                  strerror(save_errno));

which on the whole might be the most expedient thing.

            regards, tom lane

diff --git a/src/common/exec.c b/src/common/exec.c
index 4df16cd..f0d52e1 100644
*** a/src/common/exec.c
--- b/src/common/exec.c
***************
*** 25,40 ****
  #include <sys/wait.h>
  #include <unistd.h>

- #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)
- #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))
- #endif
-
  #ifdef _MSC_VER
  #define getcwd(cwd,len)  GetCurrentDirectory(len, cwd)
  #endif
--- 25,30 ----
*************** find_my_exec(const char *argv0, char *re
*** 124,131 ****

      if (!getcwd(cwd, MAXPGPATH))
      {
!         log_error(_("could not identify current directory: %s"),
!                   strerror(errno));
          return -1;
      }

--- 114,127 ----

      if (!getcwd(cwd, MAXPGPATH))
      {
! #ifndef FRONTEND
!         ereport(LOG,
!                 (errcode_for_file_access(),
!                  errmsg("could not identify current directory: %m")));
! #else
!         fprintf(stderr, _("could not identify current directory: %s\n"),
!                 strerror(errno));
! #endif
          return -1;
      }

*************** find_my_exec(const char *argv0, char *re
*** 143,149 ****
          if (validate_exec(retpath) == 0)
              return resolve_symlinks(retpath);

!         log_error(_("invalid binary \"%s\""), retpath);
          return -1;
      }

--- 139,149 ----
          if (validate_exec(retpath) == 0)
              return resolve_symlinks(retpath);

! #ifndef FRONTEND
!         ereport(LOG, (errmsg("invalid binary \"%s\"", retpath)));
! #else
!         fprintf(stderr, _("invalid binary \"%s\"\n"), retpath);
! #endif
          return -1;
      }

*************** find_my_exec(const char *argv0, char *re
*** 192,205 ****
                  case -1:        /* wasn't even a candidate, keep looking */
                      break;
                  case -2:        /* found but disqualified */
!                     log_error(_("could not read binary \"%s\""),
!                               retpath);
                      break;
              }
          } while (*endp);
      }

!     log_error(_("could not find a \"%s\" to execute"), argv0);
      return -1;
  }

--- 192,214 ----
                  case -1:        /* wasn't even a candidate, keep looking */
                      break;
                  case -2:        /* found but disqualified */
! #ifndef FRONTEND
!                     ereport(LOG, (errmsg("could not read binary \"%s\"",
!                                          retpath)));
! #else
!                     fprintf(stderr, _("could not read binary \"%s\"\n"),
!                             retpath);
! #endif
                      break;
              }
          } while (*endp);
      }

! #ifndef FRONTEND
!     ereport(LOG, (errmsg("could not find a \"%s\" to execute", argv0)));
! #else
!     fprintf(stderr, _("could not find a \"%s\" to execute\n"), argv0);
! #endif
      return -1;
  }

*************** resolve_symlinks(char *path)
*** 238,245 ****
       */
      if (!getcwd(orig_wd, MAXPGPATH))
      {
!         log_error(_("could not identify current directory: %s"),
!                   strerror(errno));
          return -1;
      }

--- 247,260 ----
       */
      if (!getcwd(orig_wd, MAXPGPATH))
      {
! #ifndef FRONTEND
!         ereport(LOG,
!                 (errcode_for_file_access(),
!                  errmsg("could not identify current directory: %m")));
! #else
!         fprintf(stderr, _("could not identify current directory: %s\n"),
!                 strerror(errno));
! #endif
          return -1;
      }

*************** resolve_symlinks(char *path)
*** 254,260 ****
              *lsep = '\0';
              if (chdir(path) == -1)
              {
!                 log_error4(_("could not change directory to \"%s\": %s"), path, strerror(errno));
                  return -1;
              }
              fname = lsep + 1;
--- 269,283 ----
              *lsep = '\0';
              if (chdir(path) == -1)
              {
! #ifndef FRONTEND
!                 ereport(LOG,
!                         (errcode_for_file_access(),
!                          errmsg("could not change directory to \"%s\": %m",
!                                 path)));
! #else
!                 fprintf(stderr, _("could not change directory to \"%s\": %s\n"),
!                         path, strerror(errno));
! #endif
                  return -1;
              }
              fname = lsep + 1;
*************** resolve_symlinks(char *path)
*** 269,275 ****
          rllen = readlink(fname, link_buf, sizeof(link_buf));
          if (rllen < 0 || rllen >= sizeof(link_buf))
          {
!             log_error(_("could not read symbolic link \"%s\""), fname);
              return -1;
          }
          link_buf[rllen] = '\0';
--- 292,303 ----
          rllen = readlink(fname, link_buf, sizeof(link_buf));
          if (rllen < 0 || rllen >= sizeof(link_buf))
          {
! #ifndef FRONTEND
!             ereport(LOG,
!                     (errmsg("could not read symbolic link \"%s\"", fname)));
! #else
!             fprintf(stderr, _("could not read symbolic link \"%s\"\n"), fname);
! #endif
              return -1;
          }
          link_buf[rllen] = '\0';
*************** resolve_symlinks(char *path)
*** 281,288 ****

      if (!getcwd(path, MAXPGPATH))
      {
!         log_error(_("could not identify current directory: %s"),
!                   strerror(errno));
          return -1;
      }
      join_path_components(path, path, link_buf);
--- 309,322 ----

      if (!getcwd(path, MAXPGPATH))
      {
! #ifndef FRONTEND
!         ereport(LOG,
!                 (errcode_for_file_access(),
!                  errmsg("could not identify current directory: %m")));
! #else
!         fprintf(stderr, _("could not identify current directory: %s\n"),
!                 strerror(errno));
! #endif
          return -1;
      }
      join_path_components(path, path, link_buf);
*************** resolve_symlinks(char *path)
*** 290,296 ****

      if (chdir(orig_wd) == -1)
      {
!         log_error4(_("could not change directory to \"%s\": %s"), orig_wd, strerror(errno));
          return -1;
      }
  #endif                            /* HAVE_READLINK */
--- 324,338 ----

      if (chdir(orig_wd) == -1)
      {
! #ifndef FRONTEND
!         ereport(LOG,
!                 (errcode_for_file_access(),
!                  errmsg("could not change directory to \"%s\": %m",
!                         orig_wd)));
! #else
!         fprintf(stderr, _("could not change directory to \"%s\": %s\n"),
!                 orig_wd, strerror(errno));
! #endif
          return -1;
      }
  #endif                            /* HAVE_READLINK */
*************** pclose_check(FILE *stream)
*** 520,536 ****
      if (exitstatus == -1)
      {
          /* pclose() itself failed, and hopefully set errno */
!         log_error(_("pclose failed: %s"), strerror(errno));
      }
      else
      {
          reason = wait_result_to_str(exitstatus);
!         log_error("%s", reason);
! #ifdef FRONTEND
!         free(reason);
  #else
!         pfree(reason);
  #endif
      }
      return exitstatus;
  }
--- 562,582 ----
      if (exitstatus == -1)
      {
          /* pclose() itself failed, and hopefully set errno */
! #ifndef FRONTEND
!         elog(LOG, "pclose failed: %m");
! #else
!         fprintf(stderr, "pclose failed: %s\n", strerror(errno));
! #endif
      }
      else
      {
          reason = wait_result_to_str(exitstatus);
! #ifndef FRONTEND
!         elog(LOG, "%s", reason);
  #else
!         fprintf(stderr, "%s\n", reason);
  #endif
+         pfree(reason);
      }
      return exitstatus;
  }
*************** AddUserToTokenDacl(HANDLE hToken)
*** 651,669 ****
              ptdd = (TOKEN_DEFAULT_DACL *) LocalAlloc(LPTR, dwSize);
              if (ptdd == NULL)
              {
!                 log_error("could not allocate %lu bytes of memory", dwSize);
                  goto cleanup;
              }

              if (!GetTokenInformation(hToken, tic, (LPVOID) ptdd, dwSize, &dwSize))
              {
!                 log_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());
              goto cleanup;
          }
      }
--- 697,727 ----
              ptdd = (TOKEN_DEFAULT_DACL *) LocalAlloc(LPTR, dwSize);
              if (ptdd == NULL)
              {
! #ifndef FRONTEND
!                 elog(LOG, "out of memory");
! #else
!                 fprintf(stderr, "out of memory\n");
! #endif
                  goto cleanup;
              }

              if (!GetTokenInformation(hToken, tic, (LPVOID) ptdd, dwSize, &dwSize))
              {
! #ifndef FRONTEND
!                 elog(LOG, "could not get token information: error code %lu", GetLastError());
! #else
!                 fprintf(stderr, "could not get token information: error code %lu\n", GetLastError());
! #endif
                  goto cleanup;
              }
          }
          else
          {
! #ifndef FRONTEND
!             elog(LOG, "could not get token information buffer size: error code %lu", GetLastError());
! #else
!             fprintf(stderr, "could not get token information buffer size: error code %lu\n", GetLastError());
! #endif
              goto cleanup;
          }
      }
*************** AddUserToTokenDacl(HANDLE hToken)
*** 673,679 ****
                             (DWORD) sizeof(ACL_SIZE_INFORMATION),
                             AclSizeInformation))
      {
!         log_error("could not get ACL information: error code %lu", GetLastError());
          goto cleanup;
      }

--- 731,741 ----
                             (DWORD) sizeof(ACL_SIZE_INFORMATION),
                             AclSizeInformation))
      {
! #ifndef FRONTEND
!         elog(LOG, "could not get ACL information: error code %lu", GetLastError());
! #else
!         fprintf(stderr, "could not get ACL information: error code %lu\n", GetLastError());
! #endif
          goto cleanup;
      }

*************** AddUserToTokenDacl(HANDLE hToken)
*** 689,701 ****
      pacl = (PACL) LocalAlloc(LPTR, dwNewAclSize);
      if (pacl == NULL)
      {
!         log_error("could not allocate %lu bytes of memory", dwNewAclSize);
          goto cleanup;
      }

      if (!InitializeAcl(pacl, dwNewAclSize, ACL_REVISION))
      {
!         log_error("could not initialize ACL: error code %lu", GetLastError());
          goto cleanup;
      }

--- 751,771 ----
      pacl = (PACL) LocalAlloc(LPTR, dwNewAclSize);
      if (pacl == NULL)
      {
! #ifndef FRONTEND
!         elog(LOG, "out of memory");
! #else
!         fprintf(stderr, "out of memory\n");
! #endif
          goto cleanup;
      }

      if (!InitializeAcl(pacl, dwNewAclSize, ACL_REVISION))
      {
! #ifndef FRONTEND
!         elog(LOG, "could not initialize ACL: error code %lu", GetLastError());
! #else
!         fprintf(stderr, "could not initialize ACL: error code %lu\n", GetLastError());
! #endif
          goto cleanup;
      }

*************** AddUserToTokenDacl(HANDLE hToken)
*** 704,716 ****
      {
          if (!GetAce(ptdd->DefaultDacl, i, (LPVOID *) &pace))
          {
!             log_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());
              goto cleanup;
          }
      }
--- 774,794 ----
      {
          if (!GetAce(ptdd->DefaultDacl, i, (LPVOID *) &pace))
          {
! #ifndef FRONTEND
!             elog(LOG, "could not get ACE: error code %lu", GetLastError());
! #else
!             fprintf(stderr, "could not get ACE: error code %lu\n", GetLastError());
! #endif
              goto cleanup;
          }

          if (!AddAce(pacl, ACL_REVISION, MAXDWORD, pace, ((PACE_HEADER) pace)->AceSize))
          {
! #ifndef FRONTEND
!             elog(LOG, "could not add ACE: error code %lu", GetLastError());
! #else
!             fprintf(stderr, "could not add ACE: error code %lu\n", GetLastError());
! #endif
              goto cleanup;
          }
      }
*************** AddUserToTokenDacl(HANDLE hToken)
*** 718,724 ****
      /* 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());
          goto cleanup;
      }

--- 796,806 ----
      /* Add the new ACE for the current user */
      if (!AddAccessAllowedAceEx(pacl, ACL_REVISION, OBJECT_INHERIT_ACE, GENERIC_ALL, pTokenUser->User.Sid))
      {
! #ifndef FRONTEND
!         elog(LOG, "could not add access allowed ACE: error code %lu", GetLastError());
! #else
!         fprintf(stderr, "could not add access allowed ACE: error code %lu\n", GetLastError());
! #endif
          goto cleanup;
      }

*************** AddUserToTokenDacl(HANDLE hToken)
*** 727,733 ****

      if (!SetTokenInformation(hToken, tic, (LPVOID) &tddNew, dwNewAclSize))
      {
!         log_error("could not set token information: error code %lu", GetLastError());
          goto cleanup;
      }

--- 809,819 ----

      if (!SetTokenInformation(hToken, tic, (LPVOID) &tddNew, dwNewAclSize))
      {
! #ifndef FRONTEND
!         elog(LOG, "could not set token information: error code %lu", GetLastError());
! #else
!         fprintf(stderr, "could not set token information: error code %lu\n", GetLastError());
! #endif
          goto cleanup;
      }

*************** GetTokenUser(HANDLE hToken, PTOKEN_USER
*** 773,785 ****

              if (*ppTokenUser == NULL)
              {
!                 log_error("could not allocate %lu bytes of memory", dwLength);
                  return FALSE;
              }
          }
          else
          {
!             log_error("could not get token information buffer size: error code %lu", GetLastError());
              return FALSE;
          }
      }
--- 859,879 ----

              if (*ppTokenUser == NULL)
              {
! #ifndef FRONTEND
!                 elog(LOG, "out of memory");
! #else
!                 fprintf(stderr, "out of memory\n");
! #endif
                  return FALSE;
              }
          }
          else
          {
! #ifndef FRONTEND
!             elog(LOG, "could not get token information buffer size: error code %lu", GetLastError());
! #else
!             fprintf(stderr, "could not get token information buffer size: error code %lu\n", GetLastError());
! #endif
              return FALSE;
          }
      }
*************** GetTokenUser(HANDLE hToken, PTOKEN_USER
*** 793,799 ****
          LocalFree(*ppTokenUser);
          *ppTokenUser = NULL;

!         log_error("could not get token information: error code %lu", GetLastError());
          return FALSE;
      }

--- 887,897 ----
          LocalFree(*ppTokenUser);
          *ppTokenUser = NULL;

! #ifndef FRONTEND
!         elog(LOG, "could not get token information: error code %lu", GetLastError());
! #else
!         fprintf(stderr, "could not get token information: error code %lu\n", GetLastError());
! #endif
          return FALSE;
      }