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

net/uds: add methods to connect/bind with a socket address #1630

Merged
merged 2 commits into from
Aug 8, 2023

Conversation

lucab
Copy link
Contributor

@lucab lucab commented Nov 14, 2022

This enhances connect and bind logic for Unix Domain Sockets (UDS), by adding methods which allow to directly use a socket address. This mirrors similar features which already exist in mio for TCP and UDP sockets.

@Thomasdezeeuw
Copy link
Collaborator

I assume this is based on rust-lang/rust#85410, which seems to be on it's way to stable.

@lucab
Copy link
Contributor Author

lucab commented Nov 14, 2022

@Thomasdezeeuw yes, kinda. My final goal would be to use abstract UDS in tokio (and higher up). For that I'm really looking forward to SocketAddr::from_abstract_namespace(&[u8]), which is still currently moving a bit. So maybe later, I'm not in a hurry.

I started with this which only tackles the connect/bind side, as it seemed quite safe to do and I noticed that TCP/UDP already had them.

@Thomasdezeeuw
Copy link
Collaborator

@Thomasdezeeuw yes, kinda. My final goal would be to use abstract UDS in tokio (and higher up). For that I'm really looking forward to SocketAddr::from_abstract_namespace(&[u8]), which is still currently moving a bit. So maybe later, I'm not in a hurry.

I think you can use socket2 in Tokio, that already support abstract namespaces.

I started with this which only tackles the connect/bind side, as it seemed quite safe to do and I noticed that TCP/UDP already had them.

@lucab
Copy link
Contributor Author

lucab commented Nov 14, 2022

The CI here is running clippy linting without a pinned version of the toolchain, which means new warnings appearing in newer toolchain versions will silently break master and block unrelated PRs.

I did add a commit on top here to fix the new clippy warnings, but you may consider pinning the linting toolchain too.

@Thomasdezeeuw
Copy link
Collaborator

The CI here is running clippy linting without a pinned version of the toolchain, which means new warnings appearing in newer toolchain versions will silently break master and block unrelated PRs.

It's really breaking anything, it's just pointing out areas of (possible) improvement.

I did add a commit on top here to fix the new clippy warnings

👍

but you may consider pinning the linting toolchain too.

We only pin the MSRV, not the maximum.

@lucab
Copy link
Contributor Author

lucab commented Nov 18, 2022

Just asking explicitly, what are the next steps here? Is this additional API fine here in mio or was the comment above suggesting to directly put this in tokio instead?

@Thomasdezeeuw
Copy link
Collaborator

Just asking explicitly, what are the next steps here? Is this additional API fine here in mio

Yes.

or was the comment above suggesting to directly put this in tokio instead?

Yes. :)

Basically, Mio follows the std lib, so I think we should wait until the API at least on the road of stabilisation in std lib. Then we match the api and aren't stuck with a different API due to a last minute name change in std lib, after we already made a release with the function.

So, if you want in Tokio today your best bet is to use socket2 directly their. But I don't decide what does and doesn't go in the Tokio api.

@lucab
Copy link
Contributor Author

lucab commented Nov 18, 2022

Ack, thanks for the feedback. I'll leave this open here for the moment then, and revisit once stdlib stabilization is done.

I'll go check Tokio next, and see if they like a socket2 implementation there. I can then hopefully switch that to use this logic directly from mio, later on.

@lucab lucab force-pushed the ups/sockaddr-target branch 2 times, most recently from 9a343a6 to f251907 Compare August 5, 2023 10:12
@lucab
Copy link
Contributor Author

lucab commented Aug 5, 2023

I got busy with many other things in life, but the good news is that in the meanwhile bind_addr() and connect_addr() got stabilized in Rust 1.70. The additional methods in this patch already use the same signatures.

I've rebased and de-conflicted this PR, it should be ready for reviewing and merging, PTAL.

Copy link
Collaborator

@Thomasdezeeuw Thomasdezeeuw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One question, otherwise LGTM.

tests/unix_stream.rs Outdated Show resolved Hide resolved
lucab and others added 2 commits August 8, 2023 12:15
This enhances `connect` and `bind` logic for Unix Domain Sockets
(UDS), by adding methods which allow to directly use a socket
address. This mirrors similar features which already exist in
`mio` for TCP and UDP sockets.
@Thomasdezeeuw Thomasdezeeuw merged commit d937493 into tokio-rs:master Aug 8, 2023
19 checks passed
@Thomasdezeeuw
Copy link
Collaborator

Thanks @lucab

@lucab lucab deleted the ups/sockaddr-target branch August 8, 2023 11:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants