#33646 log: check fclose() results and report safely in logging.cpp
https://github.com/bitcoin/bitcoin/pull/33646 · · +29/-8 in 1 files, 1 commits · labels: Utils/log/libs
Goal
- Prevent unnoticed diagnostic loss by reporting errors when closing log files
- Report close failures directly to stderr during log rotation, shrinking, or shutdown
Checks the return value of `fclose()` calls in `src/logging.cpp` via a helper function `CloseLogFile()`. Failures are formatted and written directly to stderr along with system error details to avoid re-entering the logger or taking locks during error reporting.
Problem: Errors encountered when closing log files during log rotation, shrink, or shutdown are currently ignored. If a filesystem writeback failure occurs on close, it is not reported anywhere and can lead to unnoticed diagnostics loss.
Category: Utilities (logging, arguments, libraries) (#65 of 66)
P4 · cleanup
- P4 because log files disable stream buffering, so writeback errors surface on write rather than close
- Defensive error reporting provides only a minor benefit for unexpected close failures
Checking fclose returns is good practice, but the actual impact is minimal because Bitcoin Core disables stream buffering on log files with setbuf, so I/O errors occur on write rather than close. The change provides a small defensive benefit reporting unexpected close failures to stderr.
Membership: Touches src/logging.cpp to improve error reporting for logging file handling.
Factors: security/stability 1, bug 0, performance 0, user value 0, leverage 0
Reviewability: Ready
- Ready for review as code is compact and open feedback consists only of minor nits
The code changes are compact and functional. Open comments from sedited are minor stylistic nits and a commit message clarification that do not invalidate review.
Author status: active; addressed earlier rounds of feedback but has not yet replied to minor nits from 2026-09-08.
Open concerns:
- sedited noted minor cleanups: omitting an unneeded intermediate variable, dropping `noexcept`, passing `errno` directly, and updating a potentially stale commit message.
Resolved concerns:
- maflcko and cedwies discussed fault injection testing, concluding a clean unit test is impractical without filesystem mocking since logs are unbuffered.
- sedited requested clang-format and keeping the test logger close consistent under `m_cs`, which the author addressed.
Agreement: Positive
- General agreement on defensive error checking (sedited, maflcko)
- Noted that unbuffered writes make errors surface early, confirming the change is mainly defensive (maflcko)
- Minor style nits and commit message clarification pending author response (sedited)
Positive; general agreement on defensive error checking with minor nits pending from sedited.
Reviewers consider the defensive check reasonable. Discussion around testing concluded it is difficult due to unbuffered I/O, and current review comments are minor nits.
- sedited said 'This looks ok' and provided review on formatting and locking.
- maflcko pointed out that unbuffered file writes mean write errors surface before fclose, acknowledging the defensive nature of the patch.
- sedited left minor nits regarding noexcept and variable usage on 2026-09-08.
Review verdicts (DrahtBot): 0
Files
0 lines under test/bench/ci.
- src/logging.cpp +29/-8
Card
This PR introduces a CloseLogFile helper in src/logging.cpp to verify fclose return values and write error diagnostics to stderr. It prevents silent failures during log rotation and shutdown when closing the debug log. Because debug log files are unbuffered, close failures are rare, giving this patch marginal operational impact. The change is conceptually welcomed and reviewable, with only minor nits and commit message tweaks awaiting the author.