Improve PID file error handling #15278

hebasto · 2019-01-28T20:29:27Z

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:

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.

laanwj · 2019-01-31T12:07:45Z

src/util/system.cpp

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.

laanwj · 2019-01-31T12:08:06Z

Concept ACK, thanks for adding more relevant logging for diagnostics.

practicalswift · 2019-02-01T20:09:07Z

Concept ACK. I suggest adding the NODISCARD annotations to fs::remove and CreatePidFile (after making CreatePidFile return an error status as suggested by @laanwj).

DrahtBot · 2019-02-01T23:08:07Z

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 · 2019-02-01T23:24:11Z

@laanwj @practicalswift thank you for your reviews.
Your comments have been addressed. PR description updated.

laanwj · 2019-02-12T13:11:35Z

utACK 561e375
Thanks for fixing this

laanwj · 2019-02-13T21:57:30Z

src/init.cpp

 }

+#ifndef WIN32
+NODISCARD static bool CreatePidFile()


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

laanwj · 2019-02-13T22:09:32Z

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.

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

hebasto · 2019-02-14T21:03:08Z

@laanwj's #15278 (comment) has been addressed.

laanwj · 2019-02-21T08:23:07Z

utACK 3782075

@laanwj

3782075 Move all PID file stuff to init.cpp (Hennadii Stepanov) 561e375 Make PID file creating errors fatal (Hennadii Stepanov) 745a2ac Improve PID file removing errors logging (Hennadii Stepanov) Pull request description: Digging into #15240 the lack of the proper logging has been discovered. Fixed by this PR. UPDATE (inspired by @laanwj's [comment](#15278 (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](https://user-images.githubusercontent.com/32963518/52154886-9349b600-2688-11e9-8128-470f16790305.png) **Notes for reviewers** 1. `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 2. Regarding 0.18 release process: this PR modifies 1 string and introduces 2 new ones. Tree-SHA512: ac07d0f800e61ec759e427d0afc0ca43d67f232e977662253963afdd0a220d34b871050f58149fc9fabd427bfc8e0d3f6a6032f2a38f30ad366fc0d074b0f2b3

Summary: * Improve PID file removing errors logging * Make PID file creating errors fatal * Move all PID file stuff to init.cpp It is only used from init.cpp. Move-only refactoring. This is a backport of Core [[bitcoin/bitcoin#15278 | PR15278]] The linter was skipped because we don't have that linter. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D5775

@laanwj

3782075 Move all PID file stuff to init.cpp (Hennadii Stepanov) 561e375 Make PID file creating errors fatal (Hennadii Stepanov) 745a2ac Improve PID file removing errors logging (Hennadii Stepanov) Pull request description: Digging into bitcoin#15240 the lack of the proper logging has been discovered. Fixed by this PR. UPDATE (inspired by @laanwj's [comment](bitcoin#15278 (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](https://user-images.githubusercontent.com/32963518/52154886-9349b600-2688-11e9-8128-470f16790305.png) **Notes for reviewers** 1. `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 2. Regarding 0.18 release process: this PR modifies 1 string and introduces 2 new ones. Tree-SHA512: ac07d0f800e61ec759e427d0afc0ca43d67f232e977662253963afdd0a220d34b871050f58149fc9fabd427bfc8e0d3f6a6032f2a38f30ad366fc0d074b0f2b3 Signed-off-by: pasta <[email protected]>

@laanwj

3782075 Move all PID file stuff to init.cpp (Hennadii Stepanov) 561e375 Make PID file creating errors fatal (Hennadii Stepanov) 745a2ac Improve PID file removing errors logging (Hennadii Stepanov) Pull request description: Digging into bitcoin#15240 the lack of the proper logging has been discovered. Fixed by this PR. UPDATE (inspired by @laanwj's [comment](bitcoin#15278 (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](https://user-images.githubusercontent.com/32963518/52154886-9349b600-2688-11e9-8128-470f16790305.png) **Notes for reviewers** 1. `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 2. Regarding 0.18 release process: this PR modifies 1 string and introduces 2 new ones. Tree-SHA512: ac07d0f800e61ec759e427d0afc0ca43d67f232e977662253963afdd0a220d34b871050f58149fc9fabd427bfc8e0d3f6a6032f2a38f30ad366fc0d074b0f2b3 Merge bitcoin#15278: Improve PID file error handling

fanquake added the Utils/log/libs label Jan 28, 2019

laanwj reviewed Jan 31, 2019

View reviewed changes

Improve PID file removing errors logging

745a2ac

hebasto force-pushed the 20190128-pidfile-logging branch from f5fb1bc to 8b4c5ba Compare February 1, 2019 22:52

Make PID file creating errors fatal

561e375

hebasto force-pushed the 20190128-pidfile-logging branch from 8b4c5ba to 561e375 Compare February 1, 2019 23:07

hebasto changed the title ~~Improve pidfile logging~~ Improve PID file error handling Feb 1, 2019

laanwj reviewed Feb 13, 2019

View reviewed changes

Move all PID file stuff to init.cpp

3782075

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

laanwj merged commit 3782075 into bitcoin:master Feb 21, 2019

hebasto deleted the 20190128-pidfile-logging branch February 22, 2019 18:44

riordant mentioned this pull request Feb 23, 2019

Enable PID file creation on WIN #15456

Merged

ghost mentioned this pull request Apr 28, 2019

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

Closed

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Improve PID file error handling #15278

Improve PID file error handling #15278

Uh oh!

Conversation

hebasto commented Jan 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj Jan 31, 2019

Choose a reason for hiding this comment

Uh oh!

hebasto Feb 1, 2019

Choose a reason for hiding this comment

Uh oh!

laanwj commented Jan 31, 2019

Uh oh!

practicalswift commented Feb 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Feb 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

hebasto commented Feb 1, 2019

Uh oh!

laanwj commented Feb 12, 2019

Uh oh!

laanwj Feb 13, 2019

Choose a reason for hiding this comment

Uh oh!

hebasto Feb 14, 2019

Choose a reason for hiding this comment

Uh oh!

laanwj commented Feb 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Feb 14, 2019

Uh oh!

laanwj commented Feb 21, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

hebasto commented Jan 28, 2019 •

edited

Loading

practicalswift commented Feb 1, 2019 •

edited

Loading

DrahtBot commented Feb 1, 2019 •

edited

Loading

laanwj commented Feb 13, 2019 •

edited

Loading