Skip to content

Make the unix::net::SocketAddr creation methods public #65255

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

Conversation

kleimkuhler
Copy link
Contributor

@kleimkuhler kleimkuhler commented Oct 10, 2019

Motivation

Recently tokio-rs/mio needed support for creating the
std::os::unix::net::SocketAddr type. While the type is currently public, it is
not possible to create one as both creation methods are private:

The workaround was creating a mio specific SocketAddr that is equivalent to
this type.

A use of needing SocketAddr::from_parts is shown here.

Details

This PR makes the creation methods public so that the already public type can be
created as well. Comments with examples have been added.

I did not feel an RFC process was needed for this; I hope that assumtion is
correct!

Signed-off-by: Kevin Leimkuhler kleimkuhler@icloud.com

Signed-off-by: Kevin Leimkuhler <kleimkuhler@icloud.com>
@rust-highfive
Copy link
Contributor

r? @withoutboats

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 10, 2019
@rust-highfive
Copy link
Contributor

The job mingw-check of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
2019-10-10T00:42:14.6808276Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-10-10T00:42:14.6904399Z ##[command]git config gc.auto 0
2019-10-10T00:42:14.6958600Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-10-10T00:42:14.7022710Z ##[command]git config --get-all http.proxy
2019-10-10T00:42:14.7170264Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/65255/merge:refs/remotes/pull/65255/merge
---
2019-10-10T00:47:45.7181257Z     Checking panic_abort v0.0.0 (/checkout/src/libpanic_abort)
2019-10-10T00:47:45.7675741Z     Checking rustc-std-workspace-alloc v1.99.0 (/checkout/src/tools/rustc-std-workspace-alloc)
2019-10-10T00:47:46.7067022Z     Checking panic_unwind v0.0.0 (/checkout/src/libpanic_unwind)
2019-10-10T00:47:46.9680870Z     Checking backtrace v0.3.37
2019-10-10T00:47:51.4385764Z error: item has missing stability attribute
2019-10-10T00:47:51.4387139Z     |
2019-10-10T00:47:51.4387139Z     |
2019-10-10T00:47:51.4388324Z 126 | /     pub fn new<F>(f: F) -> io::Result<SocketAddr>
2019-10-10T00:47:51.4388989Z 127 | |         where F: FnOnce(*mut libc::sockaddr, *mut libc::socklen_t) -> libc::c_int
2019-10-10T00:47:51.4390015Z 129 | |         unsafe {
2019-10-10T00:47:51.4390472Z ...   |
2019-10-10T00:47:51.4390940Z 134 | |         }
2019-10-10T00:47:51.4391431Z 135 | |     }
2019-10-10T00:47:51.4391431Z 135 | |     }
2019-10-10T00:47:51.4391991Z     | |_____^
2019-10-10T00:47:51.4392134Z 
2019-10-10T00:47:51.4396202Z error: item has missing stability attribute
2019-10-10T00:47:51.4397256Z     |
2019-10-10T00:47:51.4397256Z     |
2019-10-10T00:47:51.4398187Z 149 | /     pub fn from_parts(addr: libc::sockaddr_un, mut len: libc::socklen_t) -> io::Result<SocketAddr> {
2019-10-10T00:47:51.4398787Z 150 | |         if len == 0 {
2019-10-10T00:47:51.4399360Z 151 | |             // When there is a datagram from unnamed unix socket
2019-10-10T00:47:51.4399886Z 152 | |             // linux returns zero bytes of address
2019-10-10T00:47:51.4400851Z 162 | |         })
2019-10-10T00:47:51.4401508Z 163 | |     }
2019-10-10T00:47:51.4402039Z     | |_____^
2019-10-10T00:47:51.4402177Z 
---
2019-10-10T00:47:51.6274575Z == clock drift check ==
2019-10-10T00:47:51.6289455Z   local time: Thu Oct 10 00:47:51 UTC 2019
2019-10-10T00:47:51.7157010Z   network time: Thu, 10 Oct 2019 00:47:51 GMT
2019-10-10T00:47:51.7160460Z == end clock drift check ==
2019-10-10T00:47:52.8525106Z ##[error]Bash exited with code '1'.
2019-10-10T00:47:52.8568167Z ##[section]Starting: Checkout
2019-10-10T00:47:52.8570144Z ==============================================================================
2019-10-10T00:47:52.8570200Z Task         : Get sources
2019-10-10T00:47:52.8570248Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

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 @TimNN. (Feature Requests)

@jonas-schievink
Copy link
Contributor

These need #[unstable] attributes

@jonas-schievink jonas-schievink added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Oct 10, 2019
@KodrAus
Copy link
Contributor

KodrAus commented Oct 17, 2019

Hmm, in their current state I don’t think the new or from_parts methods are really suitable in the public API because they expose external libc types, and I find the signature for new a little surprising as anything but an implementation detail.

It looks like we do have some examples of defining libc types with friendlier names though, like RawPthread. The sockaddr_un type isn’t just a simple alias though.

So I think we’ll need to sketch out a more suitable public API for constructing SocketAddrs than what’s currently there first, that doesn’t publicly use libc and has appropriate safety.

@sfackler
Copy link
Member

Why can't mio just use the standard library's UnixListener? https://doc.rust-lang.org/std/os/unix/net/struct.UnixListener.html

@kleimkuhler
Copy link
Contributor Author

@sfackler It is mainly because accept on Socket which UnixListener::accept dispatches to does not set the O_NONBLOCK flag. It can be done with a separate fcntl call after the socket is returned, but then that is an additional call that needs to take place. We can use accept4 on the OSs that support it and set these flags atomically.

@kleimkuhler
Copy link
Contributor Author

@KodrAus That is a good point. I found the signatures a little surprising when first looking at them. If exposing libc types is an issue, there would definitely need to be some additional design around the fields returned in SocketAddr. I can close this PR for now if additional discussion should take place in the issue? It would be nice to use libstd when possible, but this understandably isn't going to be solved just by making these methods public.

@KodrAus
Copy link
Contributor

KodrAus commented Oct 18, 2019

@kleimkuhler that sounds like a good plan me to! Maybe we can also look at whether it’s feasible for mio to move to the standard UnixListener and what might need to be added to support it?

@kleimkuhler
Copy link
Contributor Author

@KodrAus So it's not just UnixListener that mio has to special case all this for, it's pretty much all the net resources that would be expected to not block when no read/write is available.

This ultimately is because the sockets need this additional flag set as part of the socket creation instead of a separate call to libstd's set_nonblocking. Maybe there is some design discussion for offering this through libstd, but it definitely would be a larger change!

Closing this PR per discussion above and further conversation can take place in #65275.

@kleimkuhler kleimkuhler deleted the kleimkuhler/unix-socketaddr-pub-create branch October 18, 2019 18:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants