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. |
cccebd5 to
b90f1c3
Compare
| @@ -6303,7 +6303,7 @@ bool ChainstateManager::ValidatedSnapshotCleanup() | |||
| fs::path p_old, | |||
| fs::path p_new, | |||
| const fs::filesystem_error& err) { | |||
| LogPrintf("Error renaming path (%s) -> (%s): %s\n", | |||
| LogError("[snapshot] Error renaming path (%s) -> (%s): %s\n", | |||
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 b90f1c3
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.
faa3f8a to
2222f25
Compare
|
re-ACK 2222f25 |
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 <stickies-v@protonmail.com>
There was a problem hiding this comment.
ACK 2222f25 - 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 |
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.