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

Don't close old debug log file handle prematurely when trying to re-open (on SIGHUP) #13159

Merged
merged 2 commits into from Aug 31, 2018

Conversation

@practicalswift
Copy link
Contributor

@practicalswift practicalswift commented May 3, 2018

Don't close old debug log file handle prematurely when trying to re-open (on SIGHUP).

Context: #13148 (comment)

Thanks @ajtowns!

@practicalswift practicalswift changed the title Don't close old debug log file handler prematurely when trying to re-open (on SIGHUP) Don't close old debug log file handle prematurely when trying to re-open (on SIGHUP) May 3, 2018
@practicalswift practicalswift force-pushed the practicalswift:handle-reopen-failed branch 2 times, most recently May 3, 2018
Copy link
Member

@luke-jr luke-jr left a comment

utACK

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Jul 7, 2018
…pen (on SIGHUP)

Github-Pull: bitcoin#13159
Rebased-From: c4671629c5ee430238222d43f140ca8565c028b1
@DrahtBot
Copy link
Contributor

@DrahtBot DrahtBot commented Jul 22, 2018

The last travis run for this pull request was 80 days ago and is thus outdated. To trigger a fresh travis build, this pull request should be closed and re-opened.
@ajtowns
Copy link
Contributor

@ajtowns ajtowns commented Aug 7, 2018

Tested ACK 37efe5b7ea059e56d8f5170c91e3160db286cad4

To test:

$ bitcoind -regtest -daemon
$ cd ~/.bitcoin/regtest
$ mv debug.log{,.old}
$ touch debug.log; chmod 400 debug.log
$ killall -HUP bitcoind
$ bitcoin-cli -regtest savemempool

Current behaviour of master is that the "Dumped mempool" message (along with any other subsequent log messages) aren't logged to a file, and further that fixing permissions on debug.log and HUP'ing the process isn't sufficient to re-enable logging to file.

With the patch, logging continues in debug.log.old, and after fixing the permissions HUP'ing the process causes logging to move to the new file.

@MarcoFalke
Copy link
Member

@MarcoFalke MarcoFalke commented Aug 20, 2018

utACK 37efe5b7ea059e56d8f5170c91e3160db286cad4

@practicalswift practicalswift force-pushed the practicalswift:handle-reopen-failed branch to 75ea00f Aug 29, 2018
@practicalswift
Copy link
Contributor Author

@practicalswift practicalswift commented Aug 29, 2018

Rebased!

@luke-jr @ajtowns @MarcoFalke Please re-review :-)

@MarcoFalke
Copy link
Member

@MarcoFalke MarcoFalke commented Aug 29, 2018

re-utACK 75ea00f

@ajtowns
Copy link
Contributor

@ajtowns ajtowns commented Aug 30, 2018

re-ACK 75ea00f

@laanwj
Copy link
Member

@laanwj laanwj commented Aug 31, 2018

utACK 75ea00f, good to get rid of the need of freopen I suppose

@laanwj laanwj merged commit 75ea00f into bitcoin:master Aug 31, 2018
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
laanwj added a commit that referenced this pull request Aug 31, 2018
…trying to re-open (on SIGHUP)

75ea00f Remove unused fsbridge::freopen (practicalswift)
cceedbc Don't close old debug log file handle prematurely when trying to re-open (on SIGHUP) (practicalswift)

Pull request description:

  Don't close old debug log file handle prematurely when trying to re-open (on `SIGHUP`).

  Context: #13148 (comment)

  Thanks @ajtowns!

Tree-SHA512: c436b4286f00fc428b60269b6d6321f435c72c7ccec3c15b2194aac71196529b30f32c2384b418ffe3ed67ba7ee8ec51f4c9c5748e65945697c0437eafcdacd1
jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Sep 13, 2019
…trying to re-open (on SIGHUP)

Summary:
75ea00f391b742e435c650aae3e827aad913d552 Remove unused fsbridge::freopen (practicalswift)
cceedbc4bf1056db17e0adf76d0db45b94777671 Don't close old debug log file handle prematurely when trying to re-open (on SIGHUP) (practicalswift)

Pull request description:

  Don't close old debug log file handle prematurely when trying to re-open (on `SIGHUP`).

  Context: bitcoin/bitcoin#13148 (comment)

  Thanks @ajtowns!

Tree-SHA512: c436b4286f00fc428b60269b6d6321f435c72c7ccec3c15b2194aac71196529b30f32c2384b418ffe3ed67ba7ee8ec51f4c9c5748e65945697c0437eafcdacd1

Backport of Core PR13159
bitcoin/bitcoin#13159

Depends on D3976

Test Plan:
  make check
  ./bitcoind -regtest -daemon
  cd <path>/.bitcoin/regtest
  mv debug.log{,.old}
  touch debug.log; chmod 400 debug.log
  killall -HUP bitcoind
  <patch>/bitcoin-abc/build/src/bitcoin-cli -regtest savemempool
Verify that the new debug.log file is empty and debug.log.old continued to be written to

Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc

Reviewed By: Fabien, O1 Bitcoin ABC, #bitcoin_abc

Differential Revision: https://reviews.bitcoinabc.org/D3979
jonspock added a commit to jonspock/devault that referenced this pull request Dec 22, 2019
…trying to re-open (on SIGHUP)

Summary:
75ea00f391b742e435c650aae3e827aad913d552 Remove unused fsbridge::freopen (practicalswift)
cceedbc4bf1056db17e0adf76d0db45b94777671 Don't close old debug log file handle prematurely when trying to re-open (on SIGHUP) (practicalswift)

Pull request description:

  Don't close old debug log file handle prematurely when trying to re-open (on `SIGHUP`).

  Context: bitcoin/bitcoin#13148 (comment)

  Thanks @ajtowns!

Tree-SHA512: c436b4286f00fc428b60269b6d6321f435c72c7ccec3c15b2194aac71196529b30f32c2384b418ffe3ed67ba7ee8ec51f4c9c5748e65945697c0437eafcdacd1

Backport of Core PR13159
bitcoin/bitcoin#13159

Depends on D3976

Test Plan:
  make check
  ./bitcoind -regtest -daemon
  cd <path>/.bitcoin/regtest
  mv debug.log{,.old}
  touch debug.log; chmod 400 debug.log
  killall -HUP bitcoind
  <patch>/bitcoin-abc/build/src/bitcoin-cli -regtest savemempool
Verify that the new debug.log file is empty and debug.log.old continued to be written to

Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc

Reviewed By: Fabien, O1 Bitcoin ABC, #bitcoin_abc

Differential Revision: https://reviews.bitcoinabc.org/D3979
proteanx added a commit to devaultcrypto/devault that referenced this pull request Dec 23, 2019
…trying to re-open (on SIGHUP)

Summary:
75ea00f391b742e435c650aae3e827aad913d552 Remove unused fsbridge::freopen (practicalswift)
cceedbc4bf1056db17e0adf76d0db45b94777671 Don't close old debug log file handle prematurely when trying to re-open (on SIGHUP) (practicalswift)

Pull request description:

  Don't close old debug log file handle prematurely when trying to re-open (on `SIGHUP`).

  Context: bitcoin/bitcoin#13148 (comment)

  Thanks @ajtowns!

Tree-SHA512: c436b4286f00fc428b60269b6d6321f435c72c7ccec3c15b2194aac71196529b30f32c2384b418ffe3ed67ba7ee8ec51f4c9c5748e65945697c0437eafcdacd1

Backport of Core PR13159
bitcoin/bitcoin#13159

Depends on D3976

Test Plan:
  make check
  ./bitcoind -regtest -daemon
  cd <path>/.bitcoin/regtest
  mv debug.log{,.old}
  touch debug.log; chmod 400 debug.log
  killall -HUP bitcoind
  <patch>/bitcoin-abc/build/src/bitcoin-cli -regtest savemempool
Verify that the new debug.log file is empty and debug.log.old continued to be written to

Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc

Reviewed By: Fabien, O1 Bitcoin ABC, #bitcoin_abc

Differential Revision: https://reviews.bitcoinabc.org/D3979
Fuzzbawls added a commit to PIVX-Project/PIVX that referenced this pull request Feb 14, 2021
84b4185 [Tests] raise minimum fee in feature_blockindexstats (random-zebra)
39d8a20 [Cleanup] Remove OldSetKeyFromPassphrase/OldEncrypt/OldDecrypt (random-zebra)
613e1da [BUG] Fix sizeof in paymentservertests (random-zebra)
0f3e961 [Cleanup] Remove useless call (random-zebra)
b1ca5e0 Remove unused fsbridge::freopen (practicalswift)
b3a1d84 Don't close old debug log file handle prematurely when trying to re-open (on SIGHUP) (practicalswift)
ecfbcfd [Cleanup] Remove unused function AddInvalidSpendsToMap (random-zebra)
15b018c [Refactor] Use InsecureRand in the unit tests (random-zebra)
81fd84c [Refactoring] Replace risky call to rand() with GetRandInt() (random-zebra)
8ba1978 [Qt] Terminate string *pszExePath after readlink and without using memset (practicalswift)
b6cd719 Remove unreachable code (g_rpcSignals.PostCommand) (practicalswift)
e4f9ab3 [BUG] Memory leak after new CNode / ConnectNode (random-zebra)
536122b Avoid rollingMinimumFeeRate never being able to decay below half (Alex Morcos)
13cad19 fix a bug if the min fee is 0 for FeeFilterRounder (Alex Morcos)
aa832d8 [Refactoring] Dereference before/after null check (random-zebra)
4630b7d [Trivial] Pass big parameters by reference, not value (random-zebra)
aa7bc7f [Refactoring] Remove unneeded CScript::IsNormalPaymentScript (random-zebra)
6cf3c8f [Trivial] Unitialized scalar fields and variables (random-zebra)

Pull request description:

  This is a collection of small fixes for the following issues/defects (discovered with the coverity tool):

  - Big parameters passed by value
  - calls to rand() function
  - Dereference before/after null-pointer check
  - Pointer to local variable out of scope
  - Resource leak with call to ConnectNode
  - Non-floating point result - unintended integer division (bitcoin#9288)
  - String not-null-terminated (bitcoin#11193)
  - Structurally dead code (bitcoin#9575)
  - Unchecked boolean return value of functions (GetOp, GetTransaction, TxOutToPublicCoin)
  - Unitialized pointers and scalar fields
  - Illegal memory access (bitcoin#13159)
  - Useless call to pubkey.IsCompressed()
  - Wrong `sizeof` argument (bitcoin#11024 + revert #494)

ACKs for top commit:
  furszy:
    Looking good, ACK 84b4185
  Fuzzbawls:
    ACK 84b4185

Tree-SHA512: 4c920a1858ccde65795c090e4becc47c50c0a78db7ab11935b4d3e2bea3f7f1f8ca3b48ce633d8112673092c35ae7cd05c444ed2b16e283a305c765874e55c1c
@practicalswift practicalswift deleted the practicalswift:handle-reopen-failed branch Apr 10, 2021
Munkybooty added a commit to Munkybooty/dash that referenced this pull request Jun 30, 2021
…y when trying to re-open (on SIGHUP)

75ea00f Remove unused fsbridge::freopen (practicalswift)
cceedbc Don't close old debug log file handle prematurely when trying to re-open (on SIGHUP) (practicalswift)

Pull request description:

  Don't close old debug log file handle prematurely when trying to re-open (on `SIGHUP`).

  Context: bitcoin#13148 (comment)

  Thanks @ajtowns!

Tree-SHA512: c436b4286f00fc428b60269b6d6321f435c72c7ccec3c15b2194aac71196529b30f32c2384b418ffe3ed67ba7ee8ec51f4c9c5748e65945697c0437eafcdacd1

# Conflicts:
#	src/logging.cpp
Munkybooty added a commit to Munkybooty/dash that referenced this pull request Jul 2, 2021
…y when trying to re-open (on SIGHUP)

75ea00f Remove unused fsbridge::freopen (practicalswift)
cceedbc Don't close old debug log file handle prematurely when trying to re-open (on SIGHUP) (practicalswift)

Pull request description:

  Don't close old debug log file handle prematurely when trying to re-open (on `SIGHUP`).

  Context: bitcoin#13148 (comment)

  Thanks @ajtowns!

Tree-SHA512: c436b4286f00fc428b60269b6d6321f435c72c7ccec3c15b2194aac71196529b30f32c2384b418ffe3ed67ba7ee8ec51f4c9c5748e65945697c0437eafcdacd1

# Conflicts:
#	src/logging.cpp
Munkybooty added a commit to Munkybooty/dash that referenced this pull request Jul 2, 2021
…y when trying to re-open (on SIGHUP)

75ea00f Remove unused fsbridge::freopen (practicalswift)
cceedbc Don't close old debug log file handle prematurely when trying to re-open (on SIGHUP) (practicalswift)

Pull request description:

  Don't close old debug log file handle prematurely when trying to re-open (on `SIGHUP`).

  Context: bitcoin#13148 (comment)

  Thanks @ajtowns!

Tree-SHA512: c436b4286f00fc428b60269b6d6321f435c72c7ccec3c15b2194aac71196529b30f32c2384b418ffe3ed67ba7ee8ec51f4c9c5748e65945697c0437eafcdacd1

# Conflicts:
#	src/logging.cpp
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants