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 _______________________________________________ Toybox mailing list [email protected] http://lists.landley.net/listinfo.cgi/toybox-landley.net
