-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Unix sockets on redox #51553
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
Unix sockets on redox #51553
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @sfackler (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
This comment has been minimized.
This comment has been minimized.
src/libstd/sys/redox/ext/net.rs
Outdated
/// ``` | ||
#[stable(feature = "unix_socket", since = "1.10.0")] | ||
#[derive(Clone)] | ||
pub struct SocketAddr; |
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 can be constructed by third party code - is that intended?
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.
Haven't thought about that at all. It's not really a problem, so should I bother to fix it? (Also, how? With a dummy variable?)
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.
pub struct SocketAddr(());
is the standard approach.
src/libstd/sys/redox/ext/net.rs
Outdated
/// }; | ||
/// let addr = socket.local_addr().expect("Couldn't get local address"); | ||
/// ``` | ||
#[stable(feature = "unix_socket", since = "1.10.0")] |
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.
Seems like these should be on their own feature gate. They definitely haven't been stable since 1.10!
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.
I copied the code from unix 😛. How do I make a feature gate?
src/libstd/sys/redox/ext/net.rs
Outdated
/// let addr = socket.local_addr().expect("Couldn't get local address"); | ||
/// ``` | ||
#[stable(feature = "unix_socket", since = "1.10.0")] | ||
pub fn local_addr(&self) -> io::Result<SocketAddr> { |
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 are these methods defined if they never succeed?
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 compatibility with the unix interface. Anything using it will still compile, just maybe not work perfectly, depending on the error handling.
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.
What kind of programs call methods like local_addr or set_write_timeout but don't care if the operation succeeds or not?
src/libstd/sys/redox/ext/net.rs
Outdated
@@ -8,7 +8,7 @@ | |||
// option. This file may not be copied, modified, or distributed | |||
// except according to those terms. | |||
|
|||
#![stable(feature = "unix_socket", since = "1.10.0")] | |||
#![stable(feature = "unix_socket_redox", since = "1.27.0")] |
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.
Stuff is always initially unstable when added to the standard library.
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.
Alright, will fix. I'm just so excited to get this working 😛
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.
Should I make a tracking issue for it and everything?
Ping from triage @sfackler! This PR needs your review. |
/// # Examples | ||
/// | ||
/// ``` | ||
/// use std::os::unix::net::UnixListener; |
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.
os::unix
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.
Redox extensions are still under os::unix
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.
Redox is considered a unix?
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.
Seems like it, since it's re-exported to os::unix in mod.rs
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.
Hmm, so my previous impression was that it wasn't. It seems less reasonable to have a different API between redox and "normal" unix in that case. I guess it might be best to go back to having a matching API and just returning errors?
Having two copies of the public-facing bits of this seems pretty hard to deal with maintenance-wise though. I think we'll want to refactor to have a single unified public interface that wraps the Redox/non-Redox impls.
/// With a pathname: | ||
/// | ||
/// ```no_run | ||
/// use std::os::unix::net::UnixListener; |
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.
os::unix
/// Without a pathname: | ||
/// | ||
/// ``` | ||
/// use std::os::unix::net::UnixDatagram; |
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.
os::unix
/// # Examples | ||
/// | ||
/// ```no_run | ||
/// use std::os::unix::net::UnixStream; |
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.
os::unix
/// # Examples | ||
/// | ||
/// ```no_run | ||
/// use std::os::unix::net::UnixStream; |
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.
^
/// # Examples | ||
/// | ||
/// ```no_run | ||
/// use std::os::unix::net::UnixStream; |
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.
^
/// # Examples | ||
/// | ||
/// ```no_run | ||
/// use std::os::unix::net::UnixStream; |
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.
^
/// # Examples | ||
/// | ||
/// ```no_run | ||
/// use std::os::unix::net::UnixStream; |
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.
^
src/libstd/sys/redox/ext/net.rs
Outdated
/// } | ||
/// ``` | ||
pub fn take_error(&self) -> io::Result<Option<io::Error>> { | ||
Ok(None) |
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 should probably be removed.
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.
Compatibility with the unix interface. Without it tokio-uds needs some extra cfg madness, meanwhile it works perfectly with it.
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.
The documentation claims that this returns the value of the SO_ERROR option but that is not the case.
/// | ||
/// ```no_run | ||
/// use std::thread; | ||
/// use std::os::unix::net::{UnixStream, UnixListener}; |
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.
^
/// # Examples | ||
/// | ||
/// ```no_run | ||
/// use std::os::unix::net::UnixListener; |
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.
^
/// # Examples | ||
/// | ||
/// ```no_run | ||
/// use std::os::unix::net::UnixListener; |
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.
^
/// # Examples | ||
/// | ||
/// ```no_run | ||
/// use std::os::unix::net::UnixListener; |
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.
^
/// # Examples | ||
/// | ||
/// ```no_run | ||
/// use std::os::unix::net::UnixListener; |
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.
^
/// # Examples | ||
/// | ||
/// ```no_run | ||
/// use std::os::unix::net::UnixListener; |
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.
^
/// | ||
/// ```no_run | ||
/// use std::thread; | ||
/// use std::os::unix::net::{UnixStream, UnixListener}; |
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.
^
src/libstd/sys/redox/ext/net.rs
Outdated
/// } | ||
/// ``` | ||
pub fn take_error(&self) -> io::Result<Option<io::Error>> { | ||
Ok(None) |
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 should probably be removed.
/// | ||
/// ```no_run | ||
/// use std::thread; | ||
/// use std::os::unix::net::{UnixStream, UnixListener}; |
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.
^
Nothing in the interface is changing code-wise since everything is re-exported, but the docs might display it differently. To clarify, I should revert the commit that merges the public side of the interfaces? |
src/libstd/sys_common/unixsocket.rs
Outdated
@@ -0,0 +1,731 @@ | |||
// Copyright 2016 The Rust Project Developers. See the COPYRIGHT |
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.
Is this module included in sys_common
or is this a stray file?
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.
Stray file, good catch!
@@ -0,0 +1,354 @@ | |||
// Copyright 2016 The Rust Project Developers. See the COPYRIGHT |
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.
Is this module included in sys/unix/ext
or is this a stray file?
I'd personally be ok with this as-is, but I'll leave approving to @sfackler |
@bors r+ |
📌 Commit 0b56e7f has been approved by |
Unix sockets on redox This is done using the ipcd daemon. It's not exactly like unix sockets because there is not actually a physical file for the path, but it's close enough for a basic implementation :) This allows mio-uds and tokio-uds to work with a few modifications as well, which is exciting!
💔 Test failed - status-travis |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
1 similar comment
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Looks like network errors, shouldn't be related to my changes |
Unix sockets on redox This is done using the ipcd daemon. It's not exactly like unix sockets because there is not actually a physical file for the path, but it's close enough for a basic implementation :) This allows mio-uds and tokio-uds to work with a few modifications as well, which is exciting!
☀️ Test successful - status-appveyor, status-travis |
Should I make a stabilization issue/PR for unix_socket_redox? I have no idea how this stuff works, this is my first PR |
For platform-specific modules there's not really much process, and especially if these APIs match the rest of libstd then it should be fine to just send a PR to stabilize them |
Awesome, thanks 😄 |
This is done using the ipcd daemon. It's not exactly like unix sockets because there is not actually a physical file for the path, but it's close enough for a basic implementation :)
This allows mio-uds and tokio-uds to work with a few modifications as well, which is exciting!