From 8b3ed78e53d8ad19d8dee3968430be258559214c Mon Sep 17 00:00:00 2001 From: kwolekr Date: Mon, 7 Jul 2014 01:20:25 -0400 Subject: [PATCH] Don't unload blocks if save failed Improve error handling in saveBlock() --- src/database-dummy.cpp | 8 ++++-- src/database-dummy.h | 4 +-- src/database-leveldb.cpp | 26 +++++++++++++------- src/database-leveldb.h | 4 +-- src/database-redis.cpp | 35 +++++++++++++++++--------- src/database-redis.h | 4 +-- src/database-sqlite3.cpp | 53 +++++++++++++++++++++++----------------- src/database-sqlite3.h | 4 +-- src/database.h | 14 +++++++---- src/map.cpp | 19 ++++++++------ src/map.h | 4 +-- 11 files changed, 108 insertions(+), 67 deletions(-) diff --git a/src/database-dummy.cpp b/src/database-dummy.cpp index c4794d281..7f715dd19 100644 --- a/src/database-dummy.cpp +++ b/src/database-dummy.cpp @@ -45,7 +45,7 @@ int Database_Dummy::Initialized(void) void Database_Dummy::beginSave() {} void Database_Dummy::endSave() {} -void Database_Dummy::saveBlock(MapBlock *block) +bool Database_Dummy::saveBlock(MapBlock *block) { DSTACK(__FUNCTION_NAME); /* @@ -53,7 +53,10 @@ void Database_Dummy::saveBlock(MapBlock *block) */ if(block->isDummy()) { - return; + v3s16 p = block->getPos(); + infostream<<"Database_Dummy::saveBlock(): WARNING: Not writing dummy block " + <<"("<resetModified(); + return true; } MapBlock* Database_Dummy::loadBlock(v3s16 blockpos) diff --git a/src/database-dummy.h b/src/database-dummy.h index c0bee97c0..d179d4226 100644 --- a/src/database-dummy.h +++ b/src/database-dummy.h @@ -33,8 +33,8 @@ public: Database_Dummy(ServerMap *map); virtual void beginSave(); virtual void endSave(); - virtual void saveBlock(MapBlock *block); - virtual MapBlock* loadBlock(v3s16 blockpos); + virtual bool saveBlock(MapBlock *block); + virtual MapBlock *loadBlock(v3s16 blockpos); virtual void listAllLoadableBlocks(std::list &dst); virtual int Initialized(void); ~Database_Dummy(); diff --git a/src/database-leveldb.cpp b/src/database-leveldb.cpp index 9fe47b34e..0cf685740 100644 --- a/src/database-leveldb.cpp +++ b/src/database-leveldb.cpp @@ -37,8 +37,8 @@ LevelDB databases #include "log.h" #define ENSURE_STATUS_OK(s) \ - if (!s.ok()) { \ - throw FileNotGoodException(std::string("LevelDB error: ") + s.ToString()); \ + if (!(s).ok()) { \ + throw FileNotGoodException(std::string("LevelDB error: ") + (s).ToString()); \ } Database_LevelDB::Database_LevelDB(ServerMap *map, std::string savedir) @@ -58,27 +58,29 @@ int Database_LevelDB::Initialized(void) void Database_LevelDB::beginSave() {} void Database_LevelDB::endSave() {} -void Database_LevelDB::saveBlock(MapBlock *block) +bool Database_LevelDB::saveBlock(MapBlock *block) { DSTACK(__FUNCTION_NAME); + + v3s16 p3d = block->getPos(); + /* Dummy blocks are not written */ if(block->isDummy()) { - return; + errorstream << "WARNING: saveBlock: Not writing dummy block " + << PP(p3d) << std::endl; + return true; } // Format used for writing u8 version = SER_FMT_VER_HIGHEST_WRITE; - // Get destination - v3s16 p3d = block->getPos(); /* [0] u8 serialization version [1] data */ - std::ostringstream o(std::ios_base::binary); o.write((char*)&version, 1); // Write basic data @@ -86,11 +88,17 @@ void Database_LevelDB::saveBlock(MapBlock *block) // Write block to database std::string tmp = o.str(); - leveldb::Status status = m_database->Put(leveldb::WriteOptions(), i64tos(getBlockAsInteger(p3d)), tmp); - ENSURE_STATUS_OK(status); + leveldb::Status status = m_database->Put(leveldb::WriteOptions(), + i64tos(getBlockAsInteger(p3d)), tmp); + if (!status.ok()) { + errorstream << "WARNING: saveBlock: LevelDB error saving block " + << PP(p3d) << ": " << status.ToString() << std::endl; + return false; + } // We just wrote it to the disk so clear modified flag block->resetModified(); + return true; } MapBlock* Database_LevelDB::loadBlock(v3s16 blockpos) diff --git a/src/database-leveldb.h b/src/database-leveldb.h index 5408f4ce6..0c20aeaef 100644 --- a/src/database-leveldb.h +++ b/src/database-leveldb.h @@ -36,8 +36,8 @@ public: Database_LevelDB(ServerMap *map, std::string savedir); virtual void beginSave(); virtual void endSave(); - virtual void saveBlock(MapBlock *block); - virtual MapBlock* loadBlock(v3s16 blockpos); + virtual bool saveBlock(MapBlock *block); + virtual MapBlock *loadBlock(v3s16 blockpos); virtual void listAllLoadableBlocks(std::list &dst); virtual int Initialized(void); ~Database_LevelDB(); diff --git a/src/database-redis.cpp b/src/database-redis.cpp index ff54753e6..595fb20ec 100644 --- a/src/database-redis.cpp +++ b/src/database-redis.cpp @@ -81,21 +81,24 @@ void Database_Redis::endSave() { freeReplyObject(reply); } -void Database_Redis::saveBlock(MapBlock *block) +bool Database_Redis::saveBlock(MapBlock *block) { DSTACK(__FUNCTION_NAME); + + v3s16 p3d = block->getPos(); + /* Dummy blocks are not written */ if(block->isDummy()) { - return; + errorstream << "WARNING: saveBlock: Not writing dummy block " + << PP(p3d) << std::endl; + return true; } // Format used for writing u8 version = SER_FMT_VER_HIGHEST_WRITE; - // Get destination - v3s16 p3d = block->getPos(); /* [0] u8 serialization version @@ -110,16 +113,26 @@ void Database_Redis::saveBlock(MapBlock *block) std::string tmp1 = o.str(); std::string tmp2 = i64tos(getBlockAsInteger(p3d)); - redisReply *reply; - reply = (redisReply*) redisCommand(ctx, "HSET %s %s %b", hash.c_str(), tmp2.c_str(), tmp1.c_str(), tmp1.size()); - if(!reply) - throw FileNotGoodException(std::string("redis command 'HSET %s %s %b' failed: ") + ctx->errstr); - if(reply->type == REDIS_REPLY_ERROR) - throw FileNotGoodException("Failed to store block in Database"); - freeReplyObject(reply); + redisReply *reply = (redisReply *)redisCommand(ctx, "HSET %s %s %b", + hash.c_str(), tmp2.c_str(), tmp1.c_str(), tmp1.size()); + if (!reply) { + errorstream << "WARNING: saveBlock: redis command 'HSET' failed on " + "block " << PP(p3d) << ": " << ctx->errstr << std::endl; + freeReplyObject(reply); + return false; + } + + if (reply->type == REDIS_REPLY_ERROR) { + errorstream << "WARNING: saveBlock: save block " << PP(p3d) + << "failed" << std::endl; + freeReplyObject(reply); + return false; + } // We just wrote it to the disk so clear modified flag block->resetModified(); + freeReplyObject(reply); + return true; } MapBlock* Database_Redis::loadBlock(v3s16 blockpos) diff --git a/src/database-redis.h b/src/database-redis.h index da76775d4..92983ee7b 100644 --- a/src/database-redis.h +++ b/src/database-redis.h @@ -36,8 +36,8 @@ public: Database_Redis(ServerMap *map, std::string savedir); virtual void beginSave(); virtual void endSave(); - virtual void saveBlock(MapBlock *block); - virtual MapBlock* loadBlock(v3s16 blockpos); + virtual bool saveBlock(MapBlock *block); + virtual MapBlock *loadBlock(v3s16 blockpos); virtual void listAllLoadableBlocks(std::list &dst); virtual int Initialized(void); ~Database_Redis(); diff --git a/src/database-sqlite3.cpp b/src/database-sqlite3.cpp index 3ec13a1a2..fb76d8eae 100644 --- a/src/database-sqlite3.cpp +++ b/src/database-sqlite3.cpp @@ -136,25 +136,24 @@ void Database_SQLite3::verifyDatabase() { infostream<<"ServerMap: SQLite3 database opened"<getPos(); + /* - Dummy blocks are not written + Dummy blocks are not written, but is not a save failure */ if(block->isDummy()) { - /*v3s16 p = block->getPos(); - infostream<<"Database_SQLite3::saveBlock(): WARNING: Not writing dummy block " - <<"("<getPos(); - #if 0 v2s16 p2d(p3d.X, p3d.Z); @@ -176,29 +175,39 @@ void Database_SQLite3::saveBlock(MapBlock *block) std::ostringstream o(std::ios_base::binary); - o.write((char*)&version, 1); + o.write((char *)&version, 1); // Write basic data block->serialize(o, version, true); // Write block to database - std::string tmp = o.str(); - const char *bytes = tmp.c_str(); - if(sqlite3_bind_int64(m_database_write, 1, getBlockAsInteger(p3d)) != SQLITE_OK) - errorstream<<"WARNING: Block position failed to bind: "<resetModified(); + sqlite3_reset(m_database_write); + return true; } MapBlock* Database_SQLite3::loadBlock(v3s16 blockpos) diff --git a/src/database-sqlite3.h b/src/database-sqlite3.h index f426f4618..1753072e8 100644 --- a/src/database-sqlite3.h +++ b/src/database-sqlite3.h @@ -36,8 +36,8 @@ public: virtual void beginSave(); virtual void endSave(); - virtual void saveBlock(MapBlock *block); - virtual MapBlock* loadBlock(v3s16 blockpos); + virtual bool saveBlock(MapBlock *block); + virtual MapBlock *loadBlock(v3s16 blockpos); virtual void listAllLoadableBlocks(std::list &dst); virtual int Initialized(void); ~Database_SQLite3(); diff --git a/src/database.h b/src/database.h index f009877d2..d8669dd9b 100644 --- a/src/database.h +++ b/src/database.h @@ -24,19 +24,23 @@ with this program; if not, write to the Free Software Foundation, Inc., #include "irr_v3d.h" #include "irrlichttypes.h" +#ifndef PP + #define PP(x) "("<<(x).X<<","<<(x).Y<<","<<(x).Z<<")" +#endif + class MapBlock; class Database { public: - virtual void beginSave()=0; - virtual void endSave()=0; + virtual void beginSave() = 0; + virtual void endSave() = 0; - virtual void saveBlock(MapBlock *block)=0; - virtual MapBlock* loadBlock(v3s16 blockpos)=0; + virtual bool saveBlock(MapBlock *block) = 0; + virtual MapBlock *loadBlock(v3s16 blockpos) = 0; s64 getBlockAsInteger(const v3s16 pos) const; v3s16 getIntegerAsBlock(s64 i) const; - virtual void listAllLoadableBlocks(std::list &dst)=0; + virtual void listAllLoadableBlocks(std::list &dst) = 0; virtual int Initialized(void)=0; virtual ~Database() {}; }; diff --git a/src/map.cpp b/src/map.cpp index 35bd485a4..9c06750b8 100644 --- a/src/map.cpp +++ b/src/map.cpp @@ -1473,11 +1473,11 @@ void Map::timerUpdate(float dtime, float unload_timeout, v3s16 p = block->getPos(); // Save if modified - if(block->getModified() != MOD_STATE_CLEAN - && save_before_unloading) + if (block->getModified() != MOD_STATE_CLEAN && save_before_unloading) { modprofiler.add(block->getModifiedReason(), 1); - saveBlock(block); + if (!saveBlock(block)) + continue; saved_blocks_count++; } @@ -3253,20 +3253,23 @@ bool ServerMap::loadSectorFull(v2s16 p2d) } #endif -void ServerMap::beginSave() { +void ServerMap::beginSave() +{ dbase->beginSave(); } -void ServerMap::endSave() { +void ServerMap::endSave() +{ dbase->endSave(); } -void ServerMap::saveBlock(MapBlock *block) +bool ServerMap::saveBlock(MapBlock *block) { - dbase->saveBlock(block); + return dbase->saveBlock(block); } -void ServerMap::loadBlock(std::string sectordir, std::string blockfile, MapSector *sector, bool save_after_load) +void ServerMap::loadBlock(std::string sectordir, std::string blockfile, + MapSector *sector, bool save_after_load) { DSTACK(__FUNCTION_NAME); diff --git a/src/map.h b/src/map.h index 7f482929e..4972046f5 100644 --- a/src/map.h +++ b/src/map.h @@ -270,7 +270,7 @@ public: // Server implements this. // Client leaves it as no-op. - virtual void saveBlock(MapBlock *block){}; + virtual bool saveBlock(MapBlock *block){ return false; }; /* Updates usage timers and unloads unused blocks and sectors. @@ -485,7 +485,7 @@ public: // Returns true if sector now resides in memory //bool deFlushSector(v2s16 p2d); - void saveBlock(MapBlock *block); + bool saveBlock(MapBlock *block); // This will generate a sector with getSector if not found. void loadBlock(std::string sectordir, std::string blockfile, MapSector *sector, bool save_after_load=false); MapBlock* loadBlock(v3s16 p);