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

Handle unsuccessful fseek(...):s #13149

Merged
merged 2 commits into from May 7, 2018

Conversation

Projects
None yet
4 participants
@practicalswift
Copy link
Member

practicalswift commented May 2, 2018

Handle unsuccessful fseek(...):s.

Note to reviewers: What is the most appropriate course of actions for each of these unsuccessful fseek(...):s?

@fanquake fanquake added the Refactoring label May 2, 2018

@laanwj

laanwj approved these changes May 2, 2018

@@ -887,7 +887,9 @@ void AllocateFileRange(FILE *file, unsigned int offset, unsigned int length) {
// Fallback version
// TODO: just write one byte per block
static const char buf[65536] = {};
fseek(file, offset, SEEK_SET);
if (fseek(file, offset, SEEK_SET)) {
return;

This comment has been minimized.

@laanwj

laanwj May 2, 2018

Member

Just returning here is fine - this call is only advisory.

@@ -254,7 +254,10 @@ void BCLog::Logger::ShrinkDebugFile()
{
// Restart the file with some of the end
std::vector<char> vch(RECENT_DEBUG_HISTORY_SIZE, 0);
fseek(file, -((long)vch.size()), SEEK_END);
if (fseek(file, -((long)vch.size()), SEEK_END)) {

This comment has been minimized.

@laanwj

laanwj May 2, 2018

Member

Not sure what should be done here. As-is it will silently fail to shrink the log file but should continue logging otherwise? Maybe log an error message?

This comment has been minimized.

@jonasschnelli

jonasschnelli May 3, 2018

Member

Agree with @laanwj: an additional error message would make sense.

@@ -268,7 +268,9 @@ bool TxIndex::FindTx(const uint256& tx_hash, uint256& block_hash, CTransactionRe
CBlockHeader header;
try {
file >> header;
fseek(file.Get(), postx.nTxOffset, SEEK_CUR);
if (fseek(file.Get(), postx.nTxOffset, SEEK_CUR)) {
return error("%s: fseek(...) failed", __func__);

This comment has been minimized.

@laanwj

laanwj May 2, 2018

Member

LGTM

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented May 3, 2018

utACK 29c9bdc (though a LogPrint in ShrinkDebugFile() would be even better).

@practicalswift

This comment has been minimized.

Copy link
Member Author

practicalswift commented May 3, 2018

@laanwj @jonasschnelli Regarding logging in ShrinkDebugFile(…) – what about the following:

diff --git a/src/logging.cpp b/src/logging.cpp
index 2a55d36..b57b17e 100644
--- a/src/logging.cpp
+++ b/src/logging.cpp
@@ -255,6 +255,7 @@ void BCLog::Logger::ShrinkDebugFile()
         // Restart the file with some of the end
         std::vector<char> vch(RECENT_DEBUG_HISTORY_SIZE, 0);
         if (fseek(file, -((long)vch.size()), SEEK_END)) {
+            LogPrintf("Failed to shrink debug file: fseek(...) failed\n");
             fclose(file);
             return;
         }

Looks good?

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented May 3, 2018

@practicalswift maybe "debug log file" instead of "debug file"?

@practicalswift

This comment has been minimized.

Copy link
Member Author

practicalswift commented May 3, 2018

@laanwj @jonasschnelli Added a commit for the logging. Please re-review :-)

@jonasschnelli
Copy link
Member

jonasschnelli left a comment

utACK 20ce5af

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented May 3, 2018

utACK 20ce5af

@laanwj laanwj merged commit 20ce5af into bitcoin:master May 7, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request May 7, 2018

Merge #13149: Handle unsuccessful fseek(...):s
20ce5af Print a log message if we fail to shrink the debug log file (practicalswift)
29c9bdc Handle unsuccessful fseek(...):s (practicalswift)

Pull request description:

  Handle unsuccessful `fseek(...)`:s.

  **Note to reviewers:** What is the most appropriate course of actions for each of these unsuccessful `fseek(...)`:s?

Tree-SHA512: 5b3d82dbdd15d434d3f08dcb4df62888da4df8541d2586f56a4e529083005f6782c39e10645acd1ec403da83061bbfd8dbf2dddc66e09268d410ad0918c61876
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.