Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: make Epoll take BorrowedFd #2232

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions changelog/2232.changed.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
`Epoll` and its functions now take `fd: BorrowedFd` rather than `fd: &Fd`

The `OwnedFd` field of `Epoll` is no longer exposed, please use the `AsFd` trait
48 changes: 34 additions & 14 deletions src/sys/epoll.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@ pub use crate::poll_timeout::PollTimeout as EpollTimeout;
use crate::Result;
use libc::{self, c_int};
use std::mem;
use std::os::unix::io::{AsFd, AsRawFd, FromRawFd, OwnedFd, RawFd};
use std::os::fd::{AsFd, BorrowedFd};
use std::os::unix::io::{AsRawFd, FromRawFd, OwnedFd, RawFd};

libc_bitflags!(
pub struct EpollFlags: c_int {
Expand Down Expand Up @@ -85,7 +86,7 @@ impl EpollEvent {
///
/// // Create eventfd & Add event
/// let eventfd = eventfd(0, EfdFlags::empty())?;
/// epoll.add(&eventfd, EpollEvent::new(EpollFlags::EPOLLIN,DATA))?;
/// epoll.add(eventfd.as_fd(), EpollEvent::new(EpollFlags::EPOLLIN,DATA))?;
///
/// // Arm eventfd & Time wait
/// write(&eventfd, &1u64.to_ne_bytes())?;
Expand All @@ -102,37 +103,50 @@ impl EpollEvent {
/// # }
/// ```
#[derive(Debug)]
pub struct Epoll(pub OwnedFd);
impl Epoll {
#[repr(transparent)]
pub struct Epoll<'fd> {
epoll_fd: OwnedFd,
_fds: std::marker::PhantomData<*mut &'fd ()>,
Copy link
Member Author

@SteveLauC SteveLauC Nov 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We cannot use _fds: std::marker::PhantomData<BorrowedFd<'fd>> here or the following code would compile without any issue:

use nix::sys::epoll::{Epoll, EpollCreateFlags, EpollEvent};
use nix::unistd::pipe;

fn main() {
    let (r, _w) = pipe().unwrap();
    let epoll = Epoll::new(EpollCreateFlags::empty()).unwrap();
    epoll.add(r, EpollEvent::empty()).unwrap();

    drop(r);
    drop(epoll);
}

I haven't figured out the reason, but using an invariant type here would work

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Friendly ping @asomers, do you have any idea about this?

}

impl<'fd> Epoll<'fd> {
/// Creates a new epoll instance and returns a file descriptor referring to that instance.
///
/// [`epoll_create1`](https://man7.org/linux/man-pages/man2/epoll_create1.2.html).
pub fn new(flags: EpollCreateFlags) -> Result<Self> {
let res = unsafe { libc::epoll_create1(flags.bits()) };
let fd = Errno::result(res)?;
let owned_fd = unsafe { OwnedFd::from_raw_fd(fd) };
Ok(Self(owned_fd))

Ok(Self {
epoll_fd: owned_fd,
_fds: std::marker::PhantomData,
})
}
/// Add an entry to the interest list of the epoll file descriptor for
/// specified in events.
///
/// [`epoll_ctl`](https://man7.org/linux/man-pages/man2/epoll_ctl.2.html) with `EPOLL_CTL_ADD`.
pub fn add<Fd: AsFd>(&self, fd: Fd, mut event: EpollEvent) -> Result<()> {
pub fn add(
&self,
fd: BorrowedFd<'fd>,
mut event: EpollEvent,
) -> Result<()> {
self.epoll_ctl(EpollOp::EpollCtlAdd, fd, &mut event)
}
/// Remove (deregister) the target file descriptor `fd` from the interest list.
///
/// [`epoll_ctl`](https://man7.org/linux/man-pages/man2/epoll_ctl.2.html) with `EPOLL_CTL_DEL` .
pub fn delete<Fd: AsFd>(&self, fd: Fd) -> Result<()> {
pub fn delete(&self, fd: BorrowedFd<'fd>) -> Result<()> {
self.epoll_ctl(EpollOp::EpollCtlDel, fd, None)
}
/// Change the settings associated with `fd` in the interest list to the new settings specified
/// in `event`.
///
/// [`epoll_ctl`](https://man7.org/linux/man-pages/man2/epoll_ctl.2.html) with `EPOLL_CTL_MOD`.
pub fn modify<Fd: AsFd>(
pub fn modify(
&self,
fd: Fd,
fd: BorrowedFd<'fd>,
event: &mut EpollEvent,
) -> Result<()> {
self.epoll_ctl(EpollOp::EpollCtlMod, fd, event)
Expand All @@ -148,7 +162,7 @@ impl Epoll {
) -> Result<usize> {
let res = unsafe {
libc::epoll_wait(
self.0.as_raw_fd(),
self.epoll_fd.as_raw_fd(),
events.as_mut_ptr().cast(),
events.len() as c_int,
timeout.into().into(),
Expand All @@ -164,10 +178,10 @@ impl Epoll {
/// When possible prefer [`Epoll::add`], [`Epoll::delete`] and [`Epoll::modify`].
///
/// [`epoll_ctl`](https://man7.org/linux/man-pages/man2/epoll_ctl.2.html)
fn epoll_ctl<'a, Fd: AsFd, T>(
fn epoll_ctl<'a, T>(
&self,
op: EpollOp,
fd: Fd,
fd: BorrowedFd<'fd>,
event: T,
) -> Result<()>
where
Expand All @@ -179,16 +193,22 @@ impl Epoll {
.unwrap_or(std::ptr::null_mut());
unsafe {
Errno::result(libc::epoll_ctl(
self.0.as_raw_fd(),
self.epoll_fd.as_raw_fd(),
op as c_int,
fd.as_fd().as_raw_fd(),
fd.as_raw_fd(),
ptr,
))
.map(drop)
}
}
}

impl AsFd for Epoll<'_> {
fn as_fd(&self) -> BorrowedFd<'_> {
self.epoll_fd.as_fd()
}
}

#[deprecated(since = "0.27.0", note = "Use Epoll::new() instead")]
#[inline]
pub fn epoll_create() -> Result<RawFd> {
Expand Down
33 changes: 15 additions & 18 deletions test/sys/test_epoll.rs
Original file line number Diff line number Diff line change
@@ -1,26 +1,23 @@
#![allow(deprecated)]

use nix::errno::Errno;
use nix::sys::epoll::{epoll_create1, epoll_ctl};
use nix::sys::epoll::{EpollCreateFlags, EpollEvent, EpollFlags, EpollOp};
use nix::{
errno::Errno,
sys::epoll::{Epoll, EpollCreateFlags, EpollEvent, EpollFlags},
};
use std::os::fd::BorrowedFd;

#[test]
pub fn test_epoll_errno() {
let efd = epoll_create1(EpollCreateFlags::empty()).unwrap();
let result = epoll_ctl(efd, EpollOp::EpollCtlDel, 1, None);
result.expect_err("assertion failed");
let epoll = Epoll::new(EpollCreateFlags::empty()).unwrap();
let fd_1 = unsafe { BorrowedFd::borrow_raw(1) };
let result = epoll.delete(fd_1);
assert_eq!(result.unwrap_err(), Errno::ENOENT);

let result = epoll_ctl(efd, EpollOp::EpollCtlAdd, 1, None);
result.expect_err("assertion failed");
assert_eq!(result.unwrap_err(), Errno::EINVAL);
}

#[test]
pub fn test_epoll_ctl() {
let efd = epoll_create1(EpollCreateFlags::empty()).unwrap();
let mut event =
EpollEvent::new(EpollFlags::EPOLLIN | EpollFlags::EPOLLERR, 1);
epoll_ctl(efd, EpollOp::EpollCtlAdd, 1, &mut event).unwrap();
epoll_ctl(efd, EpollOp::EpollCtlDel, 1, None).unwrap();
pub fn test_epoll_add_delete() {
let epoll = Epoll::new(EpollCreateFlags::empty()).unwrap();
let event = EpollEvent::new(EpollFlags::EPOLLIN | EpollFlags::EPOLLERR, 1);
let fd_1 = unsafe { BorrowedFd::borrow_raw(1) };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to get rid of the unsafe, I suggest:

Suggested change
let fd_1 = unsafe { BorrowedFd::borrow_raw(1) };
let stdin = std::io::stdin();
let fd_1 = stdin.as_fd();

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will do


epoll.add(fd_1, event).unwrap();
epoll.delete(fd_1).unwrap();
}