rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
96.97k stars 12.53k forks source link

request: Make `unix::net::SocketAddr` create methods public #65275

Closed kleimkuhler closed 2 years ago

kleimkuhler commented 4 years ago

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.

65255 was a quick fix at trying to add this, but it will need #[unstable] attribute and therefore be tied to an issue I believe. I wanted to open some brief conversation on if this would be okay and I can add the things missing in that PR.

kleimkuhler commented 4 years ago

T-Libs label is probably the correct designation.

Thomasdezeeuw commented 2 years ago

I like to revive this discussion. For Mio we currently have our own version of unix::net::SocketAddr, but we really like to move to the std's version before releasing v1 (https://github.com/tokio-rs/mio/issues/1527). We've already been through the process of incorrectly assuming the layout of the IP SocketAddr type, see https://github.com/tokio-rs/mio/issues/1386 and https://github.com/rust-lang/rust/pull/78802. We like to prevent repeat of that, but that would mean we need be able to create the type from outside std lib.

m-ou-se commented 2 years ago

Unfortunately, we can't just make those new and from_parts functions public, since they include types from libc in the interface, which is a private dependency of std. Some discussion starting here: https://github.com/rust-lang/rust/pull/65255#issuecomment-543169553

Would something like a SocketAddr::from_path(&Path) suffice? (Similar to SocketAddr::from_abstract_namespace.)

Thomasdezeeuw commented 2 years ago

Unfortunately, we can't just make those new and from_parts functions public, since they include types from libc in the interface, which is a private dependency of std. Some discussion starting here: #65255 (comment)

I understand we don't want to expose libc type.

Would something like a SocketAddr::from_path(&Path) suffice? (Similar to SocketAddr::from_abstract_namespace.)

That would be a nice start. If we can also have SocketAddr::from_ip(net::SocketAddr) (maybe with IPv4/v6 methods as well). Then I think all we need is AF_VSOCK, but do that later.

Note that socket2 already has all these methods/From impls for its SocketAddr type.

m-ou-se commented 2 years ago

So you want to be able to use unix::net::SocketAddr also for IP socket addresses? Mio currently uses separate types for IP and Unix addresses, right?

m-ou-se commented 2 years ago

Adding unix::net::SocketAddr::from_path and unix::net::SocketAddr::as_path seems fine to me. Feel free send a PR for those. We could then kick off an FCP for stabilization together with https://github.com/rust-lang/rust/issues/85410 quickly afterwards.

Extending unix::net::SocketAddr for non-unix addresses might be reasonable, but would require a bit more discussion.

Thomasdezeeuw commented 2 years ago

So you want to be able to use unix::net::SocketAddr also for IP socket addresses? Mio currently uses separate types for IP and Unix addresses, right?

Yes, but it would nice to easily convert from OS address storage to a Rust type address. But that can become future work.

Adding unix::net::SocketAddr::from_path and unix::net::SocketAddr::as_path seems fine to me. Feel free send a PR for those. We could then kick off an FCP for stabilization together with #85410 quickly afterwards.

👍 Will send a pr. What do you think about SocketAddr::unix and SocketAddr::as_unix to match the AF_UNIX constant?

Extending unix::net::SocketAddr for non-unix addresses might be reasonable, but would require a bit more discussion.

m-ou-se commented 2 years ago

+1 Will send a pr. What do you think about SocketAddr::unix and SocketAddr::as_unix to match the AF_UNIX constant?

from_abstract_namespace and as_abstract_namespace also use AF_UNIX, so I don't think that'd be a good name.

Thomasdezeeuw commented 2 years ago

from_abstract_namespace and as_abstract_namespace also use AF_UNIX, so I don't think that'd be a good name.

Should those be folded into SocketAddr::unix then? Because I copied the implementation from socket2, which supports abstract namespaces as well.

Thomasdezeeuw commented 2 years ago

Implementation pr: https://github.com/rust-lang/rust/pull/93239.

Thomasdezeeuw commented 2 years ago

Tracking issue for std::os::unix::net::SocketAddr::from_path: https://github.com/rust-lang/rust/issues/93423.

Thomasdezeeuw commented 2 years ago

I think this can be closed in favour of https://github.com/rust-lang/rust/issues/93423? Since the unix_socket_creation feature does exactly this.

Thomasdezeeuw commented 2 years ago

Since https://github.com/rust-lang/rust/issues/93423 is now stabilised (in 1.61) I think this can be closed.