CBufferedFile: convert into a non-refcounted RAII wrapper #4978

• it now takes over the passed file descriptor and closes it in the destructor
• this fixes a leak in LoadExternalBlockFile(), where an exception could cause the file to not getting closed
• disallow copies (like recently added for CAutoFile)
• make nType and nVersion private

I'd say we need something more RAII-ish here.

Easiest would be to make CBufferedFile take ownership of the FILE* - ie, close it on destruction, like CAutoFile. This is the only place CBufferedFile is used so there would be no other impact.

@laanwj You also could have been saying "Hey, nice catch, but..." :-P. I agree on your suggestion anyway.

Yes, nice catch that there is a potential leak here!

I just don't think the current solution solves it completely. Other exceptions that are not directly catched will still cause the file not to be closed.

@laanwj Converted to a RAII-style wrapper and removed the manual fclose().

Looks good to me now.

You may want to make a note here that CBufferedFile is non-refcounted. Not a huge deal since there's only one user of it now, but I suppose that could change. Problem would be (stupid example):

CBufferedFile foo(fd,...);
{
    CBufferedFile bar(fd2,...);
    foo=bar;
}
foo.read(...)

@theuni Should I place that comment in serialize.h above the class?

@Diapolo sounds good. Probably not a bad idea to make the copy ctor and operator= private for good measure.

@Diapolo I took a look at CAutoFile and it has the same behavior. See #4986.

Updated to include:

• update CAutoFile destructor to match CBufferdFile destructor
• CBufferedFile: make src protected, nType and nVersion private
• CAutoFile: make nType and nVersion private

That's a lot of red...

@laanwj I didn't check the Travis build :-/. It was caused by the now missing CAutoFile::fclose() function, which was called by CAddrDB explicitly. I removed them now as it seemed save to me. We may aswell add a CAutoFile::close() instead to still allow closing a file explicitly.

Well IMO it's fine to have a method to close the file explicitly, but it should be optional, so if the object goes out of scope the file is closed nevertheless.

Now it's green :). Shall I add an explicit close() to CAutoFile and CBufferedFile via this pull? Btw. can you first verify, if the current changes are correct perhaps :-D?

Edit: Since the commits are rather easy to review, I added the close() as a fifth commit.

Automatic sanity-testing: FAILED BUILD/TEST, see http://jenkins.bluematt.me/pull-tester/p4978_d26ef84ff33bc384671bf9b18b0fd2018ec339f2/ for binaries and test log.

This could happen for one of several reasons:

1. It chanages paths in makefile.linux-mingw or otherwise changes build scripts in a way that made them incompatible with the automated testing scripts (please tweak those patches in qa/pull-tester)
2. It adds/modifies tests which test network rules (thanks for doing that), which conflicts with a patch applied at test time
3. It does not build on either Linux i386 or Win32 (via MinGW cross compile)
4. The test suite fails on either Linux i386 or Win32
5. The block test-cases failed (lookup the first bNN identifier which failed in https://github.com/TheBlueMatt/test-scripts/blob/master/FullBlockTestGenerator.java)

If you believe this to be in error, please ping BlueMatt on freenode or TheBlueMatt here.

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 Green, finally... wohooo!

Why are the File*'s left protected? Keeping anything protected is just inviting inheritance, which would surely be disastrous for these. I'd make em private.

Agree with @theuni.

@theuni @sipa I changed them back to private for CBufferedFile and also made FILE* private for CAutoFile. Thanks for your feedback. If I get some ACKs I'll squash the commits to be not 5, but rather 2 I guess.

@laanwj Squashed into 3 much more logical commits now.

Looks good to me now. utACK.

utACK.

Sidenote: the mix of RAII/non-RAII in CAutoFile is really scary. Lots of outside functions rely on them decaying to FILE* (see FileCommit for a nasty example). That violates the principle of least surprise in a big way, imo.

Would there be much objection to trying to reign that in by slowly extending CAutoFile (or something new) to be more full-featured, and slowly eliminating bare FILE pointers? In the example above, I can't imagine why that shouldn't be foo.commit(). I'm not aware of many other big projects that allow non-abstracted low-level structures like that.

@theuni In principle I agree, but let's not spend too much time here unless there are concrete problems. I suppose we could pretty easily convert it to more C++ish ifstream/ofstream which is RAII by default (instead of inventing Yet Another File Wrapper which a new contributor has to learn). But on the other hand I'm afraid fiddling too much with this code will risk breaking things with no clear gain, and there are tons of actual issues to be fixed.

Edit: BTW, maybe we should look into moving these out of serialize.h? I'd say that the serialization should be decoupled platform functionality such as files.

@theuni @laanwj I have a working CAutoFile implementation based on std::fstream in my local build. Remeber, I tried to get this changed once and am willing to just create a new pull at any time :).