Handle leveldb::DestroyDB() errors on wipe failure #6551

Merged
merged 1 commit into from Aug 17, 2015

Conversation

Projects
None yet
5 participants
@ajweiss
Contributor

ajweiss commented Aug 12, 2015

Add error checking to CLevelDBWrapper for errors from
leveldb::DestroyDB(). Without it, if unlink() or DeleteFileW() fail to
delete files, they will fail silent. If they fail to delete any files,
CLevelDBWrapper will silently open and read the existing database.

Typically any permissions issues would be caught by leveldb as it churns
through many files as part of its compaction process, but it is
conceivable that this could cause problems on Windows with anti-virus
and indexing software.

Handle leveldb::DestroyDB() errors on wipe failure
Add error checking to CLevelDBWrapper for errors from
leveldb::DestroyDB().  Without it, if unlink() or DeleteFileW() fail to
delete files, they will fail silent.  If they fail to delete any files,
CLevelDBWrapper will silently open and read the existing database.

Typically any permissions issues would be caught by leveldb as it churns
through many files as part of its compaction process, but it is
conceivable that this could cause problems on Windows with anti-virus
and indexing software.
@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Aug 13, 2015

Contributor

utACK

Contributor

dcousens commented Aug 13, 2015

utACK

@TheBlueMatt

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Aug 13, 2015

Contributor

Not to bikeshed this, but we should figure out if we want to use the throw() syntax in function declarations. We have it in bool CLevelDBWrapper::WriteBatch(CLevelDBBatch& batch, bool fSync) throw(leveldb_error), but not in CLevelDBWrapper::CLevelDBWrapper(const boost::filesystem::path& path, size_t nCacheSize, bool fMemory, bool fWipe), which is strange.

Contributor

TheBlueMatt commented Aug 13, 2015

Not to bikeshed this, but we should figure out if we want to use the throw() syntax in function declarations. We have it in bool CLevelDBWrapper::WriteBatch(CLevelDBBatch& batch, bool fSync) throw(leveldb_error), but not in CLevelDBWrapper::CLevelDBWrapper(const boost::filesystem::path& path, size_t nCacheSize, bool fMemory, bool fWipe), which is strange.

@ajweiss

This comment has been minimized.

Show comment
Hide comment
@ajweiss

ajweiss Aug 13, 2015

Contributor

throw() is deprecated in C++11... so I vote we throw() it out entirely.

Perhaps just a comment, like:

// Throws leveldb_error
bool WriteBatch(CLevelDBBatch& batch, bool fSync = false);

...seems like a sensible replacement?

Contributor

ajweiss commented Aug 13, 2015

throw() is deprecated in C++11... so I vote we throw() it out entirely.

Perhaps just a comment, like:

// Throws leveldb_error
bool WriteBatch(CLevelDBBatch& batch, bool fSync = false);

...seems like a sensible replacement?

@TheBlueMatt

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Aug 13, 2015

Contributor

Yes, a comment would be very nice, indeed... Does clang have any compiler-enforced catch requirements?

On August 13, 2015 6:45:22 PM GMT+03:00, Adam Weiss notifications@github.com wrote:

throw() is deprecated in C++11... so I vote we throw() it out entirely.

Perhaps just a comment, like:

// Throws leveldb_error
bool WriteBatch(CLevelDBBatch& batch, bool fSync = false);

...seems like a sensible replacement?


Reply to this email directly or view it on GitHub:
#6551 (comment)

Contributor

TheBlueMatt commented Aug 13, 2015

Yes, a comment would be very nice, indeed... Does clang have any compiler-enforced catch requirements?

On August 13, 2015 6:45:22 PM GMT+03:00, Adam Weiss notifications@github.com wrote:

throw() is deprecated in C++11... so I vote we throw() it out entirely.

Perhaps just a comment, like:

// Throws leveldb_error
bool WriteBatch(CLevelDBBatch& batch, bool fSync = false);

...seems like a sensible replacement?


Reply to this email directly or view it on GitHub:
#6551 (comment)

@ajweiss

This comment has been minimized.

Show comment
Hide comment
@ajweiss

ajweiss Aug 13, 2015

Contributor

I don't think any C++ compilers support checked exceptions. That said, it
wouldn't be terrible to throw(all) the exceptions here out and replace them
with return codes...

On Thu, Aug 13, 2015 at 11:48 AM, Matt Corallo notifications@github.com
wrote:

Yes, a comment would be very nice, indeed... Does clang have any
compiler-enforced catch requirements?

On August 13, 2015 6:45:22 PM GMT+03:00, Adam Weiss <
notifications@github.com> wrote:

throw() is deprecated in C++11... so I vote we throw() it out entirely.

Perhaps just a comment, like:

// Throws leveldb_error
bool WriteBatch(CLevelDBBatch& batch, bool fSync = false);

...seems like a sensible replacement?


Reply to this email directly or view it on GitHub:
#6551 (comment)


Reply to this email directly or view it on GitHub
#6551 (comment).

Contributor

ajweiss commented Aug 13, 2015

I don't think any C++ compilers support checked exceptions. That said, it
wouldn't be terrible to throw(all) the exceptions here out and replace them
with return codes...

On Thu, Aug 13, 2015 at 11:48 AM, Matt Corallo notifications@github.com
wrote:

Yes, a comment would be very nice, indeed... Does clang have any
compiler-enforced catch requirements?

On August 13, 2015 6:45:22 PM GMT+03:00, Adam Weiss <
notifications@github.com> wrote:

throw() is deprecated in C++11... so I vote we throw() it out entirely.

Perhaps just a comment, like:

// Throws leveldb_error
bool WriteBatch(CLevelDBBatch& batch, bool fSync = false);

...seems like a sensible replacement?


Reply to this email directly or view it on GitHub:
#6551 (comment)


Reply to this email directly or view it on GitHub
#6551 (comment).

@TheBlueMatt

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Aug 13, 2015

Contributor

Yea, again, don't really want to bikeshed this, but I'm always in favor of removing exceptions.

On August 13, 2015 7:38:32 PM GMT+03:00, Adam Weiss notifications@github.com wrote:

I don't think any C++ compilers support checked exceptions. That said,
it
wouldn't be terrible to throw(all) the exceptions here out and replace
them
with return codes...

On Thu, Aug 13, 2015 at 11:48 AM, Matt Corallo
notifications@github.com
wrote:

Yes, a comment would be very nice, indeed... Does clang have any
compiler-enforced catch requirements?

On August 13, 2015 6:45:22 PM GMT+03:00, Adam Weiss <
notifications@github.com> wrote:

throw() is deprecated in C++11... so I vote we throw() it out
entirely.

Perhaps just a comment, like:

// Throws leveldb_error
bool WriteBatch(CLevelDBBatch& batch, bool fSync = false);

...seems like a sensible replacement?


Reply to this email directly or view it on GitHub:
#6551 (comment)


Reply to this email directly or view it on GitHub

#6551 (comment).


Reply to this email directly or view it on GitHub:
#6551 (comment)

Contributor

TheBlueMatt commented Aug 13, 2015

Yea, again, don't really want to bikeshed this, but I'm always in favor of removing exceptions.

On August 13, 2015 7:38:32 PM GMT+03:00, Adam Weiss notifications@github.com wrote:

I don't think any C++ compilers support checked exceptions. That said,
it
wouldn't be terrible to throw(all) the exceptions here out and replace
them
with return codes...

On Thu, Aug 13, 2015 at 11:48 AM, Matt Corallo
notifications@github.com
wrote:

Yes, a comment would be very nice, indeed... Does clang have any
compiler-enforced catch requirements?

On August 13, 2015 6:45:22 PM GMT+03:00, Adam Weiss <
notifications@github.com> wrote:

throw() is deprecated in C++11... so I vote we throw() it out
entirely.

Perhaps just a comment, like:

// Throws leveldb_error
bool WriteBatch(CLevelDBBatch& batch, bool fSync = false);

...seems like a sensible replacement?


Reply to this email directly or view it on GitHub:
#6551 (comment)


Reply to this email directly or view it on GitHub

#6551 (comment).


Reply to this email directly or view it on GitHub:
#6551 (comment)

@ajweiss

This comment has been minimized.

Show comment
Hide comment
@ajweiss

ajweiss Aug 13, 2015

Contributor

Naw, not bikeshedding. I'll pull them out and in the process will end up auditing a bunch of failure cases for leveldb. In light of recent reports of leveldb corruption, this is a good thing. Thanks for the feedback!

Contributor

ajweiss commented Aug 13, 2015

Naw, not bikeshedding. I'll pull them out and in the process will end up auditing a bunch of failure cases for leveldb. In light of recent reports of leveldb corruption, this is a good thing. Thanks for the feedback!

@ajweiss

This comment has been minimized.

Show comment
Hide comment
@ajweiss

ajweiss Aug 14, 2015

Contributor

Although, of course, this means un-RAII-ifying the leveldb wrapper. I'm happy to do this, but I have doubts as to whether or not anyone else is going to like it (given that there's so much other stuff that is RAII)...

Contributor

ajweiss commented Aug 14, 2015

Although, of course, this means un-RAII-ifying the leveldb wrapper. I'm happy to do this, but I have doubts as to whether or not anyone else is going to like it (given that there's so much other stuff that is RAII)...

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Aug 15, 2015

Contributor

@ajweiss why would it stop RAII-like behaviour?

Contributor

dcousens commented Aug 15, 2015

@ajweiss why would it stop RAII-like behaviour?

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Aug 15, 2015

Member

utACK

Member

sipa commented Aug 15, 2015

utACK

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Aug 17, 2015

Member

Please don't remove RAII behavior. If that means having to use exceptions for error reporting, that's that.

Member

laanwj commented Aug 17, 2015

Please don't remove RAII behavior. If that means having to use exceptions for error reporting, that's that.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Aug 17, 2015

Member

utACK on the actual code

Member

laanwj commented Aug 17, 2015

utACK on the actual code

@laanwj laanwj merged commit 243b80d into bitcoin:master Aug 17, 2015

1 check passed

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

laanwj added a commit that referenced this pull request Aug 17, 2015

Merge pull request #6551
243b80d Handle leveldb::DestroyDB() errors on wipe failure (Adam Weiss)

@dagurval dagurval referenced this pull request in bitcoinxt/bitcoinxt Oct 27, 2017

Merged

Handle leveldb::DestroyDB() errors on wipe failure #270

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