From a73d08319537d807a520a72bc5bd17279672c3de Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Sun, 16 Dec 2018 19:38:57 -0500 Subject: [PATCH] Modernize our code for looking up descriptive strings for Unix signals. At least as far back as the 2008 spec, POSIX has defined strsignal(3) for looking up descriptive strings for signal numbers. We hadn't gotten the word though, and were still using the crufty old sys_siglist array, which is in no standard even though most Unixen provide it. Aside from not being formally standards-compliant, this was just plain ugly because it involved #ifdef's at every place using the code. To eliminate the #ifdef's, create a portability function pg_strsignal, which wraps strsignal(3) if available and otherwise falls back to sys_siglist[] if available. The set of Unixen with neither API is probably empty these days, but on any platform with neither, you'll just get "unrecognized signal". All extant callers print the numeric signal number too, so no need to work harder than that. Along the way, upgrade pg_basebackup's child-error-exit reporting to match the rest of the system. Discussion: https://postgr.es/m/25758.1544983503@sss.pgh.pa.us --- configure | 2 +- configure.in | 1 + 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 | 3 ++ src/include/pg_config.h.win32 | 3 ++ src/include/port.h | 3 ++ src/port/Makefile | 2 +- src/port/pgstrsignal.c | 67 +++++++++++++++++++++++++++ src/test/regress/pg_regress.c | 9 +--- 12 files changed, 97 insertions(+), 54 deletions(-) create mode 100644 src/port/pgstrsignal.c diff --git a/configure b/configure index dce6d98cf6..3dddd8d0ed 100755 --- a/configure +++ b/configure @@ -15230,7 +15230,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 copyfile fdatasync getifaddrs getpeerucred getrlimit mbstowcs_l memmove poll posix_fallocate ppoll pstat pthread_is_threaded_np readlink setproctitle setproctitle_fast setsid shm_open strchrnul symlink sync_file_range utime utimes wcstombs_l +for ac_func in cbrt clock_gettime copyfile fdatasync getifaddrs getpeerucred getrlimit mbstowcs_l memmove poll posix_fallocate ppoll pstat pthread_is_threaded_np readlink setproctitle setproctitle_fast setsid shm_open strchrnul strsignal symlink sync_file_range 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" diff --git a/configure.in b/configure.in index e5123ac122..4fd6b8fc80 100644 --- a/configure.in +++ b/configure.in @@ -1621,6 +1621,7 @@ AC_CHECK_FUNCS(m4_normalize([ setsid shm_open strchrnul + strsignal symlink sync_file_range utime diff --git a/src/backend/postmaster/pgarch.c b/src/backend/postmaster/pgarch.c index f663d7befa..e88d545d65 100644 --- a/src/backend/postmaster/pgarch.c +++ b/src/backend/postmaster/pgarch.c @@ -650,17 +650,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 a33a131182..eedc617db4 100644 --- a/src/backend/postmaster/postmaster.c +++ b/src/backend/postmaster/postmaster.c @@ -3612,6 +3612,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, @@ -3622,7 +3623,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, /*------ @@ -3630,19 +3631,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 af1c4de58f..85c3812dd1 100644 --- a/src/bin/pg_basebackup/pg_basebackup.c +++ b/src/bin/pg_basebackup/pg_basebackup.c @@ -2066,7 +2066,7 @@ BaseBackup(void) { #ifndef WIN32 int status; - int r; + pid_t r; #else DWORD status; @@ -2094,7 +2094,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)); @@ -2103,19 +2103,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)) + if (status != 0) { - fprintf(stderr, _("%s: child process did not exit normally\n"), - progname); - disconnect_and_exit(1); - } - if (WEXITSTATUS(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 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.in index 6ac75cd02c..0a5ddd2e92 100644 --- a/src/include/pg_config.h.in +++ b/src/include/pg_config.h.in @@ -559,6 +559,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.win32 index 894d658a20..de0c4d9997 100644 --- a/src/include/pg_config.h.win32 +++ b/src/include/pg_config.h.win32 @@ -415,6 +415,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.h index 03e9c12a35..570a9052a2 100644 --- a/src/include/port.h +++ b/src/include/port.h @@ -209,6 +209,9 @@ extern char *pg_strerror_r(int errnum, char *buf, size_t buflen); #define strerror_r pg_strerror_r #define PG_STRERROR_R_BUFLEN 256 /* Recommended buffer size for strerror_r */ +/* 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/Makefile index 585c53757b..ae3f973ae1 100644 --- a/src/port/Makefile +++ b/src/port/Makefile @@ -37,7 +37,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 snprintf.o sprompt.o strerror.o \ tar.o thread.o diff --git a/src/port/pgstrsignal.c b/src/port/pgstrsignal.c new file mode 100644 index 0000000000..ec989229ba --- /dev/null +++ b/src/port/pgstrsignal.c @@ -0,0 +1,67 @@ +/*------------------------------------------------------------------------- + * + * 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() do not. + */ +const char * +pg_strsignal(int signum) +{ + const char *result; + + /* + * If we have strsignal(3), use that --- but check its result for NULL. + * Otherwise, if we have sys_siglist[], use that; just out of paranoia, + * check for NULL there too. (We assume there is no point in trying both + * APIs.) + */ +#if defined(HAVE_STRSIGNAL) + result = strsignal(signum); + if (result) + return result; +#elif defined(HAVE_DECL_SYS_SIGLIST) && HAVE_DECL_SYS_SIGLIST + if (signum > 0 && signum < NSIG) + { + result = sys_siglist[signum]; + if (result) + return result; + } +#endif + + /* + * Fallback case: just return "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 this. + */ + result = "unrecognized signal"; + return result; +} diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c index 3248603da1..63fe68914b 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