Change poll's timeout from c_int to Option<&Timespec>. (#1285)

* Change `poll`'s timeout from `c_int` to `Option<&Timespec>`.

This harmonizes the timeout with the rest of rustix, which uses
`Timespec` for all time values. And, it eliminates the awkwardness
of using `-1` as a sentinel value.

On platforms with `ppoll`, the `Timespec` can be passed straight
to the OS. On platforms without `ppoll`, we have to do a fallible
conversion into `c_int`.

* Return `Errno::INVAL` when converting a `Timespec` to `i32`.

* Make ppoll weak on NetBSD, as it's not present on NetBSD 9.

* Add comments about why the code adds `999_999` before dividing.

Author: sunfishcode

src/backend/libc/event/syscalls.rs

@@ -18,7 +18,7 @@ use crate::event::port::Event;
 use crate::event::EventfdFlags;
 #[cfg(any(bsd, linux_kernel, target_os = "wasi"))]
 use crate::event::FdSetElement;
-use crate::event::PollFd;
+use crate::event::{PollFd, Timespec};
 use crate::io;
 #[cfg(solarish)]
 use crate::utils::as_mut_ptr;
@@ -30,7 +30,15 @@ use crate::utils::as_ptr;
     all(feature = "alloc", any(linux_kernel, target_os = "redox")),
 ))]
 use core::mem::MaybeUninit;
-#[cfg(any(bsd, linux_kernel, target_os = "wasi"))]
+#[cfg(any(
+    bsd,
+    linux_kernel,
+    target_os = "fuchsia",
+    target_os = "haiku",
+    target_os = "hurd",
+    target_os = "netbsd",
+    target_os = "wasi"
+))]
 use core::ptr::null;
 #[cfg(any(bsd, linux_kernel, solarish, target_os = "redox", target_os = "wasi"))]
 use core::ptr::null_mut;
@@ -119,14 +127,98 @@ pub(crate) unsafe fn kevent(
 }
 
 #[inline]
-pub(crate) fn poll(fds: &mut [PollFd<'_>], timeout: c::c_int) -> io::Result<usize> {
+pub(crate) fn poll(fds: &mut [PollFd<'_>], timeout: Option<&Timespec>) -> io::Result<usize> {
     let nfds = fds
         .len()
         .try_into()
         .map_err(|_convert_err| io::Errno::INVAL)?;
 
-    ret_c_int(unsafe { c::poll(fds.as_mut_ptr().cast(), nfds, timeout) })
-        .map(|nready| nready as usize)
+    // If we have `ppoll`, it supports a `timespec` timeout, so use it.
+    #[cfg(any(
+        linux_kernel,
+        freebsdlike,
+        target_os = "fuchsia",
+        target_os = "haiku",
+        target_os = "hurd",
+        target_os = "netbsd"
+    ))]
+    {
+        // If we don't have to fix y2038 on this platform, `Timespec` is
+        // the same as `c::timespec` and it's easy.
+        #[cfg(not(fix_y2038))]
+        let timeout = crate::timespec::option_as_libc_timespec_ptr(timeout);
+
+        // If we do have to fix y2038 on this platform, convert to
+        // `c::timespec`.
+        #[cfg(fix_y2038)]
+        let converted_timeout;
+        #[cfg(fix_y2038)]
+        let timeout = match timeout {
+            None => null(),
+            Some(timeout) => {
+                converted_timeout = c::timespec {
+                    tv_sec: timeout.tv_sec.try_into().map_err(|_| io::Errno::OVERFLOW)?,
+                    tv_nsec: timeout.tv_nsec as _,
+                };
+                &converted_timeout
+            }
+        };
+
+        #[cfg(not(target_os = "netbsd"))]
+        {
+            ret_c_int(unsafe { c::ppoll(fds.as_mut_ptr().cast(), nfds, timeout, null()) })
+                .map(|nready| nready as usize)
+        }
+
+        // NetBSD 9.x lacks `ppoll`, so use a weak symbol and fall back to
+        // plain `poll` if needed.
+        #[cfg(target_os = "netbsd")]
+        {
+            weak! {
+                fn ppoll(
+                    *mut c::pollfd,
+                    c::nfds_t,
+                    *const c::timespec,
+                    *const c::sigset_t
+                ) -> c::c_int
+            }
+            if let Some(func) = ppoll.get() {
+                return ret_c_int(unsafe { func(fds.as_mut_ptr().cast(), nfds, timeout, null()) })
+                    .map(|nready| nready as usize);
+            }
+        }
+    }
+
+    // If we don't have `ppoll`, convert the timeout to `c_int` and use `poll`.
+    #[cfg(not(any(
+        linux_kernel,
+        freebsdlike,
+        target_os = "fuchsia",
+        target_os = "haiku",
+        target_os = "hurd"
+    )))]
+    {
+        let timeout = match timeout {
+            None => -1,
+            Some(timeout) => {
+                // Convert from `Timespec` to `c_int` milliseconds.
+                let secs = timeout.tv_sec;
+                if secs < 0 {
+                    return Err(io::Errno::INVAL);
+                }
+                secs.checked_mul(1000)
+                    .and_then(|millis| {
+                        // Add the nanoseconds, converted to millis, rounding
+                        // up. With Rust 1.73.0 this can use `div_ceil`.
+                        millis.checked_add((i64::from(timeout.tv_nsec) + 999_999) / 1_000_000)
+                    })
+                    .and_then(|millis| c::c_int::try_from(millis).ok())
+                    .ok_or(io::Errno::INVAL)?
+            }
+        };
+        ret_c_int(unsafe { c::poll(fds.as_mut_ptr().cast(), nfds, timeout) })
+            .map(|nready| nready as usize)
+    }
 }
 
 #[cfg(any(bsd, linux_kernel))]
@@ -135,7 +227,7 @@ pub(crate) unsafe fn select(
     readfds: Option<&mut [FdSetElement]>,
     writefds: Option<&mut [FdSetElement]>,
     exceptfds: Option<&mut [FdSetElement]>,
-    timeout: Option<&crate::timespec::Timespec>,
+    timeout: Option<&Timespec>,
 ) -> io::Result<i32> {
     let len = crate::event::fd_set_num_elements_for_bitvector(nfds);
 
@@ -212,7 +304,7 @@ pub(crate) unsafe fn select(
     readfds: Option<&mut [FdSetElement]>,
     writefds: Option<&mut [FdSetElement]>,
     exceptfds: Option<&mut [FdSetElement]>,
-    timeout: Option<&crate::timespec::Timespec>,
+    timeout: Option<&Timespec>,
 ) -> io::Result<i32> {
     let len = crate::event::fd_set_num_elements_for_fd_array(nfds as usize);
 

src/backend/libc/event/windows_syscalls.rs

@@ -2,15 +2,34 @@
 
 use crate::backend::c;
 use crate::backend::conv::ret_c_int;
-use crate::event::{FdSetElement, PollFd};
+use crate::event::{FdSetElement, PollFd, Timespec};
 use crate::io;
 
-pub(crate) fn poll(fds: &mut [PollFd<'_>], timeout: c::c_int) -> io::Result<usize> {
+pub(crate) fn poll(fds: &mut [PollFd<'_>], timeout: Option<&Timespec>) -> io::Result<usize> {
     let nfds = fds
         .len()
         .try_into()
         .map_err(|_convert_err| io::Errno::INVAL)?;
 
+    let timeout = match timeout {
+        None => -1,
+        Some(timeout) => {
+            // Convert from `Timespec` to `c_int` milliseconds.
+            let secs = timeout.tv_sec;
+            if secs < 0 {
+                return Err(io::Errno::INVAL);
+            }
+            secs.checked_mul(1000)
+                .and_then(|millis| {
+                    // Add the nanoseconds, converted to millis, rounding up.
+                    // With Rust 1.73.0 this can use `div_ceil`.
+                    millis.checked_add((i64::from(timeout.tv_nsec) + 999_999) / 1_000_000)
+                })
+                .and_then(|millis| c::c_int::try_from(millis).ok())
+                .ok_or(io::Errno::INVAL)?
+        }
+    };
+
     ret_c_int(unsafe { c::poll(fds.as_mut_ptr().cast(), nfds, timeout) })
         .map(|nready| nready as usize)
 }

src/backend/linux_raw/conv.rs

@@ -232,7 +232,6 @@ pub(super) fn opt_mut<T: Sized, Num: ArgNumber>(t: Option<&mut T>) -> ArgReg<'_,
 
 /// Convert an optional immutable reference into a `usize` for passing to a
 /// syscall.
-#[cfg(any(target_arch = "aarch64", target_arch = "riscv64"))]
 #[inline]
 pub(super) fn opt_ref<T: Sized, Num: ArgNumber>(t: Option<&T>) -> ArgReg<'_, Num> {
     // This optimizes into the equivalent of `transmute(t)`, and has the

src/backend/linux_raw/event/syscalls.rs

@@ -5,38 +5,28 @@
 //! See the `rustix::backend` module documentation for details.
 #![allow(unsafe_code, clippy::undocumented_unsafe_blocks)]
 
+#[cfg(feature = "alloc")]
 use crate::backend::c;
 use crate::backend::conv::{
-    by_ref, c_int, c_uint, ret, ret_c_int, ret_error, ret_owned_fd, ret_usize, slice_mut, zero,
+    by_ref, c_int, c_uint, opt_ref, ret, ret_c_int, ret_error, ret_owned_fd, ret_usize, size_of,
+    slice_mut, zero,
 };
-use crate::event::{epoll, EventfdFlags, FdSetElement, PollFd};
+use crate::event::{epoll, EventfdFlags, FdSetElement, PollFd, Timespec};
 use crate::fd::{BorrowedFd, OwnedFd};
 use crate::io;
-use crate::utils::as_mut_ptr;
+use crate::utils::{as_mut_ptr, option_as_ptr};
 #[cfg(feature = "alloc")]
 use core::mem::MaybeUninit;
 use core::ptr::null_mut;
-use linux_raw_sys::general::{EPOLL_CTL_ADD, EPOLL_CTL_DEL, EPOLL_CTL_MOD};
-#[cfg(any(target_arch = "aarch64", target_arch = "riscv64"))]
-use {
-    crate::backend::conv::{opt_ref, size_of},
-    linux_raw_sys::general::{__kernel_timespec, kernel_sigset_t},
-};
+use linux_raw_sys::general::{kernel_sigset_t, EPOLL_CTL_ADD, EPOLL_CTL_DEL, EPOLL_CTL_MOD};
 
 #[inline]
-pub(crate) fn poll(fds: &mut [PollFd<'_>], timeout: c::c_int) -> io::Result<usize> {
+pub(crate) fn poll(fds: &mut [PollFd<'_>], timeout: Option<&Timespec>) -> io::Result<usize> {
     let (fds_addr_mut, fds_len) = slice_mut(fds);
 
-    #[cfg(any(target_arch = "aarch64", target_arch = "riscv64"))]
+    let timeout = option_as_ptr(timeout);
+
     unsafe {
-        let timeout = if timeout >= 0 {
-            Some(__kernel_timespec {
-                tv_sec: (timeout as i64) / 1000,
-                tv_nsec: (timeout as i64) % 1000 * 1_000_000,
-            })
-        } else {
-            None
-        };
         ret_usize(syscall!(
             __NR_ppoll,
             fds_addr_mut,
@@ -46,10 +36,6 @@ pub(crate) fn poll(fds: &mut [PollFd<'_>], timeout: c::c_int) -> io::Result<usiz
             size_of::<kernel_sigset_t, _>()
         ))
     }
-    #[cfg(not(any(target_arch = "aarch64", target_arch = "riscv64")))]
-    unsafe {
-        ret_usize(syscall!(__NR_poll, fds_addr_mut, fds_len, c_int(timeout)))
-    }
 }
 
 pub(crate) unsafe fn select(

src/event/mod.rs

@@ -19,6 +19,7 @@ pub mod port;
 #[cfg(any(bsd, linux_kernel, windows, target_os = "wasi"))]
 mod select;
 
+pub use crate::timespec::{Nsecs, Secs, Timespec};
 #[cfg(any(
     linux_kernel,
     target_os = "freebsd",

src/event/poll.rs

@@ -1,9 +1,14 @@
+use crate::event::Timespec;
 use crate::{backend, io};
 
 pub use backend::event::poll_fd::{PollFd, PollFlags};
 
 /// `poll(self.fds, timeout)`—Wait for events on lists of file descriptors.
 ///
+/// Some platforms (those that don't support `ppoll`) don't support timeouts
+/// greater than `c_int::MAX` milliseconds; if an unsupported timeout is
+/// passed, this function fails with [`io::Errno::INVAL`].
+///
 /// On macOS, `poll` doesn't work on fds for /dev/tty or /dev/null, however
 /// [`select`] is available and does work on these fds.
 ///
@@ -32,6 +37,6 @@ pub use backend::event::poll_fd::{PollFd, PollFlags};
 /// [DragonFly BSD]: https://man.dragonflybsd.org/?command=poll&section=2
 /// [illumos]: https://illumos.org/man/2/poll
 #[inline]
-pub fn poll(fds: &mut [PollFd<'_>], timeout: i32) -> io::Result<usize> {
+pub fn poll(fds: &mut [PollFd<'_>], timeout: Option<&Timespec>) -> io::Result<usize> {
     backend::event::syscalls::poll(fds, timeout)
 }

src/event/select.rs

@@ -7,15 +7,14 @@
 
 #[cfg(any(linux_like, target_os = "wasi"))]
 use crate::backend::c;
+use crate::event::Timespec;
 use crate::fd::RawFd;
 use crate::{backend, io};
 #[cfg(any(windows, target_os = "wasi"))]
 use core::mem::{align_of, size_of};
 #[cfg(any(windows, target_os = "wasi"))]
 use core::slice;
 
-pub use crate::timespec::{Nsecs, Secs, Timespec};
-
 /// wasi-libc's `fd_set` type. The libc bindings for it have private fields,
 /// so we redeclare it for ourselves so that we can access the fields. They're
 /// publicly exposed in wasi-libc.

src/lib.rs

@@ -359,6 +359,7 @@ mod prctl;
 mod signal;
 #[cfg(any(
     feature = "fs",
+    feature = "event",
     feature = "process",
     feature = "runtime",
     feature = "thread",

src/timespec.rs

@@ -7,9 +7,11 @@
 use crate::backend::c;
 #[allow(unused)]
 use crate::ffi;
+#[cfg(not(fix_y2038))]
+use core::ptr::null;
 
 /// `struct timespec`
-#[derive(Debug, Clone, Copy)]
+#[derive(Debug, Clone, Copy, Default)]
 #[repr(C)]
 pub struct Timespec {
     /// Seconds.
@@ -99,6 +101,14 @@ pub(crate) fn as_libc_timespec_mut_ptr(
     timespec.as_mut_ptr().cast::<c::timespec>()
 }
 
+#[cfg(not(fix_y2038))]
+pub(crate) fn option_as_libc_timespec_ptr(timespec: Option<&Timespec>) -> *const c::timespec {
+    match timespec {
+        None => null(),
+        Some(timespec) => as_libc_timespec_ptr(timespec),
+    }
+}
+
 #[test]
 fn test_sizes() {
     assert_eq_size!(Secs, u64);

tests/event/poll.rs

@@ -8,6 +8,7 @@ use {rustix::event::poll, rustix::io::retry_on_intr};
 #[cfg(not(any(windows, target_os = "wasi")))]
 #[test]
 fn test_poll() {
+    use rustix::event::Timespec;
     use rustix::io::{read, write};
     use rustix::pipe::pipe;
 
@@ -17,7 +18,7 @@ fn test_poll() {
     assert_eq!(poll_fds[0].as_fd().as_raw_fd(), reader.as_fd().as_raw_fd());
 
     // `poll` should say there's nothing ready to be read from the pipe.
-    let num = retry_on_intr(|| poll(&mut poll_fds, 0)).unwrap();
+    let num = retry_on_intr(|| poll(&mut poll_fds, Some(&Timespec::default()))).unwrap();
     assert_eq!(num, 0);
     assert!(poll_fds[0].revents().is_empty());
     assert_eq!(poll_fds[0].as_fd().as_raw_fd(), reader.as_fd().as_raw_fd());
@@ -26,7 +27,7 @@ fn test_poll() {
     assert_eq!(retry_on_intr(|| write(&writer, b"a")).unwrap(), 1);
 
     // `poll` should now say there's data to be read.
-    let num = retry_on_intr(|| poll(&mut poll_fds, -1)).unwrap();
+    let num = retry_on_intr(|| poll(&mut poll_fds, None)).unwrap();
     assert_eq!(num, 1);
     assert_eq!(poll_fds[0].revents(), PollFlags::IN);
     assert_eq!(poll_fds[0].as_fd().as_raw_fd(), reader.as_fd().as_raw_fd());
@@ -43,7 +44,7 @@ fn test_poll() {
     assert_eq!(poll_fds[0].as_fd().as_raw_fd(), reader.as_fd().as_raw_fd());
 
     // Poll should now say there's no more data to be read.
-    let num = retry_on_intr(|| poll(&mut poll_fds, 0)).unwrap();
+    let num = retry_on_intr(|| poll(&mut poll_fds, Some(&Timespec::default()))).unwrap();
     assert_eq!(num, 0);
     assert!(poll_fds[0].revents().is_empty());
     assert_eq!(poll_fds[0].as_fd().as_raw_fd(), reader.as_fd().as_raw_fd());