Improve PID file error handling

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:

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

02019-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:

02019-02-13T22:08:36Z Error: Unable to create the PID file '/.../.bitcoin/regtest/bitcoind.pid': Permission denied
1Error: 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

Improve PID file error handling #15278

Conflicts