From: Tom Lane <tgl@sss.pgh.pa.us>
To: Thomas Munro <thomas.munro@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Cc: rosset.filipe@gmail.com
Subject: Re: sys_siglist[] is causing us trouble again
Date: Wed, 15 Jul 2020 20:14:36 -0400
Message-ID: <3218392.1594858476@sss.pgh.pa.us> (raw)
In-Reply-To: <CA+hUKGLJJjGz+qi+t7qDkFYO2=4a4YECXucH8WZoein8LPbeAA@mail.gmail.com>
References: <3179114.1594853308@sss.pgh.pa.us>
<CA+hUKGLJJjGz+qi+t7qDkFYO2=4a4YECXucH8WZoein8LPbeAA@mail.gmail.com>
Thomas Munro <thomas.munro@gmail.com> writes:
> On Thu, Jul 16, 2020 at 10:48 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> We haven't changed anything, ergo something changed at the OS level.
> It looks like glibc very recently decided[1] to hide the declaration,
> but we're using a cached configure test result.
Right. So, modulo the mis-cached result, what would happen if we do
nothing is that the back branches would lose the ability to translate
signal numbers to strings on bleeding-edge glibc. I don't think we
want that, so we need to back-patch. Attached is a lightly tested
patch for v11. (This includes 7570df0f3 as well, so that
pgstrsignal.c will be the same in all branches.)
regards, tom lane
Attachments:replace-sys_siglist-with-strsignal-v11.patch (text/x-diff, 12.3K) downloaddiff --git a/configure b/configureindex 4e1b4be7fb..7382a34d60 100755--- a/configure+++ b/configure@@ -15004,7 +15004,7 @@ fi
LIBS_including_readline="$LIBS"
LIBS=`echo "$LIBS" | sed -e 's/-ledit//g' -e 's/-lreadline//g'`
-for ac_func in cbrt clock_gettime dlopen fdatasync getifaddrs getpeerucred getrlimit mbstowcs_l memmove poll posix_fallocate pstat pthread_is_threaded_np readlink setproctitle setsid shm_open symlink sync_file_range uselocale utime utimes wcstombs_l+for ac_func in cbrt clock_gettime dlopen fdatasync getifaddrs getpeerucred getrlimit mbstowcs_l memmove poll posix_fallocate pstat pthread_is_threaded_np readlink setproctitle setsid shm_open strsignal symlink sync_file_range uselocale utime utimes wcstombs_l
do :
as_ac_var=`$as_echo "ac_cv_func_$ac_func" | $as_tr_sh`
ac_fn_c_check_func "$LINENO" "$ac_func" "$as_ac_var"
@@ -15893,24 +15893,6 @@ esac
fi
-ac_fn_c_check_decl "$LINENO" "sys_siglist" "ac_cv_have_decl_sys_siglist" "#include <signal.h>-/* NetBSD declares sys_siglist in unistd.h. */-#ifdef HAVE_UNISTD_H-# include <unistd.h>-#endif--"-if test "x$ac_cv_have_decl_sys_siglist" = xyes; then :- ac_have_decl=1-else- ac_have_decl=0-fi--cat >>confdefs.h <<_ACEOF-#define HAVE_DECL_SYS_SIGLIST $ac_have_decl-_ACEOF--
ac_fn_c_check_func "$LINENO" "syslog" "ac_cv_func_syslog"
if test "x$ac_cv_func_syslog" = xyes; then :
ac_fn_c_check_header_mongrel "$LINENO" "syslog.h" "ac_cv_header_syslog_h" "$ac_includes_default"
diff --git a/configure.in b/configure.inindex fab1658bca..defcb8ff99 100644--- a/configure.in+++ b/configure.in@@ -1622,6 +1622,7 @@ AC_CHECK_FUNCS(m4_normalize([
setproctitle
setsid
shm_open
+ strsignal
symlink
sync_file_range
uselocale
@@ -1821,14 +1822,6 @@ if test "$PORTNAME" = "cygwin"; then
AC_LIBOBJ(dirmod)
fi
-AC_CHECK_DECLS([sys_siglist], [], [],-[#include <signal.h>-/* NetBSD declares sys_siglist in unistd.h. */-#ifdef HAVE_UNISTD_H-# include <unistd.h>-#endif-])-
AC_CHECK_FUNC(syslog,
[AC_CHECK_HEADER(syslog.h,
[AC_DEFINE(HAVE_SYSLOG, 1, [Define to 1 if you have the syslog interface.])])])
diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.cindex 252220c770..08577f5e5f 100644--- a/src/backend/postmaster/pgarch.c+++ b/src/backend/postmaster/pgarch.c@@ -596,17 +596,10 @@ pgarch_archiveXlog(char *xlog)
errhint("See C include file \"ntstatus.h\" for a description of the hexadecimal value."),
errdetail("The failed archive command was: %s",
xlogarchcmd)));
-#elif defined(HAVE_DECL_SYS_SIGLIST) && HAVE_DECL_SYS_SIGLIST- ereport(lev,- (errmsg("archive command was terminated by signal %d: %s",- WTERMSIG(rc),- WTERMSIG(rc) < NSIG ? sys_siglist[WTERMSIG(rc)] : "(unknown)"),- errdetail("The failed archive command was: %s",- xlogarchcmd)));
#else
ereport(lev,
- (errmsg("archive command was terminated by signal %d",- WTERMSIG(rc)),+ (errmsg("archive command was terminated by signal %d: %s",+ WTERMSIG(rc), pg_strsignal(WTERMSIG(rc))),
errdetail("The failed archive command was: %s",
xlogarchcmd)));
#endif
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.cindex 3bfc299be1..75a9e07041 100644--- a/src/backend/postmaster/postmaster.c+++ b/src/backend/postmaster/postmaster.c@@ -3563,6 +3563,7 @@ LogChildExit(int lev, const char *procname, int pid, int exitstatus)
procname, pid, WEXITSTATUS(exitstatus)),
activity ? errdetail("Failed process was running: %s", activity) : 0));
else if (WIFSIGNALED(exitstatus))
+ {
#if defined(WIN32)
ereport(lev,
@@ -3573,7 +3574,7 @@ LogChildExit(int lev, const char *procname, int pid, int exitstatus)
procname, pid, WTERMSIG(exitstatus)),
errhint("See C include file \"ntstatus.h\" for a description of the hexadecimal value."),
activity ? errdetail("Failed process was running: %s", activity) : 0));
-#elif defined(HAVE_DECL_SYS_SIGLIST) && HAVE_DECL_SYS_SIGLIST+#else
ereport(lev,
/*------
@@ -3581,19 +3582,10 @@ LogChildExit(int lev, const char *procname, int pid, int exitstatus)
"server process" */
(errmsg("%s (PID %d) was terminated by signal %d: %s",
procname, pid, WTERMSIG(exitstatus),
- WTERMSIG(exitstatus) < NSIG ?- sys_siglist[WTERMSIG(exitstatus)] : "(unknown)"),- activity ? errdetail("Failed process was running: %s", activity) : 0));-#else- ereport(lev,-- /*------- translator: %s is a noun phrase describing a child process, such as- "server process" */- (errmsg("%s (PID %d) was terminated by signal %d",- procname, pid, WTERMSIG(exitstatus)),+ pg_strsignal(WTERMSIG(exitstatus))),
activity ? errdetail("Failed process was running: %s", activity) : 0));
#endif
+ }
else
ereport(lev,
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.cindex 305dba20cb..662ba2bc3e 100644--- a/src/bin/pg_basebackup/pg_basebackup.c+++ b/src/bin/pg_basebackup/pg_basebackup.c@@ -2014,7 +2014,7 @@ BaseBackup(void)
{
#ifndef WIN32
int status;
- int r;+ pid_t r;
#else
DWORD status;
@@ -2042,7 +2042,7 @@ BaseBackup(void)
/* Just wait for the background process to exit */
r = waitpid(bgchild, &status, 0);
- if (r == -1)+ if (r == (pid_t) -1)
{
fprintf(stderr, _("%s: could not wait for child process: %s\n"),
progname, strerror(errno));
@@ -2051,19 +2051,13 @@ BaseBackup(void)
if (r != bgchild)
{
fprintf(stderr, _("%s: child %d died, expected %d\n"),
- progname, r, (int) bgchild);+ progname, (int) r, (int) bgchild);
disconnect_and_exit(1);
}
- if (!WIFEXITED(status))- {- fprintf(stderr, _("%s: child process did not exit normally\n"),- progname);- disconnect_and_exit(1);- }- if (WEXITSTATUS(status) != 0)+ if (status != 0)
{
- fprintf(stderr, _("%s: child process exited with error %d\n"),- progname, WEXITSTATUS(status));+ fprintf(stderr, "%s: %s\n",+ progname, wait_result_to_str(status));
disconnect_and_exit(1);
}
/* Exited normally, we're happy! */
diff --git a/src/common/wait_error.c b/src/common/wait_error.cindex 27f5284998..cef36ec01a 100644--- a/src/common/wait_error.c+++ b/src/common/wait_error.c@@ -56,25 +56,17 @@ wait_result_to_str(int exitstatus)
}
}
else if (WIFSIGNALED(exitstatus))
+ {
#if defined(WIN32)
snprintf(str, sizeof(str),
_("child process was terminated by exception 0x%X"),
WTERMSIG(exitstatus));
-#elif defined(HAVE_DECL_SYS_SIGLIST) && HAVE_DECL_SYS_SIGLIST- {- char str2[256];-- snprintf(str2, sizeof(str2), "%d: %s", WTERMSIG(exitstatus),- WTERMSIG(exitstatus) < NSIG ?- sys_siglist[WTERMSIG(exitstatus)] : "(unknown)");- snprintf(str, sizeof(str),- _("child process was terminated by signal %s"), str2);- }
#else
snprintf(str, sizeof(str),
- _("child process was terminated by signal %d"),- WTERMSIG(exitstatus));+ _("child process was terminated by signal %d: %s"),+ WTERMSIG(exitstatus), pg_strsignal(WTERMSIG(exitstatus)));
#endif
+ }
else
snprintf(str, sizeof(str),
_("child process exited with unrecognized status %d"),
diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.inindex a19f6981d7..a33acbf2d8 100644--- a/src/include/pg_config.h.in+++ b/src/include/pg_config.h.in@@ -182,10 +182,6 @@
don't. */
#undef HAVE_DECL_STRTOULL
-/* Define to 1 if you have the declaration of `sys_siglist', and to 0 if you- don't. */-#undef HAVE_DECL_SYS_SIGLIST-
/* Define to 1 if you have the declaration of `vsnprintf', and to 0 if you
don't. */
#undef HAVE_DECL_VSNPRINTF
@@ -547,6 +543,9 @@
/* Define to use have a strong random number source */
#undef HAVE_STRONG_RANDOM
+/* Define to 1 if you have the `strsignal' function. */+#undef HAVE_STRSIGNAL+
/* Define to 1 if you have the `strtoll' function. */
#undef HAVE_STRTOLL
diff --git a/src/include/pg_config.h.win32 b/src/include/pg_config.h.win32index 9bc4b3fc0f..e48ded8973 100644--- a/src/include/pg_config.h.win32+++ b/src/include/pg_config.h.win32@@ -414,6 +414,9 @@
/* Define to use have a strong random number source */
#define HAVE_STRONG_RANDOM 1
+/* Define to 1 if you have the `strsignal' function. */+/* #undef HAVE_STRSIGNAL */+
/* Define to 1 if you have the `strtoll' function. */
#ifdef HAVE_LONG_LONG_INT_64
#define HAVE_STRTOLL 1
diff --git a/src/include/port.h b/src/include/port.hindex a9d557b55c..c97d9be250 100644--- a/src/include/port.h+++ b/src/include/port.h@@ -189,6 +189,9 @@ extern int pg_printf(const char *fmt,...) pg_attribute_printf(1, 2);
#endif
#endif /* USE_REPL_SNPRINTF */
+/* Wrap strsignal(), or provide our own version if necessary */+extern const char *pg_strsignal(int signum);+
/* Portable prompt handling */
extern void simple_prompt(const char *prompt, char *destination, size_t destlen,
bool echo);
diff --git a/src/port/Makefile b/src/port/Makefileindex d7467fb078..ae37898bee 100644--- a/src/port/Makefile+++ b/src/port/Makefile@@ -32,7 +32,7 @@ LIBS += $(PTHREAD_LIBS)
OBJS = $(LIBOBJS) $(PG_CRC32C_OBJS) chklocale.o erand48.o inet_net_ntop.o \
noblock.o path.o pgcheckdir.o pgmkdirp.o pgsleep.o \
- pgstrcasecmp.o pqsignal.o \+ pgstrcasecmp.o pgstrsignal.o pqsignal.o \
qsort.o qsort_arg.o quotes.o sprompt.o tar.o thread.o
ifeq ($(enable_strong_random), yes)
diff --git a/src/port/pgstrsignal.c b/src/port/pgstrsignal.cnew file mode 100644index 0000000000..7724edf56e--- /dev/null+++ b/src/port/pgstrsignal.c@@ -0,0 +1,64 @@+/*-------------------------------------------------------------------------+ *+ * pgstrsignal.c+ * Identify a Unix signal number+ *+ * On platforms compliant with modern POSIX, this just wraps strsignal(3).+ * Elsewhere, we do the best we can.+ *+ * This file is not currently built in MSVC builds, since it's useless+ * on non-Unix platforms.+ *+ * Portions Copyright (c) 1996-2018, PostgreSQL Global Development Group+ * Portions Copyright (c) 1994, Regents of the University of California+ *+ * IDENTIFICATION+ * src/port/pgstrsignal.c+ *+ *-------------------------------------------------------------------------+ */++#include "c.h"+++/*+ * pg_strsignal+ *+ * Return a string identifying the given Unix signal number.+ *+ * The result is declared "const char *" because callers should not+ * modify the string. Note, however, that POSIX does not promise that+ * the string will remain valid across later calls to strsignal().+ *+ * This version guarantees to return a non-NULL pointer, although+ * some platforms' versions of strsignal() reputedly do not.+ *+ * Note that the fallback cases just return constant strings such as+ * "unrecognized signal". Project style is for callers to print the+ * numeric signal value along with the result of this function, so+ * there's no need to work harder than that.+ */+const char *+pg_strsignal(int signum)+{+ const char *result;++ /*+ * If we have strsignal(3), use that --- but check its result for NULL.+ */+#ifdef HAVE_STRSIGNAL+ result = strsignal(signum);+ if (result == NULL)+ result = "unrecognized signal";+#else++ /*+ * We used to have code here to try to use sys_siglist[] if available.+ * However, it seems that all platforms with sys_siglist[] have also had+ * strsignal() for many years now, so that was just a waste of code.+ */+ result = "(signal names not available on this platform)";+#endif++ return result;+}diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.cindex 9c6d2efb56..2c469413a3 100644--- a/src/test/regress/pg_regress.c+++ b/src/test/regress/pg_regress.c@@ -1560,14 +1560,9 @@ log_child_failure(int exitstatus)
#if defined(WIN32)
status(_(" (test process was terminated by exception 0x%X)"),
WTERMSIG(exitstatus));
-#elif defined(HAVE_DECL_SYS_SIGLIST) && HAVE_DECL_SYS_SIGLIST- status(_(" (test process was terminated by signal %d: %s)"),- WTERMSIG(exitstatus),- WTERMSIG(exitstatus) < NSIG ?- sys_siglist[WTERMSIG(exitstatus)] : "(unknown))");
#else
- status(_(" (test process was terminated by signal %d)"),- WTERMSIG(exitstatus));+ status(_(" (test process was terminated by signal %d: %s)"),+ WTERMSIG(exitstatus), pg_strsignal(WTERMSIG(exitstatus)));
#endif
}
else
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@gmail.com, pgsql-hackers@lists.postgresql.org, rosset.filipe@gmail.com
Subject: Re: sys_siglist[] is causing us trouble again
In-Reply-To: <3218392.1594858476@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