Atomically open files with O_CLOEXEC where possible #27971
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
|
If someone has access to the BSDs or OS X, I'd be grateful for the values of O_CLOEXEC. |
|
On my OS X (10.10.5), its value is |
|
FreeBSD 10.2: |
|
@barosl Thanks for the constants! The PR should be ready to merge now. |
|
Thanks! Can you also add a comment explaining why we even though we pass |
|
@alexcrichton Done. |
|
Thanks! Could you also squash the commits together? |
On Linux the flag is just ignored if it is not supported: https://lwn.net/Articles/588444/ Touches rust-lang#24237.
|
@alexcrichton Done. |
|
Does this need a test? it would be good to grab the flags from the fd and assert that CLOEXEC is set (at least on the platforms that have bots). |
|
If this needs a test, where do we put those IO tests? |
| @@ -269,6 +269,9 @@ impl File { | |||
| libc::open(path.as_ptr(), flags, opts.mode) | |||
| })); | |||
| let fd = FileDesc::new(fd); | |||
| // Even though we open with the O_CLOEXEC flag, still set CLOEXEC here, | |||
There was a problem hiding this comment.
hm, since @alexcrichton pointed out that there are tests for this, would it be appropriate to add [cfg(not(any(target_os = "freebsd", target_os = "dragonfly", ...)))]? perhaps pedantic, but it would assert that constants added here are correct
There was a problem hiding this comment.
In some cases an older version of any OS might not implement atomic option even if the constant is correct.
There was a problem hiding this comment.
Yeah, thought of this and forgot to comment. Carry on :(
On Linux the flag is just ignored if it is not supported: https://lwn.net/Articles/588444/ Still needs the values of O_CLOEXEC on the BSDs. Touches #24237.
These commits finish up closing out #24237 by filling out all locations we create new file descriptors with variants that atomically create the file descriptor and set CLOEXEC where possible. Previous support for doing this in `File::open` was added in #27971 and support for `try_clone` was added in #27980. This commit fills out: * `Socket::new` now passes `SOCK_CLOEXEC` * `Socket::accept` now uses `accept4` * `pipe2` is used instead of `pipe` Unfortunately most of this support is Linux-specific, and most of it is post-2.6.18 (our oldest supported version), so all of the detection here is done dynamically. It looks like OSX does not have equivalent variants for these functions, so there's nothing more we can do there. Support for BSDs can be added over time if they also have these functions. Closes #24237
On Linux the flag is just ignored if it is not supported:
https://lwn.net/Articles/588444/
Still needs the values of O_CLOEXEC on the BSDs.
Touches #24237.