Use OwnedFd in epoll and kqueue selectors

Author: Thomasdezeeuw

src/sys/unix/mod.rs

@@ -4,6 +4,7 @@
 #[allow(unused_macros)]
 macro_rules! syscall {
     ($fn: ident ( $($arg: expr),* $(,)* ) ) => {{
+        #[allow(unused_unsafe)]
         let res = unsafe { libc::$fn($($arg, )*) };
         if res < 0 {
             Err(std::io::Error::last_os_error())

src/sys/unix/selector/epoll.rs

@@ -1,4 +1,4 @@
-use std::os::fd::{AsRawFd, RawFd};
+use std::os::fd::{AsRawFd, FromRawFd, OwnedFd, RawFd};
 #[cfg(debug_assertions)]
 use std::sync::atomic::{AtomicUsize, Ordering};
 use std::time::Duration;
@@ -16,20 +16,22 @@ static NEXT_ID: AtomicUsize = AtomicUsize::new(1);
 pub struct Selector {
     #[cfg(debug_assertions)]
     id: usize,
-    ep: RawFd,
+    ep: OwnedFd,
 }
 
 impl Selector {
     pub fn new() -> io::Result<Selector> {
+        // SAFETY: `epoll_create1(2)` ensures the fd is valid.
+        let ep = unsafe { OwnedFd::from_raw_fd(syscall!(epoll_create1(libc::EPOLL_CLOEXEC))?) };
         Ok(Selector {
             #[cfg(debug_assertions)]
             id: NEXT_ID.fetch_add(1, Ordering::Relaxed),
-            ep: syscall!(epoll_create1(libc::EPOLL_CLOEXEC))?,
+            ep,
         })
     }
 
     pub fn try_clone(&self) -> io::Result<Selector> {
-        syscall!(fcntl(self.ep, libc::F_DUPFD_CLOEXEC, super::LOWEST_FD)).map(|ep| Selector {
+        self.ep.try_clone().map(|ep| Selector {
             // It's the same selector, so we use the same id.
             #[cfg(debug_assertions)]
             id: self.id,
@@ -52,7 +54,7 @@ impl Selector {
 
         events.clear();
         syscall!(epoll_wait(
-            self.ep,
+            self.ep.as_raw_fd(),
             events.as_mut_ptr(),
             events.capacity() as i32,
             timeout,
@@ -72,7 +74,8 @@ impl Selector {
             _pad: 0,
         };
 
-        syscall!(epoll_ctl(self.ep, libc::EPOLL_CTL_ADD, fd, &mut event)).map(|_| ())
+        let ep = self.ep.as_raw_fd();
+        syscall!(epoll_ctl(ep, libc::EPOLL_CTL_ADD, fd, &mut event)).map(|_| ())
     }
 
     pub fn reregister(&self, fd: RawFd, token: Token, interests: Interest) -> io::Result<()> {
@@ -83,11 +86,13 @@ impl Selector {
             _pad: 0,
         };
 
-        syscall!(epoll_ctl(self.ep, libc::EPOLL_CTL_MOD, fd, &mut event)).map(|_| ())
+        let ep = self.ep.as_raw_fd();
+        syscall!(epoll_ctl(ep, libc::EPOLL_CTL_MOD, fd, &mut event)).map(|_| ())
     }
 
     pub fn deregister(&self, fd: RawFd) -> io::Result<()> {
-        syscall!(epoll_ctl(self.ep, libc::EPOLL_CTL_DEL, fd, ptr::null_mut())).map(|_| ())
+        let ep = self.ep.as_raw_fd();
+        syscall!(epoll_ctl(ep, libc::EPOLL_CTL_DEL, fd, ptr::null_mut())).map(|_| ())
     }
 }
 
@@ -102,15 +107,7 @@ cfg_io_source! {
 
 impl AsRawFd for Selector {
     fn as_raw_fd(&self) -> RawFd {
-        self.ep
-    }
-}
-
-impl Drop for Selector {
-    fn drop(&mut self) {
-        if let Err(err) = syscall!(close(self.ep)) {
-            error!("error closing epoll: {}", err);
-        }
+        self.ep.as_raw_fd()
     }
 }
 

src/sys/unix/selector/kqueue.rs

@@ -1,7 +1,7 @@
 use crate::{Interest, Token};
 use std::mem::{self, MaybeUninit};
 use std::ops::{Deref, DerefMut};
-use std::os::fd::{AsRawFd, RawFd};
+use std::os::fd::{AsRawFd, FromRawFd, OwnedFd, RawFd};
 #[cfg(debug_assertions)]
 use std::sync::atomic::{AtomicUsize, Ordering};
 use std::time::Duration;
@@ -65,24 +65,23 @@ macro_rules! kevent {
 pub struct Selector {
     #[cfg(debug_assertions)]
     id: usize,
-    kq: RawFd,
+    kq: OwnedFd,
 }
 
 impl Selector {
     pub fn new() -> io::Result<Selector> {
-        let kq = syscall!(kqueue())?;
-        let selector = Selector {
+        // SAFETY: `kqueue(2)` ensures the fd is valid.
+        let kq = unsafe { OwnedFd::from_raw_fd(syscall!(kqueue())?) };
+        syscall!(fcntl(kq.as_raw_fd(), libc::F_SETFD, libc::FD_CLOEXEC))?;
+        Ok(Selector {
             #[cfg(debug_assertions)]
             id: NEXT_ID.fetch_add(1, Ordering::Relaxed),
             kq,
-        };
-
-        syscall!(fcntl(kq, libc::F_SETFD, libc::FD_CLOEXEC))?;
-        Ok(selector)
+        })
     }
 
     pub fn try_clone(&self) -> io::Result<Selector> {
-        syscall!(fcntl(self.kq, libc::F_DUPFD_CLOEXEC, super::LOWEST_FD)).map(|kq| Selector {
+        self.kq.try_clone().map(|kq| Selector {
             // It's the same selector, so we use the same id.
             #[cfg(debug_assertions)]
             id: self.id,
@@ -106,7 +105,7 @@ impl Selector {
 
         events.clear();
         syscall!(kevent(
-            self.kq,
+            self.kq.as_raw_fd(),
             ptr::null(),
             0,
             events.as_mut_ptr(),
@@ -156,7 +155,7 @@ impl Selector {
             // the array.
             slice::from_raw_parts_mut(changes[0].as_mut_ptr(), n_changes)
         };
-        kevent_register(self.kq, changes, &[libc::EPIPE as i64])
+        kevent_register(self.kq.as_raw_fd(), changes, &[libc::EPIPE as i64])
     }
 
     pub fn reregister(&self, fd: RawFd, token: Token, interests: Interest) -> io::Result<()> {
@@ -186,7 +185,7 @@ impl Selector {
         //
         // For the explanation of ignoring `EPIPE` see `register`.
         kevent_register(
-            self.kq,
+            self.kq.as_raw_fd(),
             &mut changes,
             &[libc::ENOENT as i64, libc::EPIPE as i64],
         )
@@ -204,7 +203,7 @@ impl Selector {
         // the ENOENT error when it comes up. The ENOENT error informs us that
         // the filter wasn't there in first place, but we don't really care
         // about that since our goal is to remove it.
-        kevent_register(self.kq, &mut changes, &[libc::ENOENT as i64])
+        kevent_register(self.kq.as_raw_fd(), &mut changes, &[libc::ENOENT as i64])
     }
 
     // Used by `Waker`.
@@ -224,7 +223,8 @@ impl Selector {
             token.0
         );
 
-        syscall!(kevent(self.kq, &kevent, 1, &mut kevent, 1, ptr::null())).and_then(|_| {
+        let kq = self.kq.as_raw_fd();
+        syscall!(kevent(kq, &kevent, 1, &mut kevent, 1, ptr::null())).and_then(|_| {
             if (kevent.flags & libc::EV_ERROR) != 0 && kevent.data != 0 {
                 Err(io::Error::from_raw_os_error(kevent.data as i32))
             } else {
@@ -250,7 +250,8 @@ impl Selector {
         );
         kevent.fflags = libc::NOTE_TRIGGER;
 
-        syscall!(kevent(self.kq, &kevent, 1, &mut kevent, 1, ptr::null())).and_then(|_| {
+        let kq = self.kq.as_raw_fd();
+        syscall!(kevent(kq, &kevent, 1, &mut kevent, 1, ptr::null())).and_then(|_| {
             if (kevent.flags & libc::EV_ERROR) != 0 && kevent.data != 0 {
                 Err(io::Error::from_raw_os_error(kevent.data as i32))
             } else {
@@ -314,15 +315,7 @@ cfg_io_source! {
 
 impl AsRawFd for Selector {
     fn as_raw_fd(&self) -> RawFd {
-        self.kq
-    }
-}
-
-impl Drop for Selector {
-    fn drop(&mut self) {
-        if let Err(err) = syscall!(close(self.kq)) {
-            error!("error closing kqueue: {}", err);
-        }
+        self.kq.as_raw_fd()
     }
 }
 

src/sys/unix/selector/mod.rs

@@ -78,13 +78,3 @@ mod kqueue;
     ),
 ))]
 pub(crate) use self::kqueue::{event, Event, Events, Selector};
-
-/// Lowest file descriptor used in `Selector::try_clone`.
-///
-/// # Notes
-///
-/// Usually fds 0, 1 and 2 are standard in, out and error. Some application
-/// blindly assume this to be true, which means using any one of those a select
-/// could result in some interesting and unexpected errors. Avoid that by using
-/// an fd that doesn't have a pre-determined usage.
-const LOWEST_FD: libc::c_int = 3;

src/sys/unix/selector/poll.rs

@@ -11,7 +11,6 @@ use std::sync::{Arc, Condvar, Mutex};
 use std::time::Duration;
 use std::{cmp, fmt, io};
 
-use crate::sys::unix::selector::LOWEST_FD;
 use crate::sys::unix::waker::WakerInternal;
 use crate::{Interest, Token};
 
@@ -34,9 +33,6 @@ impl Selector {
     }
 
     pub fn try_clone(&self) -> io::Result<Selector> {
-        // Just to keep the compiler happy :)
-        let _ = LOWEST_FD;
-
         let state = self.state.clone();
 
         Ok(Selector { state })