From 9e043d93c84364e02fd81e9b26f46277c232f1b4 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Wed, 15 Jul 2020 22:05:13 -0400 Subject: [PATCH] Replace use of sys_siglist[] with strsignal(). This commit back-patches the v12-era commits a73d08319, cc92cca43, and 7570df0f3 into supported pre-v12 branches. The net effect is to eliminate our former dependency on the never-standard sys_siglist[] array, instead using POSIX-standard strsignal(3). What motivates doing this now is that glibc just removed sys_siglist[] from the set of symbols available to newly-built programs. While our code can survive without sys_siglist[], it then fails to print any description of the signal that killed a child process, which is a non-negligible loss of friendliness. We can expect that people will be wanting to build the back branches on platforms that include this change, so we need to do something. Since strsignal(3) has existed for quite a long time, and we've not had any trouble with these patches so far in v12, it seems safe to back-patch into older branches. Discussion: https://postgr.es/m/3179114.1594853308@sss.pgh.pa.us --- configure | 20 +-------- configure.in | 9 +--- src/backend/postmaster/pgarch.c | 11 +---- src/backend/postmaster/postmaster.c | 16 ++----- src/bin/pg_basebackup/pg_basebackup.c | 18 +++----- src/common/wait_error.c | 16 ++----- src/include/pg_config.h.in | 7 ++- src/include/pg_config.h.win32 | 3 ++ src/include/port.h | 3 ++ src/port/Makefile | 2 +- src/port/pgstrsignal.c | 64 +++++++++++++++++++++++++++ src/test/regress/pg_regress.c | 9 +--- 12 files changed, 94 insertions(+), 84 deletions(-) create mode 100644 src/port/pgstrsignal.c diff --git a/configure b/configure index 23f3b81fa9f..013b0615a6e 100755 --- a/configure +++ b/configure @@ -12658,7 +12658,7 @@ fi LIBS_including_readline="$LIBS" LIBS=`echo "$LIBS" | sed -e 's/-ledit//g' -e 's/-lreadline//g'` -for ac_func in cbrt 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 towlower uselocale utime utimes wcstombs wcstombs_l +for ac_func in cbrt 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 towlower uselocale utime utimes wcstombs 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" @@ -13524,24 +13524,6 @@ esac fi -ac_fn_c_check_decl "$LINENO" "sys_siglist" "ac_cv_have_decl_sys_siglist" "#include -/* NetBSD declares sys_siglist in unistd.h. */ -#ifdef HAVE_UNISTD_H -# include -#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.in index 76097f467d3..e8df59b8d93 100644 --- a/configure.in +++ b/configure.in @@ -1523,6 +1523,7 @@ AC_CHECK_FUNCS(m4_normalize([ setproctitle setsid shm_open + strsignal symlink sync_file_range towlower @@ -1724,14 +1725,6 @@ if test "$PORTNAME" = "cygwin"; then AC_LIBOBJ(dirmod) fi -AC_CHECK_DECLS([sys_siglist], [], [], -[#include -/* NetBSD declares sys_siglist in unistd.h. */ -#ifdef HAVE_UNISTD_H -# include -#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.c index 14350fb7dec..500fad75b71 100644 --- a/src/backend/postmaster/pgarch.c +++ b/src/backend/postmaster/pgarch.c @@ -598,17 +598,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.c index 17feb6bef94..b8b0d65955a 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -3558,6 +3558,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, @@ -3568,7 +3569,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, /*------ @@ -3576,19 +3577,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.c index 37ffa55d121..11fe505615c 100644 --- a/src/bin/pg_basebackup/pg_basebackup.c +++ b/src/bin/pg_basebackup/pg_basebackup.c @@ -1866,7 +1866,7 @@ BaseBackup(void) { #ifndef WIN32 int status; - int r; + pid_t r; #else DWORD status; @@ -1894,7 +1894,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)); @@ -1903,19 +1903,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.c index 4aa827d0664..e129c778d02 100644 --- a/src/common/wait_error.c +++ b/src/common/wait_error.c @@ -58,25 +58,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.in index 639c18ab579..adb0d336a33 100644 --- a/src/include/pg_config.h.in +++ b/src/include/pg_config.h.in @@ -152,10 +152,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 @@ -508,6 +504,9 @@ /* Define to 1 if you have the `strlcpy' function. */ #undef HAVE_STRLCPY +/* 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.win32 index c640ebd2eb6..28aac57cb58 100644 --- a/src/include/pg_config.h.win32 +++ b/src/include/pg_config.h.win32 @@ -361,6 +361,9 @@ /* Define to 1 if you have the header file. */ #define HAVE_STRING_H 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.h index d0aebe19583..a3e1dac66cc 100644 --- a/src/include/port.h +++ b/src/include/port.h @@ -202,6 +202,9 @@ extern char *pgwin32_setlocale(int category, const char *locale); #define setlocale(a,b) pgwin32_setlocale(a,b) #endif /* WIN32 */ +/* Wrap strsignal(), or provide our own version if necessary */ +extern const char *pg_strsignal(int signum); + /* Portable prompt handling */ extern char *simple_prompt(const char *prompt, int maxlen, bool echo); diff --git a/src/port/Makefile b/src/port/Makefile index bc9b63add04..0553aaf09e9 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 # foo_srv.o and foo.o are both built from foo.c, but only foo.o has -DFRONTEND diff --git a/src/port/pgstrsignal.c b/src/port/pgstrsignal.c new file mode 100644 index 00000000000..7724edf56e1 --- /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.c index a38a93d78db..56f277bef69 100644 --- a/src/test/regress/pg_regress.c +++ b/src/test/regress/pg_regress.c @@ -1534,14 +1534,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 -- 2.39.5