Add UDS support for Unix systems #1098

kleimkuhler · 2019-10-01T17:42:13Z

Summary

This change adds UDS support for Unix systems. The networking primitives
src/net/uds. These modules provide the structs for UnixDatagrams,
UnixListeners, and UnixStreams. Each of the methods on these structs
dispatch to the Unix system structs in src/sys/unix/uds.rs.

Use of these new structs should feel fairly similar to the Tcp[...]
equivalents. The tests added also reflect the similar behavior that would be
expected.

Details

Since this change only adds UDS support, there should be no behavior changes
anywhere else. For reviewing, I recommend starting from the networking
primitives and seeing what methods they dispatch to.

The tests added in tests/uds.rs should feel fairly similar to
tcp_stream.rs--the main difference coming from supporting PathBufs as the
socket address to connect to.

I originally pulled in some testing infrastructure added to v0.6.x in #944,
but ultimately found the functionality to not be as helpful due to a lot of
changes that have since taken place in master. I think #944 could still be
helpful, but as a separate change that is more about improving testing
functionality and not included in a change that introduces additional
functionality.

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

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

asomers · 2019-10-01T23:28:16Z

How does this PR compare to alexchrichton/mio-uds ?

src/net/uds/datagram.rs

src/sys/unix/uds.rs

src/net/uds/listener.rs

carllerche · 2019-10-02T00:28:11Z

@asomers this is updating the UDS implementation to the latest mio. It also moves it in tree because std now supports UDS and moving it in tree reduces the number of dependencies.

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

Thomasdezeeuw

Overall this looks good, I've add a couple of comments but there are mostly small nits.

I haven't had time to look at the tests yet.

src/net/uds/datagram.rs

src/sys/unix/uds.rs

src/net/uds/stream.rs

src/net/uds/listener.rs

src/net/uds/stream.rs

src/lib.rs

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

This introduced problems cross-platform Signed-off-by: Kevin Leimkuhler <kleimkuhler@icloud.com>

kleimkuhler · 2019-10-02T23:36:18Z

Same as for UnixDatagram and UnixListener:

std -> sys,

std::os::unix::net::UnixListener -> net::UnixListener, and

add SelectorId.

@Thomasdezeeuw I'm not sure I follow all the parts of this comment in UnixStream, UnixDatagram, and UnixListener.

I can rename the std field to sys, and I have added the selector_id field for debug assertions.

I'm not sure I follow std::os::unix::net::UnixListener -> net::UnixListener. These types are not available in std::net, they are only available through these paths and must be imported directly.

This commit introduces a significant restructure from the original UDS support proposal. Originally, the UDS types dispatched the the `std` UDS types from the `net` module. This was different from other `net` module types because it did not use the `sys` modules types for that OS. This was okay because currently UDS is only supported by Unix, but when windows support is added this would have been a problem. The new structure sets up most of the work for Windows UDS support as the `net` module UDS types now all dispatch to their inner `sys` module types. Signed-off-by: Kevin Leimkuhler <kleimkuhler@icloud.com>

kleimkuhler · 2019-10-03T05:31:12Z

@Thomasdezeeuw I think most of your comments should be addressed from the last commit. Below is my commit message:

This commit introduces a significant restructure from the original UDS
support proposal. Originally, the UDS types dispatched the the std UDS
types from the net module. This was different from other net module
types because it did not use the sys modules types for that OS.

This was okay because currently UDS is only supported by Unix, but when
windows support is added this would have been a problem.

The new structure sets up most of the work for Windows UDS support as
the net module UDS types now all dispatch to their inner sys module
types.

I have left all your comments from your last review that had to do with this unresolved, but I think they should all be addressed at this point.

src/sys/unix/uds/mod.rs

src/net/uds/datagram.rs

Thomasdezeeuw

This looks good to me, thanks @kleimkuhler.

tests/uds.rs

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

This did not work across all plaforms :( This reverts commit 9d9cbc6.

Thomasdezeeuw · 2019-10-03T19:37:14Z

Since this change is so large, can some double check this and merge if acceptable?

carllerche · 2019-10-07T16:09:56Z

src/sys/unix/uds/listener.rs

+///
+/// [`net::SocketAddr`]: std::os::unix::net::SocketAddr
+/// [`accept`]: #method.accept
+pub struct SocketAddr {


Let's not do it in this PR, but as a follow up it would be nice to move this type to a separate file.

carllerche · 2019-10-07T16:13:38Z

src/net/uds/datagram.rs

+use std::io;
+use std::net::Shutdown;
+use std::os::unix::io::{AsRawFd, FromRawFd, IntoRawFd, RawFd};
+use std::os::unix::net::SocketAddr;


It looks like we are using the std version of SocketAddr here. Should we focus on only using mios version?

carllerche · 2019-10-07T16:14:26Z

src/net/uds/listener.rs

+    }
+
+    /// Returns the local socket address of this listener.
+    pub fn local_addr(&self) -> io::Result<net::SocketAddr> {


Same: should we focus on only using mio's version of SocketAddr?

carllerche · 2019-10-07T16:14:40Z

src/net/uds/stream.rs

+use std::io::{self, IoSlice, IoSliceMut};
+use std::net::Shutdown;
+use std::os::unix::io::{AsRawFd, FromRawFd, IntoRawFd, RawFd};
+use std::os::unix::net::SocketAddr;


Same: should we focus on only using mio's version of SocketAddr?

carllerche

Looks great to me. One thought, which I added inline, should we only use mio's definition of SocketAddr (for consistency)? Again, if/when std allows external construction of SocketAddr, we can switch all at once.

kleimkuhler · 2019-10-07T16:43:36Z

@carllerche I had limited the use of mio's SocketAddr because it had felt more like an inconvenience that libstd's SocketAddr was not public; I leaned towards using it in the least amount of places possible.

That's a good point though about using it everywhere for now and changing over all at once when we can. I'll make that change!

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

kleimkuhler · 2019-10-07T23:31:31Z

@carllerche Had to introduce some further mio::unix::SocketAddr functionality to return it everywhere it is used, but net::SocketAddr is no longer used now.

Also fixed a cfg in the UDS tests that was not actually running them correctly.

carllerche

Thanks for the work and taking this to completion 👍

carllerche and others added 20 commits September 27, 2019 14:13

Add support for Unix Domain Sockets

2fb7adc

wip

7b9d8f8

Implement rest of UDS functions

328ca69

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

Add first pass for UDS accept

d423ba9

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

Fix SocketAddr issue for now

19d3b0f

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

Add SocketAddr impl and cleanup UDS functions

aed7a7a

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

Add SocketAddr debug impl

06e79d3

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

Fix macos target_os build

b388647

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

Remove unncessary socket_type

a2b1468

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

Add initial accept test

0dc16bf

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

Add more HUP tests (tokio-rs#944)

190f041

Add working listen, connect, and read tests

08b7fd3

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

Add more tests and vectored reads/writes

aa83c12

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

Add more tests

a08ca2b

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

Cleanup tests

911d629

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

Remove picked test file

5afb6ba

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

Fix mac build

cfbf4e1

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

Add macro exports

9adcfdc

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

Consolidate pair functions

c518d7f

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

Small change

cebee4b

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

kleimkuhler requested review from carllerche and Thomasdezeeuw October 1, 2019 17:42

kleimkuhler self-assigned this Oct 1, 2019

kleimkuhler added 5 commits October 1, 2019 11:05

Format changes

0b9532c

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

Skip accept4 for solaris and netbsd

7b896f8

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

Fix pointer cast

e2e297c

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

Use libc::c_char in pointer cast

21e8c7f

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

Remove unncessary slice function

52cc394

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

hawkw reviewed Oct 1, 2019

View reviewed changes

src/sys/unix/uds.rs Outdated Show resolved Hide resolved

src/net/uds/listener.rs Outdated Show resolved Hide resolved

Reduce unsafe usage and add a few small fixes

80d559c

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

Thomasdezeeuw reviewed Oct 2, 2019

View reviewed changes

kleimkuhler added 3 commits October 2, 2019 15:41

Add SelectorId to debug_assertions

44fa588

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

Remove use of MaybeUninit and small renamings

6256d7b

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

Add back use of MaybeUnint for libc::sockaddr_un struct initialization

9976dcc

This introduced problems cross-platform Signed-off-by: Kevin Leimkuhler <kleimkuhler@icloud.com>

Thomasdezeeuw reviewed Oct 3, 2019

View reviewed changes

src/sys/unix/uds/mod.rs Outdated Show resolved Hide resolved

src/net/uds/datagram.rs Outdated Show resolved Hide resolved

Thomasdezeeuw approved these changes Oct 3, 2019

View reviewed changes

tests/uds.rs Outdated Show resolved Hide resolved

tests/uds.rs Show resolved Hide resolved

kleimkuhler added 4 commits October 3, 2019 10:01

Add final review changes

2be1dcd

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

Fix cfg in test

ffb2f1c

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

Remove unsafe sockaddr init (test all platforms)

9d9cbc6

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

Revert "Remove unsafe sockaddr init (test all platforms)"

3904f10

This did not work across all plaforms :( This reverts commit 9d9cbc6.

carllerche reviewed Oct 7, 2019

View reviewed changes

kleimkuhler added 2 commits October 7, 2019 15:39

Remove use of stdlib unix::SocketAddr

a303398

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

Change UDS test cfg

3145a1e

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

kleimkuhler requested a review from carllerche October 7, 2019 23:30

carllerche approved these changes Oct 9, 2019

View reviewed changes

carllerche merged commit 9fe594e into tokio-rs:master Oct 9, 2019

carllerche mentioned this pull request Oct 22, 2019

Add maintainers and transfer ownership deprecrated/mio-uds#25

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add UDS support for Unix systems #1098

Add UDS support for Unix systems #1098

kleimkuhler commented Oct 1, 2019

asomers commented Oct 1, 2019

carllerche commented Oct 2, 2019

Thomasdezeeuw left a comment

kleimkuhler commented Oct 2, 2019

kleimkuhler commented Oct 3, 2019

Thomasdezeeuw left a comment

Thomasdezeeuw commented Oct 3, 2019

carllerche Oct 7, 2019

carllerche Oct 7, 2019

carllerche Oct 7, 2019

carllerche Oct 7, 2019

carllerche left a comment

kleimkuhler commented Oct 7, 2019

kleimkuhler commented Oct 7, 2019

carllerche left a comment

Add UDS support for Unix systems #1098

Add UDS support for Unix systems #1098

Conversation

kleimkuhler commented Oct 1, 2019

Summary

Details

asomers commented Oct 1, 2019

carllerche commented Oct 2, 2019

Thomasdezeeuw left a comment

Choose a reason for hiding this comment

kleimkuhler commented Oct 2, 2019

kleimkuhler commented Oct 3, 2019

Thomasdezeeuw left a comment

Choose a reason for hiding this comment

Thomasdezeeuw commented Oct 3, 2019

carllerche Oct 7, 2019

Choose a reason for hiding this comment

carllerche Oct 7, 2019

Choose a reason for hiding this comment

carllerche Oct 7, 2019

Choose a reason for hiding this comment

carllerche Oct 7, 2019

Choose a reason for hiding this comment

carllerche left a comment

Choose a reason for hiding this comment

kleimkuhler commented Oct 7, 2019

kleimkuhler commented Oct 7, 2019

carllerche left a comment

Choose a reason for hiding this comment