build: make HAVE_O_CLOEXEC available outside LevelDB (bugfix) #21250
Conversation
|
Both of the macOS builds are failing with: CXX libbitcoin_server_a-shutdown.o
shutdown.cpp:38:9: error: use of undeclared identifier 'pipe2'
if (pipe2(g_shutdown_pipe, O_CLOEXEC) != 0) {
^
1 error generated.
make[2]: *** [Makefile:8763: libbitcoin_server_a-shutdown.o] Error 1Looks like it has |
theStack
force-pushed
the
2021-02-build-pass-have_o_cloexec
branch
from
e802c96 to
239f841
Compare
|
Added a commit to change the preprocessor condition to only use popen2 if O_CLOEXEC is available and MacOS is not used. |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
luke-jr
left a comment
luke-jr
left a comment
There was a problem hiding this comment.
At the very least, have configure check specifically for pipe2...
Ideally, a code path that sets O_CLOEXEC after the normal pipe path could be nice too.
theStack
force-pushed
the
2021-02-build-pass-have_o_cloexec
branch
from
239f841 to
4b38e4f
Compare
configure.ac
Outdated
There was a problem hiding this comment.
| AC_MSG_CHECKING(for pipe2) | |
| AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[#include <unistd.h>]], | |
| [[ static int pipefd[2]; | |
| pipe2(pipefd, 0); ]])], | |
| [ AC_MSG_RESULT(yes); HAVE_PIPE2=1 ], | |
| [ AC_MSG_RESULT(no); HAVE_PIPE2=0 ] | |
| ) | |
| AC_DEFINE_UNQUOTED([HAVE_PIPE2], [$HAVE_PIPE2], [Define to 1 if pipe2 is available.]) | |
| AC_CHECK_DECLS([pipe2]) |
There was a problem hiding this comment.
Heh, nice that sometimes the solution can be so simple, thanks. Any reason why the same approach is not used for also e.g. fdatasync? (That's what I used as template in the previous version).
|
Concept ACK
I'm confused—how does that compile at all if the preprocessor flag is mot set? Our reason for using Edit: OK, that works differently than I thought apparently. I was under the impression that using |
theStack
force-pushed
the
2021-02-build-pass-have_o_cloexec
branch
from
4b38e4f to
9bac713
Compare
|
@laanwj: Yes, I would have also expected Force-pushed now with a much simpler pipe2 detection in configure, as suggested by luke-jr. |
|
Code review ACK 9bac713
I feel kind of ashamed for not realizing this for so long. I wonder if we can come up with some new convention that does ensure this! (anything I can think of inside Not in this PR, of course. |
…elDB (bugfix) 9bac713 build: make HAVE_O_CLOEXEC available outside LevelDB (bugfix) (Sebastian Falbesoner) 584fd91 init: only use pipe2 if availabile, check in configure (Sebastian Falbesoner) Pull request description: The result of the O_CLOEXEC availability check is currently only set in the Makefile and passed to LevelDB (see `LEVELDB_CPPFLAGS_INT` in `src/Makefile.leveldb.include`), but not defined to be used in our codebase. This means that code within the preprocessor conditional `#if HAVE_O_CLOEXEC` was actually never compiled. On the master branch this is currently used for pipe creation in `src/shutdown.cpp`, PR bitcoin#21007 moves this part to a new module (I found the issue while testing that PR). The fix is similar to the one in bitcoin#19803, which solved the same problem for HAVE_FDATASYNC. In the course of working on the PR it turned out that pipe2 is not available an all platforms, hence a configure check and a corresponding define HAVE_PIPE2 is introduced and used. The PR can be tested by anyone with a system that has pipe2 and O_CLOEXEC available by putting gibberish into the HAVE_O_CLOEXEC block: on master, everything should compile fine, on PR, the compiler should abort with an error. At least that's my naive way of testing preprocessor logic, happy to hear more sophisticated ways :-) ACKs for top commit: laanwj: Code review ACK 9bac713 Tree-SHA512: aec89faf6ba52b6f014c610ebef7b725d9e967207d58b42a4a71afc9f1268fcb673ecc85b33a2a3debba8105a304dd7edaba4208c5373fcef2ab83e48a170051
6 participants
The result of the O_CLOEXEC availability check is currently only set in the Makefile and passed to LevelDB (see
LEVELDB_CPPFLAGS_INTinsrc/Makefile.leveldb.include), but not defined to be used in our codebase. This means that code within the preprocessor conditional#if HAVE_O_CLOEXECwas actually never compiled. On the master branch this is currently used for pipe creation insrc/shutdown.cpp, PR #21007 moves this part to a new module (I found the issue while testing that PR).The fix is similar to the one in #19803, which solved the same problem for HAVE_FDATASYNC.
In the course of working on the PR it turned out that pipe2 is not available an all platforms, hence a configure check and a corresponding define HAVE_PIPE2 is introduced and used.
The PR can be tested by anyone with a system that has pipe2 and O_CLOEXEC available by putting gibberish into the HAVE_O_CLOEXEC block: on master, everything should compile fine, on PR, the compiler should abort with an error. At least that's my naive way of testing preprocessor logic, happy to hear more sophisticated ways :-)