From d8b30702057c18dac2f35fd766ef5d2a12786eae Mon Sep 17 00:00:00 2001 From: Jesse Gross Date: Tue, 8 Dec 2009 14:11:22 -0800 Subject: [PATCH] fatal-signal: Run signal hooks outside of actual signal handlers. Rather than running signal hooks directly from the actual signal handler, simply record the fact that the signal occured and run the hook next time around the poll loop. This allows significantly more freedom as to what can actually be done in the signal hooks. --- lib/fatal-signal.c | 127 ++++++++++++++++++--------------------- lib/fatal-signal.h | 4 +- lib/netdev.c | 2 - lib/poll-loop.c | 8 +++ lib/process.c | 8 --- tests/test-dhcp-client.c | 2 - utilities/ovs-discover.c | 2 - 7 files changed, 67 insertions(+), 86 deletions(-) diff --git a/lib/fatal-signal.c b/lib/fatal-signal.c index 81805214..64b045d3 100644 --- a/lib/fatal-signal.c +++ b/lib/fatal-signal.c @@ -20,10 +20,13 @@ #include #include #include +#include #include #include #include +#include "poll-loop.h" #include "shash.h" +#include "socket-util.h" #include "util.h" #define THIS_MODULE VLM_fatal_signal @@ -45,53 +48,32 @@ struct hook { static struct hook hooks[MAX_HOOKS]; static size_t n_hooks; -/* Number of nesting signal blockers. */ -static int block_level = 0; - -/* Signal mask saved by outermost signal blocker. */ -static sigset_t saved_signal_mask; +static int signal_fds[2]; +static volatile sig_atomic_t stored_sig_nr = SIG_ATOMIC_MAX; /* Disabled by fatal_signal_fork()? */ static bool disabled; -static void call_sigprocmask(int how, sigset_t* new_set, sigset_t* old_set); +static void fatal_signal_init(void); static void atexit_handler(void); static void call_hooks(int sig_nr); -/* Registers 'hook' to be called when a process termination signal is raised. - * If 'run_at_exit' is true, 'hook' is also called during normal process - * termination, e.g. when exit() is called or when main() returns. - * - * 'func' will be invoked from an asynchronous signal handler, so it must be - * written appropriately. For example, it must not call most C library - * functions, including malloc() or free(). */ -void -fatal_signal_add_hook(void (*func)(void *aux), void *aux, bool run_at_exit) -{ - fatal_signal_block(); - assert(n_hooks < MAX_HOOKS); - hooks[n_hooks].func = func; - hooks[n_hooks].aux = aux; - hooks[n_hooks].run_at_exit = run_at_exit; - n_hooks++; - fatal_signal_unblock(); -} - -/* Blocks program termination signals until fatal_signal_unblock() is called. - * May be called multiple times with nesting; if so, fatal_signal_unblock() - * must be called the same number of times to unblock signals. - * - * This is needed while adjusting a data structure that will be accessed by a - * fatal signal hook, so that the hook is not invoked while the data structure - * is in an inconsistent state. */ -void -fatal_signal_block(void) +static void +fatal_signal_init(void) { static bool inited = false; + if (!inited) { size_t i; inited = true; + + if (pipe(signal_fds)) { + ovs_fatal(errno, "could not create pipe"); + } + set_nonblocking(signal_fds[0]); + set_nonblocking(signal_fds[1]); + sigemptyset(&fatal_signal_set); for (i = 0; i < ARRAY_SIZE(fatal_signals); i++) { int sig_nr = fatal_signals[i]; @@ -108,23 +90,24 @@ fatal_signal_block(void) } atexit(atexit_handler); } - - if (++block_level == 1) { - call_sigprocmask(SIG_BLOCK, &fatal_signal_set, &saved_signal_mask); - } } -/* Unblocks program termination signals blocked by fatal_signal_block() is - * called. If multiple calls to fatal_signal_block() are nested, - * fatal_signal_unblock() must be called the same number of times to unblock - * signals. */ +/* Registers 'hook' to be called when a process termination signal is raised. + * If 'run_at_exit' is true, 'hook' is also called during normal process + * termination, e.g. when exit() is called or when main() returns. + * + * The hook is not called immediately from the signal handler but rather the + * next time the poll loop iterates, so it is freed from the usual restrictions + * on signal handler functions. */ void -fatal_signal_unblock(void) +fatal_signal_add_hook(void (*func)(void *aux), void *aux, bool run_at_exit) { - assert(block_level > 0); - if (--block_level == 0) { - call_sigprocmask(SIG_SETMASK, &saved_signal_mask, NULL); - } + fatal_signal_init(); + assert(n_hooks < MAX_HOOKS); + hooks[n_hooks].func = func; + hooks[n_hooks].aux = aux; + hooks[n_hooks].run_at_exit = run_at_exit; + n_hooks++; } /* Handles fatal signal number 'sig_nr'. @@ -139,12 +122,29 @@ fatal_signal_unblock(void) void fatal_signal_handler(int sig_nr) { - call_hooks(sig_nr); + ignore(write(signal_fds[1], "", 1)); + stored_sig_nr = sig_nr; +} + +void +fatal_signal_run(void) +{ + int sig_nr = stored_sig_nr; - /* Re-raise the signal with the default handling so that the program - * termination status reflects that we were killed by this signal */ - signal(sig_nr, SIG_DFL); - raise(sig_nr); + if (sig_nr != SIG_ATOMIC_MAX) { + call_hooks(sig_nr); + + /* Re-raise the signal with the default handling so that the program + * termination status reflects that we were killed by this signal */ + signal(sig_nr, SIG_DFL); + raise(sig_nr); + } +} + +void +fatal_signal_wait(void) +{ + poll_fd_wait(signal_fds[0], POLLIN); } static void @@ -189,11 +189,9 @@ fatal_signal_add_file_to_unlink(const char *file) fatal_signal_add_hook(unlink_files, NULL, true); } - fatal_signal_block(); if (!shash_find(&files, file)) { shash_add(&files, file, NULL); } - fatal_signal_unblock(); } /* Unregisters 'file' from being unlinked when the program terminates via @@ -203,12 +201,10 @@ fatal_signal_remove_file_to_unlink(const char *file) { struct shash_node *node; - fatal_signal_block(); node = shash_find(&files, file); if (node) { shash_delete(&files, node); } - fatal_signal_unblock(); } /* Like fatal_signal_remove_file_to_unlink(), but also unlinks 'file'. @@ -232,12 +228,6 @@ unlink_files(void *aux UNUSED) do_unlink_files(); } -/* This is a fatal_signal_add_hook() callback (via unlink_files()). It will be - * invoked from an asynchronous signal handler, so it cannot call most C - * library functions (unlink() is an explicit exception, see - * http://www.opengroup.org/onlinepubs/009695399/functions/xsh_chap02_04.html). - * That includes free(), so it doesn't try to free the 'files' data - * structure. */ static void do_unlink_files(void) { @@ -265,13 +255,10 @@ fatal_signal_fork(void) signal(sig_nr, SIG_IGN); } } -} - -static void -call_sigprocmask(int how, sigset_t* new_set, sigset_t* old_set) -{ - int error = sigprocmask(how, new_set, old_set); - if (error) { - fprintf(stderr, "sigprocmask: %s\n", strerror(errno)); + + /* Raise any signals that we have already received with the default + * handler. */ + if (stored_sig_nr != SIG_ATOMIC_MAX) { + raise(stored_sig_nr); } } diff --git a/lib/fatal-signal.h b/lib/fatal-signal.h index c96db86d..a84d5fd7 100644 --- a/lib/fatal-signal.h +++ b/lib/fatal-signal.h @@ -21,9 +21,9 @@ /* Basic interface. */ void fatal_signal_add_hook(void (*)(void *aux), void *aux, bool run_at_exit); -void fatal_signal_block(void); -void fatal_signal_unblock(void); void fatal_signal_fork(void); +void fatal_signal_run(void); +void fatal_signal_wait(void); /* Convenience functions for unlinking files upon termination. * diff --git a/lib/netdev.c b/lib/netdev.c index 6c87bd4b..77964f93 100644 --- a/lib/netdev.c +++ b/lib/netdev.c @@ -297,10 +297,8 @@ netdev_close(struct netdev *netdev) #endif /* Restore flags that we changed, if any. */ - fatal_signal_block(); error = restore_flags(netdev); list_remove(&netdev->node); - fatal_signal_unblock(); if (error) { VLOG_WARN("failed to restore network device flags on %s: %s", name, strerror(error)); diff --git a/lib/poll-loop.c b/lib/poll-loop.c index 32bbc134..81dc8fdd 100644 --- a/lib/poll-loop.c +++ b/lib/poll-loop.c @@ -25,6 +25,7 @@ #include "backtrace.h" #include "coverage.h" #include "dynamic-string.h" +#include "fatal-signal.h" #include "list.h" #include "timeval.h" @@ -147,6 +148,10 @@ poll_block(void) int n_pollfds; int retval; + /* Register fatal signal events before actually doing any real work for + * poll_block. */ + fatal_signal_wait(); + assert(!running_cb); if (max_pollfds < n_waiters) { max_pollfds = n_waiters; @@ -207,6 +212,9 @@ poll_block(void) timeout = -1; timeout_backtrace.n_frames = 0; + + /* Handle any pending signals before doing anything else. */ + fatal_signal_run(); } /* Registers 'function' to be called with argument 'aux' by poll_block() when diff --git a/lib/process.c b/lib/process.c index 12168f7e..3f66ddd9 100644 --- a/lib/process.c +++ b/lib/process.c @@ -201,17 +201,14 @@ process_start(char **argv, } block_sigchld(&oldsigs); - fatal_signal_block(); pid = fork(); if (pid < 0) { - fatal_signal_unblock(); unblock_sigchld(&oldsigs); VLOG_WARN("fork failed: %s", strerror(errno)); return errno; } else if (pid) { /* Running in parent process. */ *pp = process_register(argv[0], pid); - fatal_signal_unblock(); unblock_sigchld(&oldsigs); return 0; } else { @@ -220,7 +217,6 @@ process_start(char **argv, int fd; fatal_signal_fork(); - fatal_signal_unblock(); unblock_sigchld(&oldsigs); for (fd = 0; fd < fd_max; fd++) { if (is_member(fd, null_fds, n_null_fds)) { @@ -521,12 +517,10 @@ process_run_capture(char **argv, char **stdout_log, char **stderr_log, } block_sigchld(&oldsigs); - fatal_signal_block(); pid = fork(); if (pid < 0) { int error = errno; - fatal_signal_unblock(); unblock_sigchld(&oldsigs); VLOG_WARN("fork failed: %s", strerror(error)); @@ -539,7 +533,6 @@ process_run_capture(char **argv, char **stdout_log, char **stderr_log, struct process *p; p = process_register(argv[0], pid); - fatal_signal_unblock(); unblock_sigchld(&oldsigs); close(s_stdout.fds[1]); @@ -575,7 +568,6 @@ process_run_capture(char **argv, char **stdout_log, char **stderr_log, int i; fatal_signal_fork(); - fatal_signal_unblock(); unblock_sigchld(&oldsigs); dup2(get_null_fd(), 0); diff --git a/tests/test-dhcp-client.c b/tests/test-dhcp-client.c index e4471c7b..c15f6939 100644 --- a/tests/test-dhcp-client.c +++ b/tests/test-dhcp-client.c @@ -70,7 +70,6 @@ main(int argc, char *argv[]) fatal_signal_add_hook(release, cli, true); for (;;) { - fatal_signal_block(); dhclient_run(cli); if (dhclient_changed(cli)) { dhclient_configure_netdev(cli); @@ -79,7 +78,6 @@ main(int argc, char *argv[]) } } dhclient_wait(cli); - fatal_signal_unblock(); poll_block(); } } diff --git a/utilities/ovs-discover.c b/utilities/ovs-discover.c index dc91bce3..3aa28fad 100644 --- a/utilities/ovs-discover.c +++ b/utilities/ovs-discover.c @@ -122,7 +122,6 @@ main(int argc, char *argv[]) signal(SIGPIPE, SIG_IGN); for (;;) { - fatal_signal_block(); for (i = 0; i < n_ifaces; i++) { struct iface *iface = &ifaces[i]; dhclient_run(iface->dhcp); @@ -195,7 +194,6 @@ main(int argc, char *argv[]) dhclient_wait(iface->dhcp); } unixctl_server_wait(unixctl); - fatal_signal_unblock(); poll_block(); } -- 2.30.2