Received: from malur.postgresql.org ([2a02:16a8:dc51::56]) by arkaria.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1fMc1G-0006ct-1Y for pgsql-hackers@arkaria.postgresql.org; Sat, 26 May 2018 16:26:14 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1fMc1F-0002sT-49 for pgsql-hackers@arkaria.postgresql.org; Sat, 26 May 2018 16:26:13 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1fMbwz-0002HM-O3 for pgsql-hackers@lists.postgresql.org; Sat, 26 May 2018 16:21:49 +0000 Received: from sss.pgh.pa.us ([66.207.139.130]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1fMbwn-0001jv-MD for pgsql-hackers@lists.postgresql.org; Sat, 26 May 2018 16:21:48 +0000 Received: from sss1.sss.pgh.pa.us (localhost [127.0.0.1]) by sss.pgh.pa.us (8.14.4/8.14.4) with ESMTP id w4QGLXX7006026; Sat, 26 May 2018 12:21:33 -0400 From: Tom Lane To: Thomas Munro cc: PostgreSQL Hackers Subject: Re: Allowing printf("%m") only where it actually works In-reply-to: <31054.1526918600@sss.pgh.pa.us> References: <2975.1526862605@sss.pgh.pa.us> <14176.1526874887@sss.pgh.pa.us> <24986.1526877413@sss.pgh.pa.us> <31054.1526918600@sss.pgh.pa.us> Comments: In-reply-to Tom Lane message dated "Mon, 21 May 2018 12:03:20 -0400" MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0" Content-ID: <5724.1527351204.0@sss.pgh.pa.us> Date: Sat, 26 May 2018 12:21:33 -0400 Message-ID: <6025.1527351693@sss.pgh.pa.us> List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk ------- =_aaaaaaaaaa0 Content-Type: text/plain; charset="us-ascii" Content-ID: <5724.1527351204.1@sss.pgh.pa.us> I wrote: > Thomas Munro 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 ------- =_aaaaaaaaaa0 Content-Type: text/x-diff; name="make-exec.c-do-error-reporting-like-everyplace-else.patch"; charset="us-ascii" Content-ID: <5724.1527351204.2@sss.pgh.pa.us> Content-Description: make-exec.c-do-error-reporting-like-everyplace-else.patch Content-Transfer-Encoding: quoted-printable 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 #include = - #ifndef FRONTEND - /* We use only 3- and 4-parameter elog calls in this file, for simplicit= y */ - /* 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) =3D=3D 0) return resolve_symlinks(retpath); = ! log_error(_("invalid binary \"%s\""), retpath); return -1; } = --- 139,149 ---- if (validate_exec(retpath) =3D=3D 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 =3D '\0'; if (chdir(path) =3D=3D -1) { ! log_error4(_("could not change directory to \"%s\": %s"), path, stre= rror(errno)); return -1; } fname =3D lsep + 1; --- 269,283 ---- *lsep =3D '\0'; if (chdir(path) =3D=3D -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 =3D lsep + 1; *************** resolve_symlinks(char *path) *** 269,275 **** rllen =3D readlink(fname, link_buf, sizeof(link_buf)); if (rllen < 0 || rllen >=3D sizeof(link_buf)) { ! log_error(_("could not read symbolic link \"%s\""), fname); return -1; } link_buf[rllen] =3D '\0'; --- 292,303 ---- rllen =3D readlink(fname, link_buf, sizeof(link_buf)); if (rllen < 0 || rllen >=3D 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] =3D '\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) =3D=3D -1) { ! log_error4(_("could not change directory to \"%s\": %s"), orig_wd, str= error(errno)); return -1; } #endif /* HAVE_READLINK */ --- 324,338 ---- = if (chdir(orig_wd) =3D=3D -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 =3D=3D -1) { /* pclose() itself failed, and hopefully set errno */ ! log_error(_("pclose failed: %s"), strerror(errno)); } else { reason =3D wait_result_to_str(exitstatus); ! log_error("%s", reason); ! #ifdef FRONTEND ! free(reason); #else ! pfree(reason); #endif } return exitstatus; } --- 562,582 ---- if (exitstatus =3D=3D -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 =3D 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 =3D (TOKEN_DEFAULT_DACL *) LocalAlloc(LPTR, dwSize); if (ptdd =3D=3D 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", GetLast= Error()); goto cleanup; } } else { ! log_error("could not get token information buffer size: error code %l= u", GetLastError()); goto cleanup; } } --- 697,727 ---- ptdd =3D (TOKEN_DEFAULT_DACL *) LocalAlloc(LPTR, dwSize); if (ptdd =3D=3D 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", GetLast= Error()); ! #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 %l= u", GetLastError()); ! #else ! fprintf(stderr, "could not get token information buffer size: error c= ode %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", GetLastErro= r()); goto cleanup; } = --- 731,741 ---- (DWORD) sizeof(ACL_SIZE_INFORMATION), AclSizeInformation)) { ! #ifndef FRONTEND ! elog(LOG, "could not get ACL information: error code %lu", GetLastErro= r()); ! #else ! fprintf(stderr, "could not get ACL information: error code %lu\n", Get= LastError()); ! #endif goto cleanup; } = *************** AddUserToTokenDacl(HANDLE hToken) *** 689,701 **** pacl =3D (PACL) LocalAlloc(LPTR, dwNewAclSize); if (pacl =3D=3D 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 =3D (PACL) LocalAlloc(LPTR, dwNewAclSize); if (pacl =3D=3D 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", GetLastE= rror()); ! #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, GENE= RIC_ALL, pTokenUser->User.Sid)) { ! log_error("could not add access allowed ACE: error code %lu", GetLastE= rror()); goto cleanup; } = --- 796,806 ---- /* Add the new ACE for the current user */ if (!AddAccessAllowedAceEx(pacl, ACL_REVISION, OBJECT_INHERIT_ACE, GENE= RIC_ALL, pTokenUser->User.Sid)) { ! #ifndef FRONTEND ! elog(LOG, "could not add access allowed ACE: error code %lu", GetLastE= rror()); ! #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", GetLastEr= ror()); goto cleanup; } = --- 809,819 ---- = if (!SetTokenInformation(hToken, tic, (LPVOID) &tddNew, dwNewAclSize)) { ! #ifndef FRONTEND ! elog(LOG, "could not set token information: error code %lu", GetLastEr= ror()); ! #else ! fprintf(stderr, "could not set token information: error code %lu\n", G= etLastError()); ! #endif goto cleanup; } = *************** GetTokenUser(HANDLE hToken, PTOKEN_USER = *** 773,785 **** = if (*ppTokenUser =3D=3D 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 %l= u", GetLastError()); return FALSE; } } --- 859,879 ---- = if (*ppTokenUser =3D=3D 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 %l= u", GetLastError()); ! #else ! fprintf(stderr, "could not get token information buffer size: error c= ode %lu\n", GetLastError()); ! #endif return FALSE; } } *************** GetTokenUser(HANDLE hToken, PTOKEN_USER = *** 793,799 **** LocalFree(*ppTokenUser); *ppTokenUser =3D NULL; = ! log_error("could not get token information: error code %lu", GetLastEr= ror()); return FALSE; } = --- 887,897 ---- LocalFree(*ppTokenUser); *ppTokenUser =3D NULL; = ! #ifndef FRONTEND ! elog(LOG, "could not get token information: error code %lu", GetLastEr= ror()); ! #else ! fprintf(stderr, "could not get token information: error code %lu\n", G= etLastError()); ! #endif return FALSE; } = ------- =_aaaaaaaaaa0--