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

Use libc in poll.rs #399

Merged
merged 2 commits into from
Aug 31, 2016
Merged

Use libc in poll.rs #399

merged 2 commits into from
Aug 31, 2016

Conversation

fiveop
Copy link
Contributor

@fiveop fiveop commented Aug 11, 2016

I'll add a change log commit, once this is reviewed.

}

pub type nfds_t = c_uint;
pub fn revents(&self) -> Option<EventFlags> {
Copy link
Contributor

Choose a reason for hiding this comment

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

We'll also need at least read/write access to events and possibly write access to revents. This looks a little less ergonomic than direct access to the fields we have right now :( Maybe we can keep the struct definition as is? Are there any reasons to switch other than libc having a better test suite to verify the structure?

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 was wondering about that, but I could only come up with read access to revents as necessary after creation. So, thank you for the feedback.
You text sounds like you have used poll somewhere. Can you point me code that uses it?

Copy link
Contributor

Choose a reason for hiding this comment

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

I added these bindings to nix in #228 as I planned to use it in a project but I never got around to it since then. A quick code search on Github shows atleast 2 people are using it: https://github.com/tailhook/stator and https://github.com/hjr3/carp-rs; they might have some useful feedback :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both example only need read access to revents after creation.

@fiveop
Copy link
Contributor Author

fiveop commented Aug 31, 2016

I would also like to merge this soon, if noone has any further comments. I'll be happy to add read/write accessors to PollFds member fields, when they are needed.

@posborne
Copy link
Member

Hi @fiveop, changes look good to me. Based on my own usage of poll in the past, it would be very useful to be able to modify poll interests. For instance, with non-blocking sockets, it is useful to register for POLLOUT to get notifications of when a socket connects and then unregister that interest later.

I think we can add that later without issues, so @homu r+

@homu
Copy link
Contributor

homu commented Aug 31, 2016

📌 Commit f4a52b3 has been approved by posborne

homu added a commit that referenced this pull request Aug 31, 2016
Use libc in poll.rs

I'll add a change log commit, once this is reviewed.
@homu
Copy link
Contributor

homu commented Aug 31, 2016

⌛ Testing commit f4a52b3 with merge 9140e62...

@homu
Copy link
Contributor

homu commented Aug 31, 2016

☀️ Test successful - status

@homu homu merged commit f4a52b3 into nix-rust:master Aug 31, 2016
@fiveop fiveop deleted the less_ffi_poll branch October 15, 2016 17:48
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.

None yet

4 participants