From: KOSAKI Motohiro Date: 2011-05-04T16:55:11+09:00 Subject: [ruby-core:35998] Re: [Ruby 1.9 - Feature #4531] [PATCH 0/7] use poll() instead of select() in certain cases Hi 2011/5/4 Eric Wong : > KOSAKI Motohiro wrote: >> diff --git a/ext/-test-/wait_for_single_fd/wait_for_single_fd.c >> b/ext/-test-/wait_for_single_fd/wait_for_single_fd.c >> index d406724..6efd1af 100644 >> --- a/ext/-test-/wait_for_single_fd/wait_for_single_fd.c >> +++ b/ext/-test-/wait_for_single_fd/wait_for_single_fd.c >> @@ -25,6 +25,7 @@ Init_wait_for_single_fd(void) >> � � �rb_define_const(rb_cObject, "RB_WAITFD_IN", INT2NUM(RB_WAITFD_IN)); >> � � �rb_define_const(rb_cObject, "RB_WAITFD_OUT", INT2NUM(RB_WAITFD_OUT)); >> � � �rb_define_const(rb_cObject, "RB_WAITFD_PRI", INT2NUM(RB_WAITFD_PRI)); >> + � �rb_define_const(rb_cObject, "INT_MAX", INT2NUM(INT_MAX)); >> � � �rb_define_singleton_method(rb_cIO, "wait_for_single_fd", >> � � � � � � � � � � � � � � � � wait_for_single_fd, 3); >> >> Strongly disagree. Any language change should be passed matz review. > > Huh? �ext/-test-/* is only loaded during tests and never installed. > No users see anything in ext/-test-/* Oops, my bad. I misunderstood your diff. ok, I'll commit it. > >> 1) use ppoll(2) if available. and use INT_MAX if unavailable. or >> 2) fallback select(2) >> >> 1) is safe because linux has ppol(2). > > OK, good point about ppoll(), I forgot that exists. �I'll work on that > later or tomorrow. ok. > >> � � �if (result > 0) { >> - � � � /* remain compatible with select(2)-based implementation */ >> + � � � /* >> + � � � �* Remain compatible with the select(2)-based implementation: >> + � � � �* 1) mask out poll()-only revents such as POLLHUP/POLLERR >> + � � � �* 2) In case revents only consists of masked-out events, return all >> + � � � �* � �requested events in the result and force the caller to make an >> + � � � �* � �extra syscall (e.g. read/write/send/recv) to get the error. >> + � � � �*/ >> � � � � result = (int)(fds.revents & fds.events); >> � � � � return result == 0 ? events : result; >> � � �} >> >> I don't understand this. Why does this behavior help to compatible? >> When do we use it? > > We need to ensure rb_wait_for_single_fd(fd, events, timeval) returns > only a subset of its +events+ argument because that's all select() is > capable of. Yes. > > If poll() returns POLLHUP/POLLERR, we should not expose those flags to > callers of rb_wait_for_single_fd() since it would then behave > differently if poll() or select() were used. > > � �int events = RB_WAITFD_IN | RB_WAITFD_OUT; > � �int revents = rb_wait_for_single_fd(fd, events, NULL); > � �/* poll() itself may return POLLERR, but we prevent it from being in > � � * revents since select() can't return that */ > � �if (revents & RB_WAITFD_IN) { > � � � �/* since we don't know POLLERR, we fall back to fail here */ > � � � �if (read(fd, ...) < 0) > � � � � � �rb_sys_fail(0); > � �} > � �if (revents & RB_WAITFD_OUT) { > � � � �/* since we don't know POLLERR, we fall back to fail here */ > � � � �if (write(fd, ...) < 0) > � � � � � �rb_sys_fail(0); > � �} > � �/* user code shouldn't care about anything else since it only > � � * requested RB_WAITFD_IN|RB_WAITFD_OUT */ Then, correct way is /* copyed from linux */ #define POLLIN_SET (POLLRDNORM | POLLRDBAND | POLLIN | POLLHUP | POLLERR) #define POLLOUT_SET (POLLWRBAND | POLLWRNORM | POLLOUT | POLLERR) #define POLLEX_SET (POLLPRI) int revent_filter(int revents) { int ret = 0; if (revents & POLLIN_SET) ret |= RB_WAITFD_IN; if (revents & POLLOUT_SET) ret |= RB_WAITFD_OUT; if (revents & POLLEX_SET) ret |= RB_WAITFD_PRI; } this code don't make false positive. I'll commit it.