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

Solaris TCP_KEEPINTVL and TCP_KEEPCNT have wrong values #2901

Merged
merged 1 commit into from Sep 11, 2022

Conversation

gco
Copy link
Contributor

@gco gco commented Sep 5, 2022

No description provided.

@rust-highfive
Copy link

Some changes occurred in solarish module

cc @jclulow,@pfmooney

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Amanieu (or someone else) soon.

Please see the contribution instructions for more information.

@pfmooney
Copy link
Contributor

pfmooney commented Sep 5, 2022

I would have expected libc-test to catch this (I don't see that we're excluding those constants). Do the tests run clean with the change?

@pfmooney
Copy link
Contributor

pfmooney commented Sep 5, 2022

I would have expected libc-test to catch this (I don't see that we're excluding those constants). Do the tests run clean with the change?

Sorry, I misread this as being the top-level (solarish) ones. Carry on!

@gco
Copy link
Contributor Author

gco commented Sep 5, 2022

I would have expected libc-test to catch this (I don't see that we're excluding those constants). Do the tests run clean with the change?

Yeah-- nobody has run libc-test on Solaris recently, it doesn't work at all. Maybe a PR for that later.

@pfmooney
Copy link
Contributor

pfmooney commented Sep 5, 2022

I would have expected libc-test to catch this (I don't see that we're excluding those constants). Do the tests run clean with the change?

Yeah-- nobody has run libc-test on Solaris recently, it doesn't work at all. Maybe a PR for that later.

Understandable. It was not a small list to get it running cleanly on illumos. If it will require a significant delta from the existing combined "solarish" test setup, it might be worth splitting the Solaris test stuff out too (just as was done for the actual library sources)

Regardless, thanks for chasing down these issues.

@JohnTitor
Copy link
Member

Are there any public resources to check these values on Solaris?

@gco
Copy link
Contributor Author

gco commented Sep 10, 2022

Are there any public resources to check these values on Solaris?

It's unfortunate, but installing Solaris in a VM is probably the most straightforward way.

Oracle Solaris 11.4 CBE (common build environment) is free to use for personal use and free/open-source developers.

FWIW, libc-test does report the following (amongst many other complaints) after some effort getting it to run:

bad TCP_KEEPCNT value at byte 0: rust: 30 (0x1e) != c 31 (0x1f)
bad TCP_KEEPINTVL value at byte 0: rust: 31 (0x1f) != c 30 (0x1e)

Getting the header file directly from the Oracle IPS package server:

% curl -s http://pkg.oracle.com/solaris/release/file/0/bc0d5055091db79a511770601d2c0bee7420c6d3 | gzcat | grep TCP_KEEPINTVL
#define TCP_KEEPINTVL                   0x1E
%

@JohnTitor
Copy link
Member

Thanks, hmm, then adding it to libc-test would be the best way here. For now, I'm just going to r+ this, thanks again for fixing them!

@JohnTitor
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Sep 10, 2022

📌 Commit feb971a has been approved by JohnTitor

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Sep 11, 2022

⌛ Testing commit feb971a with merge c20064f...

@bors
Copy link
Contributor

bors commented Sep 11, 2022

☀️ Test successful - checks-actions, checks-cirrus-freebsd-12, checks-cirrus-freebsd-13, checks-cirrus-freebsd-14
Approved by: JohnTitor
Pushing c20064f to master...

@bors bors merged commit c20064f into rust-lang:master Sep 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants