log: use error level for critical log messages #30255
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. 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. |
maflcko
force-pushed
the
2406-logError
branch
2 times, most recently
from
cccebd5 to
b90f1c3
Compare
stickies-v
left a comment
stickies-v
left a comment
There was a problem hiding this comment.
ACK b90f1c36ff0e469bc64830ff35e591a9aab63fb2 - more consistent logging
src/validation.cpp
Outdated
There was a problem hiding this comment.
nit: since we're calling GetNotifications().fatalError() next, I think this log line can just be removed?
There was a problem hiding this comment.
My understanding is that the notification does not include err.what(), so the debug log in this line is still needed.
There was a problem hiding this comment.
Can't we just merge them?
git diff on b90f1c36ff
diff --git a/src/validation.cpp b/src/validation.cpp
index 89aa84a551..ac95824443 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -6303,12 +6303,10 @@ bool ChainstateManager::ValidatedSnapshotCleanup()
fs::path p_old,
fs::path p_new,
const fs::filesystem_error& err) {
- LogError("[snapshot] Error renaming path (%s) -> (%s): %s\n",
- fs::PathToString(p_old), fs::PathToString(p_new), err.what());
GetNotifications().fatalError(strprintf(_(
- "Rename of '%s' -> '%s' failed. "
+ "[snapshot] Rename of '%s' -> '%s' failed: %s. "
"Cannot clean up the background chainstate leveldb directory."),
- fs::PathToString(p_old), fs::PathToString(p_new)));
+ fs::PathToString(p_old), fs::PathToString(p_new), err.what()));
};
try {
There was a problem hiding this comment.
I think it is fine to have this log, but happy to review a separate pull removing it :)
There was a problem hiding this comment.
This reminded me to change the two other log lines before fatalError to use the error level. So I pushed that other change in the last push.
maflcko
force-pushed
the
2406-logError
branch
2 times, most recently
from
faa3f8a to
2222f25
Compare
|
re-ACK 2222f2573c8de0a7f7171e6fc9e844ad448e1f65 |
As per doc/developer-notes#logging, LogError should be used for severe problems that require the node to shut down. Co-Authored-By: stickies-v <[email protected]>
BrandonOdiwuor
left a comment
BrandonOdiwuor
left a comment
There was a problem hiding this comment.
ACK 2222f2573c8de0a7f7171e6fc9e844ad448e1f65 - Replacing LogPrintf with Logerror to enhance critical error logging
|
re-ACK fae3a1f, I'm ~0 on the latest force push as |
Happy to review a follow-up for the two cases (#30255 (comment)). For now I think it is risk-free and more consistent to log the fatal error twice in the |
|
ACK fae3a1f agree with #30255 (comment) but that should be good for a follow up |
|
ACK fae3a1f |
6 participants
This picks up the first commit from #29231, but extends it to also cover cases that were missed in it.
As per https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#logging, LogError should be used for severe problems that require the node to shut down.