[dovecot-cvs] dovecot/src/lib ioloop-internal.h, 1.17, 1.18 ioloop-kqueue.c, 1.9, 1.10 ioloop.c, 1.35, 1.36 ioloop.h, 1.18, 1.19

[cras at dovecot.org]

Update of /var/lib/cvs/dovecot/src/lib
In directory talvi:/tmp/cvs-serv7260

Modified Files:
	ioloop-internal.h ioloop-kqueue.c ioloop.c ioloop.h 
Log Message:
OK, so the original kqueue code wasn't actually broken, but it could have
been made much simpler. Updated comments in ioloop.h about IO_READ and
IO_ERROR usage.



Index: ioloop-internal.h
===================================================================
RCS file: /var/lib/cvs/dovecot/src/lib/ioloop-internal.h,v
retrieving revision 1.17
retrieving revision 1.18
diff -u -d -r1.17 -r1.18
--- ioloop-internal.h	16 Aug 2006 15:54:58 -0000	1.17
+++ ioloop-internal.h	17 Aug 2006 18:46:42 -0000	1.18
@@ -27,6 +27,8 @@
 	/* use a doubly linked list so that io_remove() is quick */
 	struct io *prev, *next;
 
+	int refcount;
+
 	int fd;
 	enum io_condition condition;
 

Index: ioloop-kqueue.c
===================================================================
RCS file: /var/lib/cvs/dovecot/src/lib/ioloop-kqueue.c,v
retrieving revision 1.9
retrieving revision 1.10
diff -u -d -r1.9 -r1.10
--- ioloop-kqueue.c	17 Aug 2006 17:43:58 -0000	1.9
+++ ioloop-kqueue.c	17 Aug 2006 18:46:42 -0000	1.10
@@ -16,7 +16,6 @@
 #include "array.h"
 #include "fd-close-on-exec.h"
 #include "ioloop-internal.h"
-#include "ioloop-iolist.h"
 
 #include <unistd.h>
 #include <fcntl.h>
@@ -28,7 +27,6 @@
 	int kq;
 
 	unsigned int deleted_count;
-	array_t ARRAY_DEFINE(fd_index, struct io_list *);
 	array_t ARRAY_DEFINE(events, struct kevent);
 };
 
@@ -46,74 +44,60 @@
 
 	ARRAY_CREATE(&ctx->events, ioloop->pool, struct kevent,
 		     IOLOOP_INITIAL_FD_COUNT);
-	ARRAY_CREATE(&ctx->fd_index, ioloop->pool,
-		     struct io_list *, IOLOOP_INITIAL_FD_COUNT);
 }
 
 void io_loop_handler_deinit(struct ioloop *ioloop)
 {
 	if (close(ioloop->handler_context->kq) < 0)
 		i_error("close(kqueue) in io_loop_handler_deinit() failed: %m");
-	array_free(&ioloop->handler_context->fd_index);
 	array_free(&ioloop->handler_context->events);
 	p_free(ioloop->pool, ioloop->handler_context);
 }
 
-static int io_filter(struct io *io)
-{
-	if ((io->condition & IO_WRITE) != 0)
-		return EVFILT_WRITE;
-	if ((io->condition & (IO_READ | IO_ERROR)) != 0)
-		return EVFILT_READ;
-	return 0;
-}
-
 void io_loop_handle_add(struct ioloop *ioloop, struct io *io)
 {
 	struct ioloop_handler_context *ctx = ioloop->handler_context;
-	struct io_list **list;
 	struct kevent ev;
-	bool first;
 
-	list = array_idx_modifyable(&ctx->fd_index, io->fd);
-	if (*list == NULL)
-		*list = p_new(ioloop->pool, struct io_list, 1);
-	first = ioloop_iolist_add(*list, io);
-
-	EV_SET(&ev, io->fd, io_filter(io), EV_ADD, 0, 0, *list);
-	if (kevent(ctx->kq, &ev, 1, NULL, 0, NULL) < 0)
-		i_fatal("kevent(EV_ADD, %d) failed: %m", io->fd);
-
-	if (first) {
-		/* allow kevent() to return the maximum number of events
-		   by keeping space allocated for each file descriptor */
-		if (ctx->deleted_count > 0)
-			ctx->deleted_count--;
-		else
-			(void)array_append_space(&ctx->events);
+	if ((io->condition & (IO_READ | IO_ERROR)) != 0) {
+		EV_SET(&ev, io->fd, EVFILT_READ, EV_ADD, 0, 0, io);
+		if (kevent(ctx->kq, &ev, 1, NULL, 0, NULL) < 0)
+			i_fatal("kevent(EV_ADD, %d) failed: %m", io->fd);
+	}
+	if ((io->condition & IO_WRITE) != 0) {
+		EV_SET(&ev, io->fd, EVFILT_WRITE, EV_ADD, 0, 0, io);
+		if (kevent(ctx->kq, &ev, 1, NULL, 0, NULL) < 0)
+			i_fatal("kevent(EV_ADD, %d) failed: %m", io->fd);
 	}
+
+	/* allow kevent() to return the maximum number of events
+	   by keeping space allocated for each handle */
+	if (ctx->deleted_count > 0)
+		ctx->deleted_count--;
+	else
+		(void)array_append_space(&ctx->events);
 }
 
 void io_loop_handle_remove(struct ioloop *ioloop, struct io *io)
 {
 	struct ioloop_handler_context *ctx = ioloop->handler_context;
-	struct io_list **list;
 	struct kevent ev;
-	bool last;
-	
-	list = array_idx_modifyable(&ctx->fd_index, io->fd);
-	last = ioloop_iolist_del(*list, io);
-
-	EV_SET(&ev, io->fd, io_filter(io), EV_DELETE, 0, 0, *list);
-	if (kevent(ctx->kq, &ev, 1, NULL, 0, NULL) < 0)
-		i_error("kevent(EV_DELETE, %d) failed: %m", io->fd);
 
-	if (last) {
-		/* since we're not freeing memory in any case, just increase
-		   deleted counter so next handle_add() can just decrease it
-		   insteading of appending to the events array */
-		ctx->deleted_count++;
+	if ((io->condition & (IO_READ | IO_ERROR)) != 0) {
+		EV_SET(&ev, io->fd, EVFILT_READ, EV_DELETE, 0, 0, NULL);
+		if (kevent(ctx->kq, &ev, 1, NULL, 0, NULL) < 0)
+			i_error("kevent(EV_DELETE, %d) failed: %m", io->fd);
+	}
+	if ((io->condition & IO_WRITE) != 0) {
+		EV_SET(&ev, io->fd, EVFILT_WRITE, EV_DELETE, 0, 0, NULL);
+		if (kevent(ctx->kq, &ev, 1, NULL, 0, NULL) < 0)
+			i_error("kevent(EV_DELETE, %d) failed: %m", io->fd);
 	}
+
+	/* since we're not freeing memory in any case, just increase
+	   deleted counter so next handle_add() can just decrease it
+	   insteading of appending to the events array */
+	ctx->deleted_count++;
 }
 
 void io_loop_handler_run(struct ioloop *ioloop)
@@ -123,10 +107,9 @@
 	const struct kevent *event;
 	struct timeval tv;
 	struct timespec ts;
-	struct io_list *list;
+	struct io *io;
 	unsigned int events_count, t_id;
-	int msecs, ret, i, j;
-	bool call;
+	int msecs, ret, i;
 
 	/* get the time left for next timeout task */
 	msecs = io_loop_get_wait_time(ioloop->timeouts, &tv, NULL);
@@ -145,40 +128,32 @@
 	if (!ioloop->running)
 		return;
 
+	/* reference all IOs */
+	for (i = 0; i < ret; i++) {
+		io = events[i].udata;
+		io->refcount++;
+	}
+
 	for (i = 0; i < ret; i++) {
 		/* io_loop_handle_add() may cause events array reallocation,
 		   so we have use array_idx() */
 		event = array_idx(&ctx->events, i);
-		list = (void *)event->udata;
-
-		for (j = 0; j < IOLOOP_IOLIST_IOS_PER_FD; j++) {
-			struct io *io = list->ios[j];
-
-			if (io == NULL)
-				continue;
-
-			call = FALSE;
-			if ((event->flags & EV_ERROR) != 0) {
-				errno = event->data;
-				i_error("kevent(): invalid fd %d callback "
-					"%p: %m", io->fd, (void *)io->callback);
-			} else if ((event->flags & EV_EOF) != 0)
-				call = TRUE;
-			else if ((io->condition & IO_READ) != 0)
-				call = event->filter == EVFILT_READ;
-			else if ((io->condition & IO_WRITE) != 0)
-				call = event->filter == EVFILT_WRITE;
+		io = (void *)event->udata;
 
-			if (call) {
-				t_id = t_push();
-				io->callback(io->context);
-				if (t_pop() != t_id) {
-					i_panic("Leaked a t_pop() call in "
-						"I/O handler %p",
-						(void *)io->callback);
-				}
+		/* callback is NULL if io_remove() was already called */
+		if (io->callback != NULL) {
+			t_id = t_push();
+			io->callback(io->context);
+			if (t_pop() != t_id) {
+				i_panic("Leaked a t_pop() call in "
+					"I/O handler %p",
+					(void *)io->callback);
 			}
 		}
+
+		i_assert(io->refcount > 0);
+		if (--io->refcount == 0)
+			p_free(current_ioloop->pool, io);
 	}
 }
 

Index: ioloop.c
===================================================================
RCS file: /var/lib/cvs/dovecot/src/lib/ioloop.c,v
retrieving revision 1.35
retrieving revision 1.36
diff -u -d -r1.35 -r1.36
--- ioloop.c	25 Mar 2006 10:10:41 -0000	1.35
+++ ioloop.c	17 Aug 2006 18:46:42 -0000	1.36
@@ -24,6 +24,7 @@
 	i_assert((condition & IO_NOTIFY) == 0);
 
 	io = p_new(current_ioloop->pool, struct io, 1);
+	io->refcount = 1;
 	io->fd = fd;
         io->condition = condition;
 
@@ -67,6 +68,8 @@
 
 	*_io = NULL;
 
+	i_assert(io->refcount > 0);
+
 	/* unlink from linked list */
 	if (io->prev != NULL)
 		io->prev->next = io->next;
@@ -90,7 +93,10 @@
 		io_loop_notify_remove(current_ioloop, io);
 	}
 
-	p_free(current_ioloop->pool, io);
+	io->callback = NULL;
+
+	if (--io->refcount == 0)
+		p_free(current_ioloop->pool, io);
 }
 
 static void timeout_list_insert(struct ioloop *ioloop, struct timeout *timeout)

Index: ioloop.h
===================================================================
RCS file: /var/lib/cvs/dovecot/src/lib/ioloop.h,v
retrieving revision 1.18
retrieving revision 1.19
diff -u -d -r1.18 -r1.19
--- ioloop.h	9 Apr 2006 14:48:34 -0000	1.18
+++ ioloop.h	17 Aug 2006 18:46:42 -0000	1.19
@@ -11,6 +11,8 @@
 enum io_condition {
 	IO_READ		= 0x01,
 	IO_WRITE	= 0x02,
+	/* IO_ERROR can be used to check when writable pipe's reader side
+	   closes the pipe. For other uses IO_READ should work just as well. */
 	IO_ERROR	= 0x04,
 	
 	/* internal */
@@ -28,11 +30,11 @@
 
 extern struct ioloop *current_ioloop;
 
-/* I/O listeners - you can create different handlers for IO_READ and IO_WRITE,
-   but make sure you don't create multiple handlers of same type, it's not
-   checked and removing one will stop the other from working as well.
+/* You can create different handlers for IO_READ and IO_WRITE. IO_READ and
+   IO_ERROR can't use different handlers (and there's no point anyway).
 
- */
+   Don't try to add multiple handlers for the same type. It's not checked and
+   the behavior will be undefined. */
 struct io *io_add(int fd, enum io_condition condition,
 		  io_callback_t *callback, void *context);
 struct io *io_add_notify(const char *path, io_callback_t *callback,