From d54646d34225cfeb45add05d58447c41b5bb5ea9 Mon Sep 17 00:00:00 2001 From: sfan5 Date: Wed, 26 Feb 2025 18:50:41 +0100 Subject: [PATCH] Improve error handling of map database creation --- src/database/database-postgresql.cpp | 2 +- src/database/database-postgresql.h | 33 +++++++++++++++++----------- src/database/database-sqlite3.h | 31 +++++++++++++++++--------- src/database/database.h | 5 +++++ src/emerge.cpp | 2 ++ src/server.cpp | 11 ++++++++-- src/servermap.cpp | 19 +++++++++++----- 7 files changed, 71 insertions(+), 32 deletions(-) diff --git a/src/database/database-postgresql.cpp b/src/database/database-postgresql.cpp index 2d10f0db9..20d5482f9 100644 --- a/src/database/database-postgresql.cpp +++ b/src/database/database-postgresql.cpp @@ -97,7 +97,7 @@ void Database_PostgreSQL::ping() bool Database_PostgreSQL::initialized() const { - return (PQstatus(m_conn) == CONNECTION_OK); + return m_conn && PQstatus(m_conn) == CONNECTION_OK; } PGresult *Database_PostgreSQL::checkResults(PGresult *result, bool clear) diff --git a/src/database/database-postgresql.h b/src/database/database-postgresql.h index 61e443b11..eb73960f3 100644 --- a/src/database/database-postgresql.h +++ b/src/database/database-postgresql.h @@ -9,20 +9,20 @@ #include "database.h" #include "util/basic_macros.h" -class Settings; - -class Database_PostgreSQL: public Database +// Template class for PostgreSQL based data storage +class Database_PostgreSQL : public Database { public: Database_PostgreSQL(const std::string &connect_string, const char *type); ~Database_PostgreSQL(); - void beginSave(); - void endSave(); + void beginSave() override; + void endSave() override; void rollback(); - bool initialized() const; + bool initialized() const override; + void verifyDatabase() override; protected: // Conversion helpers @@ -73,7 +73,6 @@ protected: } void createTableIfNotExists(const std::string &table_name, const std::string &definition); - void verifyDatabase(); // Database initialization void connectToDatabase(); @@ -99,6 +98,12 @@ private: int m_pgversion = 0; }; +// Not sure why why we have to do this. can't C++ figure it out on its own? +#define PARENT_CLASS_FUNCS \ + void beginSave() { Database_PostgreSQL::beginSave(); } \ + void endSave() { Database_PostgreSQL::endSave(); } \ + void verifyDatabase() { Database_PostgreSQL::verifyDatabase(); } + class MapDatabasePostgreSQL : private Database_PostgreSQL, public MapDatabase { public: @@ -110,8 +115,7 @@ public: bool deleteBlock(const v3s16 &pos); void listAllLoadableBlocks(std::vector &dst); - void beginSave() { Database_PostgreSQL::beginSave(); } - void endSave() { Database_PostgreSQL::endSave(); } + PARENT_CLASS_FUNCS protected: virtual void createDatabase(); @@ -129,6 +133,8 @@ public: bool removePlayer(const std::string &name); void listPlayers(std::vector &res); + PARENT_CLASS_FUNCS + protected: virtual void createDatabase(); virtual void initStatements(); @@ -143,8 +149,6 @@ public: AuthDatabasePostgreSQL(const std::string &connect_string); virtual ~AuthDatabasePostgreSQL() = default; - virtual void verifyDatabase() { Database_PostgreSQL::verifyDatabase(); } - virtual bool getAuth(const std::string &name, AuthEntry &res); virtual bool saveAuth(const AuthEntry &authEntry); virtual bool createAuth(AuthEntry &authEntry); @@ -152,6 +156,8 @@ public: virtual void listNames(std::vector &res); virtual void reload(); + PARENT_CLASS_FUNCS + protected: virtual void createDatabase(); virtual void initStatements(); @@ -176,10 +182,11 @@ public: bool removeModEntries(const std::string &modname); void listMods(std::vector *res); - void beginSave() { Database_PostgreSQL::beginSave(); } - void endSave() { Database_PostgreSQL::endSave(); } + PARENT_CLASS_FUNCS protected: virtual void createDatabase(); virtual void initStatements(); }; + +#undef PARENT_CLASS_FUNCS diff --git a/src/database/database-sqlite3.h b/src/database/database-sqlite3.h index 0ebd0bbf4..2f9212c16 100644 --- a/src/database/database-sqlite3.h +++ b/src/database/database-sqlite3.h @@ -19,17 +19,17 @@ class Database_SQLite3 : public Database public: virtual ~Database_SQLite3(); - void beginSave(); - void endSave(); + void beginSave() override; + void endSave() override; - bool initialized() const { return m_initialized; } + bool initialized() const override { return m_initialized; } + + /// @note not thread-safe + void verifyDatabase() override; protected: Database_SQLite3(const std::string &savedir, const std::string &dbname); - // Open and initialize the database if needed (not thread-safe) - void verifyDatabase(); - // Check if a specific table exists bool checkTable(const char *table); @@ -160,6 +160,12 @@ private: static int busyHandler(void *data, int count); }; +// Not sure why why we have to do this. can't C++ figure it out on its own? +#define PARENT_CLASS_FUNCS \ + void beginSave() { Database_SQLite3::beginSave(); } \ + void endSave() { Database_SQLite3::endSave(); } \ + void verifyDatabase() { Database_SQLite3::verifyDatabase(); } + class MapDatabaseSQLite3 : private Database_SQLite3, public MapDatabase { public: @@ -171,8 +177,8 @@ public: bool deleteBlock(const v3s16 &pos); void listAllLoadableBlocks(std::vector &dst); - void beginSave() { Database_SQLite3::beginSave(); } - void endSave() { Database_SQLite3::endSave(); } + PARENT_CLASS_FUNCS + protected: virtual void createDatabase(); virtual void initStatements(); @@ -201,6 +207,8 @@ public: bool removePlayer(const std::string &name); void listPlayers(std::vector &res); + PARENT_CLASS_FUNCS + protected: virtual void createDatabase(); virtual void initStatements(); @@ -238,6 +246,8 @@ public: virtual void listNames(std::vector &res); virtual void reload(); + PARENT_CLASS_FUNCS + protected: virtual void createDatabase(); virtual void initStatements(); @@ -273,8 +283,7 @@ public: virtual bool removeModEntries(const std::string &modname); virtual void listMods(std::vector *res); - virtual void beginSave() { Database_SQLite3::beginSave(); } - virtual void endSave() { Database_SQLite3::endSave(); } + PARENT_CLASS_FUNCS protected: virtual void createDatabase(); @@ -289,3 +298,5 @@ private: sqlite3_stmt *m_stmt_remove = nullptr; sqlite3_stmt *m_stmt_remove_all = nullptr; }; + +#undef PARENT_CLASS_FUNCS diff --git a/src/database/database.h b/src/database/database.h index 8d6efdee8..4335ef4dc 100644 --- a/src/database/database.h +++ b/src/database/database.h @@ -16,7 +16,12 @@ class Database public: virtual void beginSave() = 0; virtual void endSave() = 0; + + /// @return true if database connection is open virtual bool initialized() const { return true; } + + /// Open and initialize the database if needed + virtual void verifyDatabase() {}; }; class MapDatabase : public Database diff --git a/src/emerge.cpp b/src/emerge.cpp index be323f7a9..2dfead5b4 100644 --- a/src/emerge.cpp +++ b/src/emerge.cpp @@ -707,6 +707,8 @@ void *EmergeThread::run() { ScopeProfiler sp(g_profiler, "EmergeThread: load block - async (sum)"); MutexAutoLock dblock(m_db.mutex); + // Note: this can throw an exception, but there isn't really + // a good, safe way to handle it. m_db.loadBlock(pos, databuf); } // actually load it, then decide again diff --git a/src/server.cpp b/src/server.cpp index dd5041a56..116bbfe77 100644 --- a/src/server.cpp +++ b/src/server.cpp @@ -473,8 +473,15 @@ void Server::init() EnvAutoLock envlock(this); // Create the Map (loads map_meta.txt, overriding configured mapgen params) - auto startup_server_map = std::make_unique(m_path_world, this, - m_emerge.get(), m_metrics_backend.get()); + std::unique_ptr startup_server_map; + try { + startup_server_map = std::make_unique(m_path_world, this, + m_emerge.get(), m_metrics_backend.get()); + } catch (DatabaseException &e) { + throw ServerError(std::string( + "Failed to initialize the map database. The world may be " + "corrupted or in an unsupported format.\n") + e.what()); + } // Initialize scripting infostream << "Server: Initializing Lua" << std::endl; diff --git a/src/servermap.cpp b/src/servermap.cpp index b72626255..87e886492 100644 --- a/src/servermap.cpp +++ b/src/servermap.cpp @@ -577,27 +577,34 @@ MapDatabase *ServerMap::createDatabase( const std::string &savedir, Settings &conf) { + MapDatabase *db = nullptr; + if (name == "sqlite3") - return new MapDatabaseSQLite3(savedir); + db = new MapDatabaseSQLite3(savedir); if (name == "dummy") - return new Database_Dummy(); + db = new Database_Dummy(); #if USE_LEVELDB if (name == "leveldb") - return new Database_LevelDB(savedir); + db = new Database_LevelDB(savedir); #endif #if USE_REDIS if (name == "redis") - return new Database_Redis(conf); + db = new Database_Redis(conf); #endif #if USE_POSTGRESQL if (name == "postgresql") { std::string connect_string; conf.getNoEx("pgsql_connection", connect_string); - return new MapDatabasePostgreSQL(connect_string); + db = new MapDatabasePostgreSQL(connect_string); } #endif - throw BaseException(std::string("Database backend ") + name + " not supported."); + if (!db) + throw BaseException(std::string("Database backend ") + name + " not supported."); + // Do this to get feedback about errors asap + db->verifyDatabase(); + assert(db->initialized()); + return db; } void ServerMap::beginSave()