BSD Support (FreeBSD, OpenBSD and NetBSD) #1221
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Added support for OpenBSD, FreeBSD and NetBSD in the bldscripts/* scripts. Tested on OpenBSD only so far. Also, extracted some bldscripts 'meson "setdir" logic' (which chooses what directory to build in) into a new file, messetdirvarsfinish.sh, which is then invoked by the mes*setdirvars.sh scripts - i.e. reduced code duplication in the scripts by creating the common messetdirvarsfinish.sh file.
For OpenBSD, FreeBSD and NetBSD. Tried solely on OpenBSD so far.
The code for #defining _USE_LIBEVENT, previously in eventmeth.h, was moved to its own file emosandlibevdefs.h, which can be included without needing to include all the rest of the eventmeth.h capabilities. event2/thread.h (the libevent thread header file) was also removed from eventmeth.h. It doesn't need to be included there, and including it there exposes eventmeth.h users directly to libevent, the opposite of what we intend since eventmeth acts as a wrapper for libevent.
Replaced use of strcpy and strcat with strlcpy/strncpy and strlcat/strncat respectively to avoid a clang compiler warning that was showing up in OpenBSD.
Removed Use of CLOCK_MONOTONIC_RAW in BSD case. CLOCK_MONOTONIC_RAW is not defined on FreeBSD13.3 and OpenBSD 7.3. Also, added the include for <event2/thread.h>, which had been removed from eventmeth.h (event2/thread.h being the thread header file for libevent).
OpenBSD doesn't support either of the existing methods of counting open files hanldes, used with Linux and macOS respectively. Created a brute-force fallback method for use with BSD. Tried on openBSD only so far.
Added preprocessor directives to use the correct spelling of pthread_set[_]name_np for BSD. In macOS and Linux it is spelt pthread_setname_np, whereas in OpenBSD it is spelt pthread_set_name_np. Tried on OpenBSD only so far.
Uses TCP_NOPUSH (like macOS) not TCP_CORK (as Linux) in getsockopt/setsockopt. Small improvement to a couple of debug log messages. Added a my_sendfile function for use by BSD. OpenBSD does not have a sendfile system call.
In client.cc, the code was failing to call registerFdOneShot after ::connect returned success. Apparently in all the other *nix we've tried, ::connect returns -1 with errno EINPROGRESS, whereas in FreeBSD ::connect simply returns 0 (success); tweaked the code so registerFdOneShot is called in the "::connect returns 0" case. This makes cookie_test_3 pass, it fails on FreeBSD otherwise. emosandlibevdefs.h: Add comment on why FreeBSD uses libevent and not the FreebSD Linuxulator. .clang-format-ignore: Ignore the formatting in emosandlibevdefs.h (emosandlibevdefs.h uses the same formatting as eventmeth.h - see previous commit for why it was pulled out of eventmeth.h). meson.build: Include execinfo library explicitly for FreeBSD (it is a separate library in FreeBSD, but not in other *nix seemingly). eventmeth.cc: Fixed an indentation issue (no change in functionality).
In the test "headers_test -> last_modified_test" we allow the last-modified-time string produced to end in either "UTC" or "GMT". Previously, the test required "GMT". As of July/2024, it seems that in macOS, Linux and OpenBSD it produces "GMT", while under FreeBSD it's "UTC". Of course, they mean the same thing, and we allow either.
Needed for NetBSD when using libevent. actualFdU64Value too complex for constexpr.
gcc gives warning when compiling in NetBSD otherwise
Using #defines to cope with the different definition NetBSD has of pthread_setname_np (different to Linux macOS and to other BSDs).
Since TCP_NOPUSH/TCP_CORK don't exist in NetBSD, we use TCP_NODELAY,
with it's sense inverted, instead. ("Inverted" in the sense that, if
TCP_NOPUSH would be on, we set TCP_NODELAY off, and vice versa).
In listener_test.cc - in Linux and macOS, using AF_UNSPEC leads us to use IPv4 when available. However, in NetBSD, it causes us to use IPv6 when available. Since Pistache itself defaults to IPv4, we try IPv4 first for bind_free_port_helper, and only try AF_UNSPEC if IPv4 fails.
In http_server_test.cc - many_client_with_requests_to_multithreaded_server - extended the timeoutsfor the benefit of NetBSD in debug mode (writing to syslog, i.e. debug output, seems to slow things down more in NetBSD).
In rest_server_test.cc, the local hostname may be returned as a synonym for "localhost" in NetBSD. Coping with that case.
This library is required for some debugging log output in *BSD
Previously, for OpenBSD there was an intermittent issue with Listener::dispatchPeer (which setups a new peer for a new connection) when the client managed to close the connection before dispatchPeer had finished executing. This was causing a throw for peer->fd(), the actual-fd (derived from fd) being used an index into the set of peers, since the peer's fd was empty due to the close. In the changed code, we allow peer-fd() to return an empty fd without a throw, but also use the transport::toWriteLock to guard against the close messing up write attempts and/or new-connection dispatch attempts. Specific changes: peer.h/peer.c: Provide an actualFd() method for peer which can be used when the caller just wants the actual-fd and doesn't need to know the intermediate "fd" value (which the caller would have been previously fetching just to then call getActualFd). Allows us to protect "fd" from getting closed while that actualFd() method is executing. listener.cc: Uses peer->actualFd() as described above. transport.h/transport.cc: Add closeFd(Fd) method to transport.cc. peer.cc then uses this to close the Fd within peer::closeFd(). This allows the transport code to clean its toWrite queue upon the Fd closing. transport.cc: The "clean up buffers" for toWrite is removed from the face of Transport::removePeer, now done in closeFd(Fd). There are also a few places where we check for an Fd being empty, and some comments added pointing out where Fd is now allowed to be empty, where before the empty Fd would have been prevented by a throw.
|
Hey @dgreatwood. Great work. There's some issues popping up in CI. In particular, on Debian stable (linuxflibev): |
|
Fixing...
…On Fri, Jul 26, 2024 at 5:06 PM Kip ***@***.***> wrote:
Hey @dgreatwood <https://github.com/dgreatwood>. Great work. There's some
issues popping up in CI. In particular, on Debian stable (linuxflibev):
[21/83] Compiling C++ object src/libpistache.so.0.4.1.p/common_transport.cc.o
FAILED: src/libpistache.so.0.4.1.p/common_transport.cc.o
g++ -Isrc/libpistache.so.0.4.1.p -Isrc -I../src -Iinclude -I../include -I/usr/include -fdiagnostics-color=always -fsanitize=address -fno-omit-frame-pointer --coverage -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wextra -Wpedantic -std=c++17 -O0 -g -Wconversion -Wno-sign-conversion -Wno-missing-field-initializers -fstack-protector-all --param=ssp-buffer-size=4 -DDEBUG=1 -DPISTACHE_FORCE_LIBEVENT -fPIC -DHAS_STRING_VIEW=1 -DONLY_C_LOCALE=1 -pthread -DPISTACHE_USE_CONTENT_ENCODING_DEFLATE -MD -MQ src/libpistache.so.0.4.1.p/common_transport.cc.o -MF src/libpistache.so.0.4.1.p/common_transport.cc.o.d -o src/libpistache.so.0.4.1.p/common_transport.cc.o -c ../src/common/transport.cc
In file included from ../include/pistache/pist_check.h:19,
from ../include/pistache/common.h:25,
from ../include/pistache/eventmeth.h:173,
from ../src/common/transport.cc:14:
../src/common/transport.cc: In member function 'ssize_t Pistache::Tcp::Transport::sendFile(Pistache::Fd, int, off_t, size_t)':
../src/common/transport.cc:873:90: error: 'sendfile_fn_name' was not declared in this scope
873 | "%s fd %" PIST_QUOTE(PS_FD_PRNTFCD) " actual-fd %d, file fd %d, len %d", sendfile_fn_name,
| ^~~~~~~~~~~~~~~~
../include/pistache/pist_syslog.h:62:20: note: in definition of macro 'PS_LOG_DEBUG_ARGS'
62 | __fmt, __VA_ARGS__)
| ^~~~~~~~~~~
—
Reply to this email directly, view it on GitHub
<#1221 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAFMA24WO5APO5R3W454QC3ZOLP75AVCNFSM6AAAAABLRKXXL6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENJTGY3DGMZUGA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--
NOTICE: This email and its attachments may contain privileged and
confidential information, only for the viewing and use of the intended
recipient. If you are not the intended recipient, you are hereby notified
that any disclosure, copying, distribution, acting upon, or use of the
information contained in this email and its attachments is strictly
prohibited and that this email and its attachments must be immediately
returned to the sender and deleted from your system. If you received this
email erroneously, please notify the sender immediately. Xage Security,
Inc. and its affiliates will never request personal information (e.g.,
passwords, Social Security numbers) via email. Report suspicious emails to
***@***.*** ***@***.***>
|
|
@dennisjenkins75, this is something we're going to need your help on reviewing when you have time because you know much more about the BSD family of OSes than I do. |
Tachi107
previously requested changes
Jul 27, 2024
|
On Sun, 2024-07-28 at 20:22 -0700, dgreatwood wrote:
We can always update this and recheck once #1222 is addressed.
Merged yesterday, FYI.
…--
Kip Warner
OpenPGP signed/encrypted mail preferred
https://www.thevertigo.com
|
Specific Changes:
Building on BSD - FreeBSD, OpenBSD and NetBSD.txt:
Correct typo; move manual system-level install instructions of
libraries Pistache in any case supplies as subprojects to the end of
the file, and make it clear such system-level installs are optional;
reflect use of "doas" in OpenBSD.
emosandlibevdefs.h: Add comment explaining name
meson.build: Use "endswith('bsd')" instead of listing out individual
BSDs.
include/pistache/meson.build: Use tabs consistently
pist_timelog.h: Move strlcpy/strncpy/strcpy handling defines out of
the middle of functions where they're used, to improve code
readability. Same for strcat.
http_defs.cc: Correct comment explaining the use of the string "GMT".
transport.cc:
- Added comment explaining that TCP_NODELAY has an effect similar to
the opposite of TCP_CORK/TCP_NOPUSH.
- Added comment explaining buffer sizes in my_sendfile
- Created SENDFILE define to abstract the difference between
::sendfile and my_sendfile
fdutils.cc: Use getrlimit instead of getdtablesize. Removed tab. Ran
clang-format.
Merging general fix for issue pistacheio#1222
dgreatwood
dismissed
Tachi107’s stale review
Addressed in the push earlier this pm
The use of deps_libpistache here, rather than just pistache_dep, is no longer needed thanks to fix pistacheio#1225 (pistacheio#1225).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Extends the previous macOS support to cover FreeBSD, OpenBSD NetBSD
There are 4 substantive changes:
Fix a race condition if the client closes the connection before server-side dispatchPeer function has completed. Small change, but needed for reliable behaviour in this scenario.
Address Lack of TCP_NOPUSH/TCP_CORK in NetBSD
Proceed when client ::connect(...) returns success (old client code assumed ::connect would return -1).
Added a my_sendfile function for use by BSD (BSD not having sendfile)
Other changes are tiny, dealing with small compiler-syntax or OS-API-syntax differences and build file needs.
The new file "Building on BSD - FreeBSD, OpenBSD and NetBSD.txt" gives brief instructions on how to build.
Comments welcome, of course.
Thanks!