std: Add support for SOCK_SEQPACKET Unix sockets #50348
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Based on the patch by Rick Richardson <rick.richardson@gmail.com> for the old unix-socket crate, ported to the current libstd, with some more unit tests. The main idea is to add the two new structs: * server: `UnixSeqpacketListener` similar to `UnixListener`, with bind() and accept(). * client: `UnixSeqpacket` similar to `UnixDatagram`, but only with connect(), no bind(). I've marked it all as "unstable" for now, and issue = "0" since there is no tracking issue yet.
|
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 |
|
Thanks for this PR! The bot failed to assign a reviewer for this, picking someone from the libs team. @sfackler? |
pietroalbini
added
the
S-waiting-on-review
kennytm
added
the
T-libs-api
sfackler
reviewed
May 1, 2018
src/libstd/sys/unix/ext/net.rs
Outdated
| impl UnixSeqpacketListener { | ||
| /// Creates a new `UnixSeqpacketListener` bound to the specified socket. | ||
| /// | ||
| /// Linux provides, as a nonportable extension, a separate "abstract" |
There was a problem hiding this comment.
We don't yet support abstract addresses in the stdlib implementation.
sfackler
reviewed
May 1, 2018
src/libstd/sys/unix/ext/net.rs
Outdated
| /// } | ||
| /// } | ||
| /// ``` | ||
| #[unstable(feature = "unix_socket", issue = "0")] |
There was a problem hiding this comment.
The feature name has to be new for tidy to be happy - unix_socket_seqpacket or whatever seems fine.
sfackler
reviewed
May 1, 2018
src/libstd/sys/unix/ext/net.rs
Outdated
| impl UnixSeqpacket { | ||
| /// Connects to the socket named by `path`. | ||
| /// | ||
| /// Linux provides, as a nonportable extension, a separate "abstract" |
|
LGTM other than the couple of notes above cc @rust-lang/libs |
pietroalbini
added
S-waiting-on-author
|
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 |
pietroalbini
added
S-waiting-on-review
|
Ping from triage, @sfackler — looks like all the review comments have been addressed. |
|
@bors r+ |
|
📌 Commit d30a049 has been approved by |
bors
added
S-waiting-on-bors
|
⌛ Testing commit d30a049 with merge 32bc3ed2245383ee841f1c22488875dc386d8de3... |
|
💔 Test failed - status-appveyor |
bors
added
S-waiting-on-review
|
Seems like the documentation contains invalid links on Windows. (@TimNN the error bot didn't show up here) |
|
Oops, missed those... |
|
@bors r+ |
|
📌 Commit 85cbd7e has been approved by |
bors
added
S-waiting-on-bors
|
⌛ Testing commit 85cbd7e with merge 44a40c0671d68ab7a814053f00be596e5acbdfc4... |
|
💔 Test failed - status-travis |
bors
added
S-waiting-on-review
|
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 |
|
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 |
kennytm
added
S-waiting-on-author
|
This may be blocked until we have the portability lint then, unfortunately. We don't currently have a great way to express "unix other than macOS" right now in the std APIs :( |
|
@sfackler |
|
Sure, we can make it compile, but we don't want methods to be secretly non-portable which a pure-cfg based solution would do. It doesn't seem reasonable to define identical seqpacket APIs on a separate trait in each of the linux, freebsd, openbsd, android, etc std::os modules. |
|
OK, I'll mark this S-waiting-on-team then. |
kennytm
added
I-nominated
S-waiting-on-team
|
If this is portable everywhere on Unix but osx I'd recommend just putting all this in a crate on crates.io for now (like |
|
So @rust-lang/libs, is #50348 (comment) the decision? |
|
Er sorry yes I think I forgot to comment here after triage one week, sorry about that! We discussed this at triage and the conclusion was that we'd prefer if this started out on crates.io and then if there's enough popularity to move it into the standard library we can do so probably without a non-macos module and document that it doesn't work on OSX. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hi,
this patch adds support for SOCK_SEQPACKET unix sockets to std::os::unix::net. It is based on @rrichardson's PR for the old unix-socket crate, but ported to the current libstd, with some more unit tests.
SOCK_SEQPACKETsupport was already mentioned in the RFC for unix-socket. The main idea is to add two new structs:UnixSeqpacketListenersimilar toUnixListenerwith bind() and accept() for the server-side.UnixSeqpacketsimilar toUnixDatagrambut only with connect(), no bind(), for the client-side. This provides send() and recv() methods, but not send_to()/recv_from() since it's connection-oriented, and also no io::Read/io::Write, due to SOCK_SEQPACKET reads/writes transferring one message at a time, as opposed to being a byte stream.Without this feature in std, programs have to use plain unsafe libc directly to open the socket, and then possibly use the FromRawFd constructors of UnixDatagram/UnixListener and stick to the methods that make sense for SOCK_SEQPACKET.
I'm not sure how to handle the stability attributes for this - it's unstable, but can it be assigned to the unix_socket feature, or does it need a new feature name and tracking issue?