re-work -debug switch handling

Diapolo commented at 10:24 am on October 8, 2013: none

re-work -debug help message text
make -debug log every debugging information again (even all categories)
remove unneeded fDebug checks in front of LogPrint()/qDebug(), as that check is done in LogPrintf() when category is != NULL (true for all LogPrint() calls
remove fDebug ONLY in code which is NOT performance-critical
harmonize addrman category name
deprecate -debugnet usage, should be used via -debug=net and remove the corresponding global

Diapolo commented at 7:14 am on October 10, 2013: none

Comments?

in src/main.cpp: in 517898153a outdated

3639+        if (vInv.size() != 1)
3640             LogPrint("net", "received getdata (%"PRIszu" invsz)\n", vInv.size());
3641 
3642-        if ((fDebugNet && vInv.size() > 0) || (vInv.size() == 1))
3643+        if ((vInv.size() > 0) || (vInv.size() == 1))
3644             LogPrint("net", "received getdata for: %s\n", vInv[0].ToString().c_str());

laanwj commented at 7:28 am on October 10, 2013:

These could be in a if(fDebug) for a reason, for example because they’ll otherwise slow down the path with formatting/argument computation overhead even though the message is not logged.

Diapolo commented at 9:49 am on October 10, 2013:

Right, ProcessMessage() is for sure performance critical. Do you have a clean idea if it’s then still possible to quickly check if -debug=net was one of the specified categories or do you suggest to supply just a fDebug check here?

laanwj commented at 10:09 am on October 10, 2013:

I suggest just a fDebug check there. If any kind of debugging is enabled, performance becomes less of an issue.

Diapolo commented at 12:14 pm on October 11, 2013: none

Updated:

remove fDebug ONLY in code which is NOT performance-critical

in src/init.cpp: in 2d91206b46 outdated

430@@ -431,7 +431,11 @@ bool AppInit2(boost::thread_group& threadGroup)
431 
432     // ********************************************************* Step 3: parameter-to-internal-flags
433 
434-    if (mapMultiArgs.count("-debug")) fDebug = true;
435+    if (mapArgs.count("-debug") && mapMultiArgs["-debug"].size() > 0)
436+        fDebug = true;
437+    // -debugnet (deprecated switch) compatibility
438+    if(GetBoolArg("-debugnet", false))

sipa commented at 8:32 pm on October 11, 2013:

I think this -debugnet check needs to go first, so fDebug gets set correctly?

sipa commented at 8:34 pm on October 11, 2013:

Also, does -debug not imply all debug categories, so also fDebug?

laanwj commented at 8:55 pm on October 11, 2013:

fDebug (as least how it’s used in the code everywhere, for example in LogPrint) means any debug category is enabled, not all.

From the help message I understand that -debug (or -debug=1) implies all debug categories, but there was no special handling for that anywhere. That’s changed by this pull.

in src/util.cpp: in 2d91206b46 outdated

235+
236+        const vector<string>& categories = mapMultiArgs["-debug"];
237+
238+        // Only look for categories, if not just -debug was passed,
239+        // which implies every category should be logged.
240+        if (!categories[0].empty())

laanwj commented at 8:58 pm on October 11, 2013:

Maybe check just in case that categories.size() is 1 too. This checks for an empty category name at the first index of categories. Is that what’s in categories when just -debug (or -debug=1) is passed? Wouldn’t it contain a category called 1?

Diapolo commented at 1:03 pm on October 17, 2013:

Agreed, will update later, so you can review again.

gavinandresen commented at 9:45 pm on October 11, 2013: contributor

I don’t think I like the “-debug means -debug=all”.

I run with debug=1 in my bitcoin.conf, and will add temporary LogPrintf() statements as I develop code (and will remove them or change them to LogPrint(“category”…) before pull-requesting). Just like I used to add printf() statements during development that I’d remove.

If -debug means “print everything”, then that doesn’t work– my log messages will get lost in the blizzard of messages.

I suppose I could switch to -debug=temp and always LogPrint(“temp”, …), but that’s more typing and I’m lazy.

sipa commented at 9:55 pm on October 11, 2013: member

@gavinandresen So what would you like -debug to mean then?

gavinandresen commented at 8:52 am on October 12, 2013: contributor

-debug is the same as -debug= which has empty-string for the category. So Logprintf equals Logprint("",…) etc.

Gavin Andresen

On Oct 12, 2013, at 7:55 AM, Pieter Wuille notifications@github.com wrote:

@gavinandresen So what would you like -debug to mean then?

— Reply to this email directly or view it on GitHub.

laanwj commented at 9:32 am on October 12, 2013: member

I understand how logging with an empty category can be useful for temporary debugging, but from a user/external developer viewpoint, is it logical behavior? What would you expect plain -debug to do on a package that you don’t know deeply yet and are trying to debug?

In any case I’d be fine with -debug=all as well instead of -debug to enable all categories. But there needs to be a way (and clearly documented in the help message).

sipa commented at 7:38 pm on October 12, 2013: member

I agree with @laanwj here - having a "" debug category seems counter-intuitive.

Diapolo commented at 6:12 am on October 16, 2013: none

@sipa @laanwj @gavinandresen I’ll summarize, what I think should be default behaviour (I don’t say the pull is currently doing this already ^^):

Using -debug sets fDebug and we display ALL categories (LogPrint()). Using -debug=<category> sets fDebug and we display ONLY the speciefied categories.

Perhaps we should re-work LogPrint() to prepend the category in the log-entry, as this will make searching the log MUCH easier!

gavinandresen commented at 8:19 am on October 16, 2013: contributor

@Diapolo: -debug and -debug= are the same. They both will set mapArgs["-debug"]=std::string("")

Changing that so -debug and -debug= sets mapArgs["-debug"] to two different things is likely to break things in unexpected ways.

EDIT: just realized I’m probably misreading, and you mean -debug being different from -debug=<some_category>. EDIT2: aha! Bitten by github removing stuff in <> ….

Ok, I can live with -debug meaning -debug=all. I’ll just set my default to -debug=none to get the default behavior I want.

sipa commented at 8:32 am on October 16, 2013: member

Sounds good to me.

Related, but outside of this pullreq: I’ve been wondering whether categories shouldn’t just correspond to threads, and indeed, whether we shouldn’t just print them by default. Also, do we really worry enough to make -logtimestamps non-default anymore?

laanwj commented at 9:12 am on October 16, 2013: member

@gavinandresen: when learning a codebase or trying to find a problem in an otherwise unknown codebase I usually find it useful to enable all debug information, to get some idea what it is doing. It’s of more limited usability once you have a better idea what you’re looking for.

in src/init.cpp: in 16c1e69b45 outdated

217@@ -218,8 +218,8 @@ bool static Bind(const CService &addr, unsigned int flags) {
218         strUsage += "  -daemon                " + _("Run in the background as a daemon and accept commands") + "\n";
219 #endif
220     strUsage += "  -testnet               " + _("Use the test network") + "\n";
221-    strUsage += "  -debug                 " + _("Output extra debugging information. Implies all other -debug* options") + "\n";
222-    strUsage += "  -debugnet              " + _("Output extra network debugging information") + "\n";
223+    strUsage += "  -debug=<category>      " + _("Output debugging information (default: 0, 1 = output everything, <category> set = output category + defaults)") + "\n";

Diapolo commented at 1:18 pm on October 17, 2013:

I could need some help for the help message :).

in src/util.cpp: in 16c1e69b45 outdated

235+
236+        const vector<string>& categories = mapMultiArgs["-debug"];
237+
238+        // Only look for categories, if not just -debug or -debug=1 was passed and
239+        // category size is 1, as that implies every category should be logged.
240+        if ((!categories[0].empty() || categories[0] != "1") && categories.size() == 1)

Diapolo commented at 1:51 pm on October 17, 2013:

I messed up this check -_-, needs fixing.

Diapolo commented at 2:06 pm on October 17, 2013: none

@sipa @laanwj @gavinandresen Can you take another look?

I added some suggestions from above and as I wrote earlier, this is achieved now:

Using -debug or -debug=1 sets fDebug and we display ALL categories (all LogPrint()). Using -debug=<category> sets fDebug and we display ONLY the speciefied categories (only LogPrint(<category>)).

It would be still fine, if we could add category in front of the log entries e.g. [net] blabla happened.

Diapolo commented at 2:07 pm on October 17, 2013: none

@sipa Agreed, -logtimestamps should be default, as it’s much more readable IMO.

Diapolo commented at 6:59 am on October 22, 2013: none

@gavinandresen @sipa @laanwj Rebased, fixed a merge-conflict. Can I get some final ACKs or futher comments?

in src/util.cpp: in 7416c1ebe2 outdated

233+
234+        const vector<string>& categories = mapMultiArgs["-debug"];
235+
236+        // Only look for categories, if not just -debug or -debug=1 was passed and
237+        // category size is 1, as that implies every category should be logged.
238+        if (!categories[0].empty() && categories[0] != "1" && categories.size() == 1)

laanwj commented at 7:16 am on October 22, 2013:

I don’t think this will work, this will only go into the if if categories.size() == 1. Something like this is easier to read, and does the length check before indexing into the array:

0if (!(categories.size() == 1 && (categories[0]=="" || categories[0] =="1")))

Or even better

0bool allCategories = (categories.size() == 1) && (categories[0]=="" || categories[0] =="1");
1if(!allCategories)
2{
3....

gavinandresen commented at 7:26 am on October 22, 2013:

What should happen if user says: -debug=net -debug ?

I think you want:

0bool allCategories = categories.count("");

laanwj commented at 7:27 am on October 22, 2013:

@gavinandresen agreed

in src/init.cpp: in 7416c1ebe2 outdated

435+    // -debugnet compatibility (deprecated switch)
436+    if (GetBoolArg("-debugnet", false)) {
437+        fDebug = true;
438+        mapMultiArgs["-debug"].push_back("net");
439+    }
440+    else if (mapArgs.count("-debug") && mapMultiArgs["-debug"].size() > 0)

gavinandresen commented at 7:24 am on October 22, 2013:

This logic seemed tortured and error-prone. How about:

0if (mapArgs.count("-debugnet"))
1    LogPrintf("WARNING deprecated argument -debugnet ignored, use -debug=net");
2
3fDebug = !mapMultiArgs["-debug"].empty();

Maybe add a special case for -debug=0 / -nodebug, as long as we’re OK with the user being able to do something like -debug=net -nodebug -debug=addrman and have the -nodebug always override:

0// Special-case: if -debug=0 set, turn off debugging:
1if (mapMultiArgs["-debug"].find("0")) fDebug = false;

I’d get rid of -debugnet from the help message, and write the help string as:

0-debug=&lt;category&gt; : output debugging information for &lt;category&gt;. Output all possible debugging information if category is not set.

Diapolo commented at 8:43 am on October 22, 2013: none

Great suggestions, re-working…

Diapolo commented at 9:08 am on October 22, 2013: none

Updated:

help message
special-case -debug=0/-nodebug
replace my checks with generic checks (thanks @laanwj and @gavinandresen)

-debug overrides -debug=<category> and -debug=0 or -nodebug override all others!

I hope this is now just a final review!

Diapolo commented at 11:43 am on October 22, 2013: none

Failure because of the json-pull…

Diapolo commented at 6:51 am on October 24, 2013: none

Updated and removed an unneeded \n in InitWarning().

Diapolo commented at 10:43 am on October 26, 2013: none

Can I get some ACKs :)?

in src/init.cpp: in 6cf7a11656 outdated

214@@ -215,8 +215,7 @@ bool static Bind(const CService &addr, unsigned int flags) {
215 #endif
216 #endif
217         strUsage += "  -paytxfee=<amt>        " + _("Fee per KB to add to transactions you send") + "\n";
218-        strUsage += "  -debug                 " + _("Output extra debugging information. Implies all other -debug* options") + "\n";
219-        strUsage += "  -debugnet              " + _("Output extra network debugging information") + "\n";
220+        strUsage += "  -debug=<category>      " + _("Output debugging information for <category>. Output all possible debugging information if category is not set.") + "\n";

sipa commented at 7:09 pm on October 26, 2013:

It’s perhaps better to split this line in two, to cover the -debug=category and -debug cases.

sipa commented at 7:11 pm on October 26, 2013: member

ACK apart from the nit above.

Diapolo commented at 8:07 pm on October 26, 2013: none

@sipa I reworked the help-message for -debug once more, it reads now: bitcoind: Output debugging information for <category>. Output all possible debugging information if <category> is not set. <category> can be addrman, alert, coindb, db, lock, rand, rpc, selectcoins, mempool, net.

Bitcoin-Qt: Output debugging information for <category>. Output all possible debugging information if <category> is not set. <category> can be addrman, alert, coindb, db, lock, rand, rpc, selectcoins, mempool, net or qt.

sipa commented at 8:17 pm on October 26, 2013: member

@Diapolo It’s stil an extremely long line :)

Diapolo commented at 7:41 pm on October 27, 2013: none

@sipa Like this?

Or feel free to just suggest a string, so we can merge this :).

Diapolo commented at 11:38 am on October 28, 2013: none

New string with new-lines :-D.

bitcoind

Bitcoin-Qt

Diapolo commented at 10:10 am on October 30, 2013: none

I hope @BitcoinPullTester is now happy again and @gavinandresen or @sipa can give a final ACK.

re-work -debug switch handling

- re-work -debug help message text
- make -debug log every debugging information again (even all categories)
- remove unneeded fDebug checks in front of LogPrint()/qDebug(), as that
  check is done in LogPrintf() when category is != NULL (true for all
  LogPrint() calls
- remove fDebug ONLY in code which is NOT performance-critical
- harmonize addrman category name
- deprecate -debugnet usage, should be used via -debug=net and remove the
  corresponding global

3b570559f8

in src/init.cpp: in cff94eb4c1 outdated

226+    }
227+    else
228+    {
229+        strUsage += ". ";
230+    }
231+        strUsage +=                               _("Output all possible debugging information if <category> is not set.") + "\n";

sipa commented at 12:31 pm on October 30, 2013:

I’d still prefer to see this output line split up. I think explaining -debug=category and -debug separate is much more readable.

Diapolo commented at 1:14 pm on October 30, 2013:

Will change that, thanks for your time.

Diapolo commented at 1:40 pm on October 30, 2013:

How is this?

in src/init.cpp: in cff94eb4c1 outdated

220+        strUsage += "  -debug=<category>      " + _("Output debugging information for <category>.") + "\n";
221+        strUsage +=                               _("<category> can be:");
222+        strUsage +=                                 " addrman, alert, coindb, db, lock, rand, rpc, selectcoins, mempool, net"; // Don't translate these
223+    if (hmm == HMM_BITCOIN_QT)
224+    {
225+        strUsage += " or qt. ";

sipa commented at 12:32 pm on October 30, 2013:

You’d need to translate the “or” as well. Better to just provide a single list of options separated by comma’s.

Diapolo commented at 1:14 pm on October 30, 2013:

Right, just a list separated by comma’s is the way to go here…

BitcoinPullTester commented at 3:23 pm on October 30, 2013: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/3b570559f8d39a5d4cffd01b8091c3133f7750dc for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

sipa commented at 10:15 pm on October 30, 2013: member

ACK

gavinandresen referenced this in commit ef4b518aea on Oct 30, 2013

gavinandresen merged this on Oct 30, 2013

gavinandresen closed this on Oct 30, 2013

Bushstar referenced this in commit 9d109d6a30 on Apr 8, 2020

DrahtBot locked this on Sep 8, 2021

re-work -debug switch handling #3067