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

Tried adding else to close file to prevent file descriptor leak #2528

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@tariq786
Copy link

tariq786 commented Apr 14, 2013

No description provided.

@@ -1231,6 +1231,8 @@ void ShrinkDebugFile()
fclose(file);
}
}
else
fclose(file);

This comment has been minimized.

@laanwj

laanwj Apr 14, 2013

Member

This fix is not correct. It indeed looks like there is currently a fd leak when file && GetFilesize(file) < 10 * 1000000, but your change would also cause a fclose in the case that file == NULL (when the debug log cannot be opened; resulting in a segmentation fault).

@@ -1231,6 +1231,8 @@ void ShrinkDebugFile()
fclose(file);
}
}
else if(file != NULL)

This comment has been minimized.

@Diapolo

Diapolo Apr 14, 2013

IMHO it would be sufficient to write:
else if (file)

And please also check your indentation below!

This comment has been minimized.

@tariq786

tariq786 Apr 14, 2013

i have done the same (IMHO) and fixed the indentation issue as well

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Apr 14, 2013

ACK

@Diapolo

This comment has been minimized.

Copy link

Diapolo commented Apr 14, 2013

You need to squash the 2 commits into one and I still see the indentation issue?

@sipa

This comment has been minimized.

Copy link
Member

sipa commented Apr 15, 2013

ACK, if you squash the commits together.

@BitcoinPullTester

This comment has been minimized.

Copy link

BitcoinPullTester commented Apr 16, 2013

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/d0307c46f4013f7d60e505e3a1c737ebd6788203 for binaries and test log.
This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/
Contact BlueMatt on freenode if something looks broken.

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Apr 27, 2013

See #2584

@laanwj laanwj closed this Apr 27, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment