Skip to content

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.

@lucab lucab force-pushed the ups/sockaddr-target branch from 12d5524 to d513ca2 Compare November 14, 2022 11:22
@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.

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.
@lucab lucab force-pushed the ups/sockaddr-target branch from f251907 to 933427c Compare August 8, 2023 10:16
@Thomasdezeeuw Thomasdezeeuw merged commit d937493 into tokio-rs:master Aug 8, 2023
@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