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

[Bruno Haible]

Paul Eggert wrote:
> It's a little late for comments perhaps, since it's already been added,

It's never too late. Even though I've now already extended the
execute/pipe/wait-process facilities in GNU gettext to use the fatal-signal
module in order to provide the same interruption cleanup as you do in
'sdiff'.

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

In gettext I need fatal-signal for two purposes:
  - Reaping subprocesses. Here I call atexit() and at_fatal_signal()
    with the same cleanup function.
  - Removing temporary files. Here I don't call atexit() because if
    the program exits (due to a fatal error from the subprocess) the
    user needs to be able to look at the temporary file.

> > /* 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.

Unspecified. But you are right, 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.

Right again.

> 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".

Yes it's "no".

> > /* The list of fatal signals.
>
> You might want to add SIGRTMIN through SIGRTMAX to that list.

No, these signals are marked as "reserved for application use" by POSIX.
And in some ports of LinuxThreads, SIGRTMIN and SIGRTMIN + 1 are used
for communication with the process' thread manager. I won't touch these
signals in a general facility.

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

Exactly: All reasonable applications that use mmap() this way should
have their own SIGBUS/SIGSEGV handler installed. And then the signal
is not fatal any more and shouldn't be treated by at_fatal_signal().

And when SIGBUS/SIGSEGV is not caught, in indicates a program error,
like SIGABRT or SIGILL. In this case the best thing for the programmer is
to let the process crash immediately, without any confusing cleanup action.

> > 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

You're right, thanks.

> I suppose with overflow checking if the size gets too large.

Not needed. In practice actions_count will rarely be greater than 2.

> 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?)

The elements of the actions array need to be volatile as well. I overlooked
it in the first commit.

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

The simplicity of the API is more important than the simplicity of the
implementation, IMO. The reuse of gnulib modules in different applications
will help to ensure the correctness of the implementation.

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

But then I couldn't pass the same cleanup function to atexit() and
at_fatal_signal(). And think about it once again: Do you really want
a cleanup function that does a different thing for SIGHUP than for
SIGTERM?

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

That's OK. On shared modules discussion is much more necessary than
on code that is part of just one application.

Bruno