add poll module in Android #672

ndusart · 2017-07-12T13:47:48Z

poll functions are defined in Android as well.

libc is missing some constant, but once rust-lang/libc#663 is merged, it'll be good to merge here.

Closes #711

asomers · 2017-07-12T14:42:52Z

Good find. Could you also enable the poll tests for Android in test/test.rs ?

Susurrus · 2017-07-12T15:22:36Z

Actually poll() and related constants are defined for all unix platforms. So we can just unconditionally enable that. But ppoll is only for android, linux, and freebsd/dragonfly, though the freebsd/dragonfly support is missing from ppoll, could you change that as well @ndusart?

Susurrus · 2017-07-12T15:23:48Z

Also you'll want to change Cargo.toml to point to libc git for these tests to pass once rust-lang/libc#663 lands.

ndusart · 2017-07-12T15:46:08Z

I included the module and the test unconditionally.
Could only test for linux and android though. Waiting to see the CI results.

ndusart · 2017-07-12T15:51:24Z

There is an issue on FreeBSD, seem that the same symbols in rust-lang/libc#663 should be added to FreeBSD and DragonFly. I'll update that.

ndusart · 2017-07-13T18:44:48Z

Should be good to merge now.

Susurrus · 2017-07-13T19:19:05Z

@ndusart We're waiting for nix to release before merging this. Either that or a new libc release happens before and then we can merge this in.

In the mean time can you add a CHANGELOG entry for this? I think putting it under the Added section makes sense since this is just an exposure of more functionality on some platforms, versus a change in an actual API.

Susurrus · 2017-07-17T18:07:11Z

CHANGELOG.md

@@ -22,6 +22,8 @@ This project adheres to [Semantic Versioning](http://semver.org/).
  and nix::Error::UnsupportedOperation}`
  ([#614](https://github.com/nix-rust/nix/pull/614))
 - Added `cfmakeraw`, `cfsetspeed`, and `tcgetsid`. ([#527](https://github.com/nix-rust/nix/pull/527))
+- Added `nix::poll` module for all platforms


Can you also state that ppoll was added for FreeBSD, DragonflyBSD?

Susurrus · 2017-07-25T00:40:35Z

This needs a rebase.

I'd also like to ask if you could, as part of this PR, whip up some documentation for this module now that it's supported for all platforms. It'd be much appreciated!

Susurrus · 2017-07-25T15:24:31Z

src/poll.rs

@@ -1,19 +1,30 @@
-#[cfg(any(target_os = "linux", target_os = "android"))]
+#[cfg(any(target_os = "linux", target_os = "android", target_os = "freebsd", target_os = "dragonfly"))]


Can you alphabetize the list of target OSes here since you're already modifying it?

Susurrus · 2017-07-25T15:24:35Z

src/poll.rs

 use sys::time::TimeSpec;
-#[cfg(any(target_os = "linux", target_os = "android"))]
+#[cfg(any(target_os = "linux", target_os = "android", target_os = "freebsd", target_os = "dragonfly"))]


Can you alphabetize the list of target OSes here since you're already modifying it?

Susurrus · 2017-07-25T15:24:41Z

src/poll.rs

+/// pthread_sigmask(SIG_SETMASK, &origmask, NULL);
+/// ```
+///
+#[cfg(any(target_os = "linux", target_os = "android", target_os = "freebsd", target_os = "dragonfly"))]


Can you alphabetize the list of target OSes here since you're already modifying it?

Susurrus · 2017-07-25T15:25:15Z

src/poll.rs

@@ -23,26 +34,81 @@ impl PollFd {
        }
    }

+    /// Returns the events that occured in the last call to `poll` or `ppoll`.
    pub fn revents(&self) -> Option<EventFlags> {
        EventFlags::from_bits(self.pollfd.revents)
    }
 }

 libc_bitflags! {
    pub flags EventFlags: libc::c_short {


The bitflag itself needs a doccomment above this line.

Susurrus · 2017-07-25T15:26:33Z

src/poll.rs

@@ -53,7 +119,32 @@ pub fn poll(fds: &mut [PollFd], timeout: libc::c_int) -> Result<libc::c_int> {
    Errno::result(res)
 }

-#[cfg(any(target_os = "linux", target_os = "android"))]
+/// `ppoll()` allows an application to safely wait until either a file


Let's try to shorten this a bit so it looks better in the docs. The man page starts with "wait for some event on a file descriptor". I think that's descriptive and succinct and I suggest you use that here instead.

Ok, I wanted to focus on the difference between poll and ppoll. Should I just put the first line of man, and redirect to the web page for more info ?

Distinguishing them would be good, I just wanted the description to be a little shorter so I grabbed it from the man page. Maybe instead say "Like poll() but can also wait for signals". How's that sound?

poll returns also on signals but the caller do not control what. But I agree that something like "Like poll() but let you precise what signals may interrupt it" is enough. I'll change that.

Susurrus · 2017-07-25T15:27:38Z

src/poll.rs

+/// This is a wrapper around `libc::pollfd`.
+///
+/// It's meant to be used as an argument to the [`poll`](fn.poll.html) and
+/// [`ppoll`](fn.ppoll.html) functions to specify the events of interested


Susurrus · 2017-07-25T15:28:44Z

Thanks for the docs, it looks a lot better! There's some minor things to clean up here, but nothing major.

ndusart · 2017-07-26T08:16:18Z

The requested changes have been made.

ndusart · 2017-07-27T20:07:17Z

@Susurrus does this PR seem ok ?

Susurrus

Just some minor grammar/documentation changes. Then this is ready to roll I think. Thanks for your hard work on the docs, definitely helps me understand this functionality!

Susurrus · 2017-07-28T17:18:17Z

src/poll.rs

+/// This is a wrapper around `libc::pollfd`.
+///
+/// It's meant to be used as an argument to the [`poll`](fn.poll.html) and
+/// [`ppoll`](fn.ppoll.html) functions to specify the events of interes


Susurrus · 2017-07-28T17:18:36Z

src/poll.rs

 #[repr(C)]
 #[derive(Clone, Copy)]
 pub struct PollFd {
    pollfd: libc::pollfd,
 }

 impl PollFd {
-    pub fn new(fd: libc::c_int, events: EventFlags) -> PollFd {
+    /// Creates a new `PollFd` specifying the events of interested


Susurrus · 2017-07-28T17:21:57Z

src/poll.rs

+/// `poll` waits for one of a set of file descriptors to become ready to perform I/O.
+/// ([`poll(2)`](http://man7.org/linux/man-pages/man2/poll.2.html))
+///
+/// `fds` is a slice containing all [`PollFd`](struct.PollFd.html) to poll.


We know it's a slice by the datatype, just say "fds contains all ..."

Susurrus · 2017-07-28T17:23:17Z

src/poll.rs

+/// ([`poll(2)`](http://man7.org/linux/man-pages/man2/poll.2.html))
+///
+/// `fds` is a slice containing all [`PollFd`](struct.PollFd.html) to poll.
+/// The function will return as soon as one event registered in one of the `PollFd` occurs.


"The function will return as soon as any events occur for any of the PollFds." Something like that, this is a bit more readable I think.

Susurrus · 2017-07-28T17:23:41Z

src/poll.rs

+/// descriptor becomes ready or until a signal is caught.
+/// ([`poll(2)`](http://man7.org/linux/man-pages/man2/poll.2.html))
+///
+/// `ppoll` behaves like `poll`, but let you precise what signals may interrupt it


"specify" instead of "precise"

ndusart · 2017-07-30T12:44:55Z

@Susurrus : done, thanks for your feedback !

Susurrus · 2017-07-30T15:51:15Z

@ndusart Thanks for your hard work on this, I know it took a while to work through my rigorous review process, and to deal with upstream, but it's contributions like this that make nix great!

cc @berkowski

bors r+

672: add poll module in Android r=Susurrus `poll` functions are defined in Android as well. libc is missing some constant, but once rust-lang/libc#663 is merged, it'll be good to merge here. Closes #711

bors · 2017-07-30T16:31:30Z

Build succeeded

ndusart · 2017-07-30T17:49:12Z

@Susurrus you wouldn't be a good reviewer if you wasn't a bit nitpicking about PR :p

It didn't take too long, it's just that the different timezones make that small successive revisions takes more time unfortunately.

716: Fix changelog from bad merge r=Susurrus Missed this when merging #672.

ndusart force-pushed the android-poll branch from 70229f5 to 94d3839 Compare July 12, 2017 15:43

ndusart force-pushed the android-poll branch 4 times, most recently from 85f1e2e to d8d3ab9 Compare July 13, 2017 15:43

ndusart force-pushed the android-poll branch from d8d3ab9 to d8e070a Compare July 14, 2017 07:32

Susurrus suggested changes Jul 17, 2017

View reviewed changes

ndusart force-pushed the android-poll branch from d8e070a to 7c69781 Compare July 17, 2017 20:45

ndusart force-pushed the android-poll branch from 1b3725d to c38110a Compare July 25, 2017 08:51

Susurrus suggested changes Jul 25, 2017

View reviewed changes

add poll module for all platforms

f14c586

ndusart force-pushed the android-poll branch from c38110a to ed91f2f Compare July 26, 2017 08:14

Susurrus suggested changes Jul 28, 2017

View reviewed changes

Susurrus mentioned this pull request Jul 29, 2017

Let freebsd and openbsd access 'poll' #710

Closed

berkowski mentioned this pull request Jul 29, 2017

nix::poll not available for bsd targets. #711

Closed

document poll module

e19ffab

ndusart force-pushed the android-poll branch from ed91f2f to e19ffab Compare July 30, 2017 12:44

Susurrus approved these changes Jul 30, 2017

View reviewed changes

bors bot added a commit that referenced this pull request Jul 30, 2017

Merge #672

f8768e9

672: add poll module in Android r=Susurrus `poll` functions are defined in Android as well. libc is missing some constant, but once rust-lang/libc#663 is merged, it'll be good to merge here. Closes #711

bors bot merged commit e19ffab into nix-rust:master Jul 30, 2017

ndusart deleted the android-poll branch July 30, 2017 17:42

berkowski mentioned this pull request Jul 30, 2017

match new API of serialport-rs 2.0.0 berkowski/mio-serial#1

Merged

Susurrus mentioned this pull request Jul 31, 2017

Fix changelog from bad merge #716

Merged

bors bot added a commit that referenced this pull request Aug 1, 2017

Merge #716

607ab97

716: Fix changelog from bad merge r=Susurrus Missed this when merging #672.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add poll module in Android #672

add poll module in Android #672

ndusart commented Jul 12, 2017 •

edited by Susurrus

Loading

asomers commented Jul 12, 2017

Susurrus commented Jul 12, 2017

Susurrus commented Jul 12, 2017

ndusart commented Jul 12, 2017 •

edited

Loading

ndusart commented Jul 12, 2017 •

edited

Loading

ndusart commented Jul 13, 2017

Susurrus commented Jul 13, 2017

Susurrus Jul 17, 2017

ndusart Jul 17, 2017

Susurrus commented Jul 25, 2017

Susurrus Jul 25, 2017

Susurrus Jul 25, 2017

Susurrus Jul 25, 2017

Susurrus Jul 25, 2017

Susurrus Jul 25, 2017

ndusart Jul 25, 2017 •

edited

Loading

Susurrus Jul 25, 2017

ndusart Jul 25, 2017

Susurrus Jul 25, 2017

Susurrus commented Jul 25, 2017

ndusart commented Jul 26, 2017

ndusart commented Jul 27, 2017

Susurrus left a comment

Susurrus Jul 28, 2017

Susurrus Jul 28, 2017

Susurrus Jul 28, 2017

Susurrus Jul 28, 2017

Susurrus Jul 28, 2017

ndusart commented Jul 30, 2017

Susurrus commented Jul 30, 2017

bors bot commented Jul 30, 2017

ndusart commented Jul 30, 2017

		@@ -1,19 +1,30 @@
		#[cfg(any(target_os = "linux", target_os = "android"))]
		#[cfg(any(target_os = "linux", target_os = "android", target_os = "freebsd", target_os = "dragonfly"))]

add poll module in Android #672

add poll module in Android #672

Conversation

ndusart commented Jul 12, 2017 • edited by Susurrus Loading

asomers commented Jul 12, 2017

Susurrus commented Jul 12, 2017

Susurrus commented Jul 12, 2017

ndusart commented Jul 12, 2017 • edited Loading

ndusart commented Jul 12, 2017 • edited Loading

ndusart commented Jul 13, 2017

Susurrus commented Jul 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Susurrus commented Jul 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ndusart Jul 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Susurrus commented Jul 25, 2017

ndusart commented Jul 26, 2017

ndusart commented Jul 27, 2017

Susurrus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ndusart commented Jul 30, 2017

Susurrus commented Jul 30, 2017

bors bot commented Jul 30, 2017

Build succeeded

ndusart commented Jul 30, 2017

ndusart commented Jul 12, 2017 •

edited by Susurrus

Loading

ndusart commented Jul 12, 2017 •

edited

Loading

ndusart commented Jul 12, 2017 •

edited

Loading

ndusart Jul 25, 2017 •

edited

Loading