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.

Sign up for GitHub

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

Jump to bottom

Initial basic cmsg support for unix #316

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Initial basic cmsg support for unix #316

wants to merge 4 commits into from

Conversation

brunowonka
Copy link

Also added set_recv_ttl while I'm here to use in testing. The other RECVxxx-type socket options might make more sense with control message support now.

I went back and forth on the shape of the public API for this quite a bit, and I also don't have access to a Windows machine to check.

Fixes #313

@brunowonka brunowonka mentioned this pull request Jun 23, 2022
Open
Thomasdezeeuw
Thomasdezeeuw requested changes Jul 2, 2022
View reviewed changes
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.

I'm afraid this doesn't work at all on a number of platform, please see the CI failure. I'll do another round of review once those are fixed.

src/socket.rs Outdated Show resolved Hide resolved
src/socket.rs Outdated Show resolved Hide resolved
src/socket.rs Outdated Show resolved Hide resolved
src/cmsg.rs
use std::iter::FromIterator;

#[derive(Debug, Clone)]
struct MsgHdrWalker<B> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's remove the generic parameter, we only need &[u8], same for the other structures.

Copy link
Author

Choose a reason for hiding this comment

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

Tentatively keeping this parameter so we can reuse some logic between &[u8] and &mut [u8], but removed all the type parameters from the public API for now.

src/sys/unix.rs Outdated Show resolved Hide resolved
src/sys/unix.rs Outdated Show resolved Hide resolved
src/sys/unix.rs Outdated Show resolved Hide resolved
src/sys/unix.rs Outdated Show resolved Hide resolved
src/sys/unix.rs Outdated Show resolved Hide resolved
@brunowonka
Copy link
Author

Thank you for looking, I'm working through the other platform problems here and I'll push as soon as I have things working so you can look through.

chansuke
chansuke reviewed Jul 13, 2022
View reviewed changes
src/cmsg.rs Outdated Show resolved Hide resolved
Thomasdezeeuw
Thomasdezeeuw reviewed Jul 14, 2022
View reviewed changes
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.

Not a complete review, but a start.

tests/socket.rs Show resolved Hide resolved
src/socket.rs Outdated Show resolved Hide resolved
src/socket.rs Show resolved Hide resolved
src/socket.rs Show resolved Hide resolved
src/socket.rs Outdated Show resolved Hide resolved
@brunowonka
Address review comments in Cmsg
8ebf5ab 
- Split recv_msg from recv_msg_from.
- Introduce CmsgBuffer type.
@darconeous
Copy link

Looking at this failure:

Run cargo install cargo-hack
    Updating crates.io index
 Downloading crates ...
  Downloaded cargo-hack v0.5.15
error: failed to parse manifest at `/home/runner/.cargo/registry/src/github.com-1ecc6299db9ec[8](https://github.com/rust-lang/socket2/runs/7569907645?check_suite_focus=true#step:4:9)23/cargo-hack-0.5.15/Cargo.toml`

Caused by:
  failed to parse the `edition` key

Caused by:
  this version of Cargo is older than the `2021` edition, and only supports `2015` and `2018` editions.
Error: Process completed with exit code [10](https://github.com/rust-lang/socket2/runs/7569907645?check_suite_focus=true#step:4:11)1.

Looks unrelated to this change?

The macOS failure looks real though:

running 36 tests
...snip...
test send_from_recv_to_vectored ... FAILED
test send_recv_vectored ... FAILED
...snip...

failures:
error: test failed, to rerun pass '--test socket'

error: process didn't exit successfully: `/Users/runner/.rustup/toolchains/stable-x86_64-apple-darwin/bin/cargo test --manifest-path Cargo.toml --no-default-features` (exit status: 101)
---- send_from_recv_to_vectored stdout ----
thread 'send_from_recv_to_vectored' panicked at 'called `Result::unwrap()` on an `Err` value: Os { code: 22, kind: InvalidInput, message: "Invalid argument" }', tests/socket.rs:[57](https://github.com/rust-lang/socket2/runs/7569907933?check_suite_focus=true#step:5:58)4:10

@keepsimple1 keepsimple1 mentioned this pull request Feb 29, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chansuke chansuke chansuke left review comments

@Thomasdezeeuw Thomasdezeeuw Thomasdezeeuw requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support ancillary data over sendmsg, recvmsg
4 participants
@brunowonka @darconeous @chansuke @Thomasdezeeuw