eloop: Use kqueue or epoll to wait for a fd to become ready#612
Conversation
Use kqueue or epoll for eloop_waitfd(). This requires opening a new kqueue or epoll to handle this as this is a one shot event we don't want to touch the eloop events. This works with RLIMIT_NOFILE = 0.
WalkthroughThis PR refactors the event loop's fd-waiter implementation by introducing explicit fd-waiter management, centralizing backend event-bit translation helpers, refactoring backend-specific event registration to accept explicit fd, and updating initialization, cleanup, fork handling, and caller code accordingly. Privilege dropping and message framing logic are also updated. ChangesEvent Loop fd-Waiter API
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/eloop.c (1)
923-941:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winPreprocessor macro mismatch:
USE_POLLshould beUSE_PPOLL.The guard at line 923 checks
USE_POLL, but the codebase definesUSE_PPOLL(line 54) for non-kqueue/non-epoll platforms. This meanseloop_polleventswill never be compiled, causing a build failure on platforms that fall into theUSE_PPOLLpath sinceeloop_waitfdcallseloop_polleventsat line 992.Proposed fix
-#elif defined(USE_POLL) +#elif defined(USE_PPOLL) static unsigned short eloop_pollevents(struct pollfd *pfd)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/eloop.c` around lines 923 - 941, The conditional compilation guard for eloop_pollevents is wrong: change the `#elif` defined(USE_POLL) to `#elif` defined(USE_PPOLL) so eloop_pollevents is compiled when the project uses USE_PPOLL; ensure the block containing eloop_pollevents (and its closing `#endif`) matches the USE_PPOLL macro so calls from eloop_waitfd to eloop_pollevents resolve correctly.
🧹 Nitpick comments (1)
src/eloop.c (1)
996-1006: 💤 Low valueGuard against double-close or calling without prior open.
If
eloop_closefdwaiteris called whenwaitfdis already-1(never opened, or already closed),close(-1)returns-1withEBADF. Adding a guard would be consistent witheloop_freeand make the API more defensive.Proposed fix
int eloop_closefdwaiter(struct eloop *eloop) { `#if` defined(USE_KQUEUE) || defined(USE_EPOLL) - int err = close(eloop->waitfd); + int err = 0; + if (eloop->waitfd != -1) + err = close(eloop->waitfd); eloop->waitfd = -1; return err; `#else` return 0; `#endif` }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/eloop.c` around lines 996 - 1006, eloop_closefdwaiter should guard against closing an invalid fd: check eloop->waitfd != -1 before calling close(eloop->waitfd), only call close when valid, set eloop->waitfd = -1 after a successful/attempted close, and return 0 if it was already -1; mirror the defensive behavior used in eloop_free and keep the existing conditional compilation around USE_KQUEUE/USE_EPOLL.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/eloop.c`:
- Around line 923-941: The conditional compilation guard for eloop_pollevents is
wrong: change the `#elif` defined(USE_POLL) to `#elif` defined(USE_PPOLL) so
eloop_pollevents is compiled when the project uses USE_PPOLL; ensure the block
containing eloop_pollevents (and its closing `#endif`) matches the USE_PPOLL macro
so calls from eloop_waitfd to eloop_pollevents resolve correctly.
---
Nitpick comments:
In `@src/eloop.c`:
- Around line 996-1006: eloop_closefdwaiter should guard against closing an
invalid fd: check eloop->waitfd != -1 before calling close(eloop->waitfd), only
call close when valid, set eloop->waitfd = -1 after a successful/attempted
close, and return 0 if it was already -1; mirror the defensive behavior used in
eloop_free and keep the existing conditional compilation around
USE_KQUEUE/USE_EPOLL.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 24cc2be4-6e87-4dc3-a7c2-a1cecefda416
📒 Files selected for processing (4)
src/eloop.csrc/eloop.hsrc/privsep-root.csrc/privsep.c
On some platforms, poll/select don't work with RLIMIT_NOFILE = 0.
Luckily we can use kevent/epoll instead and it then works.
Add
eloop_openfdwaiter()andeloop_closefdwaiter()so we can setup the new polling mechanism as it might not always be required.Use RLIMIT_NOFILE = 0 on all platforms when using privsep.