[logging] add lint-logs.sh to check for newline termination. #12891

jnewbery · 2018-04-05T14:19:13Z

Check that all calls to LogPrintf() are terminated by a newline,
except those that are explicitly marked as 'continued' logs.

jnewbery · 2018-04-05T14:19:59Z

Suggested by @MarcoFalke here: #12887 (comment)

linter hints suggested by @laanwj here: #12887 (comment)

maflcko · 2018-04-05T14:28:42Z

src/dbwrapper.cpp

@@ -63,7 +63,7 @@ class CBitcoinLevelDBLogger : public leveldb::Logger {

                assert(p <= limit);
                base[std::min(bufsize - 1, (int)(p - base))] = '\0';
-                LogPrintf("leveldb: %s", base);
+                LogPrintf("leveldb: %s", base);  /* Continued */


Since you are changing those lines anyway, why not use LogPrintfContinuation or something?

My suggestion here: #12887 (comment) has inverted semantics. This Continued here comment indicates that there is a continuation log to follow. LogPrintfContinuation() would indicate that the log continues a previous log.

I think LogPrintfContinuation() could be useful, but it's a rebase-the-world PR since it removes the \n from every call to LogPrintf()

Yes, please don't change every single log message. This solution is fine.

maflcko · 2018-04-05T14:36:47Z

utACK 28682f3354a89f49a402735f94b7f79f34966aab

laanwj · 2018-04-05T16:18:45Z

utACK 28682f3

practicalswift · 2018-04-05T20:46:41Z

Very strong concept ACK. Thanks for making this fix persistent by adding a lint script.

Please apply this patch to the lint script to make it work as expected:

--- lint-logs.sh	2018-04-05 22:43:14.119412169 +0200
+++ lint-logs.sh.fixed	2018-04-05 22:42:03.787354149 +0200
@@ -13,10 +13,10 @@
 # ignored


-UNTERMINATED_LOGS=$(git grep "LogPrintf(" -- *cpp | \
-	grep -v "\\\n\"" | \
-	grep -v "/\* Continued \*/" | \
-	grep -v "LogPrintf\(\)")
+UNTERMINATED_LOGS=$(git grep "LogPrintf(" -- "*.cpp" | \
+    grep -v '\\n"' | \
+    grep -v "/\* Continued \*/" | \
+    grep -v "LogPrintf()")
 if [[ ${UNTERMINATED_LOGS} != "" ]]; then
     echo "All calls to LogPrintf() should be terminated with \\n"
     echo

The bugs:

The *cpp in git grep "LogPrintf(" -- *cpp will be expanded by the shell and match only cpp files in the current directory.
grep -v "LogPrintf\(\)" is equivalent to grep -v "LogPrintf" due to how basic regular expressions are handled by grep. From the documentation: "In basic regular expressions the meta-characters ?, +, {, |, (, and ) lose their special meaning; instead use the backslashed versions ?, +, {, |, (, and )."

ACK modulo fixing the lint script bugs :-)

jnewbery · 2018-04-06T17:05:41Z

Thanks for the review and bugfixes @practicalswift ! Your fixed version caught a non-conforming log, which I've fixed in a new commit.

maflcko · 2018-04-06T17:10:28Z

utACK 621981530b57ac983b7bf35474c0796fbf5d5ed7

practicalswift · 2018-04-06T17:12:07Z

utACK 621981530b57ac983b7bf35474c0796fbf5d5ed7

laanwj · 2018-04-07T14:15:06Z

src/wallet/wallet.cpp

@@ -3076,7 +3076,7 @@ bool CWallet::CommitTransaction(CTransactionRef tx, mapValue_t mapValue, std::ve
        wtxNew.fTimeReceivedIsTxTime = true;
        wtxNew.fFromMe = true;

-        LogPrintf("CommitTransaction:\n%s", wtxNew.tx->ToString());
+        LogPrintf("CommitTransaction:\n%s\n", wtxNew.tx->ToString());


Unfortunately this change is wrong, as tx->ToString() adds a final newline: https://github.com/bitcoin/bitcoin/blob/master/src/primitives/transaction.cpp#L99

ah, well spotted. Now fixed.

Incidentally, I hate this log. As far as I'm aware it's the only one with newlines embedded and it confuses my log parser!

Most logs should terminated with a '\n'. Some logs are built up over multiple calls to logPrintf(), so do not need a newline terminater. Comment all of these 'continued' logs as a linter hing.

Check that all calls to LogPrintf() are terminated by a newline, except those that are explicitly marked as 'continued' logs.

practicalswift · 2018-04-08T05:56:19Z

utACK d207207

laanwj · 2018-04-08T09:05:24Z

utACK d207207

…tion. d207207 [logging] add lint-logs.sh to check for newline termination. (John Newbery) 5c21e6c [logging] Comment all continuing logs. (John Newbery) Pull request description: Check that all calls to LogPrintf() are terminated by a newline, except those that are explicitly marked as 'continued' logs. Tree-SHA512: fe5162b2b2df1e8a4c807da87584fa9af97a6b8377e4090fe0caa136d90bf29a487a123cde94569bdce7101fee3478196d99aa13f1212e24bfe5f41c773604fc

…termination. d207207 [logging] add lint-logs.sh to check for newline termination. (John Newbery) 5c21e6c [logging] Comment all continuing logs. (John Newbery) Pull request description: Check that all calls to LogPrintf() are terminated by a newline, except those that are explicitly marked as 'continued' logs. Tree-SHA512: fe5162b2b2df1e8a4c807da87584fa9af97a6b8377e4090fe0caa136d90bf29a487a123cde94569bdce7101fee3478196d99aa13f1212e24bfe5f41c773604fc

maflcko reviewed Apr 5, 2018

View reviewed changes

fanquake added the Utils/log/libs label Apr 5, 2018

jnewbery force-pushed the log_lint branch from 28682f3 to 6219815 Compare April 6, 2018 17:05

laanwj reviewed Apr 7, 2018

View reviewed changes

jnewbery added 2 commits April 7, 2018 12:29

[logging] Comment all continuing logs.

5c21e6c

Most logs should terminated with a '\n'. Some logs are built up over multiple calls to logPrintf(), so do not need a newline terminater. Comment all of these 'continued' logs as a linter hing.

[logging] add lint-logs.sh to check for newline termination.

d207207

Check that all calls to LogPrintf() are terminated by a newline, except those that are explicitly marked as 'continued' logs.

jnewbery force-pushed the log_lint branch from 6219815 to d207207 Compare April 7, 2018 16:30

laanwj merged commit d207207 into bitcoin:master Apr 8, 2018

str4d mentioned this pull request Apr 12, 2021

Sync backports zcash/zcash#5057

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[logging] add lint-logs.sh to check for newline termination. #12891

[logging] add lint-logs.sh to check for newline termination. #12891

jnewbery commented Apr 5, 2018

jnewbery commented Apr 5, 2018

maflcko Apr 5, 2018

jnewbery Apr 5, 2018

laanwj Apr 5, 2018

maflcko commented Apr 5, 2018

laanwj commented Apr 5, 2018

practicalswift commented Apr 5, 2018 •

edited

jnewbery commented Apr 6, 2018

maflcko commented Apr 6, 2018

practicalswift commented Apr 6, 2018

laanwj Apr 7, 2018

jnewbery Apr 7, 2018

practicalswift commented Apr 8, 2018

laanwj commented Apr 8, 2018

[logging] add lint-logs.sh to check for newline termination. #12891

[logging] add lint-logs.sh to check for newline termination. #12891

Conversation

jnewbery commented Apr 5, 2018

jnewbery commented Apr 5, 2018

maflcko Apr 5, 2018

Choose a reason for hiding this comment

jnewbery Apr 5, 2018

Choose a reason for hiding this comment

laanwj Apr 5, 2018

Choose a reason for hiding this comment

maflcko commented Apr 5, 2018

laanwj commented Apr 5, 2018

practicalswift commented Apr 5, 2018 • edited

jnewbery commented Apr 6, 2018

maflcko commented Apr 6, 2018

practicalswift commented Apr 6, 2018

laanwj Apr 7, 2018

Choose a reason for hiding this comment

jnewbery Apr 7, 2018

Choose a reason for hiding this comment

practicalswift commented Apr 8, 2018

laanwj commented Apr 8, 2018

practicalswift commented Apr 5, 2018 •

edited