r14945@tombo: nickm | 2007-11-25 12:13:05 -0500

Make kqueue pass more unit tests. (Backport)


svn:r545

ChangeLog

@@ -4,6 +4,9 @@ Changes in current version:
  o provide event_base_new() that does not set the current_base global	
  o bufferevent_write now uses a const source argument; report from Charles Kerr
  o better documentation for event_base_loopexit; from Scott Lamb.
+ o Make kqueue have the same behavior as other backends when a signal is caught between event_add() and event_loop().  Previously, it would catch and ignore such signals.
+ o Make kqueue restore signal handlers correctly when event_del() is called.
+
 
 Changes in 1.4.0-beta:
  o allow \r or \n individually to separate HTTP headers instead of the standard "\r\n"; from Charles Kerr.

event-internal.h

@@ -73,6 +73,10 @@ struct event_base {
 } while (0)
 #endif /* TAILQ_FOREACH */
 
+int _evsignal_set_handler(struct event_base *base, int evsignal,
+			  void (*fn)(int));
+int _evsignal_restore_handler(struct event_base *base, int evsignal);
+
 #ifdef __cplusplus
 }
 #endif

kqueue.c

@@ -59,6 +59,7 @@
 
 #include "event.h"
 #include "log.h"
+#include "event-internal.h"
 
 #define EVLIST_X_KQINKERNEL	0x1000
 
@@ -293,6 +294,7 @@ kq_add(void *arg, struct event *ev)
 
 	if (ev->ev_events & EV_SIGNAL) {
 		int nsignal = EVENT_SIGNAL(ev);
+                struct timespec timeout = { 0, 0 };
 
  		memset(&kev, 0, sizeof(kev));
 		kev.ident = nsignal;
@@ -301,11 +303,14 @@ kq_add(void *arg, struct event *ev)
 		if (!(ev->ev_events & EV_PERSIST))
 			kev.flags |= EV_ONESHOT;
 		kev.udata = PTR_TO_UDATA(ev);
-		
-		if (kq_insert(kqop, &kev) == -1)
-			return (-1);
 
-		if (signal(nsignal, kq_sighandler) == SIG_ERR)
+		/* Be ready for the signal if it is sent any time between
+		 * now and the next call to kq_dispatch. */
+                if (kevent(kqop->kq, &kev, 1, NULL, 0, &timeout) == -1)
+                	return (-1);
+
+		if (_evsignal_set_handler(ev->ev_base, nsignal,
+					  kq_sighandler) == -1)
 			return (-1);
 
 		ev->ev_flags |= EVLIST_X_KQINKERNEL;
@@ -369,7 +374,7 @@ kq_del(void *arg, struct event *ev)
 		if (kq_insert(kqop, &kev) == -1)
 			return (-1);
 
-		if (signal(nsignal, SIG_DFL) == SIG_ERR)
+		if (_evsignal_restore_handler(ev->ev_base, nsignal) == -1)
 			return (-1);
 
 		ev->ev_flags &= ~EVLIST_X_KQINKERNEL;

signal.c

@@ -119,23 +119,20 @@ evsignal_init(struct event_base *base)
 	base->sig.ev_signal.ev_flags |= EVLIST_INTERNAL;
 }
 
+/* Helper: set the signal handler for evsignal to handler in base, so that
+ * we can restore the original handler when we clear the current one. */
 int
-evsignal_add(struct event *ev)
+_evsignal_set_handler(struct event_base *base,
+		      int evsignal, void (*handler)(int))
 {
-	int evsignal;
 #ifdef HAVE_SIGACTION
 	struct sigaction sa;
 #else
 	ev_sighandler_t sh;
 #endif
-	struct event_base *base = ev->ev_base;
-	struct evsignal_info *sig = &ev->ev_base->sig;
+	struct evsignal_info *sig = &base->sig;
 	void *p;
 
-	if (ev->ev_events & (EV_READ|EV_WRITE))
-		event_errx(1, "%s: EV_SIGNAL incompatible use", __func__);
-	evsignal = EVENT_SIGNAL(ev);
-
 	/*
 	 * resize saved signal handler array up to the highest signal number.
 	 * a dynamic array is used to keep footprint on the low side.
@@ -160,10 +157,9 @@ evsignal_add(struct event *ev)
 	}
 
 	/* save previous handler and setup new handler */
-	event_debug(("%s: %p: changing signal handler", __func__, ev));
 #ifdef HAVE_SIGACTION
 	memset(&sa, 0, sizeof(sa));
-	sa.sa_handler = evsignal_handler;
+	sa.sa_handler = handler;
 	sa.sa_flags |= SA_RESTART;
 	sigfillset(&sa.sa_mask);
 
@@ -173,13 +169,32 @@ evsignal_add(struct event *ev)
 		return (-1);
 	}
 #else
-	if ((sh = signal(evsignal, evsignal_handler)) == SIG_ERR) {
+	if ((sh = signal(evsignal, handler)) == SIG_ERR) {
 		event_warn("signal");
 		free(sig->sh_old[evsignal]);
 		return (-1);
 	}
 	*sig->sh_old[evsignal] = sh;
 #endif
+
+	return (0);
+}
+
+int
+evsignal_add(struct event *ev)
+{
+	int evsignal;
+	struct event_base *base = ev->ev_base;
+	struct evsignal_info *sig = &ev->ev_base->sig;
+
+	if (ev->ev_events & (EV_READ|EV_WRITE))
+		event_errx(1, "%s: EV_SIGNAL incompatible use", __func__);
+	evsignal = EVENT_SIGNAL(ev);
+
+	event_debug(("%s: %p: changing signal handler", __func__, ev));
+	if (_evsignal_set_handler(base, evsignal, evsignal_handler) == -1)
+		return (-1);
+
 	/* catch signals if they happen quickly */
 	evsignal_base = base;
 
@@ -192,21 +207,17 @@ evsignal_add(struct event *ev)
 }
 
 int
-evsignal_del(struct event *ev)
+_evsignal_restore_handler(struct event_base *base, int evsignal)
 {
-	int evsignal, ret = 0;
-	struct event_base *base = ev->ev_base;
-	struct evsignal_info *sig = &ev->ev_base->sig;
+	int ret = 0;
+	struct evsignal_info *sig = &base->sig;
 #ifdef HAVE_SIGACTION
 	struct sigaction *sh;
 #else
 	ev_sighandler_t *sh;
 #endif
 
-	evsignal = EVENT_SIGNAL(ev);
-
 	/* restore previous handler */
-	event_debug(("%s: %p: restoring signal handler", __func__, ev));
 	sh = sig->sh_old[evsignal];
 	sig->sh_old[evsignal] = NULL;
 #ifdef HAVE_SIGACTION
@@ -225,6 +236,13 @@ evsignal_del(struct event *ev)
 	return ret;
 }
 
+int
+evsignal_del(struct event *ev)
+{
+	event_debug(("%s: %p: restoring signal handler", __func__, ev));
+	return _evsignal_restore_handler(ev->ev_base, EVENT_SIGNAL(ev));
+}
+
 static void
 evsignal_handler(int sig)
 {

test/regress.c

@@ -526,18 +526,19 @@ test_signal_pipeloss(void)
 /*
  * make two bases to catch signals, use both of them.  this only works
  * for event mechanisms that use our signal pipe trick.  kqueue handles
- * signals internally, and it looks like the first kqueue always gets the
- * signal.
+ * signals internally, and all interested kqueues get all the signals.
  */
 void
 test_signal_switchbase(void)
 {
 	struct event ev1, ev2;
 	struct event_base *base1, *base2;
+        int is_kqueue;
 	test_ok = 0;
 	printf("Signal switchbase: ");
 	base1 = event_init();
 	base2 = event_init();
+        is_kqueue = !strcmp(event_get_method(),"kqueue");
 	signal_set(&ev1, SIGUSR1, signal_cb, &ev1);
 	signal_set(&ev2, SIGUSR1, signal_cb, &ev2);
 	if (event_base_set(base1, &ev1) ||
@@ -552,15 +553,22 @@ test_signal_switchbase(void)
 	/* can handle signal before loop is called */
 	raise(SIGUSR1);
 	event_base_loop(base2, EVLOOP_NONBLOCK);
+        if (is_kqueue) {
+                if (!test_ok)
+                        goto done;
+                test_ok = 0;
+        }
 	event_base_loop(base1, EVLOOP_NONBLOCK);
-	if (test_ok) {
+	if (test_ok && !is_kqueue) {
 		test_ok = 0;
+
 		/* set base1 to handle signals */
 		event_base_loop(base1, EVLOOP_NONBLOCK);
 		raise(SIGUSR1);
 		event_base_loop(base1, EVLOOP_NONBLOCK);
 		event_base_loop(base2, EVLOOP_NONBLOCK);
 	}
+ done:
 	event_base_free(base1);
 	event_base_free(base2);
 	cleanup_test();