-
Notifications
You must be signed in to change notification settings - Fork 654
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
Fix Broken SockFlag::SOCK_NONBLOCK and SockFlag::SOCK_CLOEXEC on MacOS/iOS #863
base: master
Are you sure you want to change the base?
Conversation
…LOEXEC` on macos and ios; ref nix-rust#244 nix-rust#861
src/sys/socket/mod.rs
Outdated
#[cfg(target_os = "openbsd")] | ||
SOCK_DNS; | ||
cfg_if! { | ||
if #[cfg(any(target_os = "macos", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note here: nix::sys::socket::socket
fn API requires the linux specific SockFlag::SOCK_NONBLOCK
and SockFlag::SOCK_CLOEXEC
which is not included in macOS' libc -- the work around is to provide a bitflag that mimics such support.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why can't this be fixed in libc
? Do these constants not exist on OS X and iOS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fundamentally an implementation concern.
It's because the nix::sys::socket::socket
fn is not only assigning a socket, but also setting flags on the socket in the same function to match parity with Linux (which is an improvement). The normal API is defined in socket(2) as:
int socket(int domain, int type, int protocol);
linux diverts from POSIX and BSD by supplying a SOCK_NONBLOCK
flag as is defined in linux man socket(2) as:
SOCK_NONBLOCK
Set the O_NONBLOCK file status flag on the new open
file description. Using this flag saves extra calls
to fcntl(2) to achieve the same result.
SOCK_CLOEXEC
Set the close-on-exec (FD_CLOEXEC) flag on the new
file descriptor. See the description of the
O_CLOEXEC flag in open(2) for reasons why this may be
useful.
In summery:
SOCK_NONBLOCK
andSOCK_CLOEXEC
are in linux since 2.6.27- Some BSDs have conformed to this new API and such have a define in
libc
. - macOS is the exception and sticks to directly calling
fcntl(2)
nix
takes the high road and also only callsfcntl(2)
with a new fourth parameter on the function call
Because nix
provides a new API that does not conform to POSIX or BSD (or directly Linux), this patch creates virtual definitions for macOS and iOS that are backed by their equivalent fcntl
flags.
I think this is the best course of action for two reasons:
- Redefining the function at this stage would break compatibility
- Defining virtual definitions for
SOCK_NONBLOCK
andSOCK_CLOEXEC
are only used inside ofnix
and only for the socket fn.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the sake of conversation, the alternative is:
Removing pub struct SockFlag
and having the API user call fcntl
directly.
Thus, these function defs need to be changed (breaking downstream code):
pub fn socket<T: Into<Option<SockProtocol>>>(domain: AddressFamily, ty: SockType, flags: SockFlag, protocol: T) -> Result<RawFd> {
/// Create a pair of connected sockets
///
/// [Further reading](http://pubs.opengroup.org/onlinepubs/9699919799/functions/socketpair.html)
pub fn socketpair<T: Into<Option<SockProtocol>>>(domain: AddressFamily, ty: SockType, protocol: T, flags: SockFlag) -> Result<(RawFd, RawFd)> {
/// Accept a connection on a socket
///
/// [Further reading](http://man7.org/linux/man-pages/man2/accept.2.html)
pub fn accept4(sockfd: RawFd, flags: SockFlag) -> Result<RawFd> {
fn accept4_polyfill(sockfd: RawFd, flags: SockFlag) -> Result<RawFd> {
I just searched all of |
@Susurrus I replied to your comment above here: #863 (comment)
Please read my above comment and then let me know is the right path forward. I put in the work to support macOS and iOS and am willing to maintain it. Thanks. |
…gate on `test_sockflag_cloexec`; ref nix-rust#244 nix-rust#861
Okay, so this is all pretty interesting. The code as it stands at HEAD is incorrect and this code doesn't fix the problem properly as well I don't think. What we have is that the
But what we actually do is instead of allowing the user to use these values directly on support platforms, what So I think this is part of a larger issue here of exposing these APIs. @asomers any input here? I think what we should do is disable this emulation for all platforms and only emulate it on mac/ios. Additionally I think that on mac/ios we shouldn't reuse the |
@Susurrus thanks for the reply. Again to reiterate from my earlier comment (which did not receive a reply):
This patch solves the bug and I think it is the best course of action for two reasons:
That said, it might be smart to also provide access to |
I think that Nix's purpose is to expose OS functionality in a Rusty way. I don't think we should try to emulate missing OS functionality, especially since we can't do it correctly (the emulated socket flags are not set atomically). So I'm in favor of removing the emulation altogether, even on OSX. And BTW, we already have a wrapper for |
@kristate I understand your rationale for wanting this feature as I did read your original reply, but I disagree that this functionality should exist. Regarding your two points:
So I agree with @asomers above about removing the emulation entirely. It's probably best to close this PR and submit a new one to do that. @kristate if you'd like to do that, we'd appreciate it, otherwise I'll file one later. I want to resolve this issue for the 0.11.0 release. |
@asomers @Susurrus Thanks for the replies. I really think that the API was trying to be rusty from the beginning. I was just fixing a bug. The problem is that there are two Also, if people really wanted to handle edit: the readme for this project even states "The goal is to not provide a 100% unified interface, but to unify what can be while still providing platform specific APIs." -- is this not a unified interface for |
But the code is still wrong even after this fix. All BSDs and Linux support these flags natively, so they don't need the emulation, but it's turned on for all BSDs. I'm not certain if we should add the runtime detection for BSDs or we should actually assume that we're running versions at least as new as what I listed above. 2015 releases are still pretty new, but I'm not certain it's worth the effort when we can just document it as minimum requirements for the lib. I'd also be fine dropping the runtime checks for Linux given it was added 10 years ago. @asomers do you agree? |
As to address you specific question: "the readme for this project even states "The goal is to not provide a 100% unified interface, but to unify what can be while still providing platform specific APIs." -- is this not a unified interface for socket?" This is one of those not-100% scenarios. We try to provide a unified interface, but we don't try to emulate or approximate functionality. This code change emulates an atomic API with a non-atomic implementation, which I consider bad form. We have always torn out this code and left the API more lacking in these circumstances. |
@Susurrus I think I understand your concern now. So, C programmers are used to having the The point I am making is that there needs to be a name and a value for the flags to our rust API. I think that the question we have to first ask ourselves is: Does "Emulation" is a poor way to think about this problem. We are creating a Rusty API that helps calm the waters when working with unix. I chose to work with So, I leave it to you: Does Thanks. |
Where in my code does this change emulate an atomic API with a non-atomic implementation? By the way, it does seem to check if the OS will |
@kristate it's not your code that lacks atomicity; it's the existing code. The whole reason that @Susurrus all FreeBSD, NetBSD, and OpenBSD releases that don't support these flags are EoL. I'm not sure what Dragonfly's support policy is, but they've had several releases since adding these flags. Linux 2.6.27 is pretty old, but RHEL 5 uses 2.6.18 and it's on extended support until 2020. And of course, no version of OSX supports these flags. Personally, I don't think it would be too bad if Nix ditched all the emulation. Then |
@asomers I agree with you in principle, but either way We have many years of network programming knowledge that we can pass-on downstream to users who might be less aware. Also, I am thinking about adding more documentation to match this discussion. Thoughts? |
Your implication is that we should be doing this and we've been explaining that we don't believe we should. There are numerous technical reasons why which we've enumerated previously. We all would like if we could have a nice API for this across all platforms, but that's not how things are unfortunately. So what we try to do is make the best of a bad situation. As for better documentation, I'm all about that, so if you feel you can improve our documentation in this area, we'd welcome it!
This wouldn't apply to So I think I agree with you @asomers, let's remove all emulation here. Since the constants aren't even available on macos/ios, then I think this should have minimal impact on downstream codebases. |
I guess I feel a little torn here. More or less because the Berkeley socket API sucks in so many ways and by safely normalizing as much as possible, we could build a better ecosystem in Rust. Better yet, rust's standard library sets More discussion here: rust-lang/rust#24237 |
907: Remove emulation of FD_CLOEXEC/O_NONBLOCK r=asomers a=Susurrus Rather than using the native implementation of these constants on supported platforms, the native implementation was instead emulated. This was also hidden from the user even though this could result in data races and the functionality being broken. Native functionality is, however, not support on macos/ios. Rather than enable this emulation solely for this platform, it should be removed as this is a dangerous abstraction. This is a replacement for #863. There is much previous discussion there which I recommend you read to familiarize yourself with this decision. I'm looking to push this through rather quickly as it's the last thing blocking our next 0.11.0 release. cc @aomser @kristate Co-authored-by: Bryant Mairs <bryantmairs@google.com>
Was this ever addressed in some other PRs? |
I guess no, and I am actually surprised that the And I think @asomers's thought is right, we should expose the system interface without emulating anything, I will take a look at this and possibly get it fixed. |
SockFlag::SOCK_NONBLOCK
andSockFlag::SOCK_CLOEXEC
in 0.9.0 broke macOS/iOS support in 0.10.0 (f067e73)This commit solves the issue and also provides test cases to test for continued support.
SockFlag::SOCK_NONBLOCK
SockFlag::SOCK_CLOEXEC
ref #244 #861