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

Implement the kqueue API #488

Merged
merged 1 commit into from
Mar 2, 2023
Merged

Conversation

notgull
Copy link
Contributor

@notgull notgull commented Dec 19, 2022

This implements the kqueue API, available on BSD and MacOS.

The event API is something I'm not entirely happy with; I am very open to suggestions here.

@notgull notgull force-pushed the kqueue branch 8 times, most recently from c4eb7c3 to 6434164 Compare December 20, 2022 20:30
Copy link
Member

@sunfishcode sunfishcode left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! Here are some initial comments from a first readthrough.

target_os = "dragonfly",
target_os = "openbsd",
target_os = "netbsd"
))]
Copy link
Member

Choose a reason for hiding this comment

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

Something that rustix doesn't do yet, but perhaps should start doing more, is having build.rs test for sets of targets like this, and emitting something like cargo:rustc-cfg=have_kqueue, so that most of the code can just do #[cfg(have_kqueue)] instead of naming all 8 targets like this all over the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll write a future PR for this

.gitignore Outdated Show resolved Hide resolved
src/io/kqueue.rs Show resolved Hide resolved
src/io/kqueue.rs Outdated
/// Wait for events on this queue.
pub fn kevent(
&self,
changelist: &[InEvent<'_>],
Copy link
Member

Choose a reason for hiding this comment

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

I'm not super familiar with the kqueue API, but as I understand it, the changelist registers file descriptors which are associated with the kqueue file descriptor, which may outlive the &self and `_ lifetimes here, so this lifetime description doesn't fully describe the possible file-descriptor lifetimes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe that, if a file descriptor is closed while it's registered in kqueue, it's also automatically deregistered from kqueue, so it shouldn't be a soundness issue, at the very least.

Copy link
Member

Choose a reason for hiding this comment

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

The same thing could be said about directories, socket listeners, socket connections, udp sockets, signalfd, eventfd, and other kinds of file descriptors too though. Should rustix strive to have static types for every "type" of file descriptor?

Currently, rustix's approach is to assume that the bulk of the Rust code in the world won't be using rustix directly. Abstractions like std, cap-std, etc. provide these kinds of static type distinctions, in File, ReadDir, TcpListener, and so on. With that, rustix then focuses on just being a simple low-level API that isn't opinionated about which file descriptors can be used with which system calls. Maybe someone wants to dup2 a file descriptor over a kqueue file descriptor. That's awkward to do if there's a Kqueue wrapper struct. And perhaps there are other things users want to do, that we can't anticipate. So rustix so far has erred on the side of letting users pass any file descriptor to any OS system call, because it isn't memory-unsafe, and in practice it hasn't been observed to be a problem in practice, and maybe there's a use case that rustix doesn't anticipate which users do want to do.

Copy link
Member

Choose a reason for hiding this comment

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

(Oops, that comment should have been in reply to the other thread about the Kqueue type.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the explanation! I've changed it so that there is just a freestanding kqueue and kevent function pair.

@notgull
Copy link
Contributor Author

notgull commented Jan 7, 2023

@sunfishcode Could you take another look at this? I've gotten the event API to a place where I like it.

@sunfishcode
Copy link
Member

Yes, I apologize for being slow here; I am looking forward to reviewing this! I'm not familar with the kqueue API so I need some time to read up on that, and I've been busy with holiday and real-life stuff, but I hope to get to this soon.

src/io/kqueue.rs Outdated Show resolved Hide resolved
Add an eventvec for kqueue

Remove vscode from gitignore

Replace kqueue with an arbitrary FD

Improve FilterKind ergonomics

fmt

Add references to functions

Make kevent unsafe with RawFd

Kickstart my heart!

Empty commit to re-run the CI
Copy link
Member

@sunfishcode sunfishcode left a comment

Choose a reason for hiding this comment

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

I apologize again for being slow here. Thanks a lot for working on this!

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