re-work -debug switch handling #3067

Diapolo · 2013-10-08T10:24:48Z

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 · 2013-10-10T07:14:05Z

Comments?

laanwj · 2013-10-10T07:28:30Z

src/main.cpp

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.

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?

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

Diapolo · 2013-10-11T12:14:10Z

Updated:

remove fDebug ONLY in code which is NOT performance-critical

sipa · 2013-10-11T20:32:56Z

src/init.cpp

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

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

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.

gavinandresen · 2013-10-11T21:45:00Z

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 · 2013-10-11T21:55:05Z

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

gavinandresen · 2013-10-12T08:52:42Z

-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 [email protected] wrote:

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

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

laanwj · 2013-10-12T09:32:46Z

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 · 2013-10-12T19:38:11Z

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

Diapolo · 2013-10-16T06:12:55Z

@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 · 2013-10-16T08:19:04Z

@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 · 2013-10-16T08:32:41Z

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 · 2013-10-16T09:12:46Z

@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.

Diapolo · 2013-10-17T13:18:48Z

src/init.cpp

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

Diapolo · 2013-10-17T14:06:37Z

@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 · 2013-10-17T14:07:44Z

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

Diapolo · 2013-10-22T06:59:13Z

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

laanwj · 2013-10-22T07:16:03Z

src/util.cpp

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:

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

Or even better

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

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

I think you want:

bool allCategories = categories.count("");

@gavinandresen agreed

Diapolo · 2013-10-22T08:43:42Z

Great suggestions, re-working...

Diapolo · 2013-10-22T09:08:02Z

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 · 2013-10-22T11:43:18Z

Failure because of the json-pull...

Diapolo · 2013-10-24T06:51:10Z

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

Diapolo · 2013-10-26T10:43:02Z

Can I get some ACKs :)?

sipa · 2013-10-26T19:09:18Z

src/init.cpp

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

sipa · 2013-10-26T19:11:06Z

ACK apart from the nit above.

Diapolo · 2013-10-26T20:07:21Z

@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 · 2013-10-26T20:17:50Z

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

Diapolo · 2013-10-27T19:41:00Z

@sipa Like this?

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 feel free to just suggest a string, so we can merge this :).

Diapolo · 2013-10-28T11:38:18Z

New string with new-lines :-D.

bitcoind

Output debugging information for category.
Category can be addrman, alert, coindb, db, lock, rand, rpc, selectcoins, mempool, net.
Output all possible debugging information if category is not set.

Bitcoin-Qt

Output debugging information for category.
Category can be addrman, alert, coindb, db, lock, rand, rpc, selectcoins, mempool, net or qt.
Output all possible debugging information if category is not set.

Diapolo · 2013-10-30T10:10:13Z

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

sipa · 2013-10-30T12:31:50Z

src/init.cpp

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

Will change that, thanks for your time.

How is this?

-debug=category Output debugging information (default: 0, supplying category is optional) If category is not supplied, output all debugging information. category can be: addrman, alert, coindb, db, lock, rand, rpc, selectcoins, mempool, net, qt.

- 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

BitcoinPullTester · 2013-10-30T15:23:30Z

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 · 2013-10-30T22:15:51Z

ACK

re-work -debug switch handling

…bosity=2` mode (bitcoin#3067)

laanwj reviewed Oct 10, 2013
View reviewed changes

sipa reviewed Oct 11, 2013
View reviewed changes

Diapolo reviewed Oct 17, 2013
View reviewed changes

src/init.cpp Outdated

Copy link

Author

Diapolo Oct 17, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

laanwj reviewed Oct 22, 2013
View reviewed changes

sipa reviewed Oct 26, 2013
View reviewed changes

src/init.cpp Outdated

Copy link

Member

sipa Oct 26, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

sipa reviewed Oct 30, 2013
View reviewed changes

gavinandresen added a commit that referenced this pull request Oct 30, 2013

Merge pull request #3067 from Diapolo/debug-switch

ef4b518

re-work -debug switch handling

gavinandresen merged commit ef4b518 into bitcoin:master Oct 30, 2013

Bushstar pushed a commit to Bushstar/omnicore that referenced this pull request Apr 8, 2020

Add missing instantlock/instantlock_internal to getblock's `ver…

9d109d6

…bosity=2` mode (bitcoin#3067)

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

re-work -debug switch handling #3067

re-work -debug switch handling #3067

Uh oh!

Conversation

Diapolo commented Oct 8, 2013

Uh oh!

Diapolo commented Oct 10, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Diapolo commented Oct 11, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gavinandresen commented Oct 11, 2013

Uh oh!

sipa commented Oct 11, 2013

Uh oh!

gavinandresen commented Oct 12, 2013

Uh oh!

laanwj commented Oct 12, 2013

Uh oh!

sipa commented Oct 12, 2013

Uh oh!

Diapolo commented Oct 16, 2013

Uh oh!

gavinandresen commented Oct 16, 2013

Uh oh!

sipa commented Oct 16, 2013

Uh oh!

laanwj commented Oct 16, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Diapolo commented Oct 17, 2013

Uh oh!

Diapolo commented Oct 17, 2013

Uh oh!

Diapolo commented Oct 22, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Diapolo commented Oct 22, 2013

Uh oh!

Diapolo commented Oct 22, 2013

Uh oh!

Diapolo commented Oct 22, 2013

Uh oh!

Diapolo commented Oct 24, 2013

Uh oh!

Diapolo commented Oct 26, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa commented Oct 26, 2013

Uh oh!

Diapolo commented Oct 26, 2013

Uh oh!

sipa commented Oct 26, 2013

Uh oh!

Diapolo commented Oct 27, 2013

Uh oh!

Diapolo commented Oct 28, 2013

Uh oh!

Diapolo commented Oct 30, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!