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

Combing Epoll and Inotify? #1998

Closed
VorpalBlade opened this issue Feb 6, 2023 · 7 comments · Fixed by #1999
Closed

Combing Epoll and Inotify? #1998

VorpalBlade opened this issue Feb 6, 2023 · 7 comments · Fixed by #1999
Milestone

Comments

@VorpalBlade
Copy link
Contributor

So apparently the crate is transitioning Epoll to a new safer (and better documented!) abstraction. But this has yet to be released. Fine I thought, since I'm starting a new project, I will just use the latest master for now and switch to the next release that contains this.

However, it seems that Inotify (which I also need) has lost AsRawFd, but not gained a replacement to allow it to be used together with Epoll? For my use case I need to epoll a few FDs, one of which is an inotify FD (that monitors a few files). Thus I need a way to combine these. I would prefer to avoid threads for this.

Is there no way to do this currently or am I missing something obvious?

@VorpalBlade
Copy link
Contributor Author

Note: This is probably related to issue #1750 and may be an oversight in how to combine things.

@asomers
Copy link
Member

asomers commented Feb 6, 2023

I think you're asking for a way to use AsFd with Epoll?

@VorpalBlade
Copy link
Contributor Author

VorpalBlade commented Feb 6, 2023

I would want to use AsFd on Inotify such that I can monitor for new Inotify events with Epoll while also monitoring for other things. This lets me keep my program single threaded.

I have this workaround for now (horrible mess, and probably not sound, but it works):

// Workaround for missing AsFd for Inotify in nix!
//
// There is no way to get the FD out of inotify and into epoll. We have to stupidly resort to unsafe code.
//
// This is based on Inotify::init
//
// SAFETY: Well, it actully isn't unsafe, since we aren't doing silly things like mmaping
//         this etc. It won't lead to memory issues.
//         HOWEVER: The IO safety is wrong, we are creating two copies of the same FD.
//         This can only be fixed in nix itself. But the way we use it below is okay.
unsafe fn create_inotify<'a>(
    flags: InitFlags,
) -> Result<(Inotify, BorrowedFd<'a>), Box<dyn Error>> {
    let fd = Errno::result(unsafe { libc::inotify_init1(flags.bits()) })?;
    Ok(unsafe { (Inotify::from_raw_fd(fd), BorrowedFd::borrow_raw(fd)) })
}

// [Lots of code elided here.]

pub(crate) fn monitor(
    mut listeners: Vec<Box<dyn Listener>>,
    timeout: Duration,
) -> Result<(), Box<dyn Error>> {
    // SAFETY: Epoll and inotify lives equally long. This is safe.
    let (inotify, ifd) = unsafe { create_inotify(InitFlags::IN_CLOEXEC | InitFlags::IN_NONBLOCK)? };
    let epoll = Epoll::new(EpollCreateFlags::EPOLL_CLOEXEC)?;

    epoll.add(
        ifd,
        EpollEvent::new(EpollFlags::EPOLLIN | EpollFlags::EPOLLERR, INOTIFY_HANDLE),
    )?;

    // [Add other things to epoll here]
    // [Also add things to inotify]

    loop {
        let mut events = [EpollEvent::empty(); 32];
        epoll.wait(&mut events, timeout.as_millis() as isize)?;
        // Process events
        for ref event in events {
            match event.data() {
                INOTIFY_HANDLE => {
                    for ievent in inotify.read_events()? {
                        // [Process inotify stuff here]
                    }
                }
                // [Process other file descriptors here]
            }
        }
    }
}

@VorpalBlade
Copy link
Contributor Author

VorpalBlade commented Feb 6, 2023

The way to fix this would be to add impl AsFd for Inotify.

EDIT: Oh and that lifetime on create_inotify in my code above is very wrong. I don't quite understand why the compiler accepts it in fact! I would of course like to tie the lifetime to Inotify, but I can't figure out how to do it (I'm relatively new to rust). Neither Inotify<'a> nor Inotify + 'a is accepted (with varying errors),

@VorpalBlade
Copy link
Contributor Author

Side note: I find it weird that the epoll API takes a pre-allocated array (nice, no allocations) while Inotify returns a Vec. Consistency there would be nice. I prefer the pre-allocated array (since it can be allocated on the stack and avoid heap allocations).

Also the docs for Inotify::read_events appears to be incorrect: The docs read "Returns as many events as available.". This does not seem to be the case when reading the code. It looks to me like it will read as many events as are available and fit into a 4096 byte buffer. If the buffer gets filled it will not read more. Though that may be complicated in blocking mode (what if there was exactly 4096 bytes available?) etc. Consider changing the documentation instead.

@ids1024
Copy link

ids1024 commented Feb 22, 2023

SAFETY: Well, it actully isn't unsafe, since we aren't doing silly things like mmaping this etc. It won't lead to memory issues.

This is incorrect, in general. If the fd is used after it has been close, the same fd number may be reallocated to something else, which may be mmaped. So it could in principle cause soundness issues in combination with other code that is correctly using mmap. Though I don't believe it's normal to write to an inotify fd, so to get undefined behavior here would require somewhat convoluted code.

I don't quite understand why the compiler accepts it in fact!

BorrowedFd::borrow_raw returns a BorrowedFd with a generic lifetime. It's up to the caller to make sure it is sound. It is possible (but not necessarily sound) to have your function calling return an arbitrary generic lifetime as well.

@asomers
Copy link
Member

asomers commented Apr 2, 2023

I think impl AsFd for Inotify should definitely work. Would you care to submit a PR?

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 a pull request may close this issue.

3 participants