-
Notifications
You must be signed in to change notification settings - Fork 977
Fix OpenBSD issues related to lack of full dual-stack IPv6 and EVFILT_USER #1907
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the port!
@@ -40,6 +40,11 @@ KJ_BEGIN_HEADER | |||
#endif | |||
#endif | |||
|
|||
#if __OpenBSD__ | |||
// OpenBSD doesn't have full dual-stack IPv6. | |||
#define AI_V4MAPPED 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I don't think we should be defining this symbol in a header that applications include.
It looks like this is only used in two places. In async-io-unix.c++
there's already an #if
that avoids using this on Android, perhaps we should simply extend that to include OpenBSD? The symbol is also used in async-io-test.c++
, where I think we can safely #define it to 0 if it's not already defined.
@@ -1063,10 +1065,12 @@ void UnixEventPort::captureChildExit() { | |||
} | |||
|
|||
void UnixEventPort::wake() const { | |||
#if !__OpenBSD__ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this outright breaks support for cross-thread events on OpenBSD, causing them to fail to be delivered.
I think it would be better to remove OpenBSD from the list of platforms where we define KJ_USE_KQUEUE. It will then fall back to the poll()-based implementation, which supports cross-thread events correctly.
If we want to support kqueue on OpenBSD, we'll need to implement some other mechanism for wakeups here. If OpenBSD's kqueue supports per-thread signals correctly (like FreeBSD but not MacOS) then we could send a signal instead (using SIGUSR1, or the signal set with setReservedSignal()
). If signals don't work I guess we could create a pipe that the thread waits on, and write a byte to it to trigger a wakeup.
@@ -41,7 +41,7 @@ | |||
#include <arpa/inet.h> | |||
#endif | |||
|
|||
#if __FreeBSD__ | |||
#if __FreeBSD__ || __OpenBSD__ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should just move this include to the group that is included on all non-Windows platforms. There should be no harm and maybe it fixes other systems as well. In fact, I sad the same thing when these lines were added originally, but the submitter pushed back:
The motivation for this commit is to fix the build for OpenBSD, but it may also solves the same potential problem for other systems without causing any harm. Suggested already twice, see capnproto#1846 (comment) capnproto#1907 (comment)
Fix build on OpenBSD (second take, revival of #1907)
This is a fix against the
master
/1.1-dev
branch.It fixes build issues on OpenBSD related to the lack of full dual-stack IPv6 1 and EVFILT_USER 2.
Everything builds fine. There are a few test failures on OpenBSD but they seem unrelated to the changes introduced here.