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 SockProtocol variants for ICMP{,v6} #2103

Merged
merged 1 commit into from
Oct 1, 2023

Conversation

gnaaman-dn
Copy link
Contributor

@gnaaman-dn gnaaman-dn commented Aug 22, 2023

CanRaw/KextEvent have the same value as ICMP, which means one of them has to be a constant.
I chose to make CanRaw & KextEvent constants since as far as I can tell, Icmp is always present (similar to Udp/Tcp, etc).

I started writing a test that sends an Echo-Request, but opening an ICMP/Raw socket requires root permissions, so I'm not sure if it's fit as a test.

@gnaaman-dn gnaaman-dn force-pushed the add-icmp branch 5 times, most recently from 4c95612 to bc52d14 Compare August 22, 2023 20:15
Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

It's totally possible to write a test that requires privileges. In fact, several existing tests already do. See test_so_type_unknown for an example that uses raw sockets.

src/sys/socket/mod.rs Outdated Show resolved Hide resolved
src/sys/socket/mod.rs Outdated Show resolved Hide resolved
@gnaaman-dn gnaaman-dn force-pushed the add-icmp branch 4 times, most recently from 225de32 to de9361a Compare August 29, 2023 11:41
@rcloran
Copy link

rcloran commented Sep 30, 2023

👍 These also work for creating unprivileged ICMP{,v6} sockets using SOCK_DGRAM on Linux and macOS -- not sure if that helps the testing situation at all (sounds like not really)? Looks like work has stalled a tiny bit -- anything I can do to help get this over the line?

@SteveLauC
Copy link
Member

anything I can do to help get this over the line?

Bump received, thanks!


I started writing a test that sends an Echo-Request, but opening an ICMP/Raw socket requires root permissions, so I'm not sure if it's fit as a test.

Friendly ping @asomers, can I trouble you for another review on this PR, it seems that the test using raw socket is done, and the request changes are resolved


hi, @gnaaman-dn Would you like to rebase your branch to resolve the conflict, thanks!

@gnaaman-dn
Copy link
Contributor Author

Sure can :)

@asomers asomers added this pull request to the merge queue Oct 1, 2023
Merged via the queue into nix-rust:master with commit 173bde1 Oct 1, 2023
35 checks passed
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.

None yet

4 participants