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

Fix UnixStream::pair and UnixDatagram::pair on all platforms #1142

Merged

Conversation

@kleimkuhler
Copy link
Contributor

kleimkuhler commented Nov 11, 2019

#1141 Addressed an issue with UnixStream::pair and UnixDatagram::pair
where the file descriptor array passed in to pair_descriptors was copied in
instead of passed by reference. Even if pair_descriptors succeded, the file
descriptor array used to create the socket pairs was the original array
initialized to [-1; 2].

The issue still remains for target platforms where additional fcntl sys
calls must be made. The sockets are created before the call to
pair_descriptors, but a successful return does not actually affect the
sockets.

Ultimately, both of the pair methods and pair_descriptors function were
incorrect. I feel pair_descriptors role in trying to reduce code duplication
while being generic over UnixStream and UnixDatagram adds some uncessary
complexity without actually resulting in less code.

Solution

The pair methods are now separate and no longer dispatch to a generic
function. The sockets are created after the required sys call. The fcntl
calls are made after socket creation to allow for cleanup.

Tests have been added for these specific methods.

A final PR separate from this will expand all UDS tests so that it is more
heavily tested.

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

kleimkuhler added 3 commits Nov 10, 2019
Signed-off-by: Kevin Leimkuhler <kleimkuhler@icloud.com>
Signed-off-by: Kevin Leimkuhler <kleimkuhler@icloud.com>
Signed-off-by: Kevin Leimkuhler <kleimkuhler@icloud.com>
@kleimkuhler kleimkuhler mentioned this pull request Nov 11, 2019
@kleimkuhler kleimkuhler requested a review from Thomasdezeeuw Nov 12, 2019
Copy link
Collaborator

Thomasdezeeuw left a comment

Two small nits, LGTM otherwise.

tests/uds.rs Show resolved Hide resolved
tests/uds.rs Show resolved Hide resolved
Signed-off-by: Kevin Leimkuhler <kleimkuhler@icloud.com>
@kleimkuhler

This comment has been minimized.

Copy link
Contributor Author

kleimkuhler commented Nov 12, 2019

@Thomasdezeeuw Thanks for the review. I added the readable event checks!

@kleimkuhler kleimkuhler merged commit cc1fd15 into tokio-rs:master Nov 12, 2019
22 checks passed
22 checks passed
FreeBSD 11.2 amd64 Task Summary
Details
tokio-rs.mio Build #20191112.7 succeeded
Details
tokio-rs.mio (Check rustfmt) Check rustfmt succeeded
Details
tokio-rs.mio (Clippy) Clippy succeeded
Details
tokio-rs.mio (Cross Android) Cross Android succeeded
Details
tokio-rs.mio (Cross Android_32) Cross Android_32 succeeded
Details
tokio-rs.mio (Cross Android_ARM64) Cross Android_ARM64 succeeded
Details
tokio-rs.mio (Cross NetBSD) Cross NetBSD succeeded
Details
tokio-rs.mio (Cross Solaris) Cross Solaris succeeded
Details
tokio-rs.mio (Cross iOS_32) Cross iOS_32 succeeded
Details
tokio-rs.mio (Cross iOS_64) Cross iOS_64 succeeded
Details
tokio-rs.mio (Cross iOS_ARM) Cross iOS_ARM succeeded
Details
tokio-rs.mio (Min Rust Linux) Min Rust Linux succeeded
Details
tokio-rs.mio (Min Rust MacOS) Min Rust MacOS succeeded
Details
tokio-rs.mio (Min Rust Windows) Min Rust Windows succeeded
Details
tokio-rs.mio (Minimal versions Linux) Minimal versions Linux succeeded
Details
tokio-rs.mio (Minimal versions Windows) Minimal versions Windows succeeded
Details
tokio-rs.mio (Nightly Linux) Nightly Linux succeeded
Details
tokio-rs.mio (Test --release Linux) Test --release Linux succeeded
Details
tokio-rs.mio (Test Linux) Test Linux succeeded
Details
tokio-rs.mio (Test MacOS) Test MacOS succeeded
Details
tokio-rs.mio (Test Windows) Test Windows succeeded
Details
@kleimkuhler kleimkuhler deleted the kleimkuhler:kleimkuhler/unixstream-pair-fix branch Nov 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.