Cleanup ssqlite3

diff --git a/pdns/ssqlite3.cc b/pdns/ssqlite3.cc
index 9dc4888cc5c0e198ad42cc33cdf488c5e886cd07..660c561b7810069696d7a92f352c01fd107f4f4b 100644 (file)
--- a/pdns/ssqlite3.cc
+++ b/pdns/ssqlite3.cc
@@ -27,6 +27,7 @@
 #include "ssqlite3.hh"
 #include <iostream>
 #include <fstream>
+#include <utility>
 #include "pdns/logger.hh"
 #include "misc.hh"
 #include "utility.hh"
@@ -49,21 +50,26 @@ static int pdns_sqlite3_clear_bindings(sqlite3_stmt* pStmt)
 }
 #endif
 
-static string SSQLite3ErrorString(sqlite3* db)
+static string SSQLite3ErrorString(sqlite3* database)
 {
-  return string(sqlite3_errmsg(db) + string(" (") + std::to_string(sqlite3_extended_errcode(db)) + string(")"));
+  return string(sqlite3_errmsg(database) + string(" (") + std::to_string(sqlite3_extended_errcode(database)) + string(")"));
 }
 
 class SSQLite3Statement : public SSqlStatement
 {
 public:
-  SSQLite3Statement(SSQLite3* db, bool dolog, const string& query) :
-    d_query(query),
-    d_db(db),
+  SSQLite3Statement(SSQLite3* database, bool dolog, string query) :
+    d_query(std::move(query)),
+    d_db(database),
     d_dolog(dolog)
   {
   }
 
+  SSQLite3Statement(const SSQLite3Statement&) = delete;
+  SSQLite3Statement(SSQLite3Statement&&) = delete;
+  SSQLite3Statement& operator=(const SSQLite3Statement&) = delete;
+  SSQLite3Statement& operator=(SSQLite3Statement&&) = delete;
+
   int name2idx(const string& name)
   {
     string zName = string(":") + name;
@@ -72,7 +78,7 @@ public:
     // XXX: support @ and $?
   }
 
-  SSqlStatement* bind(const string& name, bool value)
+  SSqlStatement* bind(const string& name, bool value) override
   {
     int idx = name2idx(name);
     if (idx > 0) {
@@ -80,7 +86,8 @@ public:
     };
     return this;
   }
-  SSqlStatement* bind(const string& name, int value)
+
+  SSqlStatement* bind(const string& name, int value) override
   {
     int idx = name2idx(name);
     if (idx > 0) {
@@ -88,7 +95,8 @@ public:
     };
     return this;
   }
-  SSqlStatement* bind(const string& name, uint32_t value)
+
+  SSqlStatement* bind(const string& name, uint32_t value) override
   {
     int idx = name2idx(name);
     if (idx > 0) {
@@ -96,7 +104,8 @@ public:
     };
     return this;
   }
-  SSqlStatement* bind(const string& name, long value)
+
+  SSqlStatement* bind(const string& name, long value) override
   {
     int idx = name2idx(name);
     if (idx > 0) {
@@ -104,15 +113,17 @@ public:
     };
     return this;
   }
-  SSqlStatement* bind(const string& name, unsigned long value)
+
+  SSqlStatement* bind(const string& name, unsigned long value) override
   {
     int idx = name2idx(name);
     if (idx > 0) {
-      sqlite3_bind_int64(d_stmt, idx, value);
+      sqlite3_bind_int64(d_stmt, idx, static_cast<sqlite3_int64>(value));
     };
     return this;
   }
-  SSqlStatement* bind(const string& name, long long value)
+
+  SSqlStatement* bind(const string& name, long long value) override
   {
     int idx = name2idx(name);
     if (idx > 0) {
@@ -120,23 +131,26 @@ public:
     };
     return this;
   };
-  SSqlStatement* bind(const string& name, unsigned long long value)
+
+  SSqlStatement* bind(const string& name, unsigned long long value) override
   {
     int idx = name2idx(name);
     if (idx > 0) {
-      sqlite3_bind_int64(d_stmt, idx, value);
+      sqlite3_bind_int64(d_stmt, idx, static_cast<sqlite3_int64>(value));
     };
     return this;
   }
-  SSqlStatement* bind(const string& name, const std::string& value)
+
+  SSqlStatement* bind(const string& name, const std::string& value) override
   {
     int idx = name2idx(name);
     if (idx > 0) {
-      sqlite3_bind_text(d_stmt, idx, value.c_str(), value.size(), SQLITE_TRANSIENT);
+      sqlite3_bind_text(d_stmt, idx, value.c_str(), static_cast<int>(value.size()), SQLITE_TRANSIENT);
     };
     return this;
   }
-  SSqlStatement* bindNull(const string& name)
+
+  SSqlStatement* bindNull(const string& name) override
   {
     int idx = name2idx(name);
     if (idx > 0) {
@@ -145,37 +159,42 @@ public:
     return this;
   }
 
-  SSqlStatement* execute()
+  SSqlStatement* execute() override
   {
     prepareStatement();
     if (d_dolog) {
-      g_log << Logger::Warning << "Query " << ((long)(void*)this) << ": " << d_query << endl;
+      g_log << Logger::Warning << "Query " << this << ": " << d_query << endl;
       d_dtime.set();
     }
-    int attempts = d_db->inTransaction(); // try only once
-    while (attempts < 2 && (d_rc = sqlite3_step(d_stmt)) == SQLITE_BUSY)
+
+    int attempts = d_db->inTransaction() ? 1 : 0; // try only once
+    while (attempts < 2 && (d_rc = sqlite3_step(d_stmt)) == SQLITE_BUSY) {
       attempts++;
+    }
 
     if (d_rc != SQLITE_ROW && d_rc != SQLITE_DONE) {
       // failed.
       releaseStatement();
-      if (d_rc == SQLITE_CANTOPEN)
+      if (d_rc == SQLITE_CANTOPEN) {
         throw SSqlException(string("CANTOPEN error in sqlite3, often caused by unwritable sqlite3 db *directory*: ") + SSQLite3ErrorString(d_db->db()));
+      }
       throw SSqlException(string("Error while retrieving SQLite query results: ") + SSQLite3ErrorString(d_db->db()));
     }
-    if (d_dolog)
-      g_log << Logger::Warning << "Query " << ((long)(void*)this) << ": " << d_dtime.udiffNoReset() << " us to execute" << endl;
+    if (d_dolog) {
+      g_log << Logger::Warning << "Query " << this << ": " << d_dtime.udiffNoReset() << " us to execute" << endl;
+    }
     return this;
   }
-  bool hasNextRow()
+
+  bool hasNextRow() override
   {
     if (d_dolog && d_rc != SQLITE_ROW) {
-      g_log << Logger::Warning << "Query " << ((long)(void*)this) << ": " << d_dtime.udiffNoReset() << " us total to last row" << endl;
+      g_log << Logger::Warning << "Query " << this << ": " << d_dtime.udiffNoReset() << " us total to last row" << endl;
     }
     return d_rc == SQLITE_ROW;
   }
 
-  SSqlStatement* nextRow(row_t& row)
+  SSqlStatement* nextRow(row_t& row) override
   {
     row.clear();
     int numCols = sqlite3_column_count(d_stmt);
@@ -186,6 +205,7 @@ public:
         row.emplace_back("");
       }
       else {
+        // NOLINTNEXTLINE(cppcoreguidelines-pro-type-cstyle-cast): SQLite API returns unsigned char strings and std::strings are signed char.
         const char* pData = (const char*)sqlite3_column_text(d_stmt, i);
         row.emplace_back(pData, sqlite3_column_bytes(d_stmt, i));
       }
@@ -194,7 +214,7 @@ public:
     return this;
   }
 
-  SSqlStatement* getResult(result_t& result)
+  SSqlStatement* getResult(result_t& result) override
   {
     result.clear();
     while (hasNextRow()) {
@@ -205,7 +225,7 @@ public:
     return this;
   }
 
-  SSqlStatement* reset()
+  SSqlStatement* reset() override
   {
     sqlite3_reset(d_stmt);
 #if SQLITE_VERSION_NUMBER >= 3003009
@@ -216,13 +236,13 @@ public:
     return this;
   }
 
-  ~SSQLite3Statement()
+  ~SSQLite3Statement() override
   {
     // deallocate if necessary
     releaseStatement();
   }
 
-  const string& getQuery() { return d_query; };
+  const string& getQuery() override { return d_query; };
 
 private:
   string d_query;
@@ -235,10 +255,11 @@ private:
 
   void prepareStatement()
   {
-    const char* pTail;
+    const char* pTail = nullptr;
 
-    if (d_prepared)
+    if (d_prepared) {
       return;
+    }
 #if SQLITE_VERSION_NUMBER >= 3003009
     if (sqlite3_prepare_v2(d_db->db(), d_query.c_str(), -1, &d_stmt, &pTail) != SQLITE_OK)
 #else
@@ -248,15 +269,17 @@ private:
       releaseStatement();
       throw SSqlException(string("Unable to compile SQLite statement : '") + d_query + "': " + SSQLite3ErrorString(d_db->db()));
     }
-    if (pTail && strlen(pTail) > 0)
+    if ((pTail != nullptr) && strlen(pTail) > 0) {
       g_log << Logger::Warning << "Sqlite3 command partially processed. Unprocessed part: " << pTail << endl;
+    }
     d_prepared = true;
   }
 
   void releaseStatement()
   {
-    if (d_stmt)
+    if (d_stmt != nullptr) {
       sqlite3_finalize(d_stmt);
+    }
     d_stmt = nullptr;
     d_prepared = false;
   }
@@ -266,22 +289,26 @@ private:
 SSQLite3::SSQLite3(const std::string& database, const std::string& journalmode, bool creat)
 {
   if (access(database.c_str(), F_OK) == -1) {
-    if (!creat)
+    if (!creat) {
       throw sPerrorException("SQLite database '" + database + "' does not exist yet");
+    }
   }
   else {
-    if (creat)
+    if (creat) {
       throw sPerrorException("SQLite database '" + database + "' already exists");
+    }
   }
 
-  if (sqlite3_open(database.c_str(), &m_pDB) != SQLITE_OK)
+  if (sqlite3_open(database.c_str(), &m_pDB) != SQLITE_OK) {
     throw sPerrorException("Could not connect to the SQLite database '" + database + "'");
-  m_dolog = 0;
+  }
+  m_dolog = false;
   m_in_transaction = false;
-  sqlite3_busy_handler(m_pDB, busyHandler, 0);
+  sqlite3_busy_handler(m_pDB, busyHandler, nullptr);
 
-  if (journalmode.length())
+  if (journalmode.length() != 0) {
     execute("PRAGMA journal_mode=" + journalmode);
+  }
 }
 
 void SSQLite3::setLog(bool state)
@@ -292,16 +319,17 @@ void SSQLite3::setLog(bool state)
 // Destructor.
 SSQLite3::~SSQLite3()
 {
-  int ret;
-  for (int n = 0;; ++n) {
-    if ((ret = sqlite3_close(m_pDB)) != SQLITE_OK) {
-      if (n || ret != SQLITE_BUSY) { // if we have SQLITE_BUSY, and a working m_Pstmt, try finalize
+  for (int tries = 0;; ++tries) {
+    int ret = sqlite3_close(m_pDB);
+    if (ret != SQLITE_OK) {
+      if (tries != 0 || ret != SQLITE_BUSY) { // if we have SQLITE_BUSY, and a working m_Pstmt, try finalize
         cerr << "Unable to close down sqlite connection: " << ret << endl;
         abort();
       }
     }
-    else
+    else {
       break;
+    }
   }
 }
 
@@ -312,33 +340,31 @@ std::unique_ptr<SSqlStatement> SSQLite3::prepare(const string& query, int nparam
 
 void SSQLite3::execute(const string& query)
 {
-  char* errmsg;
+  char* errmsg = nullptr;
   std::string errstr1;
-  int rc = sqlite3_exec(m_pDB, query.c_str(), nullptr, nullptr, &errmsg);
-  if (rc != SQLITE_OK) {
+  int execRet = sqlite3_exec(m_pDB, query.c_str(), nullptr, nullptr, &errmsg);
+  if (execRet != SQLITE_OK) {
     errstr1 = errmsg;
     sqlite3_free(errmsg);
   }
-  if (rc == SQLITE_BUSY) {
+  if (execRet == SQLITE_BUSY) {
     if (m_in_transaction) {
       throw SSqlException("Failed to execute query: " + errstr1);
     }
-    else {
-      rc = sqlite3_exec(m_pDB, query.c_str(), NULL, NULL, &errmsg);
-      std::string errstr2;
-      if (rc != SQLITE_OK) {
-        errstr2 = errmsg;
-        sqlite3_free(errmsg);
-        throw SSqlException("Failed to execute query: " + errstr2);
-      }
+    execRet = sqlite3_exec(m_pDB, query.c_str(), nullptr, nullptr, &errmsg);
+    std::string errstr2;
+    if (execRet != SQLITE_OK) {
+      errstr2 = errmsg;
+      sqlite3_free(errmsg);
+      throw SSqlException("Failed to execute query: " + errstr2);
     }
   }
-  else if (rc != SQLITE_OK) {
+  else if (execRet != SQLITE_OK) {
     throw SSqlException("Failed to execute query: " + errstr1);
   }
 }
 
-int SSQLite3::busyHandler(void*, int)
+int SSQLite3::busyHandler([[maybe_unused]] void* userData, [[maybe_unused]] int invocationsSoFar)
 {
   Utility::usleep(1000);
   return 1;
@@ -365,5 +391,5 @@ void SSQLite3::commit()
 // Constructs a SSqlException object.
 SSqlException SSQLite3::sPerrorException(const std::string& reason)
 {
-  return SSqlException(reason);
+  return {reason};
 }

diff --git a/pdns/ssqlite3.hh b/pdns/ssqlite3.hh
index 6c929d9d43788d948b329d8a88ca4e5fe1920ffd..29a1e10c9e99e02d4f5f8d311a46a83e48ff4f21 100644 (file)
--- a/pdns/ssqlite3.hh
+++ b/pdns/ssqlite3.hh
@@ -39,7 +39,7 @@ public:
   SSQLite3(const std::string& database, const std::string& journalmode, bool creat = false);
 
   //! Destructor.
-  ~SSQLite3();
+  ~SSQLite3() override;
 
   std::unique_ptr<SSqlStatement> prepare(const string& query, int nparams) override;
   void execute(const string& query) override;
@@ -51,7 +51,7 @@ public:
 
   sqlite3* db() { return this->m_pDB; };
 
-  bool inTransaction() { return m_in_transaction; };
+  [[nodiscard]] bool inTransaction() const { return m_in_transaction; };
 
   //! Used to create an backend specific exception message.
   SSqlException sPerrorException(const std::string& reason) override;