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

Conversation

SteveLauC
Copy link
Member

@SteveLauC SteveLauC commented Nov 27, 2023

What does this PR do

  1. Make Epoll-associated functions take BorrowedFd so that it is consistent with FdSet and PollFd
  2. Update the tests in test_epoll.rs, use the new APIs

Checklist:

  • I have read CONTRIBUTING.md
  • I have written necessary tests and rustdoc comments
  • A change log has been added if this PR modifies nix's API

#[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?

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants