log: check fclose() results and report safely in logging.cpp #33646
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33646. ReviewsSee the guideline for information on the review process. 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. |
cedwies
force-pushed
the
log-safe-fclose
branch
2 times, most recently
from
f1720f8 to
ddfef82
Compare
ajtowns
left a comment
ajtowns
left a comment
There was a problem hiding this comment.
Should rebase on top of master rather than including a merge commit.
| m_msgs_before_open.clear(); | ||
| } | ||
| if (f && fclose(f) != 0) { | ||
| ReportLogIOError("fclose", m_file_path); |
There was a problem hiding this comment.
What's the value in having this outside the m_cs guard?
There was a problem hiding this comment.
I put the fclose outside the lock to avoid holding the mutex during potentially slow I/O (like flushing the file). We swap m_fileout inside, so the state is safe, and then close the old file without blocking other threads. ReportLogIOError is designed to be lock-free anyway, if I am not missing something.
Some buffered I/O errors surface only on fclose(). Several fclose() calls in src/logging.cpp ignored the return value which can risk silent loss of log lines or misleading success during rotate/truncate. Check return of fclose() and include errno in diagnostics. For the active log sink, detach under lock and close outside the lock. On failure, report directly to stderr (which avoids recursive logging) and flush. No behavior change on success.
cedwies
force-pushed
the
log-safe-fclose
branch
from
29a52aa to
72e0cbc
Compare
3 participants
fclose()can report write errors (for example if the disk is full or the filesystem has a problem). Right now, somefclose()calls insrc/logging.cppignore the return value. This means errors might go unnoticed and log lines could be lost without warning.What this PR does:
fclose()errors tostderr(with path and errno).m_fileoutsafely and report errors.fclose(file)calls and report failures.No other behavior changes. Normal logging, rotation, and console output remain unchanged.