Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add -debuglogfile option #11781

Merged
merged 4 commits into from Dec 4, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions doc/release-notes.md
Expand Up @@ -90,6 +90,9 @@ Low-level RPC changes
* `getmininginfo`
- The wallet RPC `getreceivedbyaddress` will return an error if called with an address not in the wallet.

Changed command-line options
-----------------------------
- `-debuglogfile=<file>` can be used to specify an alternative debug logging file.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you update the text to say that this can be absolute or relative (just like you have for the help text: this can be an absolute path or a path relative to the data directory)?

Copy link
Member Author

Choose a reason for hiding this comment

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

The release notes are not documentation. This minor change does not warrant a long write-up in the release notes. If anyone wants to have documentation they can just look at bitcoind's help which is documentation.


Credits
=======
Expand Down
8 changes: 6 additions & 2 deletions src/init.cpp
Expand Up @@ -342,6 +342,7 @@ std::string HelpMessage(HelpMessageMode mode)
if (showDebug)
strUsage += HelpMessageOpt("-feefilter", strprintf("Tell other nodes to filter invs to us by our mempool min fee (default: %u)", DEFAULT_FEEFILTER));
strUsage += HelpMessageOpt("-loadblock=<file>", _("Imports blocks from external blk000??.dat file on startup"));
strUsage += HelpMessageOpt("-debuglogfile=<file>", strprintf(_("Specify location of debug log file: this can be an absolute path or a path relative to the data directory (default: %s)"), DEFAULT_DEBUGLOGFILE));
strUsage += HelpMessageOpt("-maxorphantx=<n>", strprintf(_("Keep at most <n> unconnectable transactions in memory (default: %u)"), DEFAULT_MAX_ORPHAN_TRANSACTIONS));
strUsage += HelpMessageOpt("-maxmempool=<n>", strprintf(_("Keep the transaction memory pool below <n> megabytes (default: %u)"), DEFAULT_MAX_MEMPOOL_SIZE));
strUsage += HelpMessageOpt("-mempoolexpiry=<n>", strprintf(_("Do not keep transactions in the mempool longer than <n> hours (default: %u)"), DEFAULT_MEMPOOL_EXPIRY));
Expand Down Expand Up @@ -1209,8 +1210,11 @@ bool AppInitMain(boost::thread_group& threadGroup, CScheduler& scheduler)
ShrinkDebugFile();
}

if (fPrintToDebugLog)
OpenDebugLog();
if (fPrintToDebugLog) {
if (!OpenDebugLog()) {
return InitError(strprintf("Could not open debug log file %s", GetDebugLogPath().string()));
Copy link
Member

Choose a reason for hiding this comment

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

Nit, missing test.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will add a test, though testing this particular message only makes sense after #11783 goes in

Copy link
Member Author

Choose a reason for hiding this comment

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

Added test anyway, temporarily adding "Fix shutdown in case of errors during initialization" into here

}
}

if (!fLogTimestamps)
LogPrintf("Startup time: %s\n", DateTimeStrFormat("%Y-%m-%d %H:%M:%S", GetTime()));
Expand Down
37 changes: 26 additions & 11 deletions src/util.cpp
Expand Up @@ -89,6 +89,7 @@ const int64_t nStartupTime = GetTime();

const char * const BITCOIN_CONF_FILENAME = "bitcoin.conf";
const char * const BITCOIN_PID_FILENAME = "bitcoind.pid";
const char * const DEFAULT_DEBUGLOGFILE = "debug.log";

ArgsManager gArgs;
bool fPrintToConsole = false;
Expand Down Expand Up @@ -189,26 +190,40 @@ static void DebugPrintInit()
vMsgsBeforeOpenLog = new std::list<std::string>;
}

void OpenDebugLog()
fs::path GetDebugLogPath()
{
fs::path logfile(gArgs.GetArg("-debuglogfile", DEFAULT_DEBUGLOGFILE));
if (logfile.is_absolute()) {
return logfile;
} else {
return GetDataDir() / logfile;
}
}

bool OpenDebugLog()
{
boost::call_once(&DebugPrintInit, debugPrintInitFlag);
boost::mutex::scoped_lock scoped_lock(*mutexDebugLog);

assert(fileout == nullptr);
assert(vMsgsBeforeOpenLog);
fs::path pathDebug = GetDataDir() / "debug.log";
fs::path pathDebug = GetDebugLogPath();

fileout = fsbridge::fopen(pathDebug, "a");
if (fileout) {
setbuf(fileout, nullptr); // unbuffered
// dump buffered messages from before we opened the log
while (!vMsgsBeforeOpenLog->empty()) {
FileWriteStr(vMsgsBeforeOpenLog->front(), fileout);
vMsgsBeforeOpenLog->pop_front();
}
if (!fileout) {
return false;
}

setbuf(fileout, nullptr); // unbuffered
// dump buffered messages from before we opened the log
while (!vMsgsBeforeOpenLog->empty()) {
FileWriteStr(vMsgsBeforeOpenLog->front(), fileout);
vMsgsBeforeOpenLog->pop_front();
}

delete vMsgsBeforeOpenLog;
vMsgsBeforeOpenLog = nullptr;
return true;
}

struct CLogCategoryDesc
Expand Down Expand Up @@ -355,7 +370,7 @@ int LogPrintStr(const std::string &str)
// reopen the log file, if requested
if (fReopenDebugLog) {
fReopenDebugLog = false;
fs::path pathDebug = GetDataDir() / "debug.log";
fs::path pathDebug = GetDebugLogPath();
if (fsbridge::freopen(pathDebug,"a",fileout) != nullptr)
setbuf(fileout, nullptr); // unbuffered
}
Expand Down Expand Up @@ -774,7 +789,7 @@ void ShrinkDebugFile()
// Amount of debug.log to save at end when shrinking (must fit in memory)
constexpr size_t RECENT_DEBUG_HISTORY_SIZE = 10 * 1000000;
// Scroll debug.log if it's getting too big
fs::path pathLog = GetDataDir() / "debug.log";
fs::path pathLog = GetDebugLogPath();
FILE* file = fsbridge::fopen(pathLog, "r");
// If debug.log file is more than 10% bigger the RECENT_DEBUG_HISTORY_SIZE
// trim it down by saving only the last RECENT_DEBUG_HISTORY_SIZE bytes
Expand Down
4 changes: 3 additions & 1 deletion src/util.h
Expand Up @@ -36,6 +36,7 @@ int64_t GetStartupTime();
static const bool DEFAULT_LOGTIMEMICROS = false;
static const bool DEFAULT_LOGIPS = false;
static const bool DEFAULT_LOGTIMESTAMPS = true;
extern const char * const DEFAULT_DEBUGLOGFILE;
Copy link
Contributor

Choose a reason for hiding this comment

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

Would have expected this to have accompanied

extern const char * const BITCOIN_CONF_FILENAME;
extern const char * const BITCOIN_PID_FILENAME;

lower in the file.

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought about that, but it's grouped with the other DEFAULT_ here, that makes more sense to me than grouping by type.

Copy link
Contributor

Choose a reason for hiding this comment

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

The others are defaults that can be overridden via an option; calling it BITCOIN_DEBUGLOG_FILENAME could make it fit by both name and type?

Copy link
Member Author

@laanwj laanwj Dec 1, 2017

Choose a reason for hiding this comment

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

I don't get you. This is the default for -debuglogfile, which can is an option. We use the convention DEFAULT_X where X is the option name everywhere. These are all constants that cannot be changed and are only used for getarg(...).

Copy link
Contributor

@ajtowns ajtowns Dec 1, 2017

Choose a reason for hiding this comment

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

Same thing is true for BITCOIN_{CONF,PID}_FILENAME - they're constants that can't be changed and are only used for getArg and HelpMessageOpt. But that's a better argument for renaming those to DEFAULT_X like almost every single other option, so nevermind.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree! the DEFAULT_ convention is more frequently used so would make sense to use everywhere. BITCOIN_CONF_FILENAME and BITCOIN_PID_FILENAME are very old options so they didn't follow that yet. Feel free to send a patch (but not to this PR as it's pretty out of scope :-)


/** Signals for translation. */
class CTranslationInterface
Expand Down Expand Up @@ -180,7 +181,8 @@ void CreatePidFile(const fs::path &path, pid_t pid);
#ifdef WIN32
fs::path GetSpecialFolderPath(int nFolder, bool fCreate = true);
#endif
void OpenDebugLog();
fs::path GetDebugLogPath();
bool OpenDebugLog();
void ShrinkDebugFile();
void runCommand(const std::string& strCommand);

Expand Down
59 changes: 59 additions & 0 deletions test/functional/feature_logging.py
@@ -0,0 +1,59 @@
#!/usr/bin/env python3
# Copyright (c) 2017 The Bitcoin Core developers
# Distributed under the MIT software license, see the accompanying
# file COPYING or http://www.opensource.org/licenses/mit-license.php.
"""Test debug logging."""

import os

from test_framework.test_framework import BitcoinTestFramework

class LoggingTest(BitcoinTestFramework):
def set_test_params(self):
self.num_nodes = 1
self.setup_clean_chain = True

def run_test(self):
# test default log file name
assert os.path.isfile(os.path.join(self.nodes[0].datadir, "regtest", "debug.log"))

# test alternative log file name in datadir
self.restart_node(0, ["-debuglogfile=foo.log"])
assert os.path.isfile(os.path.join(self.nodes[0].datadir, "regtest", "foo.log"))

# test alternative log file name outside datadir
tempname = os.path.join(self.options.tmpdir, "foo.log")
self.restart_node(0, ["-debuglogfile=%s" % tempname])
assert os.path.isfile(tempname)

# check that invalid log (relative) will cause error
invdir = os.path.join(self.nodes[0].datadir, "regtest", "foo")
invalidname = os.path.join("foo", "foo.log")
self.stop_node(0)
self.assert_start_raises_init_error(0, ["-debuglogfile=%s" % (invalidname)],
"Error: Could not open debug log file")
assert not os.path.isfile(os.path.join(invdir, "foo.log"))

# check that invalid log (relative) works after path exists
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: check that a previously invalid log (relative) works after path exists would be more accurate. Same for comment below.

self.stop_node(0)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: not required (the previous attempt to start the node failed)

os.mkdir(invdir)
self.start_node(0, ["-debuglogfile=%s" % (invalidname)])
assert os.path.isfile(os.path.join(invdir, "foo.log"))

# check that invalid log (absolute) will cause error
self.stop_node(0)
invdir = os.path.join(self.options.tmpdir, "foo")
invalidname = os.path.join(invdir, "foo.log")
self.assert_start_raises_init_error(0, ["-debuglogfile=%s" % invalidname],
"Error: Could not open debug log file")
assert not os.path.isfile(os.path.join(invdir, "foo.log"))

# check that invalid log (absolute) works after path exists
self.stop_node(0)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: not required (the previous attempt to start the node failed)

os.mkdir(invdir)
self.start_node(0, ["-debuglogfile=%s" % (invalidname)])
assert os.path.isfile(os.path.join(invdir, "foo.log"))


if __name__ == '__main__':
LoggingTest().main()
1 change: 1 addition & 0 deletions test/functional/test_runner.py
Expand Up @@ -126,6 +126,7 @@
'p2p-fingerprint.py',
'uacomment.py',
'p2p-acceptblock.py',
'feature_logging.py',
]

EXTENDED_SCRIPTS = [
Expand Down