Skip to content

freebsd adding further TCP stack related constants. #4196

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

Merged
merged 1 commit into from
Aug 11, 2025

Conversation

devnexen
Copy link
Contributor

@devnexen devnexen commented Dec 16, 2024

@rustbot
Copy link
Collaborator

rustbot commented Dec 16, 2024

r? @tgross35

rustbot has assigned @tgross35.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@devnexen devnexen force-pushed the fbsd_tcp_stack_2 branch 2 times, most recently from d99d599 to 4877da1 Compare December 16, 2024 09:49
Comment on lines 3795 to 3796
pub const TCP_BBR_USE_RACK_RR: c_int = 1104;
pub const TCP_BBR_USE_RACK_CHEAT: c_int = TCP_BBR_USE_RACK_RR;
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably only need one of these. It looks like TCP_BBR_USE_RACK_RR was added more recently freebsd/freebsd-src@e570d23#diff-c354fa9fe15a3e5c90079f38792a8e5458d76677437a53ee7537a62fbc72e100R255.

@tgross35
Copy link
Contributor

ref: https://man.freebsd.org/cgi/man.cgi?query=tcp

Looks like this is the wrong link, these aren't mentioned there. https://github.com/freebsd/freebsd-src/blob/main/sys/netinet/tcp.h is the header file.

One nit but the rest looks fine to me. @asomers could you double check this?

@asomers
Copy link
Contributor

asomers commented Dec 22, 2024

The definitions look correct to me, but they're awfully obscure. These constants are so obscure that there's a risk of them being deemed unstable private APIs. That's why I generally don't like to add such things to libc unless there's a real need.

For example, constants added in 9ab890d later had to be deprecated in ab57796 . Do you have an actual need for these constants, or are you just adding them for completeness's sake?

@devnexen
Copy link
Contributor Author

The definitions look correct to me, but they're awfully obscure. These constants are so obscure that there's a risk of them being deemed unstable private APIs. That's why I generally don't like to add such things to libc unless there's a real need.

For example, constants added in 9ab890d later had to be deprecated in ab57796 . Do you have an actual need for these constants, or are you just adding them for completeness's sake?

I m mostly interested by BBR constants even tough not all values have necessarily an immediate usefulness and a little bit of completeness yes.

Copy link
Contributor

@tgross35 tgross35 left a comment

Choose a reason for hiding this comment

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

@devnexen maybe based on the above, you could drop everything except the BBR constants, and perhaps maybe only the more popular ones? (I have no idea which those would be). Since we have some BBR constants already, and that's your usecase

Copy link
Contributor

@tgross35 tgross35 left a comment

Choose a reason for hiding this comment

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

Thank you!

@tgross35 tgross35 added the stable-nominated This PR should be considered for cherry-pick to libc's stable release branch label Aug 11, 2025
@tgross35 tgross35 added this pull request to the merge queue Aug 11, 2025
Merged via the queue into rust-lang:main with commit 0442ebb Aug 11, 2025
48 of 52 checks passed
@tgross35 tgross35 mentioned this pull request Aug 11, 2025
@tgross35 tgross35 added stable-applied This PR has been cherry-picked to libc's stable release branch and removed stable-nominated This PR should be considered for cherry-pick to libc's stable release branch labels Aug 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-unix S-waiting-on-author stable-applied This PR has been cherry-picked to libc's stable release branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants