From 0e006de9f23b0cc49b4a4cf8dae4f92923f905f9 Mon Sep 17 00:00:00 2001 From: Fred Morcos Date: Wed, 16 Oct 2024 13:08:26 +0200 Subject: [PATCH 01/13] Clang-tidy cleanups of lmdb-typed --- ext/lmdb-safe/lmdb-typed.hh | 123 +++++++++++++++++++----------------- 1 file changed, 66 insertions(+), 57 deletions(-) diff --git a/ext/lmdb-safe/lmdb-typed.hh b/ext/lmdb-safe/lmdb-typed.hh index 14e775e1bcc6..a55f67d63aa8 100644 --- a/ext/lmdb-safe/lmdb-typed.hh +++ b/ext/lmdb-safe/lmdb-typed.hh @@ -1,9 +1,6 @@ #pragma once #include -#include -#include -#include #include #include @@ -13,6 +10,7 @@ #include #include #include +#include #include "lmdb-safe.hh" @@ -31,7 +29,7 @@ /** * LMDB ID Vector Type. */ -typedef std::vector LmdbIdVec; +using LmdbIdVec = std::vector; /** * Return the highest ID used in a database. Returns 0 for an empty DB. This makes us @@ -75,7 +73,7 @@ inline std::string keyConv(const T& value); template ::value, T>::type* = nullptr> inline std::string keyConv(const T& value) { - return std::string((char*)&value, sizeof(value)); + return string{(char*)&value, sizeof(value)}; } /** @@ -93,7 +91,7 @@ namespace { throw std::runtime_error("combined key too short to get ID from"); } - MDBOutVal ret; + MDBOutVal ret{}; ret.d_mdbval.mv_data = combined.d_mdbval.mv_data; ret.d_mdbval.mv_size = combined.d_mdbval.mv_size - sizeof(uint32_t); @@ -105,7 +103,7 @@ namespace { throw std::runtime_error("combined key too short to get ID from"); } - MDBOutVal ret; + MDBOutVal ret{}; ret.d_mdbval.mv_data = (char*) combined.d_mdbval.mv_data + combined.d_mdbval.mv_size - sizeof(uint32_t); ret.d_mdbval.mv_size = sizeof(uint32_t); @@ -149,7 +147,7 @@ struct LMDBIndexOps void put(MDBRWTransaction& txn, const Class& t, uint32_t id, int flags=0) { - std::string sempty(""); + std::string sempty; MDBInVal empty(sempty); auto scombined = makeCombinedKey(keyConv(d_parent->getMember(t)), id); @@ -164,8 +162,9 @@ struct LMDBIndexOps auto scombined = makeCombinedKey(keyConv(d_parent->getMember(t)), id); MDBInVal combined(scombined); - if(int rc = txn->del(d_idx, combined)) { - throw std::runtime_error("Error deleting from index: " + std::string(mdb_strerror(rc))); + int errCode = txn->del(d_idx, combined); + if (errCode != 0) { + throw std::runtime_error("Error deleting from index: " + std::string(mdb_strerror(errCode))); } } @@ -190,7 +189,7 @@ struct index_on : LMDBIndexOps> return c.*PtrToMember; } - typedef Type type; + using type = Type; }; /** This is a calculated index */ @@ -205,7 +204,7 @@ struct index_on_function : LMDBIndexOps etc - typedef std::tuple tuple_t; + using tuple_t = std::tuple; tuple_t d_tuple; // We support readonly and rw transactions. Here we put the Readonly operations @@ -279,9 +278,10 @@ public: //! Get item with id, from main table directly bool get(uint32_t id, T& t) { - MDBOutVal data; - if((*d_parent.d_txn)->get(d_parent.d_parent->d_main, id, data)) + MDBOutVal data{}; + if((*d_parent.d_txn)->get(d_parent.d_parent->d_main, id, data)) { return false; + } deserializeFromBuffer(data.get(), t); return true; @@ -367,44 +367,46 @@ public: // id.d_mdbval.mv_size -= LS_HEADER_SIZE; // id.d_mdbval.mv_data = (char*)d_id.d_mdbval.mv_data + LS_HEADER_SIZE; - - if(d_on_index) { - if((*d_parent->d_txn)->get(d_parent->d_parent->d_main, d_id, d_data)) + if (d_on_index) { + if ((*d_parent->d_txn)->get(d_parent->d_parent->d_main, d_id, d_data)) { throw std::runtime_error("Missing id in constructor"); + } deserializeFromBuffer(d_data.get(), d_t); } - else + else { deserializeFromBuffer(d_id.get(), d_t); + } } - explicit iter_t(Parent* parent, typename Parent::cursor_t&& cursor, const std::string& prefix) : + explicit iter_t(Parent* parent, typename Parent::cursor_t&& cursor, std::string prefix) : d_parent(parent), d_cursor(std::move(cursor)), d_on_index(true), // is this an iterator on main database or on index? d_one_key(false), - d_prefix(prefix), - d_end(false) + d_prefix(std::move(prefix)) { - if(d_end) + if (d_end) { return; + } - if(d_cursor.get(d_key, d_id, MDB_GET_CURRENT)) { + if (d_cursor.get(d_key, d_id, MDB_GET_CURRENT)) { d_end = true; return; } d_id = getIDFromCombinedKey(d_key); - if(d_on_index) { - if((*d_parent->d_txn)->get(d_parent->d_parent->d_main, d_id, d_data)) + if (d_on_index) { + if ((*d_parent->d_txn)->get(d_parent->d_parent->d_main, d_id, d_data)) { throw std::runtime_error("Missing id in constructor"); + } deserializeFromBuffer(d_data.get(), d_t); } - else + else { deserializeFromBuffer(d_id.get(), d_t); + } } - // std::function filter; void del() { @@ -434,8 +436,8 @@ public: // implements generic ++ or -- iter_t& genoperator(MDB_cursor_op op) { - MDBOutVal data; - int rc; + MDBOutVal data{}; + int rc = 0; // next:; if (!d_one_key) { rc = d_cursor.get(d_key, d_id, op); @@ -443,7 +445,7 @@ public: if(d_one_key || rc == MDB_NOTFOUND) { d_end = true; } - else if(rc) { + else if(rc != 0) { throw std::runtime_error("in genoperator, " + std::string(mdb_strerror(rc))); } else if(!d_prefix.empty() && @@ -461,8 +463,9 @@ public: if(d_on_index) { d_id = getIDFromCombinedKey(d_key); - if((*d_parent->d_txn)->get(d_parent->d_parent->d_main, d_id, data)) + if ((*d_parent->d_txn)->get(d_parent->d_parent->d_main, d_id, data)) { throw std::runtime_error("Missing id field"); + } // if(filter && !filter(data)) // goto next; @@ -490,13 +493,11 @@ public: // get ID this iterator points to uint32_t getID() { - if(d_on_index) { + if (d_on_index) { // return d_id.get(); return d_id.getNoStripHeader(); } - else { - return d_key.getNoStripHeader(); - } + return d_key.getNoStripHeader(); } const MDBOutVal& getKey() @@ -504,13 +505,14 @@ public: return d_key; } - // transaction we are part of Parent* d_parent; typename Parent::cursor_t d_cursor; // gcc complains if I don't zero-init these, which is worrying XXX - MDBOutVal d_key{{0,0}}, d_data{{0,0}}, d_id{{0,0}}; + MDBOutVal d_key{{0, nullptr}}; + MDBOutVal d_data{{0, nullptr}}; + MDBOutVal d_id{{0, nullptr}}; bool d_on_index; bool d_one_key; std::string d_prefix; @@ -523,7 +525,8 @@ public: { typename Parent::cursor_t cursor = (*d_parent.d_txn)->getCursor(std::get(d_parent.d_parent->d_tuple).d_idx); - MDBOutVal out, id; + MDBOutVal out{}; + MDBOutVal id{}; if(cursor.get(out, id, op)) { // on_index, one_key, end @@ -549,7 +552,8 @@ public: { typename Parent::cursor_t cursor = (*d_parent.d_txn)->getCursor(d_parent.d_parent->d_main); - MDBOutVal out, id; + MDBOutVal out{}; + MDBOutVal id{}; if(cursor.get(out, id, MDB_FIRST)) { // on_index, one_key, end @@ -572,7 +576,8 @@ public: std::string keystr = makeCombinedKey(keyConv(key), MDBInVal("")); MDBInVal in(keystr); - MDBOutVal out, id; + MDBOutVal out{}; + MDBOutVal id{}; out.d_mdbval = in.d_mdbval; if(cursor.get(out, id, op)) { @@ -604,7 +609,8 @@ public: std::string keyString=makeCombinedKey(keyConv(key), MDBInVal("")); MDBInVal in(keyString); - MDBOutVal out, id; + MDBOutVal out{}; + MDBOutVal id{}; out.d_mdbval = in.d_mdbval; if(cursor.get(out, id, MDB_SET)) { @@ -623,7 +629,8 @@ public: std::string keyString=makeCombined(keyConv(key), MDBInVal("")); MDBInVal in(keyString); - MDBOutVal out, id; + MDBOutVal out{}; + MDBOutVal id{}; out.d_mdbval = in.d_mdbval; if(cursor.get(out, id, MDB_SET_RANGE) || @@ -643,7 +650,8 @@ public: std::string keyString=makeCombinedKey(keyConv(key), MDBInVal("")); MDBInVal in(keyString); - MDBOutVal out, id; + MDBOutVal out{}; + MDBOutVal id{}; out.d_mdbval = in.d_mdbval; int rc = cursor.get(out, id, MDB_SET_RANGE); @@ -664,7 +672,7 @@ public: if (sthiskey == keyString) { auto _id = getIDFromCombinedKey(out); uint64_t ts = LMDBLS::LSgetTimestamp(id.getNoStripHeader()); - uint32_t __id = _id.getNoStripHeader(); + auto __id = _id.getNoStripHeader(); if (onlyOldest) { if (ts < oldestts) { @@ -715,26 +723,24 @@ public: return d_txn; } - typedef MDBROCursor cursor_t; + using cursor_t = MDBROCursor; TypedDBI* d_parent; std::shared_ptr d_txn; }; - class RWTransaction : public ReadonlyOperations { public: - explicit RWTransaction(TypedDBI* parent) : ReadonlyOperations(*this), d_parent(parent) + explicit RWTransaction(TypedDBI* parent) : + ReadonlyOperations(*this), d_parent(parent), d_txn(std::make_shared(d_parent->d_env->getRWTransaction())) { - d_txn = std::make_shared(d_parent->d_env->getRWTransaction()); } explicit RWTransaction(TypedDBI* parent, std::shared_ptr txn) : ReadonlyOperations(*this), d_parent(parent), d_txn(txn) { } - RWTransaction(RWTransaction&& rhs) : ReadonlyOperations(*this), d_parent(rhs.d_parent), d_txn(std::move(rhs.d_txn)) @@ -772,8 +778,9 @@ public: void modify(uint32_t id, std::function func) { T t; - if(!this->get(id, t)) - throw std::runtime_error("Could not modify id "+std::to_string(id)); + if (!this->get(id, t)) { + throw std::runtime_error("Could not modify id " + std::to_string(id)); + } func(t); del(id); // this is the lazy way. We could test for changed index fields @@ -784,8 +791,9 @@ public: void del(uint32_t id) { T t; - if(!this->get(id, t)) + if (!this->get(id, t)) { return; + } (*d_txn)->del(d_parent->d_main, id); clearIndex(id, t); @@ -796,7 +804,8 @@ public: { auto cursor = (*d_txn)->getRWCursor(d_parent->d_main); bool first = true; - MDBOutVal key, data; + MDBOutVal key{}; + MDBOutVal data{}; while(!cursor.get(key, data, first ? MDB_FIRST : MDB_NEXT)) { first = false; T t; @@ -818,7 +827,7 @@ public: (*d_txn)->abort(); } - typedef MDBRWCursor cursor_t; + using cursor_t = MDBRWCursor; std::shared_ptr getTransactionHandle() { From fd6c3e3c03c6257c443d4913b875f3c1b48f2175 Mon Sep 17 00:00:00 2001 From: Fred Morcos Date: Wed, 16 Oct 2024 13:31:09 +0200 Subject: [PATCH 02/13] Use uint32_t in MDBGetMaxID and MDBGetRandomID --- ext/lmdb-safe/lmdb-typed.cc | 15 ++++++++------- ext/lmdb-safe/lmdb-typed.hh | 4 ++-- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/ext/lmdb-safe/lmdb-typed.cc b/ext/lmdb-safe/lmdb-typed.cc index e9a2d1e2c089..ff63ea8f2c8f 100644 --- a/ext/lmdb-safe/lmdb-typed.cc +++ b/ext/lmdb-safe/lmdb-typed.cc @@ -1,28 +1,29 @@ #include "lmdb-typed.hh" #include "pdns/dns_random.hh" -unsigned int MDBGetMaxID(MDBRWTransaction& txn, MDBDbi& dbi) +uint32_t MDBGetMaxID(MDBRWTransaction& txn, MDBDbi& dbi) { auto cursor = txn->getRWCursor(dbi); MDBOutVal maxidval{}; MDBOutVal maxcontent{}; - unsigned int maxid{0}; + uint32_t maxid{0}; if (cursor.get(maxidval, maxcontent, MDB_LAST) == 0) { - maxid = maxidval.getNoStripHeader(); + maxid = maxidval.getNoStripHeader(); } return maxid; } -unsigned int MDBGetRandomID(MDBRWTransaction& txn, MDBDbi& dbi) +uint32_t MDBGetRandomID(MDBRWTransaction& txn, MDBDbi& dbi) { auto cursor = txn->getRWCursor(dbi); - unsigned int newID = 0; + uint32_t newID = 0; for (int attempts = 0; attempts < 20; attempts++) { MDBOutVal key{}; MDBOutVal content{}; - // dns_random generates a random number in [0..signed_int_max-1]. We add 1 to avoid 0 and allow type_max. - // 0 is avoided because the put() interface uses it to mean "please allocate a number for me" + // dns_random generates a random number in [0..signed_int_max-1]. We add 1 to avoid 0 + // and allow type_max. 0 is avoided because the put() interface uses it to mean + // "please allocate a number for me". newID = dns_random(std::numeric_limits::max()) + 1; if (cursor.find(MDBInVal(newID), key, content) != 0) { return newID; diff --git a/ext/lmdb-safe/lmdb-typed.hh b/ext/lmdb-safe/lmdb-typed.hh index a55f67d63aa8..35cdaf1e28a3 100644 --- a/ext/lmdb-safe/lmdb-typed.hh +++ b/ext/lmdb-safe/lmdb-typed.hh @@ -35,13 +35,13 @@ using LmdbIdVec = std::vector; * Return the highest ID used in a database. Returns 0 for an empty DB. This makes us * start everything at ID=1, which might make it possible to treat id 0 as special. */ -unsigned int MDBGetMaxID(MDBRWTransaction& txn, MDBDbi& dbi); +uint32_t MDBGetMaxID(MDBRWTransaction& txn, MDBDbi& dbi); /** * Return a randomly generated ID that is unique and not zero. May throw if the database * is very full. */ -unsigned int MDBGetRandomID(MDBRWTransaction& txn, MDBDbi& dbi); +uint32_t MDBGetRandomID(MDBRWTransaction& txn, MDBDbi& dbi); /** * This is our serialization interface. It can be specialized for other types. From 1b0e658fcb743057d1d01388f613fe2cedcc4c3a Mon Sep 17 00:00:00 2001 From: Fred Morcos Date: Wed, 16 Oct 2024 15:15:11 +0200 Subject: [PATCH 03/13] Specialize MDBOutVal::get/getNoStripHeader for uint32_t --- ext/lmdb-safe/lmdb-safe.hh | 68 ++++++++++++++++++-------------------- 1 file changed, 32 insertions(+), 36 deletions(-) diff --git a/ext/lmdb-safe/lmdb-safe.hh b/ext/lmdb-safe/lmdb-safe.hh index 1c62d8802964..1cb7aa41e8d3 100644 --- a/ext/lmdb-safe/lmdb-safe.hh +++ b/ext/lmdb-safe/lmdb-safe.hh @@ -174,54 +174,50 @@ struct MDBOutVal } #ifndef DNSDIST - template ::value, - T>::type* = nullptr> const - T get() - { - T ret; - - size_t offset = LMDBLS::LScheckHeaderAndGetSize(this, sizeof(T)); - - memcpy(&ret, reinterpret_cast(d_mdbval.mv_data)+offset, sizeof(T)); - - static_assert(sizeof(T) == 4, "this code currently only supports 32 bit integers"); - ret = ntohl(ret); - return ret; - } - - template ::value, - T>::type* = nullptr> const - T getNoStripHeader() - { - T ret; - if(d_mdbval.mv_size != sizeof(T)) - throw std::runtime_error("MDB data has wrong length for type"); - - memcpy(&ret, d_mdbval.mv_data, sizeof(T)); - - static_assert(sizeof(T) == 4, "this code currently only supports 32 bit integers"); - ret = ntohl(ret); - return ret; - } + template ::value, T>::type* = nullptr> + T get() const; + template ::value, T>::type* = nullptr> + T getNoStripHeader() const; #endif /* ifndef DNSDIST */ - template ::value,T>::type* = nullptr> + template ::value, T>::type* = nullptr> T get() const; - #ifndef DNSDIST - template ::value,T>::type* = nullptr> + template ::value, T>::type* = nullptr> T getNoStripHeader() const; #endif MDB_val d_mdbval; }; +#ifndef DNSDIST +template <> +inline uint32_t MDBOutVal::get() const +{ + uint32_t ret = 0; + size_t offset = LMDBLS::LScheckHeaderAndGetSize(this, sizeof(uint32_t)); + // NOLINTNEXTLINE + memcpy(&ret, reinterpret_cast(d_mdbval.mv_data) + offset, sizeof(uint32_t)); + ret = ntohl(ret); + return ret; +} + +template <> +inline uint32_t MDBOutVal::getNoStripHeader() const +{ + uint32_t ret = 0; + if (d_mdbval.mv_size != sizeof(uint32_t)) { + throw std::runtime_error("MDB data has wrong length for type"); + } + + memcpy(&ret, d_mdbval.mv_data, sizeof(uint32_t)); + ret = ntohl(ret); + return ret; +} +#endif /* ifndef DNSDIST */ + template<> inline std::string MDBOutVal::get() const { #ifndef DNSDIST From 7459aaa15c3ab5275952343693dd4fb43775364a Mon Sep 17 00:00:00 2001 From: Fred Morcos Date: Wed, 16 Oct 2024 15:17:53 +0200 Subject: [PATCH 04/13] Use uint32_t in makeCombinedKey --- ext/lmdb-safe/lmdb-typed.hh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ext/lmdb-safe/lmdb-typed.hh b/ext/lmdb-safe/lmdb-typed.hh index 35cdaf1e28a3..9558d89af3a1 100644 --- a/ext/lmdb-safe/lmdb-typed.hh +++ b/ext/lmdb-safe/lmdb-typed.hh @@ -117,7 +117,7 @@ namespace { std::string sval((char*) val.d_mdbval.mv_data, val.d_mdbval.mv_size); if (val.d_mdbval.mv_size != 0 && // empty val case, for range queries - val.d_mdbval.mv_size != 4) { // uint32_t case + val.d_mdbval.mv_size != sizeof(uint32_t)) { throw std::runtime_error("got wrong size value in makeCombinedKey"); } From 15449ba04551e7c95a76304c1910e0b072d8cb6c Mon Sep 17 00:00:00 2001 From: Fred Morcos Date: Wed, 16 Oct 2024 15:22:32 +0200 Subject: [PATCH 05/13] Cleanup makeCombinedKey --- ext/lmdb-safe/lmdb-typed.hh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ext/lmdb-safe/lmdb-typed.hh b/ext/lmdb-safe/lmdb-typed.hh index 9558d89af3a1..ce7af61154e4 100644 --- a/ext/lmdb-safe/lmdb-typed.hh +++ b/ext/lmdb-safe/lmdb-typed.hh @@ -113,8 +113,8 @@ namespace { inline std::string makeCombinedKey(MDBInVal key, MDBInVal val) { std::string lenprefix(sizeof(uint16_t), '\0'); - std::string skey((char*) key.d_mdbval.mv_data, key.d_mdbval.mv_size); - std::string sval((char*) val.d_mdbval.mv_data, val.d_mdbval.mv_size); + std::string skey(static_cast(key.d_mdbval.mv_data), key.d_mdbval.mv_size); + std::string sval(static_cast(val.d_mdbval.mv_data), val.d_mdbval.mv_size); if (val.d_mdbval.mv_size != 0 && // empty val case, for range queries val.d_mdbval.mv_size != sizeof(uint32_t)) { From 4528d7909f42ab6e1117d8f842d830b59f2d7d8d Mon Sep 17 00:00:00 2001 From: Fred Morcos Date: Wed, 16 Oct 2024 15:25:25 +0200 Subject: [PATCH 06/13] Cleanup LMDBIndexOps --- ext/lmdb-safe/lmdb-typed.hh | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/ext/lmdb-safe/lmdb-typed.hh b/ext/lmdb-safe/lmdb-typed.hh index ce7af61154e4..ebf52bd0f8c7 100644 --- a/ext/lmdb-safe/lmdb-typed.hh +++ b/ext/lmdb-safe/lmdb-typed.hh @@ -145,21 +145,18 @@ struct LMDBIndexOps { explicit LMDBIndexOps(Parent* parent) : d_parent(parent){} - void put(MDBRWTransaction& txn, const Class& t, uint32_t id, int flags=0) + void put(MDBRWTransaction& txn, const Class& type, uint32_t idVal, int flags = 0) { - std::string sempty; - MDBInVal empty(sempty); - - auto scombined = makeCombinedKey(keyConv(d_parent->getMember(t)), id); + auto scombined = makeCombinedKey(keyConv(d_parent->getMember(type)), idVal); MDBInVal combined(scombined); // if the entry existed already, this will just update the timestamp/txid in the LS header. This is intentional, so objects and their indexes always get synced together. - txn->put(d_idx, combined, empty, flags); + txn->put(d_idx, combined, string{}, flags); } - void del(MDBRWTransaction& txn, const Class& t, uint32_t id) + void del(MDBRWTransaction& txn, const Class& type, uint32_t idVal) { - auto scombined = makeCombinedKey(keyConv(d_parent->getMember(t)), id); + auto scombined = makeCombinedKey(keyConv(d_parent->getMember(type)), idVal); MDBInVal combined(scombined); int errCode = txn->del(d_idx, combined); @@ -172,9 +169,9 @@ struct LMDBIndexOps { d_idx = env->openDB(str, flags); } + MDBDbi d_idx; Parent* d_parent; - }; /** This is an index on a field in a struct, it derives from the LMDBIndexOps */ From 048349a860ecb04e5d7a14067634686d25eb7432 Mon Sep 17 00:00:00 2001 From: Fred Morcos Date: Wed, 16 Oct 2024 15:28:02 +0200 Subject: [PATCH 07/13] Cleanup struct index_on and index_on_function --- ext/lmdb-safe/lmdb-typed.hh | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/ext/lmdb-safe/lmdb-typed.hh b/ext/lmdb-safe/lmdb-typed.hh index ebf52bd0f8c7..8d7519baeb27 100644 --- a/ext/lmdb-safe/lmdb-typed.hh +++ b/ext/lmdb-safe/lmdb-typed.hh @@ -176,29 +176,31 @@ struct LMDBIndexOps /** This is an index on a field in a struct, it derives from the LMDBIndexOps */ -template +template struct index_on : LMDBIndexOps> { - index_on() : LMDBIndexOps>(this) + index_on() : + LMDBIndexOps>(this) {} - static Type getMember(const Class& c) + static Type getMember(const Class& klass) { - return c.*PtrToMember; + return klass.*PtrToMember; } using type = Type; }; /** This is a calculated index */ -template -struct index_on_function : LMDBIndexOps > +template +struct index_on_function : LMDBIndexOps> { - index_on_function() : LMDBIndexOps >(this) + index_on_function() : + LMDBIndexOps>(this) {} - static Type getMember(const Class& c) + static Type getMember(const Class& klass) { - Func f; - return f(c); + Func function; + return function(klass); } using type = Type; From 6eed3a8bba073bde8c50ea0108da1d7aa2a997d3 Mon Sep 17 00:00:00 2001 From: Fred Morcos Date: Wed, 16 Oct 2024 15:47:19 +0200 Subject: [PATCH 08/13] Cleanup struct nullindex_t --- ext/lmdb-safe/lmdb-typed.hh | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/ext/lmdb-safe/lmdb-typed.hh b/ext/lmdb-safe/lmdb-typed.hh index 8d7519baeb27..c6e95dbac743 100644 --- a/ext/lmdb-safe/lmdb-typed.hh +++ b/ext/lmdb-safe/lmdb-typed.hh @@ -209,16 +209,15 @@ struct index_on_function : LMDBIndexOps - void put(MDBRWTransaction& /* txn */, const Class& /* t */, uint32_t /* id */, int /* flags */ =0) + template + void put(MDBRWTransaction& /* txn */, const Class& /* t */, uint32_t /* id */, int /* flags */ = 0) {} - template + template void del(MDBRWTransaction& /* txn */, const Class& /* t */, uint32_t /* id */) {} void openDB(std::shared_ptr& /* env */, string_view /* str */, int /* flags */) { - } using type = uint32_t; // dummy From c84d5b50caf0324b2224d7c9124f6e1438fb9b5d Mon Sep 17 00:00:00 2001 From: Fred Morcos Date: Wed, 16 Oct 2024 15:49:30 +0200 Subject: [PATCH 09/13] Cleanup TypedDBI --- ext/lmdb-safe/lmdb-typed.hh | 35 ++++++++++++++++++----------------- 1 file changed, 18 insertions(+), 17 deletions(-) diff --git a/ext/lmdb-safe/lmdb-typed.hh b/ext/lmdb-safe/lmdb-typed.hh index c6e95dbac743..be5608f71827 100644 --- a/ext/lmdb-safe/lmdb-typed.hh +++ b/ext/lmdb-safe/lmdb-typed.hh @@ -224,30 +224,31 @@ struct nullindex_t }; /** The main class. Templatized only on the indexes and typename right now */ -template +template class TypedDBI { + // we get a lot of our smarts from this tuple, it enables get<0> etc + using tuple_t = std::tuple; + tuple_t d_tuple; + +private: + template + auto openDB(string_view& name) + { + std::get(d_tuple).openDB(d_env, std::string(name) + "_" + std::to_string(N), MDB_CREATE); + } + public: - TypedDBI(std::shared_ptr env, string_view name) - : d_env(std::move(env)), d_name(name) + TypedDBI(std::shared_ptr env, string_view name) : + d_env(std::move(env)), d_name(name) { d_main = d_env->openDB(name, MDB_CREATE); - - // now you might be tempted to go all MPL on this so we can get rid of the - // ugly macro. I'm not very receptive to that idea since it will make things - // EVEN uglier. -#define openMacro(N) std::get(d_tuple).openDB(d_env, std::string(name)+"_"#N, MDB_CREATE); - openMacro(0); - openMacro(1); - openMacro(2); - openMacro(3); -#undef openMacro + openDB<0>(name); + openDB<1>(name); + openDB<2>(name); + openDB<3>(name); } - // we get a lot of our smarts from this tuple, it enables get<0> etc - using tuple_t = std::tuple; - tuple_t d_tuple; - // We support readonly and rw transactions. Here we put the Readonly operations // which get sourced by both kinds of transactions template From 9c61935832f5b8f0dba31f132865cc8b0254760f Mon Sep 17 00:00:00 2001 From: Fred Morcos Date: Wed, 16 Oct 2024 15:54:51 +0200 Subject: [PATCH 10/13] Some cleanups in lmdbbackend --- modules/lmdbbackend/lmdbbackend.cc | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/modules/lmdbbackend/lmdbbackend.cc b/modules/lmdbbackend/lmdbbackend.cc index e632c88ed650..6fadaafd6f4b 100644 --- a/modules/lmdbbackend/lmdbbackend.cc +++ b/modules/lmdbbackend/lmdbbackend.cc @@ -54,7 +54,7 @@ #include #endif -#include +#include #include #include "lmdbbackend.hh" @@ -66,14 +66,14 @@ BOOST_CLASS_VERSION(LMDBBackend::KeyDataDB, 1) BOOST_CLASS_VERSION(DomainInfo, 1) static bool s_first = true; -static int s_shards = 0; +static uint32_t s_shards = 0; static std::mutex s_lmdbStartupLock; std::pair LMDBBackend::getSchemaVersionAndShards(std::string& filename) { // cerr << "getting schema version for path " << filename << endl; - uint32_t schemaversion; + uint32_t schemaversion = 0; MDB_env* tmpEnv = nullptr; @@ -160,7 +160,7 @@ std::pair LMDBBackend::getSchemaVersionAndShards(std::string throw std::runtime_error("pdns.schemaversion had unexpected size"); } - uint32_t shards; + uint32_t shards = 0; key.mv_data = (char*)"shards"; key.mv_size = strlen((char*)key.mv_data); @@ -392,7 +392,7 @@ bool LMDBBackend::upgradeToSchemav5(std::string& filename) MDB_txn* txn = nullptr; - if ((rc = mdb_txn_begin(env, NULL, 0, &txn)) != 0) { + if ((rc = mdb_txn_begin(env, nullptr, 0, &txn)) != 0) { mdb_env_close(env); throw std::runtime_error("mdb_txn_begin failed"); } @@ -742,8 +742,8 @@ LMDBBackend::LMDBBackend(const std::string& suffix) auto txn = d_tdomains->getEnv()->getRWTransaction(); - MDBOutVal shards; - if (!txn->get(pdnsdbi, "shards", shards)) { + MDBOutVal shards{}; + if (txn->get(pdnsdbi, "shards", shards) == 0) { s_shards = shards.get(); if (mustDo("lightning-stream") && s_shards != 1) { @@ -759,15 +759,15 @@ LMDBBackend::LMDBBackend(const std::string& suffix) txn->put(pdnsdbi, "shards", s_shards); } - MDBOutVal gotuuid; - if (txn->get(pdnsdbi, "uuid", gotuuid)) { + MDBOutVal gotuuid{}; + if (txn->get(pdnsdbi, "uuid", gotuuid) != 0) { const auto uuid = getUniqueID(); const string uuids(uuid.begin(), uuid.end()); txn->put(pdnsdbi, "uuid", uuids); } - MDBOutVal _schemaversion; - if (txn->get(pdnsdbi, "schemaversion", _schemaversion)) { + MDBOutVal _schemaversion{}; + if (txn->get(pdnsdbi, "schemaversion", _schemaversion) != 0) { // our DB is entirely new, so we need to write the schemaversion txn->put(pdnsdbi, "schemaversion", currentSchemaVersion); } From f984dc57671b03d76c206fd4b59415521e73175e Mon Sep 17 00:00:00 2001 From: Fred Morcos Date: Thu, 17 Oct 2024 10:16:49 +0200 Subject: [PATCH 11/13] Cleanup LMDBBackend::upgradeToSchemav5 --- modules/lmdbbackend/lmdbbackend.cc | 84 +++++++++++++++++------------- 1 file changed, 49 insertions(+), 35 deletions(-) diff --git a/modules/lmdbbackend/lmdbbackend.cc b/modules/lmdbbackend/lmdbbackend.cc index 6fadaafd6f4b..d3a2be6ab306 100644 --- a/modules/lmdbbackend/lmdbbackend.cc +++ b/modules/lmdbbackend/lmdbbackend.cc @@ -364,8 +364,6 @@ void copyIndexDBI(MDB_txn* txn, MDB_dbi sdbi, MDB_dbi tdbi) bool LMDBBackend::upgradeToSchemav5(std::string& filename) { - int rc; - auto currentSchemaVersionAndShards = getSchemaVersionAndShards(filename); uint32_t currentSchemaVersion = currentSchemaVersionAndShards.first; uint32_t shards = currentSchemaVersionAndShards.second; @@ -376,23 +374,23 @@ bool LMDBBackend::upgradeToSchemav5(std::string& filename) MDB_env* env = nullptr; - if ((rc = mdb_env_create(&env)) != 0) { + if (mdb_env_create(&env) != 0) { throw std::runtime_error("mdb_env_create failed"); } - if ((rc = mdb_env_set_maxdbs(env, 20)) != 0) { + if (mdb_env_set_maxdbs(env, 20) != 0) { mdb_env_close(env); throw std::runtime_error("mdb_env_set_maxdbs failed"); } - if ((rc = mdb_env_open(env, filename.c_str(), MDB_NOSUBDIR, 0600)) != 0) { + if (mdb_env_open(env, filename.c_str(), MDB_NOSUBDIR, 0600) != 0) { mdb_env_close(env); throw std::runtime_error("mdb_env_open failed"); } MDB_txn* txn = nullptr; - if ((rc = mdb_txn_begin(env, nullptr, 0, &txn)) != 0) { + if (mdb_txn_begin(env, nullptr, 0, &txn) != 0) { mdb_env_close(env); throw std::runtime_error("mdb_txn_begin failed"); } @@ -418,31 +416,32 @@ bool LMDBBackend::upgradeToSchemav5(std::string& filename) std::cerr << "migrating shard " << shardfile << std::endl; MDB_env* shenv = nullptr; - if ((rc = mdb_env_create(&shenv)) != 0) { + if (mdb_env_create(&shenv) != 0) { throw std::runtime_error("mdb_env_create failed"); } - if ((rc = mdb_env_set_maxdbs(shenv, 8)) != 0) { + if (mdb_env_set_maxdbs(shenv, 8) != 0) { mdb_env_close(env); throw std::runtime_error("mdb_env_set_maxdbs failed"); } - if ((rc = mdb_env_open(shenv, shardfile.c_str(), MDB_NOSUBDIR, 0600)) != 0) { + if (mdb_env_open(shenv, shardfile.c_str(), MDB_NOSUBDIR, 0600) != 0) { mdb_env_close(env); throw std::runtime_error("mdb_env_open failed"); } MDB_txn* shtxn = nullptr; - if ((rc = mdb_txn_begin(shenv, NULL, 0, &shtxn)) != 0) { + if (mdb_txn_begin(shenv, nullptr, 0, &shtxn) != 0) { mdb_env_close(env); throw std::runtime_error("mdb_txn_begin failed"); } - MDB_dbi shdbi; + MDB_dbi shdbi = 0; - if ((rc = mdb_dbi_open(shtxn, "records", 0, &shdbi)) != 0) { - if (rc == MDB_NOTFOUND) { + const auto dbiOpenRc = mdb_dbi_open(shtxn, "records", 0, &shdbi); + if (dbiOpenRc != 0) { + if (dbiOpenRc == MDB_NOTFOUND) { mdb_txn_abort(shtxn); mdb_env_close(shenv); continue; @@ -452,9 +451,9 @@ bool LMDBBackend::upgradeToSchemav5(std::string& filename) throw std::runtime_error("mdb_dbi_open shard records failed"); } - MDB_dbi shdbi2; + MDB_dbi shdbi2 = 0; - if ((rc = mdb_dbi_open(shtxn, "records_v5", MDB_CREATE, &shdbi2)) != 0) { + if (mdb_dbi_open(shtxn, "records_v5", MDB_CREATE, &shdbi2) != 0) { mdb_dbi_close(shenv, shdbi); mdb_txn_abort(shtxn); mdb_env_close(shenv); @@ -478,8 +477,8 @@ bool LMDBBackend::upgradeToSchemav5(std::string& filename) mdb_env_close(shenv); } - std::array fromtypeddbi; - std::array totypeddbi; + std::array fromtypeddbi{}; + std::array totypeddbi{}; int index = 0; @@ -487,13 +486,16 @@ bool LMDBBackend::upgradeToSchemav5(std::string& filename) std::cerr << "migrating " << dbname << std::endl; std::string tdbname = dbname + "_v5"; - if ((rc = mdb_dbi_open(txn, dbname.c_str(), 0, &fromtypeddbi[index])) != 0) { + // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-constant-array-index) + if (mdb_dbi_open(txn, dbname.c_str(), 0, &fromtypeddbi[index]) != 0) { mdb_txn_abort(txn); mdb_env_close(env); throw std::runtime_error("mdb_dbi_open typeddbi failed"); } - if ((rc = mdb_dbi_open(txn, tdbname.c_str(), MDB_CREATE, &totypeddbi[index])) != 0) { + // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-constant-array-index) + if (mdb_dbi_open(txn, tdbname.c_str(), MDB_CREATE, &totypeddbi[index]) != 0) { + // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-constant-array-index) mdb_dbi_close(env, fromtypeddbi[index]); mdb_txn_abort(txn); mdb_env_close(env); @@ -501,10 +503,13 @@ bool LMDBBackend::upgradeToSchemav5(std::string& filename) } try { + // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-constant-array-index) copyTypedDBI(txn, fromtypeddbi[index], totypeddbi[index]); } catch (std::exception& e) { + // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-constant-array-index) mdb_dbi_close(env, totypeddbi[index]); + // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-constant-array-index) mdb_dbi_close(env, fromtypeddbi[index]); mdb_txn_abort(txn); mdb_env_close(env); @@ -518,8 +523,8 @@ bool LMDBBackend::upgradeToSchemav5(std::string& filename) index++; } - std::array fromindexdbi; - std::array toindexdbi; + std::array fromindexdbi{}; + std::array toindexdbi{}; index = 0; @@ -528,13 +533,16 @@ bool LMDBBackend::upgradeToSchemav5(std::string& filename) std::cerr << "migrating " << dbname << std::endl; std::string tdbname = dbname + "_v5_0"; - if ((rc = mdb_dbi_open(txn, fdbname.c_str(), 0, &fromindexdbi[index])) != 0) { + // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-constant-array-index) + if (mdb_dbi_open(txn, fdbname.c_str(), 0, &fromindexdbi[index]) != 0) { mdb_txn_abort(txn); mdb_env_close(env); throw std::runtime_error("mdb_dbi_open indexdbi failed"); } - if ((rc = mdb_dbi_open(txn, tdbname.c_str(), MDB_CREATE, &toindexdbi[index])) != 0) { + // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-constant-array-index) + if (mdb_dbi_open(txn, tdbname.c_str(), MDB_CREATE, &toindexdbi[index]) != 0) { + // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-constant-array-index) mdb_dbi_close(env, fromindexdbi[index]); mdb_txn_abort(txn); mdb_env_close(env); @@ -542,10 +550,13 @@ bool LMDBBackend::upgradeToSchemav5(std::string& filename) } try { + // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-constant-array-index) copyIndexDBI(txn, fromindexdbi[index], toindexdbi[index]); } catch (std::exception& e) { + // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-constant-array-index) mdb_dbi_close(env, toindexdbi[index]); + // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-constant-array-index) mdb_dbi_close(env, fromindexdbi[index]); mdb_txn_abort(txn); mdb_env_close(env); @@ -559,16 +570,17 @@ bool LMDBBackend::upgradeToSchemav5(std::string& filename) index++; } - MDB_dbi dbi; + MDB_dbi dbi = 0; // finally, migrate the pdns db - if ((rc = mdb_dbi_open(txn, "pdns", 0, &dbi)) != 0) { + if (mdb_dbi_open(txn, "pdns", 0, &dbi) != 0) { mdb_txn_abort(txn); mdb_env_close(env); throw std::runtime_error("mdb_dbi_open pdns failed"); } - MDB_val key, data; + MDB_val key; + MDB_val data; std::string header(LMDBLS::LS_MIN_HEADER_SIZE, '\0'); @@ -578,16 +590,15 @@ bool LMDBBackend::upgradeToSchemav5(std::string& filename) key.mv_data = (char*)keyname.c_str(); key.mv_size = keyname.size(); - if ((rc = mdb_get(txn, dbi, &key, &data))) { + if (mdb_get(txn, dbi, &key, &data) != 0) { throw std::runtime_error("mdb_get pdns.shards failed"); } - uint32_t value; - if (data.mv_size != sizeof(uint32_t)) { throw std::runtime_error("got non-uint32_t key"); } + uint32_t value = 0; memcpy((void*)&value, data.mv_data, sizeof(uint32_t)); value = htonl(value); @@ -595,17 +606,16 @@ bool LMDBBackend::upgradeToSchemav5(std::string& filename) value = htonl(5); } - std::string sdata((char*)data.mv_data, data.mv_size); - + std::string sdata(static_cast(data.mv_data), data.mv_size); + // NOLINTNEXTLINE(cppcoreguidelines-pro-type-cstyle-cast) std::string stdata = header + std::string((char*)&value, sizeof(uint32_t)); - ; MDB_val tdata; tdata.mv_data = (char*)stdata.c_str(); tdata.mv_size = stdata.size(); - if ((rc = mdb_put(txn, dbi, &key, &tdata, 0)) != 0) { + if (mdb_put(txn, dbi, &key, &tdata, 0) != 0) { throw std::runtime_error("mdb_put failed"); } } @@ -616,7 +626,7 @@ bool LMDBBackend::upgradeToSchemav5(std::string& filename) key.mv_data = (char*)keyname.c_str(); key.mv_size = keyname.size(); - if ((rc = mdb_get(txn, dbi, &key, &data))) { + if (mdb_get(txn, dbi, &key, &data) != 0) { throw std::runtime_error("mdb_get pdns.shards failed"); } @@ -629,20 +639,24 @@ bool LMDBBackend::upgradeToSchemav5(std::string& filename) tdata.mv_data = (char*)stdata.c_str(); tdata.mv_size = stdata.size(); - if ((rc = mdb_put(txn, dbi, &key, &tdata, 0)) != 0) { + if (mdb_put(txn, dbi, &key, &tdata, 0) != 0) { throw std::runtime_error("mdb_put failed"); } } for (int i = 0; i < 4; i++) { + // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-constant-array-index) mdb_drop(txn, fromtypeddbi[i], 1); + // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-constant-array-index) mdb_drop(txn, fromindexdbi[i], 1); } cerr << "txn commit=" << mdb_txn_commit(txn) << endl; for (int i = 0; i < 4; i++) { + // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-constant-array-index) mdb_dbi_close(env, totypeddbi[i]); + // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-constant-array-index) mdb_dbi_close(env, toindexdbi[i]); } mdb_env_close(env); From 0f13296d5d92dfb59576daa0480988dfa7f29b29 Mon Sep 17 00:00:00 2001 From: Fred Morcos Date: Thu, 17 Oct 2024 10:27:35 +0200 Subject: [PATCH 12/13] Refactor shards cmdline handling --- modules/lmdbbackend/lmdbbackend.cc | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/modules/lmdbbackend/lmdbbackend.cc b/modules/lmdbbackend/lmdbbackend.cc index d3a2be6ab306..f708d84fd106 100644 --- a/modules/lmdbbackend/lmdbbackend.cc +++ b/modules/lmdbbackend/lmdbbackend.cc @@ -756,20 +756,35 @@ LMDBBackend::LMDBBackend(const std::string& suffix) auto txn = d_tdomains->getEnv()->getRWTransaction(); + const auto configShardsTemp = atoi(getArg("shards").c_str()); + if (configShardsTemp < 0) { + throw std::runtime_error("a negative shards value is not supported"); + } + if (configShardsTemp == 0) { + throw std::runtime_error("a shards value of 0 is not supported"); + } + const auto configShards = static_cast(configShardsTemp); + MDBOutVal shards{}; if (txn->get(pdnsdbi, "shards", shards) == 0) { s_shards = shards.get(); if (mustDo("lightning-stream") && s_shards != 1) { - throw std::runtime_error(std::string("running with Lightning Stream support enabled requires a database with exactly 1 shard")); + throw std::runtime_error("running with Lightning Stream support enabled requires a database with exactly 1 shard"); } - if (s_shards != atoi(getArg("shards").c_str())) { - g_log << Logger::Warning << "Note: configured number of lmdb shards (" << atoi(getArg("shards").c_str()) << ") is different from on-disk (" << s_shards << "). Using on-disk shard number" << endl; + if (s_shards != configShards) { + g_log << Logger::Warning + << "Note: configured number of lmdb shards (" + << atoi(getArg("shards").c_str()) + << ") is different from on-disk (" + << s_shards + << "). Using on-disk shard number" + << endl; } } else { - s_shards = atoi(getArg("shards").c_str()); + s_shards = configShards; txn->put(pdnsdbi, "shards", s_shards); } From 7b71bce35097bba7feb81b310a775cd014a39dce Mon Sep 17 00:00:00 2001 From: Fred Morcos Date: Thu, 17 Oct 2024 10:29:31 +0200 Subject: [PATCH 13/13] Cleanup includes in lmdbbackend --- modules/lmdbbackend/lmdbbackend.cc | 41 +++++++++++++----------------- 1 file changed, 17 insertions(+), 24 deletions(-) diff --git a/modules/lmdbbackend/lmdbbackend.cc b/modules/lmdbbackend/lmdbbackend.cc index f708d84fd106..36841e619a68 100644 --- a/modules/lmdbbackend/lmdbbackend.cc +++ b/modules/lmdbbackend/lmdbbackend.cc @@ -20,45 +20,38 @@ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. */ -#include "ext/lmdb-safe/lmdb-safe.hh" -#include -#include -#include -#include -#include -#ifdef HAVE_CONFIG_H +#include "lmdbbackend.hh" + #include "config.h" -#endif -#include "pdns/utility.hh" -#include "pdns/dnsbackend.hh" +#include "ext/lmdb-safe/lmdb-safe.hh" +#include "pdns/arguments.hh" +#include "pdns/base32.hh" #include "pdns/dns.hh" +#include "pdns/dnsbackend.hh" #include "pdns/dnspacket.hh" -#include "pdns/base32.hh" #include "pdns/dnssecinfra.hh" -#include "pdns/pdnsexception.hh" #include "pdns/logger.hh" -#include "pdns/version.hh" -#include "pdns/arguments.hh" -#include "pdns/lock.hh" +#include "pdns/pdnsexception.hh" #include "pdns/uuid-utils.hh" -#include #include -#include +#include +#include #include #include +#include #include - -#include +#include +#include +#include +#include +#include +#include +#include #ifdef HAVE_SYSTEMD #include #endif -#include -#include - -#include "lmdbbackend.hh" - #define SCHEMAVERSION 5 // List the class version here. Default is 0