On Thu, Aug 12, 2004 at 03:55:41PM +0100, Joe Orton wrote:
> On Thu, Aug 12, 2004 at 10:44:29AM +0100, Joe Orton wrote:
> > warnings on LP64 platforms. Is there a cleaner alternative? epoll
> > stuff looks good, I'll commit that.
>
> Actually there's a problem: this change breaks apr_pollset_remove();
> when the query_set array is shuffled up after removing a descriptor, any
> epoll descriptors may still reference the old indexes into query_set
> from data.u32.
...
> The best fix for this would probably be to have apr_pollset_destroy
> leave holes in the query_set array which apr_pollset_add will fill in, I
> think?
e.g. how does this look? I've only tested epoll and poll with these
changes.
Index: poll/unix/poll.c
===================================================================
RCS file: /home/cvs/apr/poll/unix/poll.c,v
retrieving revision 1.46
diff -u -r1.46 poll.c
--- poll/unix/poll.c 7 Jul 2004 07:40:12 -0000 1.46
+++ poll/unix/poll.c 12 Aug 2004 16:35:15 -0000
@@ -355,10 +355,12 @@
apr_uint32_t nalloc;
#ifdef HAVE_KQUEUE
int kqueue_fd;
+ apr_uint32_t nextfree;
struct kevent kevent;
struct kevent *ke_set;
#elif defined(HAVE_EPOLL)
int epoll_fd;
+ apr_uint32_t nextfree;
struct epoll_event *pollset;
#elif defined(HAVE_POLL)
struct pollfd *pollset;
@@ -409,11 +411,13 @@
if ((*pollset)->kqueue_fd == -1) {
return APR_ENOMEM;
}
+ (*pollset)->nextfree = 0;
apr_pool_cleanup_register(p, (void*)(*pollset), backend_cleanup,
apr_pool_cleanup_null);
#elif defined(HAVE_EPOLL)
(*pollset)->epoll_fd = epoll_create(size);
(*pollset)->pollset = apr_palloc(p, size * sizeof(struct epoll_event));
+ (*pollset)->nextfree = 0;
apr_pool_cleanup_register(p, (void*)(*pollset), backend_cleanup,
apr_pool_cleanup_null);
#elif defined(HAVE_POLL)
@@ -445,24 +449,15 @@
APR_DECLARE(apr_status_t) apr_pollset_add(apr_pollset_t *pollset,
const apr_pollfd_t *descriptor)
{
-#ifdef HAVE_KQUEUE
- apr_os_sock_t fd;
-#elif defined(HAVE_EPOLL)
- struct epoll_event ev;
- int ret = -1;
-#else
-#if !defined(HAVE_POLL)
+#if defined(HAVE_KQUEUE) || defined(HAVE_EPOLL)
apr_os_sock_t fd;
+ apr_uint32_t idx;
#endif
+#ifdef HAVE_EPOLL
+ struct epoll_event ev;
#endif
- if (pollset->nelts == pollset->nalloc) {
- return APR_ENOMEM;
- }
-
- pollset->query_set[pollset->nelts] = *descriptor;
-
-#ifdef HAVE_KQUEUE
+#if defined(HAVE_KQUEUE) || defined(HAVE_EPOLL)
if (descriptor->desc_type == APR_POLL_SOCKET) {
fd = descriptor->desc.s->socketdes;
}
@@ -470,8 +465,32 @@
fd = descriptor->desc.f->filedes;
}
+ idx = pollset->nextfree;
+ if (idx == pollset->nalloc) {
+ return APR_ENOMEM;
+ }
+
+ /* Find the next unused descriptor (else nextfree == nelts). */
+ do {
+ pollset->nextfree++;
+ } while (pollset->nextfree < pollset->nelts
+ && pollset->query_set[pollset->nextfree].desc_type !=
APR_NO_DESC);
+
+ if (idx == pollset->nelts) {
+ pollset->nelts++;
+ }
+
+ pollset->query_set[idx] = *descriptor;
+#else
+ if (pollset->nelts == pollset->nalloc) {
+ return APR_ENOMEM;
+ }
+ pollset->query_set[pollset->nelts] = *descriptor;
+#endif
+
+#if defined(HAVE_KQUEUE)
if (descriptor->reqevents & APR_POLLIN) {
- EV_SET(&pollset->kevent, fd, EVFILT_READ, EV_ADD, 0, 0, NULL);
+ EV_SET(&pollset->kevent, fd, EVFILT_READ, EV_ADD, 0, 0, (void*)idx);
if (kevent(pollset->kqueue_fd, &pollset->kevent, 1, NULL, 0,
NULL) == -1) {
@@ -480,9 +499,9 @@
}
if (descriptor->reqevents & APR_POLLOUT) {
- EV_SET(&pollset->kevent, fd, EVFILT_WRITE, EV_ADD, 0, 0, NULL);
+ EV_SET(&pollset->kevent, fd, EVFILT_WRITE, EV_ADD, 0, 0, (void *)idx);
- if (kevent(pollset->kqueue_fd, &pollset->kevent, 1, NULL, 0,
+ if (kevent(pollset->kqueue_fd, &pollset->kevent, 1, NULL, 0,
NULL) == -1) {
return APR_ENOMEM;
}
@@ -490,18 +509,9 @@
#elif defined(HAVE_EPOLL)
ev.events = get_epoll_event(descriptor->reqevents);
- if (descriptor->desc_type == APR_POLL_SOCKET) {
- ev.data.fd = descriptor->desc.s->socketdes;
- ret = epoll_ctl(pollset->epoll_fd, EPOLL_CTL_ADD,
- descriptor->desc.s->socketdes, &ev);
- }
- else {
- ev.data.fd = descriptor->desc.f->filedes;
- ret = epoll_ctl(pollset->epoll_fd, EPOLL_CTL_ADD,
- descriptor->desc.f->filedes, &ev);
- }
- if (0 != ret) {
- return APR_EBADF;
+ ev.data.u32 = idx;
+ if (epoll_ctl(pollset->epoll_fd, EPOLL_CTL_ADD, fd, &ev)) {
+ return errno;
}
#elif defined(HAVE_POLL)
@@ -513,7 +523,8 @@
}
pollset->pollset[pollset->nelts].events = get_event(descriptor->reqevents);
-#else
+ pollset->nelts++;
+#else /* !(HAVE_POLL || HAVE_EPOLL || HAVE_KQUEUE) */
if (descriptor->desc_type == APR_POLL_SOCKET) {
#ifdef NETWARE
/* NetWare can't handle mixed descriptor types in select() */
@@ -563,8 +574,8 @@
if ((int)fd > pollset->maxfd) {
pollset->maxfd = (int)fd;
}
-#endif
pollset->nelts++;
+#endif
return APR_SUCCESS;
}
@@ -572,96 +583,67 @@
const apr_pollfd_t *descriptor)
{
apr_uint32_t i;
-#ifdef HAVE_KQUEUE
+#if defined(HAVE_KQUEUE) || defined(HAVE_EPOLL)
apr_os_sock_t fd;
-#elif defined(HAVE_EPOLL)
+ int found = 0;
+#endif
+#if defined(HAVE_EPOLL)
struct epoll_event ev;
- int ret = -1;
#elif !defined(HAVE_POLL)
apr_os_sock_t fd;
#endif
-#ifdef HAVE_KQUEUE
+#if defined(HAVE_KQUEUE) || defined(HAVE_EPOLL)
+ if (descriptor->desc_type == APR_POLL_SOCKET) {
+ fd = descriptor->desc.s->socketdes;
+ }
+ else {
+ fd = descriptor->desc.f->filedes;
+ }
+
for (i = 0; i < pollset->nelts; i++) {
if (descriptor->desc.s == pollset->query_set[i].desc.s) {
- /* Found an instance of the fd: remove this and any other copies
*/
- apr_uint32_t dst = i;
- apr_uint32_t old_nelts = pollset->nelts;
- pollset->nelts--;
- for (i++; i < old_nelts; i++) {
- if (descriptor->desc.s == pollset->query_set[i].desc.s) {
- pollset->nelts--;
- }
- else {
- pollset->query_set[dst] = pollset->query_set[i];
- dst++;
- }
- }
-
- if (descriptor->desc_type == APR_POLL_SOCKET) {
- fd = descriptor->desc.s->socketdes;
- }
- else {
- fd = descriptor->desc.f->filedes;
- }
-
- if (descriptor->reqevents & APR_POLLIN) {
- EV_SET(&pollset->kevent, fd,
- EVFILT_READ, EV_DELETE, 0, 0, NULL);
-
- if (kevent(pollset->kqueue_fd, &pollset->kevent, 1, NULL, 0,
- NULL) == -1) {
- return APR_EBADF;
- }
+ if (i < pollset->nextfree) {
+ pollset->nextfree = i;
}
+ pollset->query_set[i].desc_type = APR_NO_DESC;
+ found = 1;
+ }
+ }
- if (descriptor->reqevents & APR_POLLOUT) {
- EV_SET(&pollset->kevent, fd,
- EVFILT_WRITE, EV_DELETE, 0, 0, NULL);
-
- if (kevent(pollset->kqueue_fd, &pollset->kevent, 1, NULL, 0,
- NULL) == -1) {
- return APR_EBADF;
- }
- }
+ if (!found) {
+ return APR_NOTFOUND;
+ }
+#endif
- return APR_SUCCESS;
+#if defined(HAVE_KQUEUE)
+ if (descriptor->reqevents & APR_POLLIN) {
+ EV_SET(&pollset->kevent, fd,
+ EVFILT_READ, EV_DELETE, 0, 0, NULL);
+
+ if (kevent(pollset->kqueue_fd, &pollset->kevent, 1, NULL, 0,
+ NULL) == -1) {
+ return APR_EBADF;
}
}
-#elif defined(HAVE_EPOLL)
- for (i = 0; i < pollset->nelts; i++) {
- if (descriptor->desc.s == pollset->query_set[i].desc.s) {
- /* Found an instance of the fd: remove this and any other copies
*/
- apr_uint32_t dst = i;
- apr_uint32_t old_nelts = pollset->nelts;
- pollset->nelts--;
- for (i++; i < old_nelts; i++) {
- if (descriptor->desc.s == pollset->query_set[i].desc.s) {
- pollset->nelts--;
- }
- else {
- pollset->query_set[dst] = pollset->query_set[i];
- dst++;
- }
- }
- ev.events = get_epoll_event(descriptor->reqevents);
- if (descriptor->desc_type == APR_POLL_SOCKET) {
- ev.data.fd = descriptor->desc.s->socketdes;
- ret = epoll_ctl(pollset->epoll_fd, EPOLL_CTL_DEL,
- descriptor->desc.s->socketdes, &ev);
- }
- else {
- ev.data.fd = descriptor->desc.f->filedes;
- ret = epoll_ctl(pollset->epoll_fd, EPOLL_CTL_DEL,
- descriptor->desc.f->filedes, &ev);
- }
- if (ret < 0) {
- return APR_EBADF;
- }
-
- return APR_SUCCESS;
+
+ if (descriptor->reqevents & APR_POLLOUT) {
+ EV_SET(&pollset->kevent, fd,
+ EVFILT_WRITE, EV_DELETE, 0, 0, NULL);
+
+ if (kevent(pollset->kqueue_fd, &pollset->kevent, 1, NULL, 0,
+ NULL) == -1) {
+ return APR_EBADF;
}
}
+
+ return APR_SUCCESS;
+#elif defined(HAVE_EPOLL)
+ /* event argument is ignored but must not be NULL */
+ if (epoll_ctl(pollset->epoll_fd, EPOLL_CTL_DEL, fd, &ev) < 0) {
+ return errno;
+ }
+ return APR_SUCCESS;
#elif defined(HAVE_POLL)
for (i = 0; i < pollset->nelts; i++) {
if (descriptor->desc.s == pollset->query_set[i].desc.s) {
@@ -682,7 +664,7 @@
return APR_SUCCESS;
}
}
-
+ return APR_NOTFOUND;
#else /* no poll */
if (descriptor->desc_type == APR_POLL_SOCKET) {
fd = descriptor->desc.s->socketdes;
@@ -719,9 +701,8 @@
return APR_SUCCESS;
}
}
-#endif /* no poll */
-
return APR_NOTFOUND;
+#endif /* no poll */
}
#ifdef HAVE_KQUEUE
APR_DECLARE(apr_status_t) apr_pollset_poll(apr_pollset_t *pollset,
@@ -788,8 +769,7 @@
apr_int32_t *num,
const apr_pollfd_t **descriptors)
{
- int rv;
- apr_uint32_t i, j, k;
+ int rv, i;
if (timeout > 0) {
timeout /= 1000;
@@ -799,43 +779,27 @@
timeout);
(*num) = rv;
if (rv < 0) {
- return apr_get_netos_error();
+ return errno;
}
if (rv == 0) {
return APR_TIMEUP;
}
- j = 0;
- for (i = 0; i < pollset->nelts; i++) {
- if (pollset->pollset[i].events != 0) {
- /* TODO: Is there a better way to re-associate our data? */
- for (k = 0; k < pollset->nelts; k++) {
- if (pollset->query_set[k].desc_type == APR_POLL_SOCKET &&
- pollset->query_set[k].desc.s->socketdes ==
- pollset->pollset[i].data.fd) {
- pollset->result_set[j] = pollset->query_set[k];
- pollset->result_set[j].rtnevents =
- get_epoll_revent(pollset->pollset[i].events);
- j++;
- break;
- }
- else if (pollset->query_set[k].desc_type == APR_POLL_FILE
- && pollset->query_set[k].desc.f->filedes ==
- pollset->pollset[i].data.fd) {
- pollset->result_set[j] = pollset->query_set[k];
- pollset->result_set[j].rtnevents =
- get_epoll_revent(pollset->pollset[i].events);
- j++;
- break;
- }
- }
- }
+
+ for (i = 0; i < rv; i++) {
+ pollset->result_set[i] =
+ pollset->query_set[pollset->pollset[i].data.u32];
+ pollset->result_set[i].rtnevents =
+ get_epoll_revent(pollset->pollset[i].events);
}
+
if (descriptors) {
*descriptors = pollset->result_set;
}
return APR_SUCCESS;
}
+
#elif defined(HAVE_POLL)
+
APR_DECLARE(apr_status_t) apr_pollset_poll(apr_pollset_t *pollset,
apr_interval_time_t timeout,
apr_int32_t *num,
