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

OpenBSD: Fix keepalive_ms and set_keepalive_ms. #99

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

OpenBSD: Fix keepalive_ms and set_keepalive_ms. #99

wants to merge 2 commits into from

Conversation

jhatala
Copy link

@jhatala jhatala commented Mar 9, 2020 •
edited

OpenBSD (as of release 6.6) does not support the socket option TCP_KEEPIDLE (tcp(4)). To be using SO_KEEPALIVE instead made it compile but was not correct and was failing at run-time.

This pull request fixes #82. That github issue has a good description of the problem.

NetBSD supports the option (tcp(4)) and NetBSD should therefore be treated like generic unix. It therefore also did not seem right to use SO_KEEPALIVE as the KEEPALIVE_OPTION on NetBSD.

I have tested the patched net2 as of f061e5e in combination with the http client in hyper version =0.12.35. I have tested on OpenBSD 6.6 amd64 with rustc 1.38.0, on Linux x86_64 with rustc 1.43.1 and on NetBSD 9.0 amd64 with rustc 1.42.0.

@jhatala jhatala mentioned this pull request Mar 9, 2020
Open
@jhatala jhatala force-pushed the openbsd-ext-keepalive branch 2 times, most recently from ac31812 to fc79c5b Compare March 9, 2020 06:29
@jhatala
Copy link
Author

jhatala commented Mar 9, 2020

With the changes from #96 merged-in, the Travis builds seem to work (fc79c5b), but in order to use the patched net2 for my purpose I had to un-constrain cfg-if = "<0.1.10" in net2's Cargo.toml, because something in hyper 0.12.35's dependency chain wants cfg-if 0.1.10.

@pfmooney
Copy link
Member

pfmooney commented May 1, 2020

I've fixed things up so the try!() usage doesn't break the build via the deprecation warnings. If you're still interested in getting this integrated would you mind omitting the rest of the try!() clean-up to keep the change as small as possible?

@jhatala
Copy link
Author

jhatala commented May 15, 2020

would you mind omitting the rest of the try!() clean-up to keep the change as small as possible?

Will do. Thank you!

Prevent keepalive_ms and set_keepalive_ms from always failing on Open…
f061e5e 
…BSD.

OpenBSD (as of release 6.6) does not support the socket option TCP_KEEPIDLE.
To be using SO_KEEPALIVE was not correct and was failing at runtime.

NetBSD supports the option and it should therefore be treated like generic
unix.  The use of SO_KEEPALIVE as the KEEPALIVE_OPTION on NetBSD also
does not seem to be correct.
@jhatala
Copy link
Author

jhatala commented May 15, 2020

@pfmooney: Thank you for the CI fixes! I stacked my patch on top of the new master and all is well.

CI checks pass, and the fix worked in my testing in combination with the http client in hyper version =0.12.35: I have tested on OpenBSD 6.6 amd64 with rustc 1.38.0, on Linux x86_64 with rustc 1.43.1 and on NetBSD 9.0 amd64 with rustc 1.42.0.

The test program is the hyper::client example.

pfmooney
pfmooney reviewed May 16, 2020
View reviewed changes
src/ext.rs
@@ -667,8 +667,9 @@ impl<T: AsRawSocket> AsSock for T {
cfg_if! {
if #[cfg(any(target_os = "macos", target_os = "ios"))] {
use libc::TCP_KEEPALIVE as KEEPALIVE_OPTION;
} else if #[cfg(any(target_os = "openbsd", target_os = "netbsd"))] {
use libc::SO_KEEPALIVE as KEEPALIVE_OPTION;
} else if #[cfg(target_os = "openbsd")] {
Copy link
Member

Choose a reason for hiding this comment

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

This is dropping the netbsd case. Is it safe to do so?

Copy link
Author

Choose a reason for hiding this comment

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

Yes. NetBSD understands TCP_KEEPIDLE (tcp(4)), the cfg(unix) case just below this one applies to it correctly.

src/ext.rs Outdated
@@ -737,7 +738,25 @@ impl TcpStreamExt for TcpStream {
Ok(Some((secs as u32) * 1000))
}

#[cfg(unix)]
#[cfg(target_os = "openbsd")]
Copy link
Member

Choose a reason for hiding this comment

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

Style nit: Would you mind moving these two function definitions down to after their respective counterparts with the #[cfg(all(unix, not(target_os = "openbsd")))] gates? I think it would be nice list the specific openbsd case after the more general unix case.

Copy link
Author

Choose a reason for hiding this comment

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

Sure. Done in f98618d.

@jhatala jhatala requested a review from pfmooney May 20, 2020 03:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pfmooney pfmooney Awaiting requested review from pfmooney

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

Successfully merging this pull request may close these issues.

OpenBSD does not support TCP_KEEPALIVE or a timeout with SO_KEEPALIVE
2 participants
@jhatala @pfmooney