Improve PID file error handling #15278

pull hebasto wants to merge 3 commits into bitcoin:master from hebasto:20190128-pidfile-logging changing 4 files +35 −27

hebasto commented at 8:29 PM on January 28, 2019: member

Digging into #15240 the lack of the proper logging has been discovered. Fixed by this PR.

UPDATE (inspired by @laanwj's comment): Not being able to create the PID file is fatal now.

Output of bitcoind:

$ src/bitcoind -pid=/run/bitcoind/bitcoind.pid
2019-02-01T23:20:10Z Bitcoin Core version v0.17.99.0-561e375c7 (release build)
2019-02-01T23:20:10Z Assuming ancestors of block 0000000000000037a8cd3e06cd5edbfe9dd1dbcc5dacab279376ef7cfc2b4c75 have valid signatures.
2019-02-01T23:20:10Z Setting nMinimumChainWork=00000000000000000000000000000000000000000000007dbe94253893cbd463
2019-02-01T23:20:10Z Using the 'sse4(1way),sse41(4way),avx2(8way)' SHA256 implementation
2019-02-01T23:20:10Z Using RdRand as an additional entropy source
2019-02-01T23:20:11Z Error: Unable to create the PID file '/run/bitcoind/bitcoind.pid': No such file or directory
Error: Unable to create the PID file '/run/bitcoind/bitcoind.pid': No such file or directory
2019-02-01T23:20:11Z Shutdown: In progress...
2019-02-01T23:20:11Z Shutdown: Unable to remove PID file: File does not exist
2019-02-01T23:20:11Z Shutdown: done

Output of bitcoin-qt: screenshot from 2019-02-02 01-19-05

Notes for reviewers

CreatePidFile() has been moved from util/system.cpp to init.cpp for the following reasons:

to get the ability to use InitError()
now init.cpp contains code of both creating PID file and removing it

Regarding 0.18 release process: this PR modifies 1 string and introduces 2 new ones.

fanquake added the label Utils/log/libs on Jan 28, 2019
in src/util/system.cpp:968 in f5fb1bc0cb outdated
```
 967 | @@ -967,10 +968,11 @@ fs::path GetPidFile()
 968 |  void CreatePidFile(const fs::path &path, pid_t pid)
```
laanwj commented at 12:07 PM on January 31, 2019:

I don't quite understand why this doesn't return an error status, and that not being able to create the PID file is not fatal.

hebasto commented at 11:25 PM on February 1, 2019:

Fixed.
laanwj commented at 12:08 PM on January 31, 2019: member

Concept ACK, thanks for adding more relevant logging for diagnostics.
practicalswift commented at 8:09 PM on February 1, 2019: contributor

Concept ACK. I suggest adding the NODISCARD annotations to fs::remove and CreatePidFile (after making CreatePidFile return an error status as suggested by @laanwj).
Improve PID file removing errors logging 745a2ace18
hebasto force-pushed on Feb 1, 2019
Make PID file creating errors fatal 561e375c73
hebasto force-pushed on Feb 1, 2019
DrahtBot commented at 11:08 PM on February 1, 2019: member


The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #15454 (Remove the automatic creation and loading of the default wallet by achow101)
- #14776 (Add process based prctl spectre mitigation controls. by jameshilliard)
- #14045 (refactor: Fix the chainparamsbase -> util circular dependency by Empact)
- #13746 (-masterdatadir for datadir bootstrapping by kallewoof)
- #12833 ([qt] move QSettings to bitcoin_rw.conf where possible by Sjors)
- #11082 (Add new bitcoin_rw.conf file that is used for settings modified by this software itself by luke-jr)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
hebasto renamed this:
~~Improve pidfile logging~~
Improve PID file error handling
on Feb 1, 2019
hebasto commented at 11:24 PM on February 1, 2019: member

@laanwj @practicalswift thank you for your reviews. Your comments have been addressed. PR description updated.
laanwj commented at 1:11 PM on February 12, 2019: member

utACK 561e375c73a37934fe77a519762d81edf7a3325c Thanks for fixing this
in src/init.cpp:1198 in 561e375c73 outdated
```
1193 | @@ -1190,12 +1194,29 @@ bool AppInitLockDataDirectory()
1194 |      return true;
1195 |  }
1196 |  
1197 | +#ifndef WIN32
1198 | +NODISCARD static bool CreatePidFile()
```
laanwj commented at 9:57 PM on February 13, 2019:

If you're moving this function anyway, why not move GetPidFile as well, it's only used from init.cpp too

hebasto commented at 9:03 PM on February 14, 2019:

Fixed.

laanwj commented at 10:09 PM on February 13, 2019: member

Tested:

Removing the PId file while running, this results in the following warning in the log:

2019-02-13T22:07:02Z Shutdown: Unable to remove PID file: File does not exist

After making the pid file unaccessible by chowning it to root:

2019-02-13T22:08:36Z Error: Unable to create the PID file '/.../.bitcoin/regtest/bitcoind.pid': Permission denied
Error: Unable to create the PID file '/.../.bitcoin/regtest/bitcoind.pid': Permission deniez

Looks good to me.

Move all PID file stuff to init.cpp

It is only used from init.cpp.
Move-only refactoring.

3782075a5f

hebasto commented at 9:03 PM on February 14, 2019: member

@laanwj's #15278 (review) has been addressed.
laanwj commented at 8:23 AM on February 21, 2019: member

utACK 3782075a5fd4ad0c15a6119e8cdaf136898f679e
laanwj merged this on Feb 21, 2019
laanwj closed this on Feb 21, 2019
laanwj referenced this in commit 3e1ca1348c on Feb 21, 2019
hebasto deleted the branch on Feb 22, 2019
deadalnix referenced this in commit 06c00817f2 on Apr 20, 2020
ftrader referenced this in commit 2b3a31e3ff on Aug 17, 2020
PastaPastaPasta referenced this in commit 4f0648a09f on Aug 16, 2021
vijaydasmp referenced this in commit 13680a7bbf on Sep 5, 2021
vijaydasmp referenced this in commit 6a2c6ac2a9 on Sep 6, 2021
DrahtBot locked this on Dec 16, 2021

Contributors

Labels

Linked (view graph)

#15240 bitcoind wont start with datadir specified in conf file and no home dir

Improve PID file error handling #15278

Conflicts