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

Add MSG_CONFIRM and MSG_DONTROUTE to RecvFlags #499

Merged
merged 1 commit into from
Mar 12, 2024

Conversation

pd0wm
Copy link
Contributor

@pd0wm pd0wm commented Mar 12, 2024

This is useful when reading from SocketCAN sockets. The MSG_CONFIRM flag is used to indicate a successful transmission of a CAN frame.

Reference: https://www.kernel.org/doc/html/next/networking/can.html#raw-socket-option-can-raw-join-filters

Is there any reason the raw flags value is not exposed to the user? There might be more drivers that are using non-standard flags to communicate a certain state.

@Thomasdezeeuw
Copy link
Collaborator

Is there any reason the raw flags value is not exposed to the user? There might be more drivers that are using non-standard flags to communicate a certain state.

We're trying to provide a cross platform API, but for Windows RecvFlags is completely fake.

@pd0wm pd0wm changed the title Add MSG_CONFIRM to RecvFlags Add MSG_CONFIRM and MSG_DONTROUTE to RecvFlags Mar 12, 2024
@pd0wm
Copy link
Contributor Author

pd0wm commented Mar 12, 2024

Looks like the build is failing due to missing fields for some targets. Shall I limit the new functions to target_os = "linux"?

@Thomasdezeeuw
Copy link
Collaborator

Looks like the build is failing due to missing fields for some targets. Shall I limit the new functions to target_os = "linux"?

It's it's Linux only then yes. But it will require the all feature as well.

@pd0wm
Copy link
Contributor Author

pd0wm commented Mar 12, 2024

Should be supported on Android as well. Is dc71380 what you mean by requiring both the all feature and gating on the os?

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.

Is there a way we can get tests for this? I know the code is fairly trivial, but would be nice to know it works.

Otherwise the code LGTM, minus some small docs nits.

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

pd0wm commented Mar 12, 2024

I think MSG_CONFIRM is normally used as a flag for calling sendmsg, and not used on received messages. So not sure if I can trigger this with an end-to-end test without using SocketCAN. SocketCAN cannot be easily tested inside a docker container, as the host needs to create a virtual SocketCAN device first.

Should I add the flags to the impl std::fmt::Debug as well? https://github.com/rust-lang/socket2/blob/master/src/sys/unix.rs#L579-L589

I can add a test that the value is false under normal circumstances here: https://github.com/rust-lang/socket2/blob/master/tests/socket.rs#L685-L686

@pd0wm pd0wm force-pushed the msg-config branch 3 times, most recently from 1f8679d to 5c60e48 Compare March 12, 2024 16:27
@Thomasdezeeuw
Copy link
Collaborator

Should I add the flags to the impl std::fmt::Debug as well? https://github.com/rust-lang/socket2/blob/master/src/sys/unix.rs#L579-L589

I can add a test that the value is false under normal circumstances here: https://github.com/rust-lang/socket2/blob/master/tests/socket.rs#L685-L686

Yes to both. After that we can merge.

@pd0wm
Copy link
Contributor Author

pd0wm commented Mar 12, 2024

Should I add the flags to the impl std::fmt::Debug as well? https://github.com/rust-lang/socket2/blob/master/src/sys/unix.rs#L579-L589
I can add a test that the value is false under normal circumstances here: https://github.com/rust-lang/socket2/blob/master/tests/socket.rs#L685-L686

Yes to both. After that we can merge.

Already done :)

@Thomasdezeeuw Thomasdezeeuw merged commit c93cdcc into rust-lang:master Mar 12, 2024
41 checks passed
@Thomasdezeeuw
Copy link
Collaborator

Thanks @pd0wm

@pd0wm
Copy link
Contributor Author

pd0wm commented Mar 12, 2024

Thanks for your quick feedback!

@pd0wm pd0wm deleted the msg-config branch March 12, 2024 16:38
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