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

Add epoll_create1 #410

Merged
merged 7 commits into from
Sep 16, 2016
Merged

Add epoll_create1 #410

merged 7 commits into from
Sep 16, 2016

Conversation

fiveop
Copy link
Contributor

@fiveop fiveop commented Aug 31, 2016

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.

@posborne
Copy link
Member

posborne commented Aug 31, 2016

Changes look good to me. I don't see any negative impact for mio here. So, r=me once you're comfortable @fiveop.

EDIT: I missed the minor, good interface change. Probably worth testing mio with this before merging and prepping changes there if required.

@fiveop
Copy link
Contributor Author

fiveop commented Sep 8, 2016

I added readers for EpollEvent's fields and made the constructor public (doh!). Now, mio can be easily adapted to the changes (see https://github.com/fiveop/mio/tree/nix_epoll_change).

@kamalmarhubi
Copy link
Member

Seems fine to me. There's a bit of possible "conventions" work on the flags and stuff, but that can wait. We should perhaps let @carllerche weigh in. Also I imagine epoll_create1 could be handy for mio, since it's unlikely you want to propagate epoll fds to children.

@kamalmarhubi kamalmarhubi mentioned this pull request Sep 9, 2016
@fiveop
Copy link
Contributor Author

fiveop commented Sep 9, 2016

He had his chance :) But seriously, after having tested adapting mio code to the change, I don't really see a problem.

I would like to have it merged after we have a new dev version.

@kamalmarhubi
Copy link
Member

Oh I missed that he was pinged already. In that case, @homu r+

@homu
Copy link
Contributor

homu commented Sep 9, 2016

📌 Commit 584794d has been approved by kamalmarhubi

@kamalmarhubi
Copy link
Member

@homu r-

@kamalmarhubi
Copy link
Member

Can we at least export EPOLL_CLOEXEC here, or add it as a single-element flags type for the second arg to epoll_create1?

@kamalmarhubi
Copy link
Member

Naming will get complicated from conventions view as there are all the EPOLLET EPOLLIN flags, as well as this single EPOLL_CLOEXEC flag.

@fiveop
Copy link
Contributor Author

fiveop commented Sep 15, 2016

I opted for EpollFlags and EpollCreateFlags. The latter breaks convention, but there has to be a break in convention :)

I tried to use libc_bitflags for EpollFlags, but EPOLLEXCLUSIVE is not available very widely, even on different linux platforms.
That the constants have type c_int in libc, but the field epoll_event.events has type uint32_t is another problem, when using a libc_bitflags macro (or the constants from libc in general).

But that's for another PR!

@posborne
Copy link
Member

Looks good to me. Since we recently cut a release, this is probably a pretty good time to get the change in so we can integrate mio changes into upstream in preparation.

@homu r+

@homu
Copy link
Contributor

homu commented Sep 16, 2016

📌 Commit 2b0c991 has been approved by posborne

@homu
Copy link
Contributor

homu commented Sep 16, 2016

⚡ Test exempted - status

@homu homu merged commit 2b0c991 into nix-rust:master Sep 16, 2016
homu added a commit that referenced this pull request Sep 16, 2016
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.
@fiveop fiveop deleted the epoll_create1 branch October 15, 2016 17:47
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.

5 participants