-
Notifications
You must be signed in to change notification settings - Fork 654
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
Support for socket options to get/set TOS/TTL info #2203
base: master
Are you sure you want to change the base?
Conversation
Hi! Thanks for your interest in contributing to Nix! :) A libc with that patch hasn't been released yet, so we have to wait. And a CHANGELOG entry is needed, please see CONTRIBUTING.md on how to add one. |
@SteveLauC Thanks! I will convert this to a draft PR until libc has been released. Will also update CHANGELOG. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting feature. Would it be possible to add a test?
Hi @larseggert, we now use the libc dependency from git, so you don't need to patch it yourself, rebasing your branch should make things work |
time-to-live (TTL) information, and IPv6 traffic class (TC) and hop count information.
Hi, sorry for the late response, I think I will take another look at this PR tomorrow:) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, I have left some comments:)
And, since IpDontFrag
is also supported in this PR, it will be good if we can have a test for this option.
And, since
IpDontFrag
is also supported in this PR
This change should be recorded in the changelog
, something like:
- Add
IpDontFrag
for Linux/Android/FreeBSD - Add
Ipv6DontFrag
for FreeBSD
src/sys/socket/sockopt.rs
Outdated
#[cfg(any(apple_targets, linux_android, target_os = "freebsd",))] | ||
#[cfg(feature = "net")] | ||
sockopt_impl!( | ||
/// Retrieve the current time-to-live field for every | ||
/// IPv4 packet received on this socket. | ||
IpTtl, | ||
Both, | ||
libc::IPPROTO_IP, | ||
libc::IP_TTL, | ||
libc::c_int | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is an Ipv4Ttl
above this, and they seem to be the same thing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that was there already, but the Ipv4Ttl
name is unfortunate, because it's not obvious that this is the socket option that should be used for per-packet CMSG use. What do you prefer I do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, they are the same thing under the hood, but for different usages, then this is a really unfortunate naming, do you mind giving them some documents on their typical use cases and how they should be properly used, some examples would be great!
test/sys/test_sockopt.rs
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a test for Ipv4Ttl
(test_ttl_opts()
), which should be updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in which way? Those tests are for setting the TTL with a a sockopt for all packets, not via CMSG per packet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then let's leave it unchanged:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that we can enable that test on apple_targets?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CI failed due to the reason that Ipv6Ttl
is not enabled on apple targets, I added it in #2287, once it is merged, we can enable that test for apple targets.
Add support for getting/setting IPv4 type-of-service (TOS) and time-to-live (TTL) information, and IPv6 traffic class (TC) and hop count information.
Depends on rust-lang/libc#3450, which was recently merged.
(This is my first PR to nix, and I'm also reasonably new to Rust. Please let me know if I missed anything I should have changed; am happy to rework this.)