log: Remove NOLINT(bitcoin-unterminated-logprintf) #30485

1. 
   maflcko commented at 1:17 pm on July 19, 2024: member

   NOLINT(bitcoin-unterminated-logprintf) is used to document a missing trailing \n char in the format string. This has many issues:

   • It is just documentation, assuming that a trailing \n ends up in the formatted string. It is not enforced at compile-time, so it is brittle.
   • If the newline was truly missing and NOLINT(bitcoin-unterminated-logprintf) were used to document a “continued” line, the log stream would be racy/corrupt, because any other thread may inject a log message in the meantime.
   • If the newline was accidentally missing, nothing is there to correct the mistake.
   • The intention of all code is to always end a log line with a new line. However, historic code exists to deal with the case where the new line was missing (m_started_new_line). This is problematic, because the presumed dead code has to be maintained (https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1682963306).

   Fix almost all issues by removing the NOLINT(bitcoin-unterminated-logprintf), ensuring that a new line is always present.

   A follow-up will remove the dead logging code.

2. log: Remove NOLINT(bitcoin-unterminated-logprintf) fa18fc7050
3. 
   DrahtBot commented at 1:17 pm on July 19, 2024: contributor

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Code Coverage

   For detailed information about the code coverage, see the test coverage report.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	TheCharlatan, ryanofsky
   Concept ACK	luke-jr

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

   Conflicts

   No conflicts as of last run.

4. DrahtBot added the label Utils/log/libs on Jul 19, 2024
5. maflcko force-pushed on Jul 19, 2024
6. in src/dbwrapper.cpp:103 in fa18fc7050

    99@@ -100,7 +100,7 @@ class CBitcoinLevelDBLogger : public leveldb::Logger {
   100 
   101                 assert(p <= limit);
   102                 base[std::min(bufsize - 1, (int)(p - base))] = '\0';
   103-                LogPrintLevel(BCLog::LEVELDB, BCLog::Level::Debug, "%s", base); // NOLINT(bitcoin-unterminated-logprintf)
   104+                LogDebug(BCLog::LEVELDB, "%s\n", util::RemoveSuffixView(base, "\n"));
   

   ---

   ---

   

   maflcko commented at 2:17 pm on July 19, 2024:

   Arguably, this is a “bugfix” to add a missing \n in the unlikely case where the buffer is exactly filled and the last character is overwritten from \n to \0.

   However, I am not sure if anyone ever ran into this logging bug, so I am just leaving a comment here.

   ---

   

   maflcko commented at 2:46 pm on July 19, 2024:

   (It is possible to test this “bugfix” by reducing both buffer sizes sufficiently and then running with -debug=leveldb -printtoconsole)
7. TheCharlatan approved
8. 
   TheCharlatan commented at 8:46 am on July 23, 2024: contributor

   ACK fa18fc705084f3620be566d8c6639b29117ccf68
9. fanquake requested review from theuni on Jul 25, 2024
10. luke-jr approved
11. 
    luke-jr commented at 10:43 pm on July 31, 2024: member

    utACK
12. luke-jr referenced this in commit 16a5dc62ae on Aug 1, 2024
13. ryanofsky approved
14. 
    ryanofsky commented at 6:29 pm on August 6, 2024: contributor

    Code review ACK fa18fc705084f3620be566d8c6639b29117ccf68
15. DrahtBot requested review from luke-jr on Aug 6, 2024
16. ryanofsky merged this on Aug 6, 2024
17. ryanofsky closed this on Aug 6, 2024

    ---

18. maflcko deleted the branch on Aug 7, 2024

Contributors
maflcko DrahtBot TheCharlatan luke-jr ryanofsky

Review Requested
theuni luke-jr

Labels
Utils/log/libs