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

Add support for IPV6_TCLASS and IPV6_RECVTCLASS #364

Merged
merged 1 commit into from
Dec 21, 2022
Merged

Add support for IPV6_TCLASS and IPV6_RECVTCLASS #364

merged 1 commit into from
Dec 21, 2022

Conversation

duskmoon314
Copy link
Contributor

A simple implementation of the IPV6_TCLASS and IPV6_RECVTCLASS modeled after the existing IP_TOS and IP_RECVTOS implementations.

close #363

@duskmoon314 duskmoon314 mentioned this pull request Dec 19, 2022
Closed
Thomasdezeeuw
Thomasdezeeuw reviewed Dec 20, 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.

Can you switch the ordering of the functions, first the getter than the setter.

src/socket.rs Outdated Show resolved Hide resolved
src/socket.rs Outdated Show resolved Hide resolved
src/socket.rs Outdated Show resolved Hide resolved
@duskmoon314
Copy link
Contributor Author

Done

Should I remove let recv_tos = i32::from(recv_tos); too?

@Thomasdezeeuw
Copy link
Collaborator

Should I remove let recv_tos = i32::from(recv_tos); too?

Sure 👍

@Thomasdezeeuw
Copy link
Collaborator

@duskmoon314 can you fix the failing CI for Windows? See https://github.com/rust-lang/socket2/actions/runs/3740167945/jobs/6348219101

@duskmoon314
Copy link
Contributor Author

Sure. I am looking for MS documentation on this setting

@duskmoon314
Copy link
Contributor Author

It seems that Microsoft doesn't allow modifying the traffic class of IPv6 win-socket as in Unix-like OS.

For IPv4, using IP_TOS is allowed and may correspond to history. And the doc notes that QoS API is recommended:

Do not use. Type of Service (TOS) settings should only be set using the Quality of Service API. See Differentiated Services in the Quality of Service section of the Platform SDK for more information.

https://learn.microsoft.com/en-us/windows/win32/winsock/ipproto-ip-socket-options

And for IPv6, no IPV6_TCLASS is mentioned in the doc. So this is probably why the test fails since I assume it is usable and just not be mentioned.

I am considering adding target_os = windows to the cfg not attribute. Any suggestions?

@Thomasdezeeuw
Copy link
Collaborator

I am considering adding target_os = windows to the cfg not attribute. Any suggestions?

Yes, that's the way to go I think. However if it's not available on Windows it need to be behind the all feature and we'll likely want to move it to sys/unix.rs. For an example see

socket2/src/sys/unix.rs

Lines 1048 to 1076 in fa67e04

#[cfg(all(
feature = "all",
any(
target_os = "android",
target_os = "dragonfly",
target_os = "freebsd",
target_os = "fuchsia",
target_os = "illumos",
target_os = "linux",
target_os = "netbsd",
target_os = "openbsd"
)
))]
#[cfg_attr(
docsrs,
doc(cfg(all(
feature = "all",
any(
target_os = "android",
target_os = "dragonfly",
target_os = "freebsd",
target_os = "fuchsia",
target_os = "illumos",
target_os = "linux",
target_os = "netbsd",
target_os = "openbsd"
)
)))
)]

@duskmoon314
Copy link
Contributor Author

Sure. I will modify the code ASAP.

@duskmoon314 duskmoon314 force-pushed the ipv6_tclass branch 3 times, most recently from d9cdd0f to 085c9e1 Compare December 21, 2022 12:54
Thomasdezeeuw
Thomasdezeeuw approved these changes Dec 21, 2022
View reviewed changes
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
@Thomasdezeeuw
Copy link
Collaborator

I'll squash the commit I've added.

@duskmoon314 @Thomasdezeeuw
Add support for IPV6_TCLASS and IPV6_RECVTCLASS
bada246 
A simple implementation of the IPV6_TCLASS and IPV6_RECVTCLASS modeled
after the existing IP_TOS and IP_RECVTOS implementations.

Remove useless bool to i32 conversion in `set_recv_tos`

close rust-lang#363
@Thomasdezeeuw Thomasdezeeuw merged commit 43f286c into rust-lang:master Dec 21, 2022
@Thomasdezeeuw
Copy link
Collaborator

Thanks @duskmoon314!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Thomasdezeeuw Thomasdezeeuw Thomasdezeeuw approved these changes

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

Successfully merging this pull request may close these issues.

Support IPV6_TCLASS
2 participants
@duskmoon314 @Thomasdezeeuw