Re: [Bug-gnulib] addition: fatal-signal.h, fatal-signal.c

[Paul Eggert]

It's a little late for comments perhaps, since it's already been added,
but here we go anyway.

Bruno Haible <address@hidden> writes:

> > 1) A general at_fatal_signal facility, similar to atexit, which allows to
> >    register cleanup handlers for those fatal signals that can be caught.

I'm uncomfortable with the idea of having a separate facility for
fatal signals.  It's usually the case that every cleanup to be done
for a fatal signal, also needs to be done with normal exit.  It'd
be nicer to deal with a single facility.

> /* Register a cleanup function to be executed when a catchable fatal signal
>    occurs.  */
> extern void at_fatal_signal (void (*function) (void));

Should fatal signals be blocked during cleanup?
Either way, this should be documented.

> /* Temporarily delay the catchable fatal signals.  */
> extern void block_fatal_signals (void);

Can one nest blocks and unblocks?  This should be documented.

Which functions can cleanup functions invoke?  This should be documented.
In particular, can a cleanup function invoke at_fatal_signal,
or block_fatal_signals?  I think that the answer is "no".

> /* The list of fatal signals.

You might want to add SIGRTMIN through SIGRTMAX to that list.

Also, it would make sense to optionally add SIGBUS and SIGSEGV, since
mmapped memory can result in SIGBUS or SIGSEGV signals being sent to
the process merely because some other process shortened the underlying
file, or if there was an I/O error in reading from that file.  (POSIX
specifies SIGBUS, but many older systems use SIGSEGV.)  Not every
application uses mmap, but those that do should catch these signals.

> static size_t volatile actions_count = 0;

It doesn't suffice to make 'actions_count' volatile, since storing
into it may not be an atomic operation.  You need to have it be of
type sig_atomic_t, not size_t -- I suppose with overflow checking if
the size gets too large.  (The size overflow checking needs to be done
anyway.)

If actions_count is sig_atomic_t volatile, I don't think anything else
needs to be volatile, since it's a sequence point to store or load
from actions_count.  (Perhaps I'm missing something?)

I'd feel more comfortable having an initialization function for the
module, rather than having booleans that get set when the module is
not initialized.  Partly this is a style thing, but more important
it's cleaner and easier-to-understand if one initializes things
properly in the presence of signals.

Also, it'd be nicer in some circumstances if actions had all the
arguments available to them that were available to the original
signal handlers.

(I know, I know, a lot of suggestions but not any code....  :-)