Add epoll_create1. #384
Add epoll_create1. #384
Conversation
| @@ -80,6 +88,13 @@ pub fn epoll_create() -> Result<RawFd> { | |||
| } | |||
|
|
|||
| #[inline] | |||
| pub fn epoll_create1(flags: EpollFdFlag) -> Result<RawFd> { | |||
| let res = unsafe { ffi::epoll_create1(flags.bits() | EPOLL_CLOEXEC.bits) }; | |||
There was a problem hiding this comment.
- The way you are setting the default flag is actually enforcing it, therefore making it impossible for someone to call
epoll_create1without theEPOLL_CLOEXECflag. - I don't think that any of the flags should be turned on by default, especially not without documentation. (Principle of Least Surprise)
There was a problem hiding this comment.
In Rust's standard library close-on-exec flag is set by default.
I'm not sure why so, however, I think it's security reason.
Thugh, someone might be surpised when I force to using fcntl(fd, F_SETFD, !O_CLOEXEC) if he want.
Okay, I'll fix it.
kubo39
changed the title
| @@ -8,12 +8,20 @@ mod ffi { | |||
|
|
|||
| extern { | |||
| pub fn epoll_create(size: c_int) -> c_int; | |||
| pub fn epoll_create1(flags: c_int) -> c_int; | |||
| pub fn epoll_ctl(epfd: c_int, op: c_int, fd: c_int, event: *const EpollEvent) -> c_int; | |||
| pub fn epoll_wait(epfd: c_int, events: *mut EpollEvent, max_events: c_int, timeout: c_int) -> c_int; | |||
There was a problem hiding this comment.
Hi @kubo39 , although this is not directly related to your PR we have been moving to defining ffi constants entirely in libc and using those rather than defining our own (see CONVENTIONS.md). it looks like the existing constants are present but epoll_create1 is not. Can you please do the following:
- Open up a PR to libc adding
epoll_create1 - Update
epoll.rsto use the ffi definitions from libc.
Also, thanks for the change!
|
There is a new libc version with your changes. Now, you can amend the pull request to use the new libc version and your additions to it. After that, we can merge this pull request. |
|
@kubo39 Can you just rebase your commits on master rather than having the merge commit back into your branch? Other than that, looks good to me. |
|
This will probably create a merge conflict, because of the libc version. Please use the existing form of the dependency targeting the git repository instead of a fixed version. |
|
I'm sorry for being late. |
|
@kubo39 I think we want to remove the extern functions from nix and just depend on them directly from libc. |
| pub fn epoll_ctl(epfd: c_int, op: c_int, fd: c_int, event: *const EpollEvent) -> c_int; | ||
| pub fn epoll_wait(epfd: c_int, events: *mut EpollEvent, max_events: c_int, timeout: c_int) -> c_int; | ||
| } | ||
| } | ||
|
|
||
| bitflags!( | ||
| flags EpollFdFlag: c_int { | ||
| const EPOLL_NONBLOCK = 0x800, | ||
| const EPOLL_CLOEXEC = 0x80000 |
There was a problem hiding this comment.
These constants should also come out of libc if possible.
|
☔ The latest upstream changes (presumably #423) made this pull request unmergeable. Please resolve the merge conflicts. |
Add epoll_create1 In order to get @kubo39's PR #384 forward, I cleaned up the commit history a bit and added `EpollEvent` back. Since this module is used by mio, maybe @carllerche could comment on these changes.
No description provided.