[Toybox] signal or xsignal for sigatexit()?

[Rob Landley]

On 09/21/2018 07:06 PM, Gavin Howard wrote:
> Rob,
> 
> I'm not better than you at C, but I did have to dig deep into this for
> my `bc', so I thought I would offer my 2 cents.
> 
> First off, sigaction() is complex, but it is better in every other
> way. It gives you a lot more options, has better ways of querying the
> current signal handlers, has better behavior, etc.

Which is why my xsignal() wrapper uses sigaction internally, yes.

> For multithreaded programs on Linux, the man page
> (http://man7.org/linux/man-pages/man2/signal.2.html) says that the
> behavior of signal() is unspecified. It also encourages the use of
> sigaction().

I'm avoiding threads in toybox for a reason. (I did years of threaded
programming, I have to debug smp weirdness in the kernel all the time. I _could_
go there, but really dowanna.)

> Second, the GNU page for glibc's signal() and sigaction()
> (http://www.gnu.org/software/libc/manual/html_node/Signal-and-Sigaction.html)
> says that they discourage the use of both signal() and sigaction() in
> the same program.

I try not to care what gnu/anything has to say. I read the kernel code for
syscalls to see what they do, use my "git repo going back to 1.0" to see how
long it's done it, and rely on Linus's strong assertions of binary compatibility:

  https://twitter.com/landley/status/1034898415300304896

If glibc is managing to seriously screw up a syscall wrapper, glibc is broken
and portability.h or portability.c turns it _back_ into a syscall wrapper for
that libc.

That said, a wrapper around sigaction() providing signal() semantics with
defined behavior is a reasonable approach, My discomfort is that "to restart or
not to restart" is a bad assumption to change after the fact. But it should be
consistent.

> There is one more wrinkle, but I don't think it will be a problem for
> toybox: signal() is more widely available. However, sigaction() has
> been defined in POSIX since at least 2001, and you told me that you
> generally require POSIX 2008, so I don't think that availability will
> be a problem at all.

Indeed. That's not what I'm worried about.

> Also, my `bc' already uses sigaction().

I'm trying to close things down and finish up half-finished stuff for a release
at the end of the month. Not opening the bc review can of worms just yet. :)

> Thus, I suggest you use xsignal()/sigaction(). I can send you a patch
> for that. Would you want me to do that? And if so, would you want it
> before or after the next release?

I've had the patch locally in my tree for a while, it's 2 lines.

@@ -854,9 +855,13 @@ void sigatexit(void *handler)
   int i;

   for (i=0; signames[i].num != SIGCHLD; i++)
-    signal(signames[i].num, handler ? exit_signal : SIG_DFL);
+    if (signames[i].num != SIGKILL)
+      xsignal(signames[i].num, handler ? exit_signal : SIG_DFL);
+
   if (handler) {
     al = xmalloc(sizeof(struct arg_list));

What I really need to do is make an xsignal_flags() that the other xsignal() is
a wrapper for, then I can request SA_RESTART and so on as needed.

> Gavin Howard

Rob