From cc0a592a718e84e8355ae2af83d6bd5d903d1f97 Mon Sep 17 00:00:00 2001 From: Peter van Dijk Date: Tue, 11 Nov 2025 15:27:21 +0100 Subject: [PATCH 01/35] NFC: rename records dbi to rdbi --- modules/lmdbbackend/lmdbbackend.cc | 54 +++++++++++++++--------------- modules/lmdbbackend/lmdbbackend.hh | 2 +- 2 files changed, 28 insertions(+), 28 deletions(-) diff --git a/modules/lmdbbackend/lmdbbackend.cc b/modules/lmdbbackend/lmdbbackend.cc index 8d95c1cbc89e..5464fec36d11 100644 --- a/modules/lmdbbackend/lmdbbackend.cc +++ b/modules/lmdbbackend/lmdbbackend.cc @@ -1244,7 +1244,7 @@ static uint32_t peekAtTtl(const string_view& buffer) void LMDBBackend::deleteDomainRecords(RecordsRWTransaction& txn, const std::string& match, QType qtype) { - auto cursor = txn.txn->getCursor(txn.db->dbi); + auto cursor = txn.txn->getCursor(txn.db->rdbi); MDBOutVal key{}; MDBOutVal val{}; @@ -1381,13 +1381,13 @@ void LMDBBackend::deleteNSEC3RecordPair(const std::shared_ptrtxn->get(txn->db->dbi, key, val) == 0) { + if (txn->txn->get(txn->db->rdbi, key, val) == 0) { LMDBResourceRecord lrr; if (deserializeFromBuffer(val.get(), lrr)) { DNSName ordername(lrr.content.c_str(), lrr.content.size(), 0, false); - txn->txn->del(txn->db->dbi, co(domain_id, ordername, QType::NSEC3)); + txn->txn->del(txn->db->rdbi, co(domain_id, ordername, QType::NSEC3)); } - txn->txn->del(txn->db->dbi, key); + txn->txn->del(txn->db->rdbi, key); } } @@ -1408,14 +1408,14 @@ void LMDBBackend::writeNSEC3RecordPair(const std::shared_ptrtxn->get(txn->db->dbi, co(domain_id, qname, QType::NSEC3), val) == 0) { + if (txn->txn->get(txn->db->rdbi, co(domain_id, qname, QType::NSEC3), val) == 0) { LMDBResourceRecord lrr; if (deserializeFromBuffer(val.get(), lrr)) { DNSName prevordername(lrr.content.c_str(), lrr.content.size(), 0, false); if (prevordername == ordername) { return; // nothing to do! (assuming the other record also exists) } - txn->txn->del(txn->db->dbi, co(domain_id, prevordername, QType::NSEC3)); + txn->txn->del(txn->db->rdbi, co(domain_id, prevordername, QType::NSEC3)); } } @@ -1427,14 +1427,14 @@ void LMDBBackend::writeNSEC3RecordPair(const std::shared_ptrtxn->put_header_in_place(txn->db->dbi, co(domain_id, ordername, QType::NSEC3), ser); + txn->txn->put_header_in_place(txn->db->rdbi, co(domain_id, ordername, QType::NSEC3), ser); // Write qname -> ordername forward chain record with ttl set to 1 lrr.ttl = 1; lrr.content = ordername.toDNSString(); ser = MDBRWTransactionImpl::stringWithEmptyHeader(); serializeToBuffer(ser, lrr); - txn->txn->put_header_in_place(txn->db->dbi, co(domain_id, qname, QType::NSEC3), ser); + txn->txn->put_header_in_place(txn->db->rdbi, co(domain_id, qname, QType::NSEC3), ser); } // Check if the only records found for this particular name are a single NSEC3 @@ -1479,11 +1479,11 @@ bool LMDBBackend::feedRecord(const DNSResourceRecord& r, const DNSName& ordernam string rrs = MDBRWTransactionImpl::stringWithEmptyHeader(); MDBOutVal _rrs; - if (!d_rwtxn->txn->get(d_rwtxn->db->dbi, matchName, _rrs)) { + if (!d_rwtxn->txn->get(d_rwtxn->db->rdbi, matchName, _rrs)) { rrs.append(_rrs.get()); } serializeToBuffer(rrs, lrr); - d_rwtxn->txn->put_header_in_place(d_rwtxn->db->dbi, matchName, rrs); + d_rwtxn->txn->put_header_in_place(d_rwtxn->db->rdbi, matchName, rrs); if (lrr.hasOrderName) { writeNSEC3RecordPair(d_rwtxn, lrr.domain_id, lrr.qname, ordername); @@ -1503,7 +1503,7 @@ bool LMDBBackend::feedEnts(domainid_t domain_id, map& nonterm) std::string ser = MDBRWTransactionImpl::stringWithEmptyHeader(); serializeToBuffer(ser, lrr); - d_rwtxn->txn->put_header_in_place(d_rwtxn->db->dbi, co(domain_id, lrr.qname, QType::ENT), ser); + d_rwtxn->txn->put_header_in_place(d_rwtxn->db->rdbi, co(domain_id, lrr.qname, QType::ENT), ser); } return true; } @@ -1520,7 +1520,7 @@ bool LMDBBackend::feedEnts3(domainid_t domain_id, const DNSName& domain, maptxn->put_header_in_place(d_rwtxn->db->dbi, co(domain_id, lrr.qname, QType::ENT), ser); + d_rwtxn->txn->put_header_in_place(d_rwtxn->db->rdbi, co(domain_id, lrr.qname, QType::ENT), ser); if (lrr.hasOrderName) { ordername = DNSName(toBase32Hex(hashQNameWithSalt(ns3prc, nt.first))); @@ -1571,7 +1571,7 @@ bool LMDBBackend::replaceRRSet(domainid_t domain_id, const DNSName& qname, const deleteNSEC3RecordPair(txn, domain_id, relative); } else { - auto cursor = txn->txn->getCursor(txn->db->dbi); + auto cursor = txn->txn->getCursor(txn->db->rdbi); MDBOutVal key{}; MDBOutVal val{}; bool hadOrderName{false}; @@ -1605,7 +1605,7 @@ bool LMDBBackend::replaceRRSet(domainid_t domain_id, const DNSName& qname, const } std::string ser = MDBRWTransactionImpl::stringWithEmptyHeader(); serializeToBuffer(ser, adjustedRRSet); - txn->txn->put_header_in_place(txn->db->dbi, match, ser); + txn->txn->put_header_in_place(txn->db->rdbi, match, ser); } if (needCommit) @@ -1807,7 +1807,7 @@ std::shared_ptr LMDBBackend::getRecordsRWTran if (!shard.env) { shard.env = getMDBEnv((getArg("filename") + "-" + std::to_string(id % s_shards)).c_str(), MDB_NOSUBDIR | MDB_NORDAHEAD | d_asyncFlag, 0600, d_mapsize_shards); - shard.dbi = shard.env->openDB("records_v5", MDB_CREATE); + shard.rdbi = shard.env->openDB("records_v5", MDB_CREATE); } auto ret = std::make_shared(shard.env->getRWTransaction()); ret->db = std::make_shared(shard); @@ -1825,7 +1825,7 @@ std::shared_ptr LMDBBackend::getRecordsROTran } shard.env = getMDBEnv((getArg("filename") + "-" + std::to_string(id % s_shards)).c_str(), MDB_NOSUBDIR | MDB_NORDAHEAD | d_asyncFlag, 0600, d_mapsize_shards); - shard.dbi = shard.env->openDB("records_v5", MDB_CREATE); + shard.rdbi = shard.env->openDB("records_v5", MDB_CREATE); } if (rwtxn) { @@ -2013,7 +2013,7 @@ void LMDBBackend::lookupStart(domainid_t domain_id, const std::string& match, bo { d_rotxn = getRecordsROTransaction(domain_id, d_rwtxn); d_txnorder = true; - d_lookupstate.cursor = std::make_shared(d_rotxn->txn->getCursor(d_rotxn->db->dbi)); + d_lookupstate.cursor = std::make_shared(d_rotxn->txn->getCursor(d_rotxn->db->rdbi)); // Make sure we start with fresh data d_lookupstate.rrset.clear(); @@ -2159,7 +2159,7 @@ bool LMDBBackend::getSerial(DomainInfo& di) auto txn = getRecordsROTransaction(di.id); compoundOrdername co; MDBOutVal val; - if (!txn->txn->get(txn->db->dbi, co(di.id, g_rootdnsname, QType::SOA), val)) { + if (!txn->txn->get(txn->db->rdbi, co(di.id, g_rootdnsname, QType::SOA), val)) { LMDBResourceRecord lrr; if (deserializeFromBuffer(val.get(), lrr)) { if (lrr.content.size() >= sizeof(soatimes)) { @@ -2345,7 +2345,7 @@ void LMDBBackend::getUnfreshSecondaryInfos(vector* domains) auto txn2 = getRecordsROTransaction(di.id); compoundOrdername co; MDBOutVal val; - if (!txn2->txn->get(txn2->db->dbi, co(di.id, g_rootdnsname, QType::SOA), val)) { + if (!txn2->txn->get(txn2->db->rdbi, co(di.id, g_rootdnsname, QType::SOA), val)) { LMDBResourceRecord lrr; if (deserializeFromBuffer(val.get(), lrr)) { if (lrr.content.size() >= sizeof(soatimes)) { @@ -2719,7 +2719,7 @@ bool LMDBBackend::getBeforeAndAfterNamesAbsolute(domainid_t id, const DNSName& q compoundOrdername co; auto txn = getRecordsROTransaction(id); - auto cursor = txn->txn->getCursor(txn->db->dbi); + auto cursor = txn->txn->getCursor(txn->db->rdbi); MDBOutVal key, val; string matchkey = co(id, qname, QType::NSEC3); @@ -2862,7 +2862,7 @@ bool LMDBBackend::getBeforeAndAfterNames(domainid_t domainId, const ZoneName& zo compoundOrdername co; auto txn = getRecordsROTransaction(domainId); - auto cursor = txn->txn->getCursor(txn->db->dbi); + auto cursor = txn->txn->getCursor(txn->db->rdbi); MDBOutVal key, val; DNSName qname2 = qname.makeRelative(zonename); @@ -3003,7 +3003,7 @@ bool LMDBBackend::updateDNSSECOrderNameAndAuth(domainid_t domain_id, const DNSNa compoundOrdername co; string matchkey = co(domain_id, rel); - auto cursor = txn->txn->getCursor(txn->db->dbi); + auto cursor = txn->txn->getCursor(txn->db->rdbi); MDBOutVal key, val; if (cursor.prefix(matchkey, key, val) != 0) { // cout << "Could not find anything"<& in // deleteDomainRecords() would do, as we also need to remove // NSEC3 records for these ENT, if any. { - auto cursor = txn->txn->getCursor(txn->db->dbi); + auto cursor = txn->txn->getCursor(txn->db->rdbi); MDBOutVal key{}; MDBOutVal val{}; std::vector names; @@ -3133,9 +3133,9 @@ bool LMDBBackend::updateEmptyNonTerminals(domainid_t domain_id, set& in name.makeUsRelative(info.zone); std::string match = order(domain_id, name, QType::ENT); MDBOutVal val{}; - if (txn->txn->get(txn->db->dbi, match, val) == 0) { + if (txn->txn->get(txn->db->rdbi, match, val) == 0) { bool hadOrderName = peekAtHasOrderName(val.get()); - txn->txn->del(txn->db->dbi, match); + txn->txn->del(txn->db->rdbi, match); if (hadOrderName) { deleteNSEC3RecordPair(txn, domain_id, name); } @@ -3149,7 +3149,7 @@ bool LMDBBackend::updateEmptyNonTerminals(domainid_t domain_id, set& in lrr.auth = true; std::string ser = MDBRWTransactionImpl::stringWithEmptyHeader(); serializeToBuffer(ser, lrr); - txn->txn->put_header_in_place(txn->db->dbi, order(domain_id, lrr.qname, QType::ENT), ser); + txn->txn->put_header_in_place(txn->db->rdbi, order(domain_id, lrr.qname, QType::ENT), ser); // cout <<" +"<& argv) // without disturbing the current get() cursor. compoundOrdername order; MDBOutVal val{}; - if (d_rotxn->txn->get(d_rotxn->db->dbi, order(info.id, basename, QType::NSEC3), val) == 0) { + if (d_rotxn->txn->get(d_rotxn->db->rdbi, order(info.id, basename, QType::NSEC3), val) == 0) { LMDBResourceRecord nsec3rr; if (deserializeFromBuffer(val.get(), nsec3rr)) { DNSName ordername(nsec3rr.content.c_str(), nsec3rr.content.size(), 0, false); diff --git a/modules/lmdbbackend/lmdbbackend.hh b/modules/lmdbbackend/lmdbbackend.hh index 9aa1a9c132d8..d7f752a4f411 100644 --- a/modules/lmdbbackend/lmdbbackend.hh +++ b/modules/lmdbbackend/lmdbbackend.hh @@ -299,7 +299,7 @@ private: struct RecordsDB { shared_ptr env; - MDBDbi dbi; + MDBDbi rdbi; // records }; struct RecordsROTransaction From d6b1358c3fe56f851accac6daff50ba8b49658f0 Mon Sep 17 00:00:00 2001 From: Peter van Dijk Date: Tue, 11 Nov 2025 15:30:56 +0100 Subject: [PATCH 02/35] add comments dbi --- modules/lmdbbackend/lmdbbackend.cc | 4 ++++ modules/lmdbbackend/lmdbbackend.hh | 1 + 2 files changed, 5 insertions(+) diff --git a/modules/lmdbbackend/lmdbbackend.cc b/modules/lmdbbackend/lmdbbackend.cc index 5464fec36d11..857f5cdf56c6 100644 --- a/modules/lmdbbackend/lmdbbackend.cc +++ b/modules/lmdbbackend/lmdbbackend.cc @@ -1808,6 +1808,7 @@ std::shared_ptr LMDBBackend::getRecordsRWTran shard.env = getMDBEnv((getArg("filename") + "-" + std::to_string(id % s_shards)).c_str(), MDB_NOSUBDIR | MDB_NORDAHEAD | d_asyncFlag, 0600, d_mapsize_shards); shard.rdbi = shard.env->openDB("records_v5", MDB_CREATE); + shard.cdbi = shard.env->openDB("comments_v7", MDB_CREATE); } auto ret = std::make_shared(shard.env->getRWTransaction()); ret->db = std::make_shared(shard); @@ -1826,6 +1827,7 @@ std::shared_ptr LMDBBackend::getRecordsROTran shard.env = getMDBEnv((getArg("filename") + "-" + std::to_string(id % s_shards)).c_str(), MDB_NOSUBDIR | MDB_NORDAHEAD | d_asyncFlag, 0600, d_mapsize_shards); shard.rdbi = shard.env->openDB("records_v5", MDB_CREATE); + shard.cdbi = shard.env->openDB("comments_v7", MDB_CREATE); } if (rwtxn) { @@ -3464,6 +3466,8 @@ bool LMDBBackend::hasCreatedLocalFiles() const // not all of them did. // But since this information is for the sake of pdnsutil, this is not // really a problem. + // However, there is a false positive if we make new databases (dbis) inside + // existing LMDB files on disk. This is not easy to avoid. return MDBDbi::d_creationCount != 0; } diff --git a/modules/lmdbbackend/lmdbbackend.hh b/modules/lmdbbackend/lmdbbackend.hh index d7f752a4f411..96d651d1d8d5 100644 --- a/modules/lmdbbackend/lmdbbackend.hh +++ b/modules/lmdbbackend/lmdbbackend.hh @@ -300,6 +300,7 @@ private: { shared_ptr env; MDBDbi rdbi; // records + MDBDbi cdbi; // comments }; struct RecordsROTransaction From c98acf0bb7e2be0fee0122e922311a7e8e460f73 Mon Sep 17 00:00:00 2001 From: Peter van Dijk Date: Tue, 11 Nov 2025 17:04:01 +0100 Subject: [PATCH 03/35] expose pdns_bswap64 to LMDBLS consumers --- ext/lmdb-safe/lmdb-safe.hh | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/ext/lmdb-safe/lmdb-safe.hh b/ext/lmdb-safe/lmdb-safe.hh index 78bcf7ba5875..66900c058bf0 100644 --- a/ext/lmdb-safe/lmdb-safe.hh +++ b/ext/lmdb-safe/lmdb-safe.hh @@ -123,24 +123,23 @@ std::shared_ptr getMDBEnv(const char* fname, int flags, int mode, uint64 struct MDBOutVal; // forward declaration because of how the functions below tie in with MDBOutVal namespace LMDBLS { - class __attribute__((__packed__)) LSheader { - private: - // Some systems #define bswap64 to __builtin_bswap64, and the body below would cause infinite - // recursion if we would name the function bswap64 - static auto pdns_bswap64(uint64_t value) -> uint64_t - { + // Some systems #define bswap64 to __builtin_bswap64, and the body below would cause infinite + // recursion if we would name the function bswap64 + static auto pdns_bswap64(uint64_t value) -> uint64_t + { #if !defined(__BYTE_ORDER__) || !defined(__ORDER_LITTLE_ENDIAN__) || !defined(__ORDER_BIG_ENDIAN__) #error "your compiler does not define byte order macros" #endif #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__ - // FIXME: Do something more portable than __builtin_bswap64. - return __builtin_bswap64(value); + // FIXME: Do something more portable than __builtin_bswap64. + return __builtin_bswap64(value); #else - return value; + return value; #endif - } + } + class __attribute__((__packed__)) LSheader { public: uint64_t d_timestamp; uint64_t d_txnid; From 3426de9bde47e1b2b355528bf60eb01029e92adb Mon Sep 17 00:00:00 2001 From: Peter van Dijk Date: Tue, 11 Nov 2025 17:13:03 +0100 Subject: [PATCH 04/35] implement functional but slightly wrong feedComment --- modules/lmdbbackend/lmdbbackend.cc | 35 ++++++++++++++++++++++++++++++ modules/lmdbbackend/lmdbbackend.hh | 1 + 2 files changed, 36 insertions(+) diff --git a/modules/lmdbbackend/lmdbbackend.cc b/modules/lmdbbackend/lmdbbackend.cc index 857f5cdf56c6..c2f2e5413471 100644 --- a/modules/lmdbbackend/lmdbbackend.cc +++ b/modules/lmdbbackend/lmdbbackend.cc @@ -1161,6 +1161,25 @@ static std::string serializeContent(uint16_t qtype, const DNSName& domain, const return drc->serialize(domain, false); } +// comment serialization: +// 8 bytes timestamp, network order +// comment content, \0 terminated +// account name, \0 terminated + +// perhaps this can also create the compound order name for us +static std::string serializeComment(const Comment& c) +{ + string ret; + + uint64_t ts = LMDBLS::pdns_bswap64(c.modified_at); + ret.reserve(sizeof(uint64_t) + c.content.size() + c.account.size() + 2); // one timestamp, two strings, 2 \0 delimiters + + ret.assign((char *)(&ts), sizeof(ts)); // FIXME i bet there's a cleaner way + ret += c.content + string(1, (char)0) + c.account + string(1, (char)0); + + return ret; +} + static std::shared_ptr deserializeContentZR(uint16_t qtype, const DNSName& qname, const std::string& content) { if (qtype == QType::A && content.size() == 4) { @@ -1491,6 +1510,22 @@ bool LMDBBackend::feedRecord(const DNSResourceRecord& r, const DNSName& ordernam return true; } +// d_rwtxn must be set here +bool LMDBBackend::feedComment(const Comment& comment) +{ + compoundOrdername co; + auto qname = comment.qname.makeRelative(d_transactiondomain); + string key = co(comment.domain_id, qname, comment.qtype); // FIXME: add hash to distinguish multiple comments + + // we serialized domain_id, qname, qtype + // that leaves us with content, modified_at, account + string value = serializeComment(comment); + + d_rwtxn->txn->put(d_rwtxn->db->cdbi, key, value); + + return true; +} + bool LMDBBackend::feedEnts(domainid_t domain_id, map& nonterm) { LMDBResourceRecord lrr; diff --git a/modules/lmdbbackend/lmdbbackend.hh b/modules/lmdbbackend/lmdbbackend.hh index 96d651d1d8d5..cca369986d60 100644 --- a/modules/lmdbbackend/lmdbbackend.hh +++ b/modules/lmdbbackend/lmdbbackend.hh @@ -84,6 +84,7 @@ public: bool commitTransaction() override; bool abortTransaction() override; bool feedRecord(const DNSResourceRecord& r, const DNSName& ordername, bool ordernameIsNSEC3 = false) override; + bool feedComment(const Comment& c) override; bool feedEnts(domainid_t domain_id, map& nonterm) override; bool feedEnts3(domainid_t domain_id, const DNSName& domain, map& nonterm, const NSEC3PARAMRecordContent& ns3prc, bool narrow) override; bool replaceRRSet(domainid_t domain_id, const DNSName& qname, const QType& qt, const vector& rrset) override; From 719f806cdf17dbc694a8e790f478856b703c6036 Mon Sep 17 00:00:00 2001 From: Peter van Dijk Date: Thu, 13 Nov 2025 11:33:16 +0100 Subject: [PATCH 05/35] new command: pdnsutil comment add --- pdns/pdnsutil.cc | 46 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/pdns/pdnsutil.cc b/pdns/pdnsutil.cc index 69d021403f97..d990b155f1b8 100644 --- a/pdns/pdnsutil.cc +++ b/pdns/pdnsutil.cc @@ -1,5 +1,6 @@ #include #include +#include #include #include #include @@ -86,6 +87,7 @@ static int activateTSIGKey(vector& cmds, std::string_view synopsis); static int activateZoneKey(vector& cmds, std::string_view synopsis); static int addAutoprimary(vector& cmds, std::string_view synopsis); static int addMeta(vector& cmds, std::string_view synopsis); +static int addComment(vector& cmds, std::string_view synopsis); static int addRecord(vector& cmds, std::string_view synopsis); static int addZoneKey(vector& cmds, std::string_view synopsis); static int backendCmd(vector& cmds, std::string_view synopsis); @@ -511,6 +513,13 @@ static const groupCommandDispatcher zoneKeyCommands{ "\tUnpublish the zone key with key id KEY_ID in ZONE"}}} }; +static const groupCommandDispatcher commentCommands{ + "Comment", + {{"add", {true, addComment, + "ZONE NAME TYPE COMMENT [ACCOUNT]", + "\tAdd a comment"}}} +}; + // OTHER (NO OBJECT NAME PREFIX) static const groupCommandDispatcher otherCommands{ @@ -580,6 +589,7 @@ using commandDispatcher = std::map& cmds, const std::string_vi } } +static int addComment(vector& cmds, const std::string_view synopsis) +{ + if(cmds.size() < 4) { + return usage(synopsis); + } + + UtilBackend B; + DomainInfo di; + ZoneName zone(cmds.at(0)); + if (!B.getDomainInfo(zone, di)) { + cerr << "Zone '" << zone << "' doesn't exist" << endl; + return EXIT_FAILURE; + } + + Comment comment; + + comment.domain_id = di.id; + comment.qname = DNSName(cmds.at(1)); + comment.qtype = cmds.at(2); + comment.content = cmds.at(3); + if(cmds.size() > 4) { + comment.account = cmds.at(4); + } + comment.modified_at = time(nullptr); + + di.backend->startTransaction(zone, UnknownDomainID); + if (!di.backend->feedComment(comment)) { + cerr << "Backend does not support comments" << endl; + di.backend->abortTransaction(); + return EXIT_FAILURE; + } + + di.backend->commitTransaction(); + return EXIT_SUCCESS; +} + static int addRecord(vector& cmds, const std::string_view synopsis) { if(cmds.size() < 4) { From cd960d8b6c3a18755f07fe092958b6e639b1d97c Mon Sep 17 00:00:00 2001 From: Peter van Dijk Date: Thu, 13 Nov 2025 11:48:50 +0100 Subject: [PATCH 06/35] expand serializeComment, simplify feedComment --- modules/lmdbbackend/lmdbbackend.cc | 30 +++++++++++++++--------------- modules/lmdbbackend/lmdbbackend.hh | 2 ++ 2 files changed, 17 insertions(+), 15 deletions(-) diff --git a/modules/lmdbbackend/lmdbbackend.cc b/modules/lmdbbackend/lmdbbackend.cc index c2f2e5413471..5ce18ad7adbb 100644 --- a/modules/lmdbbackend/lmdbbackend.cc +++ b/modules/lmdbbackend/lmdbbackend.cc @@ -1167,17 +1167,23 @@ static std::string serializeContent(uint16_t qtype, const DNSName& domain, const // account name, \0 terminated // perhaps this can also create the compound order name for us -static std::string serializeComment(const Comment& c) +std::pair LMDBBackend::serializeComment(const Comment& comment) { - string ret; + compoundOrdername co; + auto qname = comment.qname.makeRelative(d_transactiondomain); + string key = co(comment.domain_id, qname, comment.qtype); // FIXME: add hash to distinguish multiple comments - uint64_t ts = LMDBLS::pdns_bswap64(c.modified_at); - ret.reserve(sizeof(uint64_t) + c.content.size() + c.account.size() + 2); // one timestamp, two strings, 2 \0 delimiters + // we serialized domain_id, qname, qtype + // that leaves us with content, modified_at, account - ret.assign((char *)(&ts), sizeof(ts)); // FIXME i bet there's a cleaner way - ret += c.content + string(1, (char)0) + c.account + string(1, (char)0); + string val; + uint64_t ts = LMDBLS::pdns_bswap64(comment.modified_at); + val.reserve(sizeof(uint64_t) + comment.content.size() + comment.account.size() + 2); // one timestamp, two strings, 2 \0 delimiters - return ret; + val.assign((char *)(&ts), sizeof(ts)); // FIXME i bet there's a cleaner way + val += comment.content + string(1, (char)0) + comment.account + string(1, (char)0); + + return {key, val}; } static std::shared_ptr deserializeContentZR(uint16_t qtype, const DNSName& qname, const std::string& content) @@ -1513,15 +1519,9 @@ bool LMDBBackend::feedRecord(const DNSResourceRecord& r, const DNSName& ordernam // d_rwtxn must be set here bool LMDBBackend::feedComment(const Comment& comment) { - compoundOrdername co; - auto qname = comment.qname.makeRelative(d_transactiondomain); - string key = co(comment.domain_id, qname, comment.qtype); // FIXME: add hash to distinguish multiple comments - - // we serialized domain_id, qname, qtype - // that leaves us with content, modified_at, account - string value = serializeComment(comment); + auto [key,val] = serializeComment(comment); - d_rwtxn->txn->put(d_rwtxn->db->cdbi, key, value); + d_rwtxn->txn->put(d_rwtxn->db->cdbi, key, val); return true; } diff --git a/modules/lmdbbackend/lmdbbackend.hh b/modules/lmdbbackend/lmdbbackend.hh index cca369986d60..b7c80383231f 100644 --- a/modules/lmdbbackend/lmdbbackend.hh +++ b/modules/lmdbbackend/lmdbbackend.hh @@ -362,6 +362,8 @@ private: static void deleteNSEC3RecordPair(const std::shared_ptr& txn, domainid_t domain_id, const DNSName& qname); void writeNSEC3RecordPair(const std::shared_ptr& txn, domainid_t domain_id, const DNSName& qname, const DNSName& ordername); + std::pair serializeComment(const Comment& c); + string directBackendCmd_list(std::vector& argv); // Transient DomainInfo data, not necessarily synchronized with the From a584a8929a57cad96a86071f328c203f2e031ab9 Mon Sep 17 00:00:00 2001 From: Peter van Dijk Date: Thu, 13 Nov 2025 11:54:35 +0100 Subject: [PATCH 07/35] append hash to key --- modules/lmdbbackend/lmdbbackend.cc | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/modules/lmdbbackend/lmdbbackend.cc b/modules/lmdbbackend/lmdbbackend.cc index 5ce18ad7adbb..3f901515e646 100644 --- a/modules/lmdbbackend/lmdbbackend.cc +++ b/modules/lmdbbackend/lmdbbackend.cc @@ -34,6 +34,7 @@ #include "pdns/logger.hh" #include "pdns/misc.hh" #include "pdns/pdnsexception.hh" +#include "pdns/sha.hh" #include "pdns/uuid-utils.hh" #include #include @@ -1171,7 +1172,7 @@ std::pair LMDBBackend::serializeComment(const Comment& { compoundOrdername co; auto qname = comment.qname.makeRelative(d_transactiondomain); - string key = co(comment.domain_id, qname, comment.qtype); // FIXME: add hash to distinguish multiple comments + string key = co(comment.domain_id, qname, comment.qtype); // we serialized domain_id, qname, qtype // that leaves us with content, modified_at, account @@ -1183,6 +1184,10 @@ std::pair LMDBBackend::serializeComment(const Comment& val.assign((char *)(&ts), sizeof(ts)); // FIXME i bet there's a cleaner way val += comment.content + string(1, (char)0) + comment.account + string(1, (char)0); + auto hash = pdns::sha256sum(val); + + key.append(hash); + return {key, val}; } From f957e77fa847d5d21d2a7ad9621a26d355b0e72a Mon Sep 17 00:00:00 2001 From: Peter van Dijk Date: Thu, 13 Nov 2025 13:39:28 +0100 Subject: [PATCH 08/35] new command: pdnsutil comment list --- pdns/pdnsutil.cc | 43 ++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 42 insertions(+), 1 deletion(-) diff --git a/pdns/pdnsutil.cc b/pdns/pdnsutil.cc index d990b155f1b8..bed46cf715b9 100644 --- a/pdns/pdnsutil.cc +++ b/pdns/pdnsutil.cc @@ -88,6 +88,7 @@ static int activateZoneKey(vector& cmds, std::string_view synopsis); static int addAutoprimary(vector& cmds, std::string_view synopsis); static int addMeta(vector& cmds, std::string_view synopsis); static int addComment(vector& cmds, std::string_view synopsis); +static int listComments(vector& cmds, std::string_view synopsis); static int addRecord(vector& cmds, std::string_view synopsis); static int addZoneKey(vector& cmds, std::string_view synopsis); static int backendCmd(vector& cmds, std::string_view synopsis); @@ -517,7 +518,10 @@ static const groupCommandDispatcher commentCommands{ "Comment", {{"add", {true, addComment, "ZONE NAME TYPE COMMENT [ACCOUNT]", - "\tAdd a comment"}}} + "\tAdd a comment"}}, + {"list", {true, listComments, + "ZONE", + "\tList comments for a zone"}}} }; // OTHER (NO OBJECT NAME PREFIX) @@ -1888,6 +1892,30 @@ static int listZone(const ZoneName &zone) { return EXIT_SUCCESS; } +static int listComments(const ZoneName &zone) { + UtilBackend B; //NOLINT(readability-identifier-length) + DomainInfo di; + + if (! B.getDomainInfo(zone, di)) { + cerr << "Zone '" << zone << "' not found!" << endl; + return EXIT_FAILURE; + } + if ((di.backend->getCapabilities() & DNSBackend::CAP_LIST) == 0) { + cerr << "Backend for zone '" << zone << "' does not support listing its contents." << endl; + return EXIT_FAILURE; + } + + Comment comment; + + di.backend->listComments(di.id); + while(di.backend->getComment(comment)) { + cout<& cmds, const std::string_view synopsis) return EXIT_SUCCESS; } +static int listComments(vector& cmds, const std::string_view synopsis) +{ + if(cmds.size() != 1) { + return usage(synopsis); + } + if (cmds.at(0) == ".") { + cmds.at(0).clear(); + } + + return listComments(ZoneName(cmds.at(0))); +} + + static int addRecord(vector& cmds, const std::string_view synopsis) { if(cmds.size() < 4) { From 394b4af5a595ffc97f5e9d540afe7066a7a963f4 Mon Sep 17 00:00:00 2001 From: Peter van Dijk Date: Thu, 13 Nov 2025 13:39:40 +0100 Subject: [PATCH 09/35] lmdb listComments --- modules/lmdbbackend/lmdbbackend.cc | 78 +++++++++++++++++++++++++++++- modules/lmdbbackend/lmdbbackend.hh | 8 +++ 2 files changed, 85 insertions(+), 1 deletion(-) diff --git a/modules/lmdbbackend/lmdbbackend.cc b/modules/lmdbbackend/lmdbbackend.cc index 3f901515e646..09760ea21cbc 100644 --- a/modules/lmdbbackend/lmdbbackend.cc +++ b/modules/lmdbbackend/lmdbbackend.cc @@ -1959,6 +1959,24 @@ bool LMDBBackend::deleteDomain(const ZoneName& domain) return true; } +bool LMDBBackend::listComments(domainid_t domain_id) +{ + DomainInfo info; + if (!findDomain(domain_id, info)) { + throw DBException("Domain with id '" + std::to_string(domain_id) + "not found"); + } + + d_lookupstate.domain = info.zone; + d_lookupstate.submatch.clear(); + d_lookupstate.comments = true; + + compoundOrdername order; + std::string match = order(domain_id); + + lookupStart(domain_id, match, false); + return true; +} + bool LMDBBackend::list(const ZoneName& target, domainid_t domain_id, bool include_disabled) { d_lookupstate.domain = target; @@ -2055,7 +2073,12 @@ void LMDBBackend::lookupStart(domainid_t domain_id, const std::string& match, bo { d_rotxn = getRecordsROTransaction(domain_id, d_rwtxn); d_txnorder = true; - d_lookupstate.cursor = std::make_shared(d_rotxn->txn->getCursor(d_rotxn->db->rdbi)); + if (d_lookupstate.comments) { + d_lookupstate.cursor = std::make_shared(d_rotxn->txn->getCursor(d_rotxn->db->cdbi)); + } + else { + d_lookupstate.cursor = std::make_shared(d_rotxn->txn->getCursor(d_rotxn->db->rdbi)); + } // Make sure we start with fresh data d_lookupstate.rrset.clear(); @@ -2080,6 +2103,46 @@ void LMDBBackend::lookupStart(domainid_t domain_id, const std::string& match, bo bool LMDBBackend::getInternal(DNSName& basename, std::string_view& key) { + // FIXME: bit of duplication from below here, but much simpler + if (d_lookupstate.comments) { + if (d_lookupstate.cursor && d_lookupstate.cursor->next(d_lookupstate.key, d_lookupstate.val) != 0) { + d_lookupstate.reset(); + } + + if (!d_lookupstate.cursor) { + d_rotxn.reset(); + return false; + } + + key = d_lookupstate.key.getNoStripHeader(); + // remove hash from the key so compoundOrdername::get* work + key = key.substr(0, key.size() - 256/8); + + basename = compoundOrdername::getQName(key); + + uint64_t ts; + auto val = d_lookupstate.val.get(); + if (val.size() < sizeof(uint64_t)) { + throw DBException("got invalid serialized comment"); + } + + memcpy(&ts, val.data(), sizeof(uint64_t)); // ts in network order + + val = val.substr(8, std::string_view::npos); + + d_lookupstate.comment.domain_id=compoundOrdername::getDomainID(key); + d_lookupstate.comment.qname=basename + d_lookupstate.domain.operator const DNSName&(); + d_lookupstate.comment.qtype=compoundOrdername::getQType(key); + d_lookupstate.comment.modified_at=LMDBLS::pdns_bswap64(ts); + auto pos = val.find('\0', 1); // FIXME check + d_lookupstate.comment.content=val.substr(0, pos); // pos-1? + val = val.substr(pos, std::string_view::npos); + pos = val.find('\0', 1); // FIXME check + d_lookupstate.comment.account=val.substr(0, pos); // pos-1? + + return true; + } + for (;;) { if (!d_lookupstate.rrset.empty()) { if (++d_lookupstate.rrsetpos >= d_lookupstate.rrset.size()) { @@ -2146,6 +2209,19 @@ bool LMDBBackend::getInternal(DNSName& basename, std::string_view& key) return true; } +bool LMDBBackend::getComment(Comment& comment) // NOLINT(readability-identifier-length) +{ + DNSName basename; + std::string_view key; + + if (!getInternal(basename, key)) { + return false; + } + comment = d_lookupstate.comment; + + return true; +} + bool LMDBBackend::get(DNSZoneRecord& zr) // NOLINT(readability-identifier-length) { DNSName basename; diff --git a/modules/lmdbbackend/lmdbbackend.hh b/modules/lmdbbackend/lmdbbackend.hh index b7c80383231f..67e04094b930 100644 --- a/modules/lmdbbackend/lmdbbackend.hh +++ b/modules/lmdbbackend/lmdbbackend.hh @@ -85,6 +85,9 @@ public: bool abortTransaction() override; bool feedRecord(const DNSResourceRecord& r, const DNSName& ordername, bool ordernameIsNSEC3 = false) override; bool feedComment(const Comment& c) override; + bool listComments(domainid_t domain_id) override; + bool getComment(Comment& comment) override; + bool feedEnts(domainid_t domain_id, map& nonterm) override; bool feedEnts3(domainid_t domain_id, const DNSName& domain, map& nonterm, const NSEC3PARAMRecordContent& ns3prc, bool narrow) override; bool replaceRRSet(domainid_t domain_id, const DNSName& qname, const QType& qt, const vector& rrset) override; @@ -409,6 +412,10 @@ private: MDBOutVal val; // whether to include disabled records in the results bool includedisabled; + // whether we are doing comments (false=records, true=comments) + bool comments; + // comment found by last call to getInternal + Comment comment; void reset() { @@ -417,6 +424,7 @@ private: rrset.clear(); rrsetpos = 0; cursor.reset(); + comments = false; } } d_lookupstate; From cb1f64bc92d14f37ebf7ff22817282b3cf275acc Mon Sep 17 00:00:00 2001 From: Peter van Dijk Date: Fri, 14 Nov 2025 13:01:41 +0100 Subject: [PATCH 10/35] init bool --- modules/lmdbbackend/lmdbbackend.cc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/modules/lmdbbackend/lmdbbackend.cc b/modules/lmdbbackend/lmdbbackend.cc index 09760ea21cbc..16fb9f27b2fe 100644 --- a/modules/lmdbbackend/lmdbbackend.cc +++ b/modules/lmdbbackend/lmdbbackend.cc @@ -1982,6 +1982,7 @@ bool LMDBBackend::list(const ZoneName& target, domainid_t domain_id, bool includ d_lookupstate.domain = target; d_lookupstate.submatch.clear(); d_lookupstate.includedisabled = include_disabled; + d_lookupstate.comments = false; compoundOrdername order; std::string match = order(domain_id); @@ -2009,6 +2010,7 @@ bool LMDBBackend::listSubZone(const ZoneName& target, domainid_t domain_id) d_lookupstate.domain = std::move(info.zone); d_lookupstate.submatch = std::move(relqname); d_lookupstate.includedisabled = true; + d_lookupstate.comments = false; compoundOrdername order; std::string match = order(domain_id); @@ -2056,6 +2058,7 @@ void LMDBBackend::lookupInternal(const QType& type, const DNSName& qdomain, doma d_lookupstate.domain = std::move(info.zone); d_lookupstate.submatch.clear(); d_lookupstate.includedisabled = include_disabled; + d_lookupstate.comments = false; compoundOrdername order; std::string match; From 1bff585723cf865440febe6c8459e5dbf17bd525 Mon Sep 17 00:00:00 2001 From: Peter van Dijk Date: Fri, 14 Nov 2025 14:08:39 +0100 Subject: [PATCH 11/35] set lookupstate key/val early; fix comment deserial --- modules/lmdbbackend/lmdbbackend.cc | 20 +++++++++----------- modules/lmdbbackend/lmdbbackend.hh | 2 +- 2 files changed, 10 insertions(+), 12 deletions(-) diff --git a/modules/lmdbbackend/lmdbbackend.cc b/modules/lmdbbackend/lmdbbackend.cc index 16fb9f27b2fe..a3a385da9d5d 100644 --- a/modules/lmdbbackend/lmdbbackend.cc +++ b/modules/lmdbbackend/lmdbbackend.cc @@ -2087,9 +2087,7 @@ void LMDBBackend::lookupStart(domainid_t domain_id, const std::string& match, bo d_lookupstate.rrset.clear(); d_lookupstate.rrsetpos = 0; - MDBOutVal key{}; - MDBOutVal val{}; - if (d_lookupstate.cursor->prefix(match, key, val) != 0) { + if (d_lookupstate.cursor->prefix(match, d_lookupstate.key, d_lookupstate.val) != 0) { d_lookupstate.reset(); // will cause get() to fail if (dolog) { SLOG(g_log << Logger::Warning << "Query " << ((long)(void*)this) << ": " << d_dtime.udiffNoReset() << " us to execute (found nothing)" << endl, @@ -2108,10 +2106,6 @@ bool LMDBBackend::getInternal(DNSName& basename, std::string_view& key) { // FIXME: bit of duplication from below here, but much simpler if (d_lookupstate.comments) { - if (d_lookupstate.cursor && d_lookupstate.cursor->next(d_lookupstate.key, d_lookupstate.val) != 0) { - d_lookupstate.reset(); - } - if (!d_lookupstate.cursor) { d_rotxn.reset(); return false; @@ -2137,11 +2131,15 @@ bool LMDBBackend::getInternal(DNSName& basename, std::string_view& key) d_lookupstate.comment.qname=basename + d_lookupstate.domain.operator const DNSName&(); d_lookupstate.comment.qtype=compoundOrdername::getQType(key); d_lookupstate.comment.modified_at=LMDBLS::pdns_bswap64(ts); - auto pos = val.find('\0', 1); // FIXME check - d_lookupstate.comment.content=val.substr(0, pos); // pos-1? + auto pos = val.find('\0', 1) + 1; // FIXME check + d_lookupstate.comment.content=val.substr(0, pos - 1); // pos-1? val = val.substr(pos, std::string_view::npos); - pos = val.find('\0', 1); // FIXME check - d_lookupstate.comment.account=val.substr(0, pos); // pos-1? + pos = val.find('\0', 1) + 1; // FIXME check + d_lookupstate.comment.account=val.substr(0, pos - 1); // pos-1? + + if (d_lookupstate.cursor && d_lookupstate.cursor->next(d_lookupstate.key, d_lookupstate.val) != 0) { + d_lookupstate.reset(); // this invalidates cursor and makes us return false on the next round + } return true; } diff --git a/modules/lmdbbackend/lmdbbackend.hh b/modules/lmdbbackend/lmdbbackend.hh index 67e04094b930..096fa306367b 100644 --- a/modules/lmdbbackend/lmdbbackend.hh +++ b/modules/lmdbbackend/lmdbbackend.hh @@ -424,7 +424,7 @@ private: rrset.clear(); rrsetpos = 0; cursor.reset(); - comments = false; + // comments bool explicitly not reset here, so getInternal can note absence the right way } } d_lookupstate; From 1604007b3cfb13567c47b0edea23bbd7419fe4a6 Mon Sep 17 00:00:00 2001 From: Peter van Dijk Date: Fri, 14 Nov 2025 14:08:52 +0100 Subject: [PATCH 12/35] allow this one api test to already notice comments somewhat working in lmdb --- regression-tests.api/test_Zones.py | 5 ----- 1 file changed, 5 deletions(-) diff --git a/regression-tests.api/test_Zones.py b/regression-tests.api/test_Zones.py index 172bed4e842f..397c72d7985f 100644 --- a/regression-tests.api/test_Zones.py +++ b/regression-tests.api/test_Zones.py @@ -430,11 +430,6 @@ def test_create_zone_with_comments(self): }, ] - if is_auth_lmdb(): - # No comments in LMDB - self.create_zone(name=name, rrsets=rrsets, expect_error="Hosting backend does not support editing comments.") - return - name, _, data = self.create_zone(name=name, rrsets=rrsets) # NS records have been created self.assertEqual(len(data['rrsets']), len(rrsets) + 1) From 12fd7780f768bd305cc3b0e67f4ce31423372bf0 Mon Sep 17 00:00:00 2001 From: Peter van Dijk Date: Fri, 14 Nov 2025 14:16:59 +0100 Subject: [PATCH 13/35] format --- modules/lmdbbackend/lmdbbackend.cc | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/modules/lmdbbackend/lmdbbackend.cc b/modules/lmdbbackend/lmdbbackend.cc index a3a385da9d5d..73887159ce37 100644 --- a/modules/lmdbbackend/lmdbbackend.cc +++ b/modules/lmdbbackend/lmdbbackend.cc @@ -1181,7 +1181,7 @@ std::pair LMDBBackend::serializeComment(const Comment& uint64_t ts = LMDBLS::pdns_bswap64(comment.modified_at); val.reserve(sizeof(uint64_t) + comment.content.size() + comment.account.size() + 2); // one timestamp, two strings, 2 \0 delimiters - val.assign((char *)(&ts), sizeof(ts)); // FIXME i bet there's a cleaner way + val.assign((char*)(&ts), sizeof(ts)); // FIXME i bet there's a cleaner way val += comment.content + string(1, (char)0) + comment.account + string(1, (char)0); auto hash = pdns::sha256sum(val); @@ -1524,7 +1524,7 @@ bool LMDBBackend::feedRecord(const DNSResourceRecord& r, const DNSName& ordernam // d_rwtxn must be set here bool LMDBBackend::feedComment(const Comment& comment) { - auto [key,val] = serializeComment(comment); + auto [key, val] = serializeComment(comment); d_rwtxn->txn->put(d_rwtxn->db->cdbi, key, val); @@ -2113,7 +2113,7 @@ bool LMDBBackend::getInternal(DNSName& basename, std::string_view& key) key = d_lookupstate.key.getNoStripHeader(); // remove hash from the key so compoundOrdername::get* work - key = key.substr(0, key.size() - 256/8); + key = key.substr(0, key.size() - 256 / 8); basename = compoundOrdername::getQName(key); @@ -2127,15 +2127,15 @@ bool LMDBBackend::getInternal(DNSName& basename, std::string_view& key) val = val.substr(8, std::string_view::npos); - d_lookupstate.comment.domain_id=compoundOrdername::getDomainID(key); - d_lookupstate.comment.qname=basename + d_lookupstate.domain.operator const DNSName&(); - d_lookupstate.comment.qtype=compoundOrdername::getQType(key); - d_lookupstate.comment.modified_at=LMDBLS::pdns_bswap64(ts); + d_lookupstate.comment.domain_id = compoundOrdername::getDomainID(key); + d_lookupstate.comment.qname = basename + d_lookupstate.domain.operator const DNSName&(); + d_lookupstate.comment.qtype = compoundOrdername::getQType(key); + d_lookupstate.comment.modified_at = LMDBLS::pdns_bswap64(ts); auto pos = val.find('\0', 1) + 1; // FIXME check - d_lookupstate.comment.content=val.substr(0, pos - 1); // pos-1? + d_lookupstate.comment.content = val.substr(0, pos - 1); // pos-1? val = val.substr(pos, std::string_view::npos); pos = val.find('\0', 1) + 1; // FIXME check - d_lookupstate.comment.account=val.substr(0, pos - 1); // pos-1? + d_lookupstate.comment.account = val.substr(0, pos - 1); // pos-1? if (d_lookupstate.cursor && d_lookupstate.cursor->next(d_lookupstate.key, d_lookupstate.val) != 0) { d_lookupstate.reset(); // this invalidates cursor and makes us return false on the next round From 09918ac750417393e32dac044c4d19b289f32023 Mon Sep 17 00:00:00 2001 From: Peter van Dijk Date: Fri, 14 Nov 2025 16:23:20 +0100 Subject: [PATCH 14/35] implement replaceRecords minus the delete step --- modules/lmdbbackend/lmdbbackend.cc | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/modules/lmdbbackend/lmdbbackend.cc b/modules/lmdbbackend/lmdbbackend.cc index 73887159ce37..a1ed1ba7c61a 100644 --- a/modules/lmdbbackend/lmdbbackend.cc +++ b/modules/lmdbbackend/lmdbbackend.cc @@ -1654,12 +1654,14 @@ bool LMDBBackend::replaceRRSet(domainid_t domain_id, const DNSName& qname, const return true; } -// NOLINTNEXTLINE(readability-identifier-length) -bool LMDBBackend::replaceComments([[maybe_unused]] domainid_t domain_id, [[maybe_unused]] const DNSName& qname, [[maybe_unused]] const QType& qt, const vector& comments) +bool LMDBBackend::replaceComments(const domainid_t /* domain_id */, const DNSName& /* qname */, const QType& /* qt */, const vector& comments) { - // if the vector is empty, good, that's what we do here (LMDB does not store comments) - // if it's not, report failure - return comments.empty(); + // FIXME: remove old comments + for(const auto& comment: comments) { + feedComment(comment); + } + + return true; } // FIXME: this is not very efficient From a3e0d8135b771e214d5eb673e543a3088a4d754c Mon Sep 17 00:00:00 2001 From: Peter van Dijk Date: Fri, 14 Nov 2025 16:24:00 +0100 Subject: [PATCH 15/35] test: actually have something to delete --- regression-tests.api/test_Zones.py | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/regression-tests.api/test_Zones.py b/regression-tests.api/test_Zones.py index 397c72d7985f..8575ad3fcb9a 100644 --- a/regression-tests.api/test_Zones.py +++ b/regression-tests.api/test_Zones.py @@ -2482,7 +2482,12 @@ def test_zone_comment_delete(self): 'changetype': 'replace', 'name': name, 'type': 'NS', - 'comments': [] + 'comments': [ + { + 'account': 'test4', + 'content': 'this should not show up after delete' + } + ] } payload = {'rrsets': [rrset]} r = self.session.patch( @@ -2490,6 +2495,17 @@ def test_zone_comment_delete(self): data=json.dumps(payload), headers={'content-type': 'application/json'}) self.assert_success(r) + data = self.get_zone(name) + serverset = get_rrset(data, name, 'NS') + print(serverset) + self.assertNotEqual(serverset['records'], []) + self.assertNotEqual(serverset['comments'], []) + + payload['rrsets'][0]['comments'] = [] + r = self.session.patch( + self.url("/api/v1/servers/localhost/zones/" + name), + data=json.dumps(payload), + headers={'content-type': 'application/json'}) # make sure the NS records are still present data = self.get_zone(name) serverset = get_rrset(data, name, 'NS') From 2ec12c411eb975c67229e6e037af9414c53c19c5 Mon Sep 17 00:00:00 2001 From: Peter van Dijk Date: Fri, 14 Nov 2025 16:24:13 +0100 Subject: [PATCH 16/35] enable comment tests for lmdb, except for search --- regression-tests.api/test_Zones.py | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/regression-tests.api/test_Zones.py b/regression-tests.api/test_Zones.py index 8575ad3fcb9a..35393399dfd0 100644 --- a/regression-tests.api/test_Zones.py +++ b/regression-tests.api/test_Zones.py @@ -2456,11 +2456,7 @@ def test_zone_comment_create(self): self.url("/api/v1/servers/localhost/zones/" + name), data=json.dumps(payload), headers={'content-type': 'application/json'}) - if is_auth_lmdb(): - self.assert_error_json(r) # No comments in LMDB - return - else: - self.assert_success(r) + self.assert_success(r) # make sure the comments have been set, and that the NS # records are still present data = self.get_zone(name, rrset_name=name, rrset_type="NS") @@ -2513,7 +2509,6 @@ def test_zone_comment_delete(self): self.assertNotEqual(serverset['records'], []) self.assertEqual(serverset['comments'], []) - @unittest.skipIf(is_auth_lmdb(), "No comments in LMDB") def test_zone_comment_out_of_range_modified_at(self): # Test if a modified_at outside of the 32 bit range throws an error name, payload, zone = self.create_zone() @@ -2537,7 +2532,6 @@ def test_zone_comment_out_of_range_modified_at(self): self.assertEqual(r.status_code, 422) self.assert_in_json_error("Key 'modified_at' is out of range", r.json()) - @unittest.skipIf(is_auth_lmdb(), "No comments in LMDB") def test_zone_comment_stay_intact(self): # Test if comments on an rrset stay intact if the rrset is replaced name, payload, zone = self.create_zone() From 07a0fafe0c77d793c3b58f26632d8304644555f1 Mon Sep 17 00:00:00 2001 From: Peter van Dijk Date: Fri, 14 Nov 2025 16:36:43 +0100 Subject: [PATCH 17/35] format --- modules/lmdbbackend/lmdbbackend.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/lmdbbackend/lmdbbackend.cc b/modules/lmdbbackend/lmdbbackend.cc index a1ed1ba7c61a..b21b9958281a 100644 --- a/modules/lmdbbackend/lmdbbackend.cc +++ b/modules/lmdbbackend/lmdbbackend.cc @@ -1657,7 +1657,7 @@ bool LMDBBackend::replaceRRSet(domainid_t domain_id, const DNSName& qname, const bool LMDBBackend::replaceComments(const domainid_t /* domain_id */, const DNSName& /* qname */, const QType& /* qt */, const vector& comments) { // FIXME: remove old comments - for(const auto& comment: comments) { + for (const auto& comment : comments) { feedComment(comment); } From 367df9906f04016f1f3712f98482d9992f3c578a Mon Sep 17 00:00:00 2001 From: Peter van Dijk Date: Mon, 17 Nov 2025 18:22:50 +0100 Subject: [PATCH 18/35] replaceComments: first delete comments --- modules/lmdbbackend/lmdbbackend.cc | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/modules/lmdbbackend/lmdbbackend.cc b/modules/lmdbbackend/lmdbbackend.cc index b21b9958281a..231b9106e897 100644 --- a/modules/lmdbbackend/lmdbbackend.cc +++ b/modules/lmdbbackend/lmdbbackend.cc @@ -1654,9 +1654,26 @@ bool LMDBBackend::replaceRRSet(domainid_t domain_id, const DNSName& qname, const return true; } -bool LMDBBackend::replaceComments(const domainid_t /* domain_id */, const DNSName& /* qname */, const QType& /* qt */, const vector& comments) +bool LMDBBackend::replaceComments(const domainid_t domain_id, const DNSName& qname, const QType& qtype, const vector& comments) { - // FIXME: remove old comments + // delete all existing comments + // this could be smarter and not del+replace unchanged comments + auto cursor = d_rwtxn->txn->getCursor(d_rwtxn->db->cdbi); + MDBOutVal key{}; + MDBOutVal val{}; + + compoundOrdername co; + + auto relqname = qname.makeRelative(d_transactiondomain); + + string match = co(domain_id, relqname, qtype); + + if (cursor.prefix(match, key, val) == 0) { + do { + cursor.del(key); + } while (cursor.next(key, val) == 0); + } + for (const auto& comment : comments) { feedComment(comment); } From 229a3047289d5006e699a6ea1bf62063f3fd4dcf Mon Sep 17 00:00:00 2001 From: Peter van Dijk Date: Tue, 18 Nov 2025 14:47:18 +0100 Subject: [PATCH 19/35] switch val encoding to protobuf --- modules/lmdbbackend/lmdbbackend.cc | 30 ++++++++++++++---------------- 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/modules/lmdbbackend/lmdbbackend.cc b/modules/lmdbbackend/lmdbbackend.cc index 231b9106e897..60c87bae627d 100644 --- a/modules/lmdbbackend/lmdbbackend.cc +++ b/modules/lmdbbackend/lmdbbackend.cc @@ -43,6 +43,8 @@ #include #include #include +#include +#include #include #include #include @@ -1178,11 +1180,11 @@ std::pair LMDBBackend::serializeComment(const Comment& // that leaves us with content, modified_at, account string val; - uint64_t ts = LMDBLS::pdns_bswap64(comment.modified_at); - val.reserve(sizeof(uint64_t) + comment.content.size() + comment.account.size() + 2); // one timestamp, two strings, 2 \0 delimiters - val.assign((char*)(&ts), sizeof(ts)); // FIXME i bet there's a cleaner way - val += comment.content + string(1, (char)0) + comment.account + string(1, (char)0); + protozero::pbf_writer val_writer{val}; + val_writer.add_sfixed64(1, comment.modified_at); + val_writer.add_string(2, comment.account); + val_writer.add_string(3, comment.content); auto hash = pdns::sha256sum(val); @@ -2136,25 +2138,21 @@ bool LMDBBackend::getInternal(DNSName& basename, std::string_view& key) basename = compoundOrdername::getQName(key); - uint64_t ts; - auto val = d_lookupstate.val.get(); + auto val = d_lookupstate.val.get(); // FIXME see if can be string_view again if (val.size() < sizeof(uint64_t)) { throw DBException("got invalid serialized comment"); } - memcpy(&ts, val.data(), sizeof(uint64_t)); // ts in network order - - val = val.substr(8, std::string_view::npos); - d_lookupstate.comment.domain_id = compoundOrdername::getDomainID(key); d_lookupstate.comment.qname = basename + d_lookupstate.domain.operator const DNSName&(); d_lookupstate.comment.qtype = compoundOrdername::getQType(key); - d_lookupstate.comment.modified_at = LMDBLS::pdns_bswap64(ts); - auto pos = val.find('\0', 1) + 1; // FIXME check - d_lookupstate.comment.content = val.substr(0, pos - 1); // pos-1? - val = val.substr(pos, std::string_view::npos); - pos = val.find('\0', 1) + 1; // FIXME check - d_lookupstate.comment.account = val.substr(0, pos - 1); // pos-1? + protozero::pbf_reader message{val}; + message.next(); // FIXME error handling + d_lookupstate.comment.modified_at = message.get_sfixed64(); + message.next(); + d_lookupstate.comment.account = message.get_string(); + message.next(); + d_lookupstate.comment.content = message.get_string(); if (d_lookupstate.cursor && d_lookupstate.cursor->next(d_lookupstate.key, d_lookupstate.val) != 0) { d_lookupstate.reset(); // this invalidates cursor and makes us return false on the next round From ec133003f3f5c63cf08fdf90f4f205f975ee0b7a Mon Sep 17 00:00:00 2001 From: Peter van Dijk Date: Wed, 19 Nov 2025 11:48:00 +0100 Subject: [PATCH 20/35] skip pre-protobuf serialised comments --- modules/lmdbbackend/lmdbbackend.cc | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/modules/lmdbbackend/lmdbbackend.cc b/modules/lmdbbackend/lmdbbackend.cc index 60c87bae627d..74b8f495a744 100644 --- a/modules/lmdbbackend/lmdbbackend.cc +++ b/modules/lmdbbackend/lmdbbackend.cc @@ -2146,13 +2146,18 @@ bool LMDBBackend::getInternal(DNSName& basename, std::string_view& key) d_lookupstate.comment.domain_id = compoundOrdername::getDomainID(key); d_lookupstate.comment.qname = basename + d_lookupstate.domain.operator const DNSName&(); d_lookupstate.comment.qtype = compoundOrdername::getQType(key); - protozero::pbf_reader message{val}; - message.next(); // FIXME error handling - d_lookupstate.comment.modified_at = message.get_sfixed64(); - message.next(); - d_lookupstate.comment.account = message.get_string(); - message.next(); - d_lookupstate.comment.content = message.get_string(); + try { + protozero::pbf_reader message{val}; + message.next(); // FIXME error handling + d_lookupstate.comment.modified_at = message.get_sfixed64(); + message.next(); + d_lookupstate.comment.account = message.get_string(); + message.next(); + d_lookupstate.comment.content = message.get_string(); + } + catch (protozero::exception &e) { + // very little we can do about this + } if (d_lookupstate.cursor && d_lookupstate.cursor->next(d_lookupstate.key, d_lookupstate.val) != 0) { d_lookupstate.reset(); // this invalidates cursor and makes us return false on the next round From 284197e771b77cc9f7599e19be5f843b2f47f0a5 Mon Sep 17 00:00:00 2001 From: Peter van Dijk Date: Thu, 20 Nov 2025 10:37:14 +0100 Subject: [PATCH 21/35] explicit field numbers. not sure this is an improvement --- modules/lmdbbackend/lmdbbackend.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/modules/lmdbbackend/lmdbbackend.cc b/modules/lmdbbackend/lmdbbackend.cc index 74b8f495a744..3ce01ff067a5 100644 --- a/modules/lmdbbackend/lmdbbackend.cc +++ b/modules/lmdbbackend/lmdbbackend.cc @@ -2148,11 +2148,11 @@ bool LMDBBackend::getInternal(DNSName& basename, std::string_view& key) d_lookupstate.comment.qtype = compoundOrdername::getQType(key); try { protozero::pbf_reader message{val}; - message.next(); // FIXME error handling + message.next(1); // FIXME error handling d_lookupstate.comment.modified_at = message.get_sfixed64(); - message.next(); + message.next(2); d_lookupstate.comment.account = message.get_string(); - message.next(); + message.next(3); d_lookupstate.comment.content = message.get_string(); } catch (protozero::exception &e) { From 49988810e4799cb5cce107b1e408b0a7ffb378cd Mon Sep 17 00:00:00 2001 From: Peter van Dijk Date: Thu, 20 Nov 2025 11:33:56 +0100 Subject: [PATCH 22/35] fix autotools build? --- modules/lmdbbackend/Makefile.am | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/modules/lmdbbackend/Makefile.am b/modules/lmdbbackend/Makefile.am index 91c0fbde1df1..fc75cda0a791 100644 --- a/modules/lmdbbackend/Makefile.am +++ b/modules/lmdbbackend/Makefile.am @@ -1,4 +1,5 @@ -AM_CPPFLAGS += $(LMDB_CFLAGS) $(LIBCRYPTO_INCLUDES) +AM_CPPFLAGS += $(LMDB_CFLAGS) $(LIBCRYPTO_INCLUDES) -I$(top_srcdir)/ext/protozero/include + pkglib_LTLIBRARIES = liblmdbbackend.la From 6cd1308e7ecb19edd6caa2f6d04c5a07ef41898f Mon Sep 17 00:00:00 2001 From: Peter van Dijk Date: Thu, 20 Nov 2025 11:36:01 +0100 Subject: [PATCH 23/35] format --- modules/lmdbbackend/lmdbbackend.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/lmdbbackend/lmdbbackend.cc b/modules/lmdbbackend/lmdbbackend.cc index 3ce01ff067a5..c7421ff2a920 100644 --- a/modules/lmdbbackend/lmdbbackend.cc +++ b/modules/lmdbbackend/lmdbbackend.cc @@ -2155,7 +2155,7 @@ bool LMDBBackend::getInternal(DNSName& basename, std::string_view& key) message.next(3); d_lookupstate.comment.content = message.get_string(); } - catch (protozero::exception &e) { + catch (protozero::exception& e) { // very little we can do about this } From f6cb113bed9a4c21dbe8a2004fe2adff142b400f Mon Sep 17 00:00:00 2001 From: Peter van Dijk Date: Thu, 20 Nov 2025 12:27:14 +0100 Subject: [PATCH 24/35] comment search --- pdns/dnsbackend.cc | 36 ++++++++++++++++++++++++++++++ pdns/dnsbackend.hh | 5 +---- regression-tests.api/test_Zones.py | 1 - 3 files changed, 37 insertions(+), 5 deletions(-) diff --git a/pdns/dnsbackend.cc b/pdns/dnsbackend.cc index 738f4b107467..42ee1e8ebe69 100644 --- a/pdns/dnsbackend.cc +++ b/pdns/dnsbackend.cc @@ -109,6 +109,42 @@ bool DNSBackend::searchRecords(const string& pattern, size_t maxResults, vector< return true; } +// Default search logic, for backends which can enumerate their comments. +bool DNSBackend::searchComments(const string& pattern, size_t maxResults, vector& result) +{ + // We depend upon working list(), but also getAllDomains(), which is why we + // are checking explicitly for CAP_SEARCH in addition to CAP_LIST - the + // default getAllDomains() implementation below is not safe to use here. + if ((getCapabilities() & (CAP_LIST | CAP_SEARCH)) != (CAP_LIST | CAP_SEARCH)) { + return false; + } + + SimpleMatch simpleMatch(pattern, true); + std::vector domains; + getAllDomains(&domains, false, true); + for (const auto& info : domains) { + if (!listComments(info.id)) { + return false; + } + Comment comment; + while (getComment(comment)) { + if (maxResults == 0) { + // No need to look any further + lookupEnd(); + break; + } + if (simpleMatch.match(comment.qname) || simpleMatch.match(comment.content)) { + result.emplace_back(comment); + --maxResults; + } + } + if (maxResults == 0) { + break; + } + } + return true; +} + void BackendFactory::declare(const string& suffix, const string& param, const string& explanation, const string& value) { string fullname = d_name + suffix + "-" + param; diff --git a/pdns/dnsbackend.hh b/pdns/dnsbackend.hh index bc1f2dbfb0fc..71c48cd793bc 100644 --- a/pdns/dnsbackend.hh +++ b/pdns/dnsbackend.hh @@ -475,10 +475,7 @@ public: virtual bool searchRecords(const string& pattern, size_t maxResults, vector& result); //! Search for comments, returns true if search was done successfully. - virtual bool searchComments(const string& /* pattern */, size_t /* maxResults */, vector& /* result */) - { - return false; - } + virtual bool searchComments(const string& /* pattern */, size_t /* maxResults */, vector& /* result */); virtual void viewList(vector& /* result */) { diff --git a/regression-tests.api/test_Zones.py b/regression-tests.api/test_Zones.py index 35393399dfd0..e3453a8f360d 100644 --- a/regression-tests.api/test_Zones.py +++ b/regression-tests.api/test_Zones.py @@ -2652,7 +2652,6 @@ def test_search_rr_case_insensitive(self): # should return zone, SOA, ns1, ns2 self.assertEqual(len(r.json()), 4) - @unittest.skipIf(is_auth_lmdb(), "No comments in LMDB") def test_search_rr_comment(self): name = unique_zone_name() rrsets = [{ From 460149cd38692adbd04a518448e6125ed5253edc Mon Sep 17 00:00:00 2001 From: Peter van Dijk Date: Thu, 20 Nov 2025 13:50:17 +0100 Subject: [PATCH 25/35] nolint --- pdns/pdnsutil.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pdns/pdnsutil.cc b/pdns/pdnsutil.cc index bed46cf715b9..8dc9a9e658fc 100644 --- a/pdns/pdnsutil.cc +++ b/pdns/pdnsutil.cc @@ -1894,7 +1894,7 @@ static int listZone(const ZoneName &zone) { static int listComments(const ZoneName &zone) { UtilBackend B; //NOLINT(readability-identifier-length) - DomainInfo di; + DomainInfo di; //NOLINT(readability-identifier-length) if (! B.getDomainInfo(zone, di)) { cerr << "Zone '" << zone << "' not found!" << endl; From 8af286becff67f0bba6e639975f15ed551e04c82 Mon Sep 17 00:00:00 2001 From: Peter van Dijk Date: Tue, 25 Nov 2025 14:59:38 +0100 Subject: [PATCH 26/35] nits --- modules/lmdbbackend/lmdbbackend.cc | 2 +- pdns/pdnsutil.cc | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/modules/lmdbbackend/lmdbbackend.cc b/modules/lmdbbackend/lmdbbackend.cc index c7421ff2a920..bc46e4b74145 100644 --- a/modules/lmdbbackend/lmdbbackend.cc +++ b/modules/lmdbbackend/lmdbbackend.cc @@ -2133,7 +2133,7 @@ bool LMDBBackend::getInternal(DNSName& basename, std::string_view& key) } key = d_lookupstate.key.getNoStripHeader(); - // remove hash from the key so compoundOrdername::get* work + // remove hash from the key so compoundOrdername::get* works key = key.substr(0, key.size() - 256 / 8); basename = compoundOrdername::getQName(key); diff --git a/pdns/pdnsutil.cc b/pdns/pdnsutil.cc index 8dc9a9e658fc..f48719308632 100644 --- a/pdns/pdnsutil.cc +++ b/pdns/pdnsutil.cc @@ -1900,8 +1900,8 @@ static int listComments(const ZoneName &zone) { cerr << "Zone '" << zone << "' not found!" << endl; return EXIT_FAILURE; } - if ((di.backend->getCapabilities() & DNSBackend::CAP_LIST) == 0) { - cerr << "Backend for zone '" << zone << "' does not support listing its contents." << endl; + if ((di.backend->getCapabilities() & DNSBackend::CAP_COMMENTS) == 0) { + cerr << "Backend for zone '" << zone << "' does not support listing its comments." << endl; return EXIT_FAILURE; } From 4df2c5acabeca8c00ee555cb19b684cc421ac520 Mon Sep 17 00:00:00 2001 From: Peter van Dijk Date: Tue, 25 Nov 2025 15:00:13 +0100 Subject: [PATCH 27/35] Revert "expose pdns_bswap64 to LMDBLS consumers" This reverts commit c7969571c29670d55b5b566133c8286d9ce710ea. --- ext/lmdb-safe/lmdb-safe.hh | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/ext/lmdb-safe/lmdb-safe.hh b/ext/lmdb-safe/lmdb-safe.hh index 66900c058bf0..78bcf7ba5875 100644 --- a/ext/lmdb-safe/lmdb-safe.hh +++ b/ext/lmdb-safe/lmdb-safe.hh @@ -123,23 +123,24 @@ std::shared_ptr getMDBEnv(const char* fname, int flags, int mode, uint64 struct MDBOutVal; // forward declaration because of how the functions below tie in with MDBOutVal namespace LMDBLS { - // Some systems #define bswap64 to __builtin_bswap64, and the body below would cause infinite - // recursion if we would name the function bswap64 - static auto pdns_bswap64(uint64_t value) -> uint64_t - { + class __attribute__((__packed__)) LSheader { + private: + // Some systems #define bswap64 to __builtin_bswap64, and the body below would cause infinite + // recursion if we would name the function bswap64 + static auto pdns_bswap64(uint64_t value) -> uint64_t + { #if !defined(__BYTE_ORDER__) || !defined(__ORDER_LITTLE_ENDIAN__) || !defined(__ORDER_BIG_ENDIAN__) #error "your compiler does not define byte order macros" #endif #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__ - // FIXME: Do something more portable than __builtin_bswap64. - return __builtin_bswap64(value); + // FIXME: Do something more portable than __builtin_bswap64. + return __builtin_bswap64(value); #else - return value; + return value; #endif - } + } - class __attribute__((__packed__)) LSheader { public: uint64_t d_timestamp; uint64_t d_txnid; From f959906d589cd188b44ecc210c8045d8f2de66c6 Mon Sep 17 00:00:00 2001 From: Peter van Dijk Date: Tue, 25 Nov 2025 15:44:52 +0100 Subject: [PATCH 28/35] nolint --- pdns/pdnsutil.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pdns/pdnsutil.cc b/pdns/pdnsutil.cc index f48719308632..efa036db0a7d 100644 --- a/pdns/pdnsutil.cc +++ b/pdns/pdnsutil.cc @@ -4387,8 +4387,8 @@ static int addComment(vector& cmds, const std::string_view synopsis) return usage(synopsis); } - UtilBackend B; - DomainInfo di; + UtilBackend B; //NOLINT(readability-identifier-length) + DomainInfo di; //NOLINT(readability-identifier-length) ZoneName zone(cmds.at(0)); if (!B.getDomainInfo(zone, di)) { cerr << "Zone '" << zone << "' doesn't exist" << endl; From 7273a0e7d2ac4fbed14f14490bdd72da710e45ed Mon Sep 17 00:00:00 2001 From: Peter van Dijk Date: Tue, 25 Nov 2025 15:51:01 +0100 Subject: [PATCH 29/35] expand comment --- modules/lmdbbackend/lmdbbackend.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/lmdbbackend/lmdbbackend.cc b/modules/lmdbbackend/lmdbbackend.cc index bc46e4b74145..d5b852be3ebc 100644 --- a/modules/lmdbbackend/lmdbbackend.cc +++ b/modules/lmdbbackend/lmdbbackend.cc @@ -1658,7 +1658,7 @@ bool LMDBBackend::replaceRRSet(domainid_t domain_id, const DNSName& qname, const bool LMDBBackend::replaceComments(const domainid_t domain_id, const DNSName& qname, const QType& qtype, const vector& comments) { - // delete all existing comments + // delete all existing comments for the RRset // this could be smarter and not del+replace unchanged comments auto cursor = d_rwtxn->txn->getCursor(d_rwtxn->db->cdbi); MDBOutVal key{}; From ec889918451babc0f2b6c1e11c4ce4fb923a4544 Mon Sep 17 00:00:00 2001 From: Peter van Dijk Date: Tue, 25 Nov 2025 15:57:21 +0100 Subject: [PATCH 30/35] drop dead code and old comment --- modules/lmdbbackend/lmdbbackend.cc | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/modules/lmdbbackend/lmdbbackend.cc b/modules/lmdbbackend/lmdbbackend.cc index d5b852be3ebc..cf0beaa7c2f6 100644 --- a/modules/lmdbbackend/lmdbbackend.cc +++ b/modules/lmdbbackend/lmdbbackend.cc @@ -2139,16 +2139,13 @@ bool LMDBBackend::getInternal(DNSName& basename, std::string_view& key) basename = compoundOrdername::getQName(key); auto val = d_lookupstate.val.get(); // FIXME see if can be string_view again - if (val.size() < sizeof(uint64_t)) { - throw DBException("got invalid serialized comment"); - } d_lookupstate.comment.domain_id = compoundOrdername::getDomainID(key); d_lookupstate.comment.qname = basename + d_lookupstate.domain.operator const DNSName&(); d_lookupstate.comment.qtype = compoundOrdername::getQType(key); try { protozero::pbf_reader message{val}; - message.next(1); // FIXME error handling + message.next(1); d_lookupstate.comment.modified_at = message.get_sfixed64(); message.next(2); d_lookupstate.comment.account = message.get_string(); From 0dcac1c0510a12d3139965d140cef14139e52977 Mon Sep 17 00:00:00 2001 From: Peter van Dijk Date: Tue, 25 Nov 2025 16:05:30 +0100 Subject: [PATCH 31/35] const is almost as cool as string_view --- modules/lmdbbackend/lmdbbackend.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/lmdbbackend/lmdbbackend.cc b/modules/lmdbbackend/lmdbbackend.cc index cf0beaa7c2f6..27f8342eaef7 100644 --- a/modules/lmdbbackend/lmdbbackend.cc +++ b/modules/lmdbbackend/lmdbbackend.cc @@ -2138,7 +2138,7 @@ bool LMDBBackend::getInternal(DNSName& basename, std::string_view& key) basename = compoundOrdername::getQName(key); - auto val = d_lookupstate.val.get(); // FIXME see if can be string_view again + const auto& val = d_lookupstate.val.get(); d_lookupstate.comment.domain_id = compoundOrdername::getDomainID(key); d_lookupstate.comment.qname = basename + d_lookupstate.domain.operator const DNSName&(); From 175753ec31ba60888afc91c25a597c63826bdba5 Mon Sep 17 00:00:00 2001 From: Peter van Dijk Date: Tue, 25 Nov 2025 16:25:53 +0100 Subject: [PATCH 32/35] actually declare comments support capability --- modules/lmdbbackend/lmdbbackend.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/lmdbbackend/lmdbbackend.cc b/modules/lmdbbackend/lmdbbackend.cc index 27f8342eaef7..aa7e55148cef 100644 --- a/modules/lmdbbackend/lmdbbackend.cc +++ b/modules/lmdbbackend/lmdbbackend.cc @@ -892,7 +892,7 @@ void LMDBBackend::openAllTheDatabases() unsigned int LMDBBackend::getCapabilities() { - unsigned int caps = CAP_DNSSEC | CAP_DIRECT | CAP_LIST | CAP_CREATE | CAP_SEARCH; + unsigned int caps = CAP_DNSSEC | CAP_DIRECT | CAP_LIST | CAP_CREATE | CAP_SEARCH | CAP_COMMENTS; if (d_views) { caps |= CAP_VIEWS; } From ba2d37b84db4c5aa1b3c07c1466f5fe218332c95 Mon Sep 17 00:00:00 2001 From: Peter van Dijk Date: Tue, 25 Nov 2025 16:29:27 +0100 Subject: [PATCH 33/35] check if name is in zone --- pdns/pdnsutil.cc | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/pdns/pdnsutil.cc b/pdns/pdnsutil.cc index efa036db0a7d..4c7af8e88306 100644 --- a/pdns/pdnsutil.cc +++ b/pdns/pdnsutil.cc @@ -4406,6 +4406,10 @@ static int addComment(vector& cmds, const std::string_view synopsis) } comment.modified_at = time(nullptr); + if (!comment.qname.isPartOf(zone)) { + throw PDNSException("Name \"" + comment.qname.toString() + "\" to add comment to is not part of zone \"" + zone.toString() + "\"."); + } + di.backend->startTransaction(zone, UnknownDomainID); if (!di.backend->feedComment(comment)) { cerr << "Backend does not support comments" << endl; From d976bd37928db4a663bc8a06a9aafc64065d0036 Mon Sep 17 00:00:00 2001 From: Peter van Dijk Date: Fri, 6 Mar 2026 11:25:33 +0100 Subject: [PATCH 34/35] throw DBException when we encounter bad comment data Co-authored-by: Miod Vallat Signed-off-by: Peter van Dijk --- modules/lmdbbackend/lmdbbackend.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/lmdbbackend/lmdbbackend.cc b/modules/lmdbbackend/lmdbbackend.cc index aa7e55148cef..4f14b958ffc7 100644 --- a/modules/lmdbbackend/lmdbbackend.cc +++ b/modules/lmdbbackend/lmdbbackend.cc @@ -2153,7 +2153,7 @@ bool LMDBBackend::getInternal(DNSName& basename, std::string_view& key) d_lookupstate.comment.content = message.get_string(); } catch (protozero::exception& e) { - // very little we can do about this + throw DBException("got invalid serialized comment: " + e.what()); } if (d_lookupstate.cursor && d_lookupstate.cursor->next(d_lookupstate.key, d_lookupstate.val) != 0) { From b57cce85e1787a391c233cbc5a3361532a9246cb Mon Sep 17 00:00:00 2001 From: Peter van Dijk Date: Fri, 6 Mar 2026 11:26:58 +0100 Subject: [PATCH 35/35] fix quote and space Co-authored-by: Miod Vallat Signed-off-by: Peter van Dijk --- modules/lmdbbackend/lmdbbackend.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/lmdbbackend/lmdbbackend.cc b/modules/lmdbbackend/lmdbbackend.cc index 4f14b958ffc7..f7897447d84f 100644 --- a/modules/lmdbbackend/lmdbbackend.cc +++ b/modules/lmdbbackend/lmdbbackend.cc @@ -1984,7 +1984,7 @@ bool LMDBBackend::listComments(domainid_t domain_id) { DomainInfo info; if (!findDomain(domain_id, info)) { - throw DBException("Domain with id '" + std::to_string(domain_id) + "not found"); + throw DBException("Domain with id '" + std::to_string(domain_id) + "' not found"); } d_lookupstate.domain = info.zone;