Skip to content
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

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

Closed
kleimkuhler opened this issue Oct 10, 2019 · 13 comments
Closed

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

kleimkuhler opened this issue Oct 10, 2019 · 13 comments
Labels
C-feature-request Category: A feature request, i.e: not implemented / a PR. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@kleimkuhler
Copy link
Contributor

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
Copy link
Contributor Author

T-Libs label is probably the correct designation.

@Centril Centril added C-feature-request Category: A feature request, i.e: not implemented / a PR. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Oct 10, 2019
@Thomasdezeeuw
Copy link
Contributor

Thomasdezeeuw commented Jan 23, 2022

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 (tokio-rs/mio#1527). We've already been through the process of incorrectly assuming the layout of the IP SocketAddr type, see tokio-rs/mio#1386 and #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
Copy link
Member

m-ou-se commented Jan 23, 2022

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)

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

@Thomasdezeeuw
Copy link
Contributor

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
Copy link
Member

m-ou-se commented Jan 23, 2022

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
Copy link
Member

m-ou-se commented Jan 23, 2022

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.

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

@Thomasdezeeuw
Copy link
Contributor

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
Copy link
Member

m-ou-se commented Jan 23, 2022

+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
Copy link
Contributor

Thomasdezeeuw commented Jan 23, 2022

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
Copy link
Contributor

Implementation pr: #93239.

@Thomasdezeeuw
Copy link
Contributor

Tracking issue for std::os::unix::net::SocketAddr::from_path: #93423.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jan 28, 2022
…r=m-ou-se

Add os::unix::net::SocketAddr::from_path

Creates a new SocketAddr from a path, supports both regular paths and
abstract namespaces.

Note that `SocketAddr::from_abstract_namespace` could be removed after this as `SocketAddr::unix` also supports abstract namespaces.

Updates rust-lang#65275
Unblocks tokio-rs/mio#1527

r? `@m-ou-se`
@Thomasdezeeuw
Copy link
Contributor

I think this can be closed in favour of #93423? Since the unix_socket_creation feature does exactly this.

@Thomasdezeeuw
Copy link
Contributor

Since #93423 is now stabilised (in 1.61) I think this can be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature-request Category: A feature request, i.e: not implemented / a PR. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants