Use the real pipe2(2) on most targets #777

asomers · 2017-10-08T17:06:01Z

FreeBSD added pipe2(2) in version 10.0.

Susurrus · 2017-10-14T15:54:41Z

This needs an Added CHANGELOG entry and could also use comments for the tests.

Susurrus

Does it even make sense to keep the bind2() emulation around if according to libc pip2 exists on android, emscripten, and linux?

Susurrus · 2017-10-18T04:27:35Z

src/unistd.rs

@@ -827,7 +827,8 @@ pub fn pipe() -> Result<(RawFd, RawFd)> {
 // libc only defines `pipe2` in `libc::notbsd`.
 #[cfg(any(target_os = "linux",
          target_os = "android",
-          target_os = "emscripten"))]
+          target_os = "emscripten",
+          target_os = "freebsd"))]


Since you're here, can you reorder the OSes to be alphabetical?

Susurrus · 2017-10-18T04:27:58Z

src/unistd.rs

@@ -840,7 +841,8 @@ pub fn pipe2(flags: OFlag) -> Result<(RawFd, RawFd)> {

 #[cfg(not(any(target_os = "linux",
              target_os = "android",
-              target_os = "emscripten")))]
+              target_os = "emscripten",
+              target_os = "freebsd")))]


Susurrus · 2017-10-18T04:28:04Z

src/unistd.rs

@@ -855,7 +857,8 @@ pub fn pipe2(flags: OFlag) -> Result<(RawFd, RawFd)> {

 #[cfg(not(any(target_os = "linux",
              target_os = "android",
-              target_os = "emscripten")))]
+              target_os = "emscripten",
+              target_os = "freebsd")))]


Susurrus · 2017-10-18T04:31:25Z

src/unistd.rs

@@ -827,7 +827,8 @@ pub fn pipe() -> Result<(RawFd, RawFd)> {
 // libc only defines `pipe2` in `libc::notbsd`.


Can you clean up this comment? Seems out of date since freebsd support is being added.

Additionally, it appears that this is supported by NetBSD and OpenBSD according to libc, so let's add support for bind2 on those platforms as well.

I didn't check those platforms. It looks like pretty much everything supports it now, even Illumos.

Susurrus · 2017-10-18T04:32:42Z

CHANGELOG.md

@@ -32,6 +32,8 @@ This project adheres to [Semantic Versioning](http://semver.org/).
  ([#663](https://github.com/nix-rust/nix/pull/663))

 ### Changed
+- On FreeBSD, bind directly to `pipe2` instead of emulating it.  Users should


Since we tell the user they'll notice no difference, I think it'd be good to say why we're doing this. I'm not certain why we were emulating this in the first place, so that could be clarified here.

Also, this should be updated for NetBSD and OpenBSD also.

Susurrus · 2017-10-20T15:40:20Z

Looks like it needs a rebase and I'm not certain why testing failed, let's give it another run.

Susurrus · 2017-10-24T05:30:10Z

Needs a rebase then I'm r+ on it.

Susurrus · 2017-11-06T05:07:09Z

Looks like Darwin doesn't support it or it's just not implemented in libc yet, which is why testing fails.

Also, it'd be good if you added doccomments for pipe2 as part of this work since you're touching the function already and it's missing that.

Susurrus · 2017-11-11T20:35:06Z

Note that pipe2 doesn't exist on Mac, so this will need to be restricted from those targets.

asomers · 2017-11-19T17:37:35Z

Oops, I forgot to update doc comments. I'll do that shortly.

Susurrus · 2017-11-21T04:30:05Z

test/test_unistd.rs

+fn test_pipe2() {
+    let (fd0, fd1) = pipe2(fcntl::O_CLOEXEC).unwrap();
+    let f0 = fcntl::FdFlag::from_bits_truncate(fcntl::fcntl(fd0, fcntl::F_GETFD).unwrap());
+    assert!(f0.contains(fcntl::FD_CLOEXEC));


Shouldn't you check that both of these fds don't have this flag to begin with?

What do you mean "to begin with"? pipe2 sets that flag atomically as the pipe is created.

You're correct, ignore this comment.

Susurrus · 2017-11-21T04:30:30Z

src/unistd.rs

+/// `O_CLOEXEC`:    Set the close-on-exec flag for the new file descriptors.
+/// `O_NONBLOCK`:   Set the non-blocking flag for the ends of the pipe.
+#[cfg(any(target_os = "ios",
+          target_os = "macos"))]
 pub fn pipe2(flags: OFlag) -> Result<(RawFd, RawFd)> {


Does it make sense to keep this emulation for just ios/macos? We've removed emulated functionality before, and I'd like to suggest we remove it here as well.

Removing it would make me happy. But perhaps we should mark it as deprecated for a release cycle first?

Sure, let's mark it deprecated for 0.10 and then remove for the next release.

asomers · 2017-11-22T23:55:15Z

This is unusual. The build failures in Travis were caused by a merge error. Travis's merge error. Travis pulled the master branch then tried to apply my patch, and it looks like it duped a line. I'll rebase.

Susurrus · 2017-12-03T04:41:06Z

bors r+

777: Use the real pipe2(2) on most targets r=Susurrus a=asomers FreeBSD added pipe2(2) in version 10.0.

bors · 2017-12-03T05:21:30Z

Build failed

buildbot/nix-rust/nix amd64_fbsd11

Susurrus · 2017-12-03T05:23:34Z

Ahh, yes. Tests need to be updated because #801 was merged.

Susurrus · 2017-12-04T00:49:40Z

src/unistd.rs

-#[cfg(not(any(target_os = "linux",
-              target_os = "android",
-              target_os = "emscripten")))]
+#[cfg(any(target_os = "ios",


This should be just 1 line.

Susurrus · 2017-12-04T00:49:46Z

src/unistd.rs

+///
+/// `O_CLOEXEC`:    Set the close-on-exec flag for the new file descriptors.
+/// `O_NONBLOCK`:   Set the non-blocking flag for the ends of the pipe.
+#[cfg(any(target_os = "ios",


Should just be 1 line also.

Susurrus

I think it's just the CHANGELOG fix and then this LGTM.

Susurrus · 2017-12-05T15:28:08Z

CHANGELOG.md

@@ -47,6 +47,8 @@ This project adheres to [Semantic Versioning](http://semver.org/).
  ([#800](https://github.com/nix-rust/nix/pull/800))

 ### Changed
+- Use native `pipe` on all BSD targets.  Users should notice no difference.


Don't you actually mean pipe2 here?

All supported non-Apple platforms now use the native syscall. Only ios and macos lack it. Deprecate pipe2 on those platforms, because it's impossible to guarantee atomicity with a userland implementation. It was added in: * DragonflyBSD 4.2 * FreeBSD 10.0 * NetBSD 6.0 * OpenBSD 5.7

asomers · 2017-12-08T04:08:42Z

bors r+ susurrus

777: Use the real pipe2(2) on most targets r=asomers a=asomers FreeBSD added pipe2(2) in version 10.0.

Susurrus · 2017-12-08T04:33:47Z

Probably should have added that we deprecated our wrapper on other platforms in the CHANGELOG as well.

bors · 2017-12-08T06:53:46Z

Build succeeded

asomers force-pushed the pipe2_fbsd branch from cf4b954 to f50aa34 Compare October 14, 2017 23:51

Susurrus suggested changes Oct 18, 2017

View reviewed changes

asomers force-pushed the pipe2_fbsd branch from f50aa34 to 474f42e Compare October 19, 2017 00:26

Susurrus approved these changes Oct 20, 2017

View reviewed changes

Susurrus changed the title ~~Use the real pipe2(2) on FreeBSD~~ Use the real pipe2(2) on all targets Oct 20, 2017

asomers added this to the 0.10.0 milestone Nov 5, 2017

asomers force-pushed the pipe2_fbsd branch from 474f42e to 36ea95d Compare November 6, 2017 03:37

asomers force-pushed the pipe2_fbsd branch from 36ea95d to f60fcfd Compare November 19, 2017 17:28

asomers changed the title ~~Use the real pipe2(2) on all targets~~ Use the real pipe2(2) on most targets Nov 19, 2017

asomers force-pushed the pipe2_fbsd branch from f60fcfd to ea98f44 Compare November 19, 2017 17:55

Susurrus suggested changes Nov 21, 2017

View reviewed changes

asomers force-pushed the pipe2_fbsd branch from ea98f44 to 49ef0b1 Compare November 22, 2017 04:52

asomers force-pushed the pipe2_fbsd branch from 49ef0b1 to 7613b90 Compare November 23, 2017 00:07

Susurrus approved these changes Dec 3, 2017

View reviewed changes

bors bot added a commit that referenced this pull request Dec 3, 2017

Merge #777

154a127

777: Use the real pipe2(2) on most targets r=Susurrus a=asomers FreeBSD added pipe2(2) in version 10.0.

asomers force-pushed the pipe2_fbsd branch from 7613b90 to d5381f3 Compare December 3, 2017 21:22

Susurrus suggested changes Dec 4, 2017

View reviewed changes

asomers force-pushed the pipe2_fbsd branch from d5381f3 to 40e58c4 Compare December 4, 2017 01:51

Susurrus suggested changes Dec 5, 2017

View reviewed changes

asomers force-pushed the pipe2_fbsd branch from 40e58c4 to 6c939fb Compare December 8, 2017 04:08

bors bot added a commit that referenced this pull request Dec 8, 2017

Merge #777

d96518c

777: Use the real pipe2(2) on most targets r=asomers a=asomers FreeBSD added pipe2(2) in version 10.0.

Susurrus approved these changes Dec 8, 2017

View reviewed changes

bors bot merged commit 6c939fb into nix-rust:master Dec 8, 2017

oconnor663 mentioned this pull request Jul 27, 2018

deprecation warning in nix on OSX oconnor663/os_pipe.rs#14

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use the real pipe2(2) on most targets #777

Use the real pipe2(2) on most targets #777

asomers commented Oct 8, 2017

Susurrus commented Oct 14, 2017

Susurrus left a comment

Susurrus Oct 18, 2017

Susurrus Oct 18, 2017

Susurrus Oct 18, 2017

Susurrus Oct 18, 2017

asomers Oct 19, 2017

Susurrus Oct 18, 2017

Susurrus commented Oct 20, 2017

Susurrus commented Oct 24, 2017

Susurrus commented Nov 6, 2017

Susurrus commented Nov 11, 2017

asomers commented Nov 19, 2017

Susurrus Nov 21, 2017

asomers Nov 21, 2017

Susurrus Nov 22, 2017

Susurrus Nov 21, 2017

asomers Nov 21, 2017

Susurrus Nov 22, 2017

asomers commented Nov 22, 2017

Susurrus commented Dec 3, 2017

bors bot commented Dec 3, 2017

Susurrus commented Dec 3, 2017

Susurrus Dec 4, 2017

Susurrus Dec 4, 2017

Susurrus left a comment

Susurrus Dec 5, 2017

asomers commented Dec 8, 2017

Susurrus commented Dec 8, 2017

bors bot commented Dec 8, 2017

		@@ -827,7 +827,8 @@ pub fn pipe() -> Result<(RawFd, RawFd)> {
		// libc only defines `pipe2` in `libc::notbsd`.

Use the real pipe2(2) on most targets #777

Use the real pipe2(2) on most targets #777

Conversation

asomers commented Oct 8, 2017

Susurrus commented Oct 14, 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

Choose a reason for hiding this comment

Susurrus commented Oct 20, 2017

Susurrus commented Oct 24, 2017

Susurrus commented Nov 6, 2017

Susurrus commented Nov 11, 2017

asomers commented Nov 19, 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

Choose a reason for hiding this comment

asomers commented Nov 22, 2017

Susurrus commented Dec 3, 2017

bors bot commented Dec 3, 2017

Build failed

Susurrus commented Dec 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Susurrus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asomers commented Dec 8, 2017

Susurrus commented Dec 8, 2017

bors bot commented Dec 8, 2017

Build succeeded