Bugfix: Define and use HAVE_FDATASYNC correctly outside LevelDB #19803
Conversation
This file contains hidden or 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
Contributor
|
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. |
Member
|
Thanks for catching, and splitting this out. I'm sure I checked that we dropped into the |
Member
|
Whoa. Thanks for paying attention here. |
fanquake
approved these changes
Aug 31, 2020
Member
fanquake
left a comment
fanquake
left a comment
There was a problem hiding this comment.
ACK c4b85ba - thanks for catching and fixing my mistake.
Clearly #if … is safer here because at least it'll raise an error
It'll also warn when not defined if you pass -Wundef,
util/system.cpp:1022:9: warning: "HAVE_FDATASYNC" is not defined, evaluates to 0 [-Wundef]
#if HAVE_FDATASYNC
^~~~~~~~~~~~~~which would have made this more obvious. I had looked at turning this on before, as it has caught other problems (#18359), but there were issues with dependencies. I'm going to follow up with up #16419.
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Aug 31, 2020
…outside LevelDB c4b85ba Bugfix: Define and use HAVE_FDATASYNC correctly outside LevelDB (Luke Dashjr) Pull request description: Fixes a bug introduced in bitcoin#19614 The LevelDB-specific fdatasync check was only using `AC_SUBST`, which works for Makefiles, but doesn't define anything for C++. Furthermore, the #define is typically 0 or 1, never undefined. This fixes both issues by defining it and checking its value instead of whether it is merely defined. Pulled out of bitcoin#14501 by fanquake's request ACKs for top commit: fanquake: ACK c4b85ba - thanks for catching and fixing my mistake. laanwj: Code review ACK c4b85ba Tree-SHA512: 91d5d426ba000b4f3ee7e2315635e24bbb23ceff16269ddf4f65a63d25fc9e9cf94a3b236eed2f8031cc36ddcf78aeb5916efcb244f415943a8a12f907ede8f9
laanwj
added a commit
that referenced
this pull request
Feb 23, 2021
…ugfix) 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 #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 :-) ACKs for top commit: laanwj: Code review ACK 9bac713 Tree-SHA512: aec89faf6ba52b6f014c610ebef7b725d9e967207d58b42a4a71afc9f1268fcb673ecc85b33a2a3debba8105a304dd7edaba4208c5373fcef2ab83e48a170051
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Feb 24, 2021
…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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
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.
Fixes a bug introduced in #19614
The LevelDB-specific fdatasync check was only using
AC_SUBST, which works for Makefiles, but doesn't define anything for C++. Furthermore, the #define is typically 0 or 1, never undefined.This fixes both issues by defining it and checking its value instead of whether it is merely defined.
Pulled out of #14501 by fanquake's request