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.
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.
utACK
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.
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?
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)
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).
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
Reply to this email directly or view it on GitHub: #6551 (comment)
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!
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)...
utACK
Please don't remove RAII behavior. If that means having to use exceptions for error reporting, that's that.
utACK on the actual code