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

Support openbsd #1863

Merged
merged 1 commit into from
May 21, 2024
Merged

Support openbsd #1863

merged 1 commit into from
May 21, 2024

Conversation

flub
Copy link
Contributor

@flub flub commented May 15, 2024

This makes Quinn compile on openbsd and the test suite passes as well.

As far as I understand ECN is not supported as part of a control message on recvmsg. So I disabled the ECN tests for it.

Copy link
Collaborator

@Ralith Ralith left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

As far as I understand ECN is not supported as part of a control message on recvmsg. So I disabled the ECN tests for it.

OpenBSD does seem to define IPV6_RECVTCLASS: https://github.com/openbsd/src/blob/35659bf2453d7eeb12fd24089cf668ca13361c18/sys/netinet6/in6.h#L330C9-L330C24

quinn-udp/src/unix.rs Outdated Show resolved Hide resolved
quinn-udp/src/unix.rs Show resolved Hide resolved
Copy link
Member

@djc djc left a comment

Choose a reason for hiding this comment

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

Seems reasonable to merge this even without ECN support for now.

@flub
Copy link
Contributor Author

flub commented May 16, 2024

OpenBSD does seem to define IPV6_RECVTCLASS: https://github.com/openbsd/src/blob/35659bf2453d7eeb12fd24089cf668ca13361c18/sys/netinet6/in6.h#L330C9-L330C24

Indeed, and the socket option is being set as well still. Let me try again if I can get ENC to work for just IPv6.

@flub
Copy link
Contributor Author

flub commented May 16, 2024

Thanks for working on this!

As far as I understand ECN is not supported as part of a control message on recvmsg. So I disabled the ECN tests for it.

OpenBSD does seem to define IPV6_RECVTCLASS: https://github.com/openbsd/src/blob/35659bf2453d7eeb12fd24089cf668ca13361c18/sys/netinet6/in6.h#L330C9-L330C24

Ok, ECN now works and is tested on IPv6 but not IPv4. I've had to split some tests for that. I left the existing ecn_ipv6 test unmodified in the end but renamed it rather than fully splitting it in two. It still makes sense on it's own.

@flub flub requested a review from Ralith May 16, 2024 12:03
@flub
Copy link
Contributor Author

flub commented May 16, 2024

This will close #1469 I believe.

quinn-udp/src/unix.rs Show resolved Hide resolved
quinn-udp/tests/tests.rs Outdated Show resolved Hide resolved
quinn/src/tests.rs Outdated Show resolved Hide resolved
OpenBSD supports ECN on IPv6 but not on IPv4 it seems.  This splits
off the test cases so that IPv6 ECN support is tested in isolation.
@flub
Copy link
Contributor Author

flub commented May 21, 2024

It is not clear to me if the failing test is flaky or due to something I did, any hints?

Copy link
Collaborator

@Ralith Ralith left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@Ralith Ralith merged commit 170a9cd into quinn-rs:main May 21, 2024
8 checks passed
@djc djc mentioned this pull request May 22, 2024
@flub
Copy link
Contributor Author

flub commented May 22, 2024

@Ralith thanks for the prodding, definitely better than the first version!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants