uvw OpenBSD support

[elindsey]

We got a report of one of our projects that uses uvw failing to build on OpenBSD - https://github.com/DNS-OARC/flamethrower/issues/44

Looking into it, uvw almost works on OpenBSD, but there are a few small things preventing it. 🙂

From a clean OpenBSD 6.7 install with uvw cloned into /tmp:

openbsd# pkg_add libuv
openbsd# pwd
/tmp/uvw
openbsd# ls
test.cpp uvw      uvw.hpp
openbsd# cat test.cpp                                                                                                                                                                                  
#include "uvw.hpp"

int main() {

}

openbsd# clang++ -std=c++17 test.cpp -I/usr/local/include 
In file included from test.cpp:1:
In file included from ./uvw.hpp:1:
In file included from ./uvw/async.hpp:7:
./uvw/handle.hpp:267:22: error: expected member name or ';' after declaration specifiers
    OSFileDescriptor fileno() const {
    ~~~~~~~~~~~~~~~~ ^
/usr/include/stdio.h:406:20: note: expanded from macro 'fileno'
#define fileno(p)       (!__isthreaded ? __sfileno(p) : (fileno)(p))
                         ^
In file included from test.cpp:1:
In file included from ./uvw.hpp:1:
In file included from ./uvw/async.hpp:7:
./uvw/handle.hpp:267:22: error: expected ')'
/usr/include/stdio.h:406:20: note: expanded from macro 'fileno'
#define fileno(p)       (!__isthreaded ? __sfileno(p) : (fileno)(p))
                         ^
./uvw/handle.hpp:267:22: note: to match this '('
/usr/include/stdio.h:406:19: note: expanded from macro 'fileno'
#define fileno(p)       (!__isthreaded ? __sfileno(p) : (fileno)(p))
                        ^
2 errors generated.

The main issue is that on OpenBSD, fileno is a macro and will clash with the name of this handle method. The work is small, but it would be a breaking API change to rename Handle::fileno.

There are four things that would need to be done for OpenBSD support:

1. Rename the Handle::fileno method
2. Small cmake fixes - don't try to link librt on BSD; adjust error levels so implicit integer conversion in gtest doesn't fail the build
3. ifdef out sendfile functionality (not present on OpenBSD so those tests fail)
4. Investigate the fs_events tests (those stall on OpenBSD; I haven't looked into OS support)

Is this something you'd be interested in accepting patches for?

[stefanofiorentino]

What about #undef fileno? libuv is already built at instantiation time of this header, so should not be a problem. If any problem arises, there is always the push/pop solution:

#define fileno_backup fileno
#undef fileno
...
#define fileno fileno_backup

[elindsey]

I'm not sure if you're recommending a temporary undef when the project includes uvw.hpp, or a temporary undef when fileno is defined in handle.hpp - but yes, both are options to get this specific project building, but both are real hacky. It makes sense if you don't want to support OpenBSD (which I would totally understand 🙂), but it does put the burden of working around the API on all projects using uvw, making them either undef on include or undef any time they want to call Handle::fileno (which would include uvw's own test suite).

[skypjack]

I don't see any reason to not support OpenBSD to be honest. I'd like to go deeper on point 4 but I can't really test anything on OpenBSD, so I need help here. All in all, I'm fine also with point 1. A breaking change can be documented in the changelog and that's it.

What patch did you want to propose @elindsey ? Let's agree on the solution before spending your/our time. :wink:

[Mno-hime]

I am happy to test a patch on OpenBSD. Just let me know.

[elindsey]

https://gist.github.com/elindsey/784fe0bffa2b25d7c620aeaec12e4c65

@skypjack this is the work-in-progress patch that I have, renaming fileno() to fd() and fixing some build options. This allows a clean build on OpenBSD and allows the test suite to run with two failures.

@Mno-hime if you don't mind, could you see if flamethrower builds in OpenBSD with those fileno -> fd changes?

I looked a little more into the fs_events failure. On first run it segfaults, on subsequent runs it stalls because ctest didn't clean up the files in the testdir. So the root problem is that segfault - I grabbed a stack but haven't had time to dig into it further. The crash is in libuv's threading layer, but from the stack alone I can't tell if it's a libuv problem or uvw violating libuv invariants.

* thread #1, stop reason = signal SIGSEGV
  * frame #0: 0x00000759ec15298a libc.so.96.0`_libc_futex at -:3
    frame #1: 0x00000759ec159cb5 libc.so.96.0`_rthread_cond_timedwait [inlined] _twait(p=<unavailable>, val=0, clockid=3, abs=<unavailable>) at synch.h:0
    frame #2: 0x00000759ec159c23 libc.so.96.0`_rthread_cond_timedwait(cond=0x0000075a7e506df0, mutexp=0x00000757af4b3510, abs=0x0000000000000000) at rthread_cond.c:106
    frame #3: 0x00000757af42da9a fs_event`uv_cond_wait(cond=0x00000757af4b3518, mutex=0x00000757af4b3510) at thread.c:782:7
    frame #4: 0x00000757af412701 fs_event`worker(arg=0x0000000000000000) at threadpool.c:76:7
    frame #5: 0x0000075a8e1bf0d1 libpthread.so.26.1`_rthread_start(v=<unavailable>) at rthread.c:96:11
    frame #6: 0x00000759ec143828 libc.so.96.0`__tfork_thread at tfork_thread.S:77

[skypjack]

@elindsey What's the purpose of the changes to the CMakeLists.txt? Why are they needed in this case? Not sure I get it.

[elindsey]

Lines 28/29: OpenBSD doesn't have librt, so it will fail to link. I couldn't tell why it was required at all so dropped that block entirely for my local testing, but if you want to preserve linking of librt on Linux then that condition can change to something like this:

elseif(NOT APPLE AND NOT CMAKE_SYSTEM_NAME MATCHES OpenBSD)

Line 21: The default cflags that cmake is picking up on OpenBSD include Wall/Werror, causing gtest to fail to build due to a few trivial implicit conversions. I explicitly turned off Werror to work around it - not really worth trying to compile an upstream test dependency with stricter flags than they use.

[Mno-hime]

Flamethrower 0.10.2 builds fine on OpenBSD 6.7 (libuv 1.30.1 from ports), thanks @elindsey! (I used only the first hunk from the patch, Flamethrower does not seem to need more than that?)

Flamethrower master also needed #include <sys/socket.h> in flame/query.cpp, otherwise:

/home/newman/flamethrower-master/flame/query.cpp:280:23: error: use of undeclared identifier 'AF_INET6'
            inet_pton(AF_INET6, cidr.c_str(), &(sa6.sin6_addr));
                      ^
/home/newman/flamethrower-master/flame/query.cpp:282:23: error: use of undeclared identifier 'AF_INET'
            inet_pton(AF_INET, cidr.c_str(), &(sa.sin_addr));
                      ^
2 errors generated.

However, Flamethrower fails to operate saying just generator error: std::bad_alloc. I run Flamethrower line this:

/usr/local/bin/flame --dnssec -P udp -F inet -g file -f /home/newman/stress/query_datafile -Q 10000 -p 5300 10.53.0.2
/usr/local/bin/flame --dnssec -P udp -F inet6 -g file -f /home/newman/stress/query_datafile -Q 10000 -p 5300 fd92:7065:b8e:ffff::2
/usr/local/bin/flame --dnssec -P tcp -F inet -g file -f /home/newman/stress/query_datafile -Q 10000 -p 5300 10.53.0.2
/usr/local/bin/flame --dnssec -P tcp -F inet6 -g file -f /home/newman/stress/query_datafile -Q 10000 -p 5300 fd92:7065:b8e:ffff::2

With -v 99:

--class: "IN"
--dnssec: true
--help: false
--qps-flow: null
--targets: null
--version: false
-F: "inet6"
-P: "tcp"
-Q: "10000"
-R: false
-T: "A"
-b: null
-c: "10"
-d: "1"
-f: "/home/newman/stress/query_datafile"
-g: "file"
-l: "0"
-n: "0"
-o: null
-p: "5300"
-q: "10"
-r: "test.com"
-t: "3"
-v: "99"
GENOPTS: []
TARGET: "fd92:7065:b8e:ffff::2"
file: push "091522.test.example."
file: push "092604.test.example."
file: push "012142.test.example."
file: push "066466.test.example."
file: push "088136.test.example."
file: push "091450.test.example."
file: push "014232.test.example."
file: push "002166.test.example."
file: push "024149.test.example."
file: push "068614.test.example."
generator error: std::bad_alloc

Without -f /home/newman/stress/query_datafile the generation is fine.

[elindsey]

Awesome! Thanks for checking @Mno-hime. The first hunk is the only one necessary because the other changes are to get uvw's test suite running on OpenBSD.

I'll get a patch for the missing includes and try to look into the -f problem on Monday. I'll track that over in https://github.com/DNS-OARC/flamethrower/issues/44 since those are now flame issues and not uvw issues. If you don't mind sharing your query_datafile in that issue, it would get me a quicker repro - but totally understand if you'd prefer to keep that private. 🙂

[skypjack]

@elindsey do you want to open a PR for this or can I proceed with the patch above?

[skypjack]

Merged upstream on experimental. I'll merge it on master soon and cut a new patch release. I'm closing this in the meantime. Feel free to reopen the issue if you find that experimental doesn't work as expected. Thanks.