call pipe2 directly on Linux #427
Conversation
|
It looks like support for this used to exist, but was removed in 9e93533. Am I repeating the mistakes of the past? |
| fn pipe2_setflags(fd1: RawFd, fd2: RawFd, flags: OFlag) -> Result<()> { | ||
| use fcntl::O_NONBLOCK; |
There was a problem hiding this comment.
Why do you pull these use statements down from the file header?
There was a problem hiding this comment.
Since this is the only function that uses these imports, and it's no longer building on Linux, we get "unused import" warnings if we leave these at the top. Is there another good way to avoid those?
| target_os = "android", | ||
| target_os = "emscripten"))] | ||
| pub fn pipe2(flags: OFlag) -> Result<(RawFd, RawFd)> { | ||
| unsafe { |
There was a problem hiding this comment.
Could you use unsafe as finegrained as possible, even if you need more instances.
There was a problem hiding this comment.
Done in b28005f. I notice GitHub's fancy new review system doesn't automatically collapse comments in the same way though.
| fn pipe2_setflags(fd1: RawFd, fd2: RawFd, flags: OFlag) -> Result<()> { | ||
| use fcntl::O_NONBLOCK; |
| ### Changed | ||
| - `pipe2` now calls `libc::pipe2` where available. Previously it was emulated | ||
| using `pipe`, which meant that setting `O_CLOEXEC` was not atomic. | ||
|
|
There was a problem hiding this comment.
([#427](https://github.com/nix-rust/nix/pull/427))
|
Anything else I should do on my end to make this good to go? |
|
@homu r+ It's perfect now. Thank you. |
|
📌 Commit 1d262d0 has been approved by |
call pipe2 directly on Linux A first shot at fixing #414. This approach keeps the old implementation for platforms other than `notbsd`, because `libc` only exposes `pipe2` in the `notbsd` module. I've tested this by hand on my Linux machine in a couple ways: - Create a toy program that opens a pipe and passes it to `cat`, which hags if `O_CLOEXEC` doesn't get set properly. Confirm that it doesn't hang, but that it does if I pass `0` as the flags to `libc::pipe2`. - Delete the new implementation entirely, and delete the `cfg` guards on the old implementation, and confirm that above is still true. I haven't actually tested a cross compilation build though. Is there a standard way to do that?
|
☀️ Test successful - status |
|
@oconnor663 I just came across the fact that the released version doesn't do this. I'll see if we can release this week to get it out there. And tthanks for making this change! |
|
@kamalmarhubi BSD support has also landed in |
|
@oconnor663 oh cool! They are usually happy to release when asked. I'm trying to figure out if there are more things we want before asking. Otherwise we ought to be able to expand it quite soon indeed! :-) |
A first shot at fixing #414. This approach keeps the old implementation for platforms other than
notbsd, becauselibconly exposespipe2in thenotbsdmodule.I've tested this by hand on my Linux machine in a couple ways:
cat, which hags ifO_CLOEXECdoesn't get set properly. Confirm that it doesn't hang, but that it does if I pass0as the flags tolibc::pipe2.cfgguards on the old implementation, and confirm that above is still true.I haven't actually tested a cross compilation build though. Is there a standard way to do that?