From d28680949e953388c871f0ca1dbcabbdda5d6753 Mon Sep 17 00:00:00 2001 From: Fred Morcos Date: Sun, 29 Oct 2023 17:47:38 +0100 Subject: [PATCH] Get rid of manual deletion of backends in UeberBackend --- .../remotebackend/test-remotebackend-http.cc | 2 +- .../remotebackend/test-remotebackend-json.cc | 2 +- .../remotebackend/test-remotebackend-pipe.cc | 2 +- .../remotebackend/test-remotebackend-post.cc | 2 +- .../remotebackend/test-remotebackend-unix.cc | 2 +- .../test-remotebackend-zeromq.cc | 2 +- pdns/dnsbackend.cc | 38 ++++----- pdns/dnsbackend.hh | 2 +- pdns/pdnsutil.cc | 45 ++++++---- pdns/test-ueberbackend_cc.cc | 4 +- pdns/ueberbackend.cc | 84 +++++++++---------- pdns/ueberbackend.hh | 4 +- 12 files changed, 91 insertions(+), 98 deletions(-) diff --git a/modules/remotebackend/test-remotebackend-http.cc b/modules/remotebackend/test-remotebackend-http.cc index 824db56203..3123752b8a 100644 --- a/modules/remotebackend/test-remotebackend-http.cc +++ b/modules/remotebackend/test-remotebackend-http.cc @@ -76,7 +76,7 @@ struct RemotebackendSetup // then get us a instance of it ::arg().set("remote-connection-string") = "http:url=http://localhost:62434/dns"; ::arg().set("remote-dnssec") = "yes"; - be = BackendMakers().all()[0]; + be = BackendMakers().all()[0].get(); } catch (PDNSException& ex) { BOOST_TEST_MESSAGE("Cannot start remotebackend: " << ex.reason); diff --git a/modules/remotebackend/test-remotebackend-json.cc b/modules/remotebackend/test-remotebackend-json.cc index 48d023ed82..6a47b772c9 100644 --- a/modules/remotebackend/test-remotebackend-json.cc +++ b/modules/remotebackend/test-remotebackend-json.cc @@ -75,7 +75,7 @@ struct RemotebackendSetup // then get us a instance of it ::arg().set("remote-connection-string") = "http:url=http://localhost:62434/dns/endpoint.json,post=1,post_json=1"; ::arg().set("remote-dnssec") = "yes"; - be = BackendMakers().all()[0]; + be = BackendMakers().all()[0].get(); } catch (PDNSException& ex) { BOOST_TEST_MESSAGE("Cannot start remotebackend: " << ex.reason); diff --git a/modules/remotebackend/test-remotebackend-pipe.cc b/modules/remotebackend/test-remotebackend-pipe.cc index 6ad872c281..15ca1571e8 100644 --- a/modules/remotebackend/test-remotebackend-pipe.cc +++ b/modules/remotebackend/test-remotebackend-pipe.cc @@ -75,7 +75,7 @@ struct RemotebackendSetup // then get us a instance of it ::arg().set("remote-connection-string") = "pipe:command=unittest_pipe.rb"; ::arg().set("remote-dnssec") = "yes"; - be = BackendMakers().all()[0]; + be = BackendMakers().all()[0].get(); // load few record types to help out SOARecordContent::report(); NSRecordContent::report(); diff --git a/modules/remotebackend/test-remotebackend-post.cc b/modules/remotebackend/test-remotebackend-post.cc index fe129623f1..d35baa00ae 100644 --- a/modules/remotebackend/test-remotebackend-post.cc +++ b/modules/remotebackend/test-remotebackend-post.cc @@ -75,7 +75,7 @@ struct RemotebackendSetup // then get us a instance of it ::arg().set("remote-connection-string") = "http:url=http://localhost:62434/dns,post=1"; ::arg().set("remote-dnssec") = "yes"; - be = BackendMakers().all()[0]; + be = BackendMakers().all()[0].get(); } catch (PDNSException& ex) { BOOST_TEST_MESSAGE("Cannot start remotebackend: " << ex.reason); diff --git a/modules/remotebackend/test-remotebackend-unix.cc b/modules/remotebackend/test-remotebackend-unix.cc index fd2c1b15a6..7d20a2ad32 100644 --- a/modules/remotebackend/test-remotebackend-unix.cc +++ b/modules/remotebackend/test-remotebackend-unix.cc @@ -75,7 +75,7 @@ struct RemotebackendSetup // then get us a instance of it ::arg().set("remote-connection-string") = "unix:path=/tmp/remotebackend.sock"; ::arg().set("remote-dnssec") = "yes"; - be = BackendMakers().all()[0]; + be = BackendMakers().all()[0].get(); // load few record types to help out SOARecordContent::report(); NSRecordContent::report(); diff --git a/modules/remotebackend/test-remotebackend-zeromq.cc b/modules/remotebackend/test-remotebackend-zeromq.cc index bccf076837..5244a6dab8 100644 --- a/modules/remotebackend/test-remotebackend-zeromq.cc +++ b/modules/remotebackend/test-remotebackend-zeromq.cc @@ -77,7 +77,7 @@ struct RemotebackendSetup // then get us a instance of it ::arg().set("remote-connection-string") = "zeromq:endpoint=ipc:///tmp/remotebackend.0"; ::arg().set("remote-dnssec") = "yes"; - be = BackendMakers().all()[0]; + be = BackendMakers().all()[0].get(); // load few record types to help out SOARecordContent::report(); NSRecordContent::report(); diff --git a/pdns/dnsbackend.cc b/pdns/dnsbackend.cc index aef1b99a3a..8cceece213 100644 --- a/pdns/dnsbackend.cc +++ b/pdns/dnsbackend.cc @@ -19,6 +19,7 @@ * along with this program; if not, write to the Free Software * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. */ +#include #ifdef HAVE_CONFIG_H #include "config.h" #endif @@ -180,12 +181,13 @@ size_t BackendMakerClass::numLauncheable() const return d_instances.size(); } -vector BackendMakerClass::all(bool metadataOnly) +vector> BackendMakerClass::all(bool metadataOnly) { - vector ret; - if(d_instances.empty()) + if(d_instances.empty()) { throw PDNSException("No database backends configured for launch, unable to function"); + } + vector> ret; ret.reserve(d_instances.size()); std::string current; // to make the exception text more useful @@ -193,35 +195,23 @@ vector BackendMakerClass::all(bool metadataOnly) try { for (const auto& instance : d_instances) { current = instance.first + instance.second; - DNSBackend *made = nullptr; - - if (metadataOnly) { - made = d_repository[instance.first]->makeMetadataOnly(instance.second); - } - else { - made = d_repository[instance.first]->make(instance.second); - } - - if (!made) { + auto* repo = d_repository[instance.first]; + std::unique_ptr made{metadataOnly ? repo->makeMetadataOnly(instance.second) : repo->make(instance.second)}; + if (made == nullptr) { throw PDNSException("Unable to launch backend '" + instance.first + "'"); } - - ret.push_back(made); + ret.push_back(std::move(made)); } } catch(const PDNSException &ae) { - g_log< all(bool skipBIND=false); + vector> all(bool metadataOnly=false); void load(const string &module); [[nodiscard]] size_t numLauncheable() const; vector getModules(); diff --git a/pdns/pdnsutil.cc b/pdns/pdnsutil.cc index 4b7687da1f..cbec56b17e 100644 --- a/pdns/pdnsutil.cc +++ b/pdns/pdnsutil.cc @@ -1,4 +1,5 @@ +#include #ifdef HAVE_CONFIG_H #include "config.h" #endif @@ -2361,7 +2362,7 @@ static int testSchema(DNSSECKeeper& dk, const DNSName& zone) cout<<"Constructing UeberBackend"<createSlaveDomain("127.0.0.1", zone, "", "_testschema"); cout << "Secondary zone created" << endl; @@ -2505,6 +2506,7 @@ static int addOrSetMeta(const DNSName& zone, const string& kind, const vectorgetPrefix() == cmds.at(1)) - src = b; - if (b->getPrefix() == cmds.at(2)) - tgt = b; + unique_ptr src{nullptr}; + unique_ptr tgt{nullptr}; + + for (auto& backend : BackendMakers().all()) { + if (backend->getPrefix() == cmds.at(1)) { + src = std::move(backend); + } + else if (backend->getPrefix() == cmds.at(2)) { + tgt = std::move(backend); + } } + if (src == nullptr) { cerr << "Unknown source backend '" << cmds.at(1) << "'" << endl; return 1; @@ -4190,21 +4200,22 @@ try return 1; } - DNSBackend *db = nullptr; + std::unique_ptr matchingBackend{nullptr}; - for(DNSBackend *b : BackendMakers().all()) { - if (b->getPrefix() == cmds.at(1)) - db = b; + for (auto& backend : BackendMakers().all()) { + if (backend->getPrefix() == cmds.at(1)) { + matchingBackend = std::move(backend); + } } - if (db == nullptr) { + if (matchingBackend == nullptr) { cerr << "Unknown backend '" << cmds.at(1) << "'" << endl; return 1; } - for(auto i=next(begin(cmds),2); i != end(cmds); ++i) { - cerr<<"== "<<*i<directBackendCmd(*i); + for (auto i = next(begin(cmds), 2); i != end(cmds); ++i) { + cerr << "== " << *i << endl; + cout << matchingBackend->directBackendCmd(*i); } return 0; diff --git a/pdns/test-ueberbackend_cc.cc b/pdns/test-ueberbackend_cc.cc index 4a13aafad0..1bde35389a 100644 --- a/pdns/test-ueberbackend_cc.cc +++ b/pdns/test-ueberbackend_cc.cc @@ -1053,8 +1053,10 @@ BOOST_AUTO_TEST_CASE(test_multi_backends_best_soa) { auto testFunction = [](UeberBackend& ub) -> void { { - auto sbba = dynamic_cast(ub.backends.at(0)); + auto* sbba = dynamic_cast(ub.backends.at(0).get()); BOOST_REQUIRE(sbba != nullptr); + + // NOLINTNEXTLINE (clang-analyzer-core.NullDereference): Not sure. sbba->d_authLookupCount = 0; // test getAuth() diff --git a/pdns/ueberbackend.cc b/pdns/ueberbackend.cc index 5ba73e7724..3db6d3971d 100644 --- a/pdns/ueberbackend.cc +++ b/pdns/ueberbackend.cc @@ -19,6 +19,7 @@ * along with this program; if not, write to the Free Software * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. */ +#include #ifdef HAVE_CONFIG_H #include "config.h" #endif @@ -121,7 +122,7 @@ void UeberBackend::go() bool UeberBackend::getDomainInfo(const DNSName& domain, DomainInfo& domainInfo, bool getSerial) { - for (auto* backend : backends) { + for (auto& backend : backends) { if (backend->getDomainInfo(domain, domainInfo, getSerial)) { return true; } @@ -131,8 +132,8 @@ bool UeberBackend::getDomainInfo(const DNSName& domain, DomainInfo& domainInfo, bool UeberBackend::createDomain(const DNSName& domain, const DomainInfo::DomainKind kind, const vector& masters, const string& account) { - for (DNSBackend* mydb : backends) { - if (mydb->createDomain(domain, kind, masters, account)) { + for (auto& backend : backends) { + if (backend->createDomain(domain, kind, masters, account)) { return true; } } @@ -141,7 +142,7 @@ bool UeberBackend::createDomain(const DNSName& domain, const DomainInfo::DomainK bool UeberBackend::doesDNSSEC() { - for (auto* backend : backends) { + for (auto& backend : backends) { if (backend->doesDNSSEC()) { return true; } @@ -152,7 +153,7 @@ bool UeberBackend::doesDNSSEC() bool UeberBackend::addDomainKey(const DNSName& name, const DNSBackend::KeyData& key, int64_t& keyID) { keyID = -1; - for (DNSBackend* backend : backends) { + for (auto& backend : backends) { if (backend->addDomainKey(name, key, keyID)) { return true; } @@ -161,7 +162,7 @@ bool UeberBackend::addDomainKey(const DNSName& name, const DNSBackend::KeyData& } bool UeberBackend::getDomainKeys(const DNSName& name, std::vector& keys) { - for (DNSBackend* backend : backends) { + for (auto& backend : backends) { if (backend->getDomainKeys(name, keys)) { return true; } @@ -171,7 +172,7 @@ bool UeberBackend::getDomainKeys(const DNSName& name, std::vector>& meta) { - for (DNSBackend* backend : backends) { + for (auto& backend : backends) { if (backend->getAllDomainMetadata(name, meta)) { return true; } @@ -181,7 +182,7 @@ bool UeberBackend::getAllDomainMetadata(const DNSName& name, std::map& meta) { - for (DNSBackend* backend : backends) { + for (auto& backend : backends) { if (backend->getDomainMetadata(name, kind, meta)) { return true; } @@ -202,7 +203,7 @@ bool UeberBackend::getDomainMetadata(const DNSName& name, const std::string& kin bool UeberBackend::setDomainMetadata(const DNSName& name, const std::string& kind, const std::vector& meta) { - for (DNSBackend* backend : backends) { + for (auto& backend : backends) { if (backend->setDomainMetadata(name, kind, meta)) { return true; } @@ -221,7 +222,7 @@ bool UeberBackend::setDomainMetadata(const DNSName& name, const std::string& kin bool UeberBackend::activateDomainKey(const DNSName& name, unsigned int keyID) { - for (DNSBackend* backend : backends) { + for (auto& backend : backends) { if (backend->activateDomainKey(name, keyID)) { return true; } @@ -231,7 +232,7 @@ bool UeberBackend::activateDomainKey(const DNSName& name, unsigned int keyID) bool UeberBackend::deactivateDomainKey(const DNSName& name, unsigned int keyID) { - for (DNSBackend* backend : backends) { + for (auto& backend : backends) { if (backend->deactivateDomainKey(name, keyID)) { return true; } @@ -241,7 +242,7 @@ bool UeberBackend::deactivateDomainKey(const DNSName& name, unsigned int keyID) bool UeberBackend::publishDomainKey(const DNSName& name, unsigned int keyID) { - for (DNSBackend* backend : backends) { + for (auto& backend : backends) { if (backend->publishDomainKey(name, keyID)) { return true; } @@ -251,7 +252,7 @@ bool UeberBackend::publishDomainKey(const DNSName& name, unsigned int keyID) bool UeberBackend::unpublishDomainKey(const DNSName& name, unsigned int keyID) { - for (DNSBackend* backend : backends) { + for (auto& backend : backends) { if (backend->unpublishDomainKey(name, keyID)) { return true; } @@ -261,7 +262,7 @@ bool UeberBackend::unpublishDomainKey(const DNSName& name, unsigned int keyID) bool UeberBackend::removeDomainKey(const DNSName& name, unsigned int keyID) { - for (DNSBackend* backend : backends) { + for (auto& backend : backends) { if (backend->removeDomainKey(name, keyID)) { return true; } @@ -324,7 +325,7 @@ void UeberBackend::getUpdatedMasters(vector& domains, std::unordered bool UeberBackend::inTransaction() { - for (auto* backend : backends) { + for (auto& backend : backends) { if (backend->inTransaction()) { return true; } @@ -363,7 +364,7 @@ bool UeberBackend::fillSOAFromZoneRecord(DNSName& shorter, const int zoneId, SOA return false; } - soaData->db = backends.size() == 1 ? *backends.begin() : nullptr; + soaData->db = backends.size() == 1 ? backends.begin()->get() : nullptr; // Leave database handle in a consistent state. while (get(zoneRecord)) { @@ -381,7 +382,7 @@ UeberBackend::CacheResult UeberBackend::fillSOAFromCache(SOAData* soaData, DNSNa DLOG(g_log << Logger::Error << "has pos cache entry: " << shorter << endl); fillSOAData(d_answers[0], *soaData); - soaData->db = backends.size() == 1 ? *backends.begin() : nullptr; + soaData->db = backends.size() == 1 ? backends.begin()->get() : nullptr; soaData->qname = shorter; } else if (cacheResult == CacheResult::NegativeMatch && d_negcache_ttl != 0U) { @@ -391,7 +392,7 @@ UeberBackend::CacheResult UeberBackend::fillSOAFromCache(SOAData* soaData, DNSNa return cacheResult; } -static std::vector::iterator findBestMatchingBackend(std::vector& backends, std::vector>& bestMatches, const DNSName& shorter, SOAData* soaData) +static std::vector>::iterator findBestMatchingBackend(std::vector>& backends, std::vector>& bestMatches, const DNSName& shorter, SOAData* soaData) { auto backend = backends.begin(); for (auto bestMatch = bestMatches.begin(); backend != backends.end() && bestMatch != bestMatches.end(); ++backend, ++bestMatch) { @@ -552,7 +553,7 @@ bool UeberBackend::getSOAUncached(const DNSName& domain, SOAData& soaData) d_question.qname = domain; d_question.zoneId = -1; - for (auto* backend : backends) { + for (auto& backend : backends) { if (backend->getSOA(domain, soaData)) { if (domain != soaData.qname) { throw PDNSException("getSOA() returned an SOA for the wrong zone. Question: '" + domain.toLogString() + "', answer: '" + soaData.qname.toLogString() + "'"); @@ -579,7 +580,7 @@ bool UeberBackend::getSOAUncached(const DNSName& domain, SOAData& soaData) bool UeberBackend::superMasterAdd(const AutoPrimary& primary) { - for (auto* backend : backends) { + for (auto& backend : backends) { if (backend->superMasterAdd(primary)) { return true; } @@ -589,7 +590,7 @@ bool UeberBackend::superMasterAdd(const AutoPrimary& primary) bool UeberBackend::autoPrimaryRemove(const AutoPrimary& primary) { - for (auto* backend : backends) { + for (auto& backend : backends) { if (backend->autoPrimaryRemove(primary)) { return true; } @@ -599,7 +600,7 @@ bool UeberBackend::autoPrimaryRemove(const AutoPrimary& primary) bool UeberBackend::autoPrimariesList(std::vector& primaries) { - for (auto* backend : backends) { + for (auto& backend : backends) { if (backend->autoPrimariesList(primaries)) { return true; } @@ -609,7 +610,7 @@ bool UeberBackend::autoPrimariesList(std::vector& primaries) bool UeberBackend::superMasterBackend(const string& ip, const DNSName& domain, const vector& nsset, string* nameserver, string* account, DNSBackend** dnsBackend) { - for (auto* backend : backends) { + for (auto& backend : backends) { if (backend->superMasterBackend(ip, domain, nsset, nameserver, account, dnsBackend)) { return true; } @@ -629,22 +630,6 @@ UeberBackend::UeberBackend(const string& pname) backends = BackendMakers().all(pname == "key-only"); } -static void del(DNSBackend* backend) -{ - delete backend; -} - -void UeberBackend::cleanup() -{ - { - auto instances = d_instances.lock(); - remove(instances->begin(), instances->end(), this); - instances->resize(instances->size() - 1); - } - - for_each(backends.begin(), backends.end(), del); -} - // returns -1 for miss, 0 for negative match, 1 for hit enum UeberBackend::CacheResult UeberBackend::cacheHas(const Question& question, vector& resourceRecords) const { @@ -705,7 +690,14 @@ void UeberBackend::alsoNotifies(const DNSName& domain, set* ips) UeberBackend::~UeberBackend() { DLOG(g_log << Logger::Error << "UeberBackend destructor called, removing ourselves from instances, and deleting our backends" << endl); - cleanup(); + + { + auto instances = d_instances.lock(); + [[maybe_unused]] auto end = remove(instances->begin(), instances->end(), this); + instances->resize(instances->size() - 1); + } + + backends.clear(); } // this handle is more magic than most @@ -747,7 +739,7 @@ void UeberBackend::lookup(const QType& qtype, const DNSName& qname, int zoneId, // cout<<"UeberBackend::lookup("<lookup(d_handle.qtype, d_handle.qname, d_handle.zoneId, d_handle.pkt_p); + (d_handle.d_hinterBackend = backends[d_handle.i++].get())->lookup(d_handle.qtype, d_handle.qname, d_handle.zoneId, d_handle.pkt_p); ++(*s_backendQueries); } else if (cacheResult == CacheResult::NegativeMatch) { @@ -815,7 +807,7 @@ bool UeberBackend::get(DNSZoneRecord& resourceRecord) // bool UeberBackend::setTSIGKey(const DNSName& name, const DNSName& algorithm, const string& content) { - for (auto* backend : backends) { + for (auto& backend : backends) { if (backend->setTSIGKey(name, algorithm, content)) { return true; } @@ -828,7 +820,7 @@ bool UeberBackend::getTSIGKey(const DNSName& name, DNSName& algorithm, string& c algorithm.clear(); content.clear(); - for (auto* backend : backends) { + for (auto& backend : backends) { if (backend->getTSIGKey(name, algorithm, content)) { break; } @@ -840,7 +832,7 @@ bool UeberBackend::getTSIGKeys(std::vector& keys) { keys.clear(); - for (auto* backend : backends) { + for (auto& backend : backends) { if (backend->getTSIGKeys(keys)) { return true; } @@ -850,7 +842,7 @@ bool UeberBackend::getTSIGKeys(std::vector& keys) bool UeberBackend::deleteTSIGKey(const DNSName& name) { - for (auto* backend : backends) { + for (auto& backend : backends) { if (backend->deleteTSIGKey(name)) { return true; } @@ -904,7 +896,7 @@ bool UeberBackend::handle::get(DNSZoneRecord& record) DLOG(g_log << "Backend #" << i << " of " << parent->backends.size() << " out of answers, taking next" << endl); - d_hinterBackend = parent->backends[i++]; + d_hinterBackend = parent->backends[i++].get(); d_hinterBackend->lookup(qtype, qname, zoneId, pkt_p); ++(*s_backendQueries); } diff --git a/pdns/ueberbackend.hh b/pdns/ueberbackend.hh index 07c24310b7..d6a0323e2a 100644 --- a/pdns/ueberbackend.hh +++ b/pdns/ueberbackend.hh @@ -65,9 +65,7 @@ public: /** This contains all registered backends. The DynListener modifies this list for us when new modules are loaded */ - vector backends; - - void cleanup(); + vector> backends; //! the very magic handle for UeberBackend questions class handle -- 2.47.2