Hi Eric, > I've gone ahead and done an reduced implementation of sigaction for mingw, > getting the parts that were easy to implement and which were exercised within > other gnulib modules.
That's fully sufficient. Nice work! > The implementation is NOT async-signal-safe, but since > it seems that mingw is the only platform lacking a native sigaction, this > does > not seem to be much of a loss Remember that there are at least two async signals: Ctrl-C leading to SIGINT, and also Ctrl-Break leading to SIGBREAK. > mingw lacks sigprocmask. There is a gnulib module replacement, but it too > suffers from the race condition. The sigprocmask replacement may, when a signal arrives just after it has been unblocked, still execute the handler for this signal. This is more benign than the race condition that you introduce here: When a signal arrives between the signal (sig, SIG_DFL) and signal (sig, sigaction_handler) lines, the signal will undergo default handling. Executing either the old or the new handler would be excusable, but not an open race like this. > +Posix recommends using @code{sigaction} with SA_NODEFER instead. s/Posix/POSIX/, no? > +/* We assume that a platform without POSIX sigaction implements SysV > + semantics in signal() (ie. the handler is uninstalled before it is > + invoked). Yes, this is how msvcrt behaves. > +static struct sigaction action_array[NSIG] /* = 0 */; This array needs to be marked volatile, because you access it from a signal handler. (You must forbid the compiler to reorder statements in the sigaction() function in a non-obvious way.) > +/* Signal handler that is installed for signals. */ > +static void > +sigaction_handler (int sig) > +{ > + handler_t h; > + sigset_t oldmask; > + sigset_t mask; > + if (sig < 0 || NSIG <= sig || !action_array[sig].sa_handler) If you need to access the action_array from within the handler, you need to change the statements if (signal (sig, sigaction_handler) == SIG_ERR) return -1; /* Consider what happens if a signal arrives ---HERE--- */ action_array[sig] = *act; to action_array[sig] = *act; if (signal (sig, sigaction_handler) == SIG_ERR) return -1; > + { > + /* Unexpected situation; be careful to avoid recursive abort. */ > + if (sig == SIGABRT) > + exit (1); > + abort (); I don't like exit(1) here, because it leaves no clue where to look when it really occurs. Why not like this: if (sig == SIGABRT) signal (sig, SIG_DFL); abort (); > + /* Reinstall the signal handler when required. Do this prior to > + blocking any signals, since sigprocmask replacement doesn't like > + changing a blocked signal. */ > + h = action_array[sig].sa_handler; > + if ((action_array[sig].sa_flags & SA_RESETHAND) == 0) > + signal (sig, sigaction_handler); > + else > + action_array[sig].sa_handler = NULL; > + > + /* Block appropriate signals. */ > + mask = action_array[sig].sa_mask; > + if ((action_array[sig].sa_flags & SA_NODEFER) == 0) > + sigaddset (&mask, sig); > + sigprocmask (SIG_BLOCK, &mask, &oldmask); > + > + /* Invoke the user's handler, then restore prior mask. */ > + h (sig); > + sigprocmask (SIG_SETMASK, &oldmask, NULL); > +} Looks good. > + if (oact) > + { > + if (action_array[sig].sa_handler) > + *oact = action_array[sig]; > + else > + { > + /* This opens a slight window where an async signal can call > + wrong handler. Oh well. */ > + oact->sa_handler = signal (sig, SIG_DFL); > + signal (sig, oact->sa_handler); > + if (oact->sa_handler == SIG_ERR) > + return -1; > + oact->sa_flags = SA_RESETHAND | SA_NODEFER; > + sigemptyset (&oact->sa_mask); > + } > + } > + > + if (act) > + { > + if (act->sa_handler == SIG_DFL || act->sa_handler == SIG_IGN) > + { > + if (signal (sig, act->sa_handler) == SIG_ERR) > + return -1; > + action_array[sig].sa_handler = NULL; > + } > + else > + { > + if (signal (sig, sigaction_handler) == SIG_ERR) > + return -1; Like Paul, I would like to see atomicity here. Can you merge the two signal() calls, to guarantee atomicity of the change? > + action_array[sig] = *act; This statement is not atomic, because action_array[sig] consists of several memory words. Yet you access all three words from the signal handler. What happens if a signal occurs right in the middle of this copying operation? > +# if [EMAIL PROTECTED]@ > +/* Present to allow compilation, but unsupported by gnulib. */ > +union sigval > +{ > + int sival_int; > + void *sival_ptr; > +}; > + > +struct siginfo_t > +{ > + int si_signo; > + int si_code; > + int si_errno; > + pid_t si_pid; > + uid_t si_uid; > + void *si_addr; > + int si_status; > + long si_band; > + union sigval si_value; > +}; I think this requires a #include <sys/types.h>. > --- a/lib/fatal-signal.c > +++ b/lib/fatal-signal.c > @@ -1,5 +1,5 @@ > /* Emergency actions in case of a fatal signal. > - Copyright (C) 2003-2004, 2006-2007 Free Software Foundation, Inc. > + Copyright (C) 2003-2004, 2006-2008 Free Software Foundation, Inc. > Written by Bruno Haible <[EMAIL PROTECTED]>, 2003. > > This program is free software: you can redistribute it and/or modify > @@ -30,6 +30,9 @@ > > #define SIZEOF(a) (sizeof(a) / sizeof(a[0])) > > +#ifndef SA_SIGINFO > +# define SA_SIGINFO 0 > +#endif > > /* ========================================================================= > */ > > @@ -88,7 +91,6 @@ init_fatal_signals (void) > static bool fatal_signals_initialized = false; > if (!fatal_signals_initialized) > { > -#if HAVE_SIGACTION > size_t i; > > for (i = 0; i < num_fatal_signals; i++) > @@ -96,11 +98,10 @@ init_fatal_signals (void) > struct sigaction action; > > if (sigaction (fatal_signals[i], NULL, &action) >= 0 > + && (action.sa_flags & SA_SIGINFO) == 0 > && action.sa_handler == SIG_IGN) > fatal_signals[i] = -1; > } > -#endif > - What does this SA_SIGINFO business mean? Why do you need to check whether (sa_flags & SA_SIGINFO) == 0 when sa_handler is known to be SIG_IGN? Bruno