Discussion:
signal or xsignal for sigatexit()?
(too old to reply)
Rob Landley
2018-09-21 22:52:14 UTC
Permalink
signal() defaults to SA_RESTART, xsignal() uses sigaction() which does not. I'm
not sure which is correct? (It's currently using signal() and things seem ok
with what?)

*shrug* The readall() code assumes no SA_RESTART. The main downside of signal by
default I'm aware of is having to poll before read() when you want a timeout via
alarm() (unless you siglongjmp)...

Just wondering if anybody out there better than me at C has an opinion. :)

Rob
Gavin Howard
2018-09-22 00:06:23 UTC
Permalink
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.

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().

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.

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.

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

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?

Gavin Howard
Post by Rob Landley
signal() defaults to SA_RESTART, xsignal() uses sigaction() which does not. I'm
not sure which is correct? (It's currently using signal() and things seem ok
with what?)
*shrug* The readall() code assumes no SA_RESTART. The main downside of signal by
default I'm aware of is having to poll before read() when you want a timeout via
alarm() (unless you siglongjmp)...
Just wondering if anybody out there better than me at C has an opinion. :)
Rob
_______________________________________________
Toybox mailing list
http://lists.landley.net/listinfo.cgi/toybox-landley.net
Rob Landley
2018-09-26 16:07:11 UTC
Permalink
Post by Gavin Howard
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.
Post by Gavin Howard
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.)
Post by Gavin Howard
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.
Post by Gavin Howard
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.
Post by Gavin Howard
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. :)
Post by Gavin Howard
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.
Post by Gavin Howard
Gavin Howard
Rob

Loading...