Обсуждение: Why aren't we using strsignal(3) ?

While poking at the signal-reporting bug just pointed out by
Erik Rijkers, I couldn't help noticing how many places we have
that are doing some equivalent of this ugly dance:

#if 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));
#endif

(Plus, there's at least one place that *should* be doing this and is not.)

Not only is this repetitive and unreadable, but it's also obsolete:
at least as far back as POSIX:2008, there's a function strsignal()
that you're supposed to use instead.

I propose to replace all these places with code like

        snprintf(str, sizeof(str),
                 _("child process was terminated by signal %d: %s"),
                 WTERMSIG(exitstatus), pg_strsignal(WTERMSIG(exitstatus)));

where pg_strsignal is a trivial wrapper around strsignal() if that
exists, else it uses sys_siglist[] if that exists, else it just
returns "unrecognized signal".

            regards, tom lane

On 2018-Dec-16, Tom Lane wrote:

> I propose to replace all these places with code like
> 
>         snprintf(str, sizeof(str),
>                  _("child process was terminated by signal %d: %s"),
>                  WTERMSIG(exitstatus), pg_strsignal(WTERMSIG(exitstatus)));
> 
> where pg_strsignal is a trivial wrapper around strsignal() if that
> exists, else it uses sys_siglist[] if that exists, else it just
> returns "unrecognized signal".

LGTM.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2018-Dec-16, Tom Lane wrote:
>> I propose to replace all these places with code like
>> 
>>     snprintf(str, sizeof(str),
>>              _("child process was terminated by signal %d: %s"),
>>              WTERMSIG(exitstatus), pg_strsignal(WTERMSIG(exitstatus)));
>> 
>> where pg_strsignal is a trivial wrapper around strsignal() if that
>> exists, else it uses sys_siglist[] if that exists, else it just
>> returns "unrecognized signal".

> LGTM.

Done at a73d08319.  Early returns from the buildfarm show that we have
some systems that have strsignal() but not sys_siglist[], e.g.
damselfly and Noah's AIX herd.  So this patch does provide a useful
increment of portability.  What I'm finding interesting is that there
seem to be no systems that have sys_siglist[] but not strsignal().

Digging around on the net suggests that sys_siglist[] is a BSD-ism
but the BSDs all adopted strsignal() a very long time ago.  For instance,
OpenBSD's man page for it says "The strsignal() function first appeared
in AT&T System V Release 4 UNIX and was reimplemented for NetBSD 1.0."
OpenBSD has it at least back to OpenBSD 2.2 (oldest man pages that
they have online), while FreeBSD adopted it at FreeBSD 4.0.

There are systems that have neither API (just the old HPUX critters)
so we can't dispense with the wrapper entirely.  But it looks like
we could drop the sys_siglist support for an undetectably small penalty:
even if, somewhere, there's a platform that has sys_siglist[] but not
strsignal(), it'd just mean that you get only a signal number and have
to look up its meaning.

While a dozen lines in pgstrsignal.c certainly are not worth worrying
over, getting rid of the configure test for sys_siglist[] would save
some cycles on every build.  So I'm tempted to drop it.  Thoughts?

            regards, tom lane

At 2018-12-17 11:52:03 -0500, tgl@sss.pgh.pa.us wrote:
>
> While a dozen lines in pgstrsignal.c certainly are not worth worrying
> over, getting rid of the configure test for sys_siglist[] would save
> some cycles on every build.  So I'm tempted to drop it.  Thoughts?

Removing it makes sense to me.

-- Abhijit

On 2018-Dec-17, Tom Lane wrote:

> But it looks like
> we could drop the sys_siglist support for an undetectably small penalty:
> even if, somewhere, there's a platform that has sys_siglist[] but not
> strsignal(), it'd just mean that you get only a signal number and have
> to look up its meaning.
> 
> While a dozen lines in pgstrsignal.c certainly are not worth worrying
> over, getting rid of the configure test for sys_siglist[] would save
> some cycles on every build.  So I'm tempted to drop it.  Thoughts?

+1 for nuking it.  configure times grow larger, and there's seldom a
change to make them shorter.  In this case, per your analysis, it
doesn't look like we're losing anything worthwhile.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services