Add -logdir option to control where debug.log lives #11741

eklitzke · 2017-11-20T22:05:53Z

I would like bitcoind to put its log file in /var/log/bitcoin. This PR adds a new -logdir option that controls where the debug.log file is put.

Note: This flag affects bitcoind and bitcoin-qt. There appears to be a db.log file that is used for wallets, but I didn't update that code path. It would be a minor change to update that as well.

meshcollider · 2017-11-20T22:44:28Z

Concept ACK
If only one file moves, I'd prefer just -log rather than logdir though

jonasschnelli · 2017-11-20T22:51:53Z

src/util.cpp

+  if (gArgs.IsArgSet("-logdir")) {
+    fs::path logdirPath = fs::system_complete(gArgs.GetArg("-logdir", ""));
+    if (!logdirPath.empty()) {
+      fs::create_directories(logdirPath);


Not sure if it should create directories,.. though if it does, in case the creation failed, it should not return an empty path, should rather return GetDataDir() in any failed case.

I don't think the code (as written) can return an empty path. If -logdir is unset the outer if statement is skipped completely, the inner if statement is to handle the case where -logdir is set to the empty string. As I understand the boost::filesystem docs, if the mkdir fails then fs::create_directories will actually throw an exception.

I'm OK with removing the fs::create_directories call, the precedent I was following is that GetDataDir() creates its directory, although that's a bit different.

Hmm.. not sure. I just found this in the docs: Returns: true if a new directory was created, otherwise false.

As I understand the documentation: fs::create_directories returns true if it actually had to issue a mkdir(2) call, and returns false if it did not (because the directory already existed). If the directory needs to be created and the underlying mkdir(2) call failed, then an exception is raised.

Oh.. yes.. your completely right. Nevermind then.

eklitzke · 2017-11-20T23:55:16Z

@meshcollider Do you think it's possible there would be other log files in the future? I'm interested in the future on working on a multi-process architecture for bitcoind, so that parts of the process could be sandboxed on Linux using seccomp(). In that world I'd want a log directory so there could be a log file per process, but maybe I'm overthinking things since there's just one log file today.

promag · 2017-11-21T00:00:42Z

Running regtest and testnet will use the same log file?

eklitzke · 2017-11-21T04:19:45Z

@promag Can you explain further? That may be the case, but I am somewhat new and don't understand the issue you're describing.

jonasschnelli · 2017-11-21T06:12:53Z

@eklitzke. I think what @promag means is, that in the default configuration (without the PR), the debug.log file is placed in <datadir>/testnet/ or <datadir>/regtest/ in case of testnet / regtest. Your logdir command would required one to switch the logdir when switching to -regtest, etc. Not sure if it's a real problem...

laanwj · 2017-11-21T07:34:51Z

If only one file moves, I'd prefer just -log rather than logdir though

Would prefer just -logfile or such too. The log is just a text file, it needs to claim no environment, and a directory is not a good abstraction for it. If someone wants bitcoin to log to an existing directory with other log files e.g. /var/log/bitcoinlog that should be possible. The only requirement is write access.

so that parts of the process could be sandboxed on Linux using seccomp().

FWIW in my experiments with running bitcoin with CloudABI I've used -printtoconsole to log to a stream instead. This is much easier in restricted environments, and the stream can be processed and stored anywhere desired by an external process. I usually recommend that for more advanced logging setups.

Do you think it's possible there would be other log files in the future?

Possible - maybe an auditing / RPC log instead of a debug log. However there is no reason it'd need to be in the same directory, this could have a new option to place it.

There appears to be a db.log file that is used for wallets, but I didn't update that code path. It would be a minor change to update that as well.

db.log belongs with the wallet directory, no need to touch it here.

promag · 2017-11-21T15:37:25Z

it needs to claim no environment

@laanwj what do you mean? Currently there is debug.log, testnet3/debug.log and regtest/debug.log.

laanwj · 2017-11-21T15:47:44Z

@laanwj what do you mean? Currently there is debug.log, testnet3/debug.log and regtest/debug.log.

Yes, the regtest/testnet edge case again. It really sucks that we have these configuration options shared between networks.
It's not clear that users even want that layout when they manually specify the logfile! A similar issue to #11726.
If we can get per-network configuration in (e.g. #10996) then most of this nonsense will go away.

ajtowns · 2017-11-23T03:43:52Z

I'm not sure per-network configuration gets the nonsense to go away :( There's three ways that it could work, I think:

logs always go in -logdir, if you start a regtest/testnet instance having specified it in bitcoin.conf, your logs get messy, you get told to RTFM, and cry bitter tears
if specified in bitcoin.conf, logdir only affects mainnet; but you can specify logdir via network.conf to choose different locations. (or logdir in bitcoin.conf is an error/ignored)
-logdir only has an effect on mainnet, you have to use -regtest-logdir or -testnet-logdir or similar to have debug.log go elsewhere

I think the last one seems like it might be the least surprising behaviour? I've got some draft patches for both the latter options though.

meshcollider · 2017-11-23T10:47:04Z

@ajtowns I definitely prefer the first or second of those 3 options, I don't want to see a -regtest-logdir and -testnet-logdir argument as well, I think that's much too messy. Alternatively if we stick with -logdir instead of -logfile, we just call the log chain.log e.g. regtest.log, testnet3.log

laanwj · 2017-11-28T09:09:38Z

logs always go in -logdir, if you start a regtest/testnet instance having specified it in bitcoin.conf, your logs get messy, you get told to RTFM, and cry bitter tears

For the log it'd be remotely acceptable, but you'd never want a similar thing to happen for wallets.

This is nothing new, by the way!

-port -bind -rpcport -rpcbind already suffer from this. They are really options that one would want to specify per network.

-logdir only has an effect on mainnet, you have to use -regtest-logdir or -testnet-logdir or similar to have debug.log go elsewhere

I'm starting to like this as this is explicit, they don't depend on context, e.g., what file something was specified it. It works as well with -conf=<file> as with the automatic way of finding configuration files. But it's a major change to argument handling nevertheless...

5a7c09a test: Add tests for `-debuglogfile` with subdirs (Anthony Towns) 4158734 doc: Update release notes for `-debuglogfile` (Wladimir J. van der Laan) 2323242 test: Add test for `-debuglogfile` (Wladimir J. van der Laan) cf5f432 Add `-debuglogfile` option (Wladimir J. van der Laan) Pull request description: This patch adds an option to configure the name and/or directory of the debug log file. The user can specify either a relative path, in which case the path is relative to the (network specific) data directory. They can also specify an absolute path to put the log anywhere else in the file system. Alternative to #11741 that gets rid of the concept of a "log directory" by specifying the path for the specific kind of log, the debug log. Which happens to be the only kind of log we have at this point*, but a hypothetical new kind of log (say, an audit log) would get a new option. This has more flexibility than specifying a directory which has to contain all of them. \* excluding `db.log` which is internally generated by the wallet database library, but that one moves along with `-walletdir`. Tree-SHA512: 4434d0e598dc23504e5c9e67fdbaef56db4f0fd490f9f54fd503e69d4dda9b5b69c539e1794ed841e72161b7b1dc3374d2f1193dd431b057566750e56fd8f24b

laanwj · 2017-12-06T14:16:10Z

Closing because #11781 was merged which covers this.

5a7c09a test: Add tests for `-debuglogfile` with subdirs (Anthony Towns) 4158734 doc: Update release notes for `-debuglogfile` (Wladimir J. van der Laan) 2323242 test: Add test for `-debuglogfile` (Wladimir J. van der Laan) cf5f432 Add `-debuglogfile` option (Wladimir J. van der Laan) Pull request description: This patch adds an option to configure the name and/or directory of the debug log file. The user can specify either a relative path, in which case the path is relative to the (network specific) data directory. They can also specify an absolute path to put the log anywhere else in the file system. Alternative to bitcoin#11741 that gets rid of the concept of a "log directory" by specifying the path for the specific kind of log, the debug log. Which happens to be the only kind of log we have at this point*, but a hypothetical new kind of log (say, an audit log) would get a new option. This has more flexibility than specifying a directory which has to contain all of them. \* excluding `db.log` which is internally generated by the wallet database library, but that one moves along with `-walletdir`. Tree-SHA512: 4434d0e598dc23504e5c9e67fdbaef56db4f0fd490f9f54fd503e69d4dda9b5b69c539e1794ed841e72161b7b1dc3374d2f1193dd431b057566750e56fd8f24b

add -logdir option

7832efa

fanquake added the Docs label Nov 20, 2017

jonasschnelli reviewed Nov 20, 2017

View reviewed changes

laanwj mentioned this pull request Nov 28, 2017

Add -debuglogfile option #11781

Merged

laanwj added Utils/log/libs and removed Docs labels Nov 30, 2017

laanwj closed this Dec 6, 2017

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

Add -logdir option to control where debug.log lives #11741

Add -logdir option to control where debug.log lives #11741

Uh oh!

Conversation

eklitzke commented Nov 20, 2017

Uh oh!

meshcollider commented Nov 20, 2017

Uh oh!

jonasschnelli Nov 20, 2017

Choose a reason for hiding this comment

Uh oh!

eklitzke Nov 20, 2017

Choose a reason for hiding this comment

Uh oh!

jonasschnelli Nov 21, 2017

Choose a reason for hiding this comment

Uh oh!

eklitzke Nov 21, 2017

Choose a reason for hiding this comment

Uh oh!

jonasschnelli Nov 21, 2017

Choose a reason for hiding this comment

Uh oh!

eklitzke commented Nov 20, 2017

Uh oh!

promag commented Nov 21, 2017

Uh oh!

eklitzke commented Nov 21, 2017

Uh oh!

jonasschnelli commented Nov 21, 2017

Uh oh!

laanwj commented Nov 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Nov 21, 2017

Uh oh!

laanwj commented Nov 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ajtowns commented Nov 23, 2017

Uh oh!

meshcollider commented Nov 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Nov 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Dec 6, 2017

Uh oh!

Uh oh!

laanwj commented Nov 21, 2017 •

edited

Loading

laanwj commented Nov 21, 2017 •

edited

Loading

meshcollider commented Nov 23, 2017 •

edited

Loading

laanwj commented Nov 28, 2017 •

edited

Loading