net: add support for TCLASS option on IPv6#7781
Conversation
|
We can't rename functions. |
|
Not even when keeping the original function as deprecated? I followed the example of 28c9a14. I can drop the rename and keep the comment change, if the policy changed. |
martin-g
left a comment
martin-g
left a comment
There was a problem hiding this comment.
The proposed changes look good to me!
The semver CI check also does not find any issues.
I'd suggest to add some tests though!
I do not see any setsockopt tests in https://github.com/tokio-rs/tokio/blob/master/tokio/tests/udp.rs I guess I could check if getter returns the value passed to a setter like the underlying library does but not sure if it is worth it duplicating it here. |
I think it is's worth it. If Tokio's setter gets broken somehow and stops delegating to the underlying library then the getter will return the wrong value. I understand that it is an effort! You could wait for others' opinion on the current changes first! And I could help with the tests if you like ! |
ADD-SP
added
the
S-waiting-on-author
|
Hi @jtojnar , could you add a simple getter/setter test? |
jtojnar
changed the title
jtojnar
force-pushed
the
ipv6-tos
branch
3 times, most recently
from
76aec40 to
c03efa5
Compare
|
Added option test for all existing options, mirroring the tests in socket2 library. For some reason |
tokio/tests/udp.rs
Outdated
|
|
||
| #[cfg(not(target_os = "windows"))] | ||
| // failed to set option: Os { code: 10022, kind: InvalidInput, message: "An invalid argument was supplied." } | ||
| #[cfg(not(target_os = "macos"))] // failed to get initial value: Os { code: 22, kind: InvalidInput, message: "Invalid argument" } |
There was a problem hiding this comment.
It seems the same is needed for freebsd too - https://github.com/tokio-rs/tokio/pull/7781/checks?check_run_id=59027543250
There was a problem hiding this comment.
Weirdly, it fails even on Linux on CI for socket2 library so not sure if I should even include it here. Edit: Never mind, that was because I accidentally used SOCK_STREAM instead of SOCK_DGRAM in tests. Now socket2 is all green, even without using c_uchar, which is unexpected the other way around.
jtojnar
force-pushed
the
ipv6-tos
branch
2 times, most recently
from
8779a0b to
1656a28
Compare
ADD-SP
removed
the
S-waiting-on-author
It is IPv4-specific. Follow the rename from socket2 0.6.0.
The macro is based on the one from socket2 library: https://github.com/rust-lang/socket2/blob/a18be6a302b7f9c127c3593edec5d8d2690839a7/tests/socket.rs#L1345 Co-authored-by: Thomas de Zeeuw <thomasdezeeuw@gmail.com>
This allows setting `Traffic Class` field, which is the equivalent of IPv4 `Type of Service` field: https://man.openbsd.org/ip6#IPV6_TCLASS Not supported on Windows: https://learn.microsoft.com/en-us/windows/win32/winsock/ipproto-ipv6-socket-options
ADD-SP
left a comment
ADD-SP
left a comment
There was a problem hiding this comment.
Overall looks good to me, just two minor questions.
ADD-SP
added
S-waiting-on-author
ADD-SP
changed the title
TCLASS option on IPv6
4 participants
{Udp,Tcp}Socket::tostotos_v4as it is IPv4-specific, following the rename from socket2 0.6.0.{Tcp,Udp}Socket::tclass_v6as its IPv6 equivalent.Motivation
I want to be able to set IPv6 Traffic Class field as conveniently as the corresponding IPv4 Type of Service field, which allows setting the Differentiated Services subfield necessary for Quality of Service requirements.
Solution
Just addding an extra getter/setter exposing the underlying API.