bytecodealliance / rustix

Safe Rust bindings to POSIX-ish APIs
Other
1.45k stars 149 forks source link

Weird epoll comment around close #1100

Open SUPERCILEX opened 1 month ago

SUPERCILEX commented 1 month ago

https://docs.rs/rustix/latest/rustix/event/epoll/fn.add.html says that closing a FD doesn't do anything which directly contradicts the man pages: https://man7.org/linux/man-pages/man7/epoll.7.html#:~:text=Will%20closing%20a%20file%20descriptor%20cause%20it%20to%20be%20removed%20from%20all%0A%20%20%20%20%20%20%20%20%20%20epoll%20interest%20lists%3F

@notgull is there context I'm missing or can we just remove that comment in favor of the man pages?

SUPERCILEX commented 1 month ago

Hmmm, also confused about why EventVec is a thing. Shouldn't that just use the same API as https://docs.rs/rustix/latest/rustix/fs/fn.readlink.html? If you and @sunfishcode code are ok with that, I'd like to get rid of it. Also the event_list.events.set_len(0); is very suspicious—like yes everything is copy so whatever, but why not just call clear?

SUPERCILEX commented 1 month ago

Actually the Into interface seems annoying since an &mut Vec<Event> would do. Though we can go a step further and take an &mut [MaybeUninit<Event>] and return an &mut [Event] so you can pass in stack arrays. The interface would look like this:

fn wait<I: Into<SliceOrVec>>(..., event_buf: I) -> io::Result<&mut [Event]>

enum SliceOrVec<'a> {
  Slice(&'a mut [MaybeUninit<Event>]),
  Vec(&'a mut Vec<Event>),
}

impl From slice/vec for SliceOrVec

// Usage
for event in wait(..., &mut stack OR vec.spare_capacity_mut()) {}

let mut vec = ...;
wait(..., vec.spare_capacity_mut());
do_stuff(&vec)

Thoughts? This seems way more flexible and less code than the current version.

SUPERCILEX commented 1 month ago

Also I don't think the API should clear/set_len(0) the vec at all, that's the caller's problem (use drain) and they might want to only partially process events for some reason. I guess maybe it's fine? But seems weird to kill stuff in your buffer.

SUPERCILEX commented 1 month ago

Ooo, pretty sure this is backwards compatible with a from impl for EventVec! So we could do all this in 0.38 with a deprecation and only drop EventVec later.