pg.ddx.io pgsql-hackers@postgresql.org mailing list archive help / color / mirror / Atom feed From: Tom Lane <tgl@sss.pgh.pa.us>
To: Thomas Munro <thomas.munro@enterprisedb.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Allowing printf("%m") only where it actually works
Date: Sat, 26 May 2018 12:21:33 -0400
Message-ID: <6025.1527351693@sss.pgh.pa.us> (raw )
In-Reply-To: <31054.1526918600@sss.pgh.pa.us >
References: <2975.1526862605@sss.pgh.pa.us >
<CAEepm=29o4wi5WEBFfwhBUuiYSymbTcORw_cvdaaxPPoAkrsvA@mail.gmail.com >
<14176.1526874887@sss.pgh.pa.us >
<24986.1526877413@sss.pgh.pa.us >
<CAEepm=2-7qBw6U-vy2OgL=Z=J6VeOzPS5jsK2PwmG0PWTDv1cg@mail.gmail.com >
<31054.1526918600@sss.pgh.pa.us >
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
Attachments:
make-exec.c-do-error-reporting-like-everyplace-else.patch (text/x-diff, 13.1K) download
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;
}
=
view thread (50+ messages) latest in thread
Message-ID: <6025.1527351693@sss.pgh.pa.us>
Permalink: ../6025.1527351693@sss.pgh.pa.us/
Also on: postgresql.org/message-id/6025.1527351693@sss.pgh.pa.us
copy link · copy postgr.es · copy git am
reply Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tgl@sss.pgh.pa.us, thomas.munro@enterprisedb.com, pgsql-hackers@lists.postgresql.org
Subject: Re: Allowing printf("%m") only where it actually works
In-Reply-To: <6025.1527351693@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox