diff --git a/src/lib/datasrc/memory/domaintree.h b/src/lib/datasrc/memory/domaintree.h index 20f46936e7..7e68f53bf0 100644 --- a/src/lib/datasrc/memory/domaintree.h +++ b/src/lib/datasrc/memory/domaintree.h @@ -1080,55 +1080,25 @@ public: /// of it. In that case, node parameter is left intact. //@{ - /// \brief Simple find. + /// \brief Simple find /// /// Acts as described in the \ref find section. - Result find(const isc::dns::Name& name, - DomainTreeNode** node) const { - DomainTreeNodeChain node_path; - const isc::dns::LabelSequence ls(name); - return (find(ls, node, node_path, NULL, NULL)); - } - - /// \brief Simple find returning immutable node. - /// - /// Acts as described in the \ref find section, but returns immutable node - /// pointer. Result find(const isc::dns::Name& name, const DomainTreeNode** node) const { DomainTreeNodeChain node_path; - DomainTreeNode *target_node = NULL; const isc::dns::LabelSequence ls(name); - Result ret = (find(ls, &target_node, node_path, NULL, NULL)); - if (ret != NOTFOUND) { - *node = target_node; - } + Result ret = (find(ls, node, node_path, NULL, NULL)); return (ret); } /// \brief Simple find, with node_path tracking /// /// Acts as described in the \ref find section. - Result find(const isc::dns::Name& name, DomainTreeNode** node, - DomainTreeNodeChain& node_path) const - { - const isc::dns::LabelSequence ls(name); - return (find(ls, node, node_path, NULL, NULL)); - } - - /// \brief Simple find returning immutable node, with node_path tracking - /// - /// Acts as described in the \ref find section, but returns immutable node - /// pointer. Result find(const isc::dns::Name& name, const DomainTreeNode** node, DomainTreeNodeChain& node_path) const { - DomainTreeNode *target_node = NULL; const isc::dns::LabelSequence ls(name); - Result ret = (find(ls, &target_node, node_path, NULL, NULL)); - if (ret != NOTFOUND) { - *node = target_node; - } + Result ret = (find(ls, node, node_path, NULL, NULL)); return (ret); } @@ -1143,13 +1113,9 @@ public: bool (*callback)(const DomainTreeNode&, CBARG), CBARG callback_arg) const { - DomainTreeNode* target_node = NULL; const isc::dns::LabelSequence ls(name); - Result ret = find(ls, &target_node, node_path, callback, + Result ret = find(ls, node, node_path, callback, callback_arg); - if (ret != NOTFOUND) { - *node = target_node; - } return (ret); } @@ -1229,30 +1195,10 @@ public: /// \c true, it returns immediately with the current node. template Result find(const isc::dns::LabelSequence& target_labels_orig, - DomainTreeNode** node, - DomainTreeNodeChain& node_path, - bool (*callback)(const DomainTreeNode&, CBARG), - CBARG callback_arg) const; - - /// \brief Simple find returning immutable node. - /// - /// Acts as described in the \ref find section, but returns immutable - /// node pointer. - template - Result find(const isc::dns::LabelSequence& target_labels, const DomainTreeNode** node, DomainTreeNodeChain& node_path, bool (*callback)(const DomainTreeNode&, CBARG), - CBARG callback_arg) const - { - DomainTreeNode* target_node = NULL; - Result ret = find(target_labels, &target_node, node_path, - callback, callback_arg); - if (ret != NOTFOUND) { - *node = target_node; - } - return (ret); - } + CBARG callback_arg) const; //@} /// \brief return the next bigger node in DNSSEC order from a given node @@ -1515,7 +1461,7 @@ template template typename DomainTree::Result DomainTree::find(const isc::dns::LabelSequence& target_labels_orig, - DomainTreeNode** target, + const DomainTreeNode** target, DomainTreeNodeChain& node_path, bool (*callback)(const DomainTreeNode&, CBARG), CBARG callback_arg) const @@ -1526,11 +1472,11 @@ DomainTree::find(const isc::dns::LabelSequence& target_labels_orig, " and label sequence"); } - DomainTreeNode* node; + const DomainTreeNode* node; if (!node_path.isEmpty()) { // Get the top node in the node chain - node = const_cast*>(node_path.top()); + node = node_path.top(); // Start searching from its down pointer node = node->getDown(); } else { diff --git a/src/lib/datasrc/memory/memory_client.cc b/src/lib/datasrc/memory/memory_client.cc index 5f6f510306..16096114fc 100644 --- a/src/lib/datasrc/memory/memory_client.cc +++ b/src/lib/datasrc/memory/memory_client.cc @@ -627,22 +627,20 @@ InMemoryClient::InMemoryClientImpl::load( // node must point to a valid node now assert(node != NULL); - std::string* tstr = node->setData(new std::string(filename)); + const std::string* tstr = node->setData(new std::string(filename)); delete tstr; - ZoneTable::AddResult result(zone_table_->addZone(mem_sgmt_, rrclass_, - zone_name)); + const ZoneTable::AddResult result(zone_table_->addZone(mem_sgmt_, rrclass_, + zone_name, + holder.release())); if (result.code == result::SUCCESS) { // Only increment the zone count if the zone doesn't already // exist. ++zone_count_; } - - ZoneTable::FindResult fr(zone_table_->setZoneData(zone_name, - holder.release())); - assert(fr.code == result::SUCCESS); - if (fr.zone_data != NULL) { - ZoneData::destroy(mem_sgmt_, fr.zone_data, rrclass_); + // Destroy the old instance of the zone if there was any + if (result.zone_data != NULL) { + ZoneData::destroy(mem_sgmt_, result.zone_data, rrclass_); } return (result.code); @@ -732,9 +730,9 @@ InMemoryClient::load(const isc::dns::Name& zone_name, const std::string InMemoryClient::getFileName(const isc::dns::Name& zone_name) const { - FileNameNode* node(NULL); - FileNameTree::Result result = impl_->file_name_tree_->find(zone_name, - &node); + const FileNameNode* node(NULL); + const FileNameTree::Result result = impl_->file_name_tree_->find(zone_name, + &node); if (result == FileNameTree::EXACTMATCH) { return (*node->getData()); } else { @@ -742,24 +740,6 @@ InMemoryClient::getFileName(const isc::dns::Name& zone_name) const { } } -result::Result -InMemoryClient::add(const isc::dns::Name& zone_name, - const ConstRRsetPtr& rrset) -{ - const ZoneTable::FindResult result = - impl_->zone_table_->findZone(zone_name); - if (result.code != result::SUCCESS) { - isc_throw(DataSourceError, "No such zone: " + zone_name.toText()); - } - - const ConstRRsetPtr sig_rrset = - rrset ? rrset->getRRsig() : ConstRRsetPtr(); - impl_->add(rrset, sig_rrset, zone_name, *result.zone_data); - - // add() doesn't allow duplicate add, so we always return SUCCESS. - return (result::SUCCESS); -} - namespace { class MemoryIterator : public ZoneIterator { diff --git a/src/lib/datasrc/memory/memory_client.h b/src/lib/datasrc/memory/memory_client.h index 330d62e15f..c37ad53c0b 100644 --- a/src/lib/datasrc/memory/memory_client.h +++ b/src/lib/datasrc/memory/memory_client.h @@ -139,35 +139,6 @@ public: /// zone from a file before. const std::string getFileName(const isc::dns::Name& zone_name) const; - /// \brief Inserts an rrset into the zone. - /// - /// It puts another RRset into the zone. - /// - /// In the current implementation, this method doesn't allow an existing - /// RRset to be updated or overridden. So the caller must make sure that - /// all RRs of the same type and name must be given in the form of a - /// single RRset. The current implementation will also require that - /// when an RRSIG is added, the RRset to be covered has already been - /// added. These restrictions are probably too strict when this data - /// source accepts various forms of input, so they should be revisited - /// later. - /// - /// Except for NullRRset and OutOfZone, this method does not guarantee - /// strong exception safety (it is currently not needed, if it is needed - /// in future, it should be implemented). - /// - /// \throw NullRRset \c rrset is a NULL pointer. - /// \throw OutOfZone The owner name of \c rrset is outside of the - /// origin of the zone. - /// \throw AddError Other general errors. - /// \throw Others This method might throw standard allocation exceptions. - /// - /// \param rrset The set to add. - /// \return SUCCESS or EXIST (if an rrset for given name and type already - /// exists). - result::Result add(const isc::dns::Name& zone_name, - const isc::dns::ConstRRsetPtr& rrset); - /// \brief RRset is NULL exception. /// /// This is thrown if the provided RRset parameter is NULL. diff --git a/src/lib/datasrc/memory/zone_finder.cc b/src/lib/datasrc/memory/zone_finder.cc index 86dd87428c..11188a038b 100644 --- a/src/lib/datasrc/memory/zone_finder.cc +++ b/src/lib/datasrc/memory/zone_finder.cc @@ -428,7 +428,7 @@ FindNodeResult findNode(const ZoneData& zone_data, ZoneFinder::FindOptions options, bool out_of_zone_ok = false) { - ZoneNode* node = NULL; + const ZoneNode* node = NULL; FindState state((options & ZoneFinder::FIND_GLUE_OK) != 0); const ZoneTree& tree(zone_data.getZoneTree()); diff --git a/src/lib/datasrc/memory/zone_table.cc b/src/lib/datasrc/memory/zone_table.cc index a74a61dd27..836b020088 100644 --- a/src/lib/datasrc/memory/zone_table.cc +++ b/src/lib/datasrc/memory/zone_table.cc @@ -69,17 +69,13 @@ ZoneTable::destroy(util::MemorySegment& mem_sgmt, ZoneTable* ztable, ZoneTable::AddResult ZoneTable::addZone(util::MemorySegment& mem_sgmt, RRClass zone_class, - const Name& zone_name) + const Name& zone_name, ZoneData* content) { - // Create a new ZoneData instance first. If the specified name already - // exists in the table, the new data will soon be destroyed, but we want - // to make sure if this allocation fails the tree won't be changed to - // provide as strong guarantee as possible. In practice, we generally - // expect the caller tries to add a zone only when it's a new one, so - // this should be a minor concern. - SegmentObjectHolder holder( - mem_sgmt, ZoneData::create(mem_sgmt, zone_name), zone_class); - + if (content == NULL) { + isc_throw(isc::BadValue, "Zone content must not be NULL"); + } + SegmentObjectHolder holder(mem_sgmt, content, + zone_class); // Get the node where we put the zone ZoneTableNode* node(NULL); switch (zones_->insert(mem_sgmt, zone_name, &node)) { @@ -94,18 +90,18 @@ ZoneTable::addZone(util::MemorySegment& mem_sgmt, RRClass zone_class, // Can Not Happen assert(node != NULL); - // Is it empty? We either just created it or it might be nonterminal - if (node->isEmpty()) { - node->setData(holder.get()); - return (AddResult(result::SUCCESS, holder.release())); - } else { // There's something there already - return (AddResult(result::EXIST, node->getData())); + // We can release now, setData never throws + ZoneData* old = node->setData(holder.release()); + if (old != NULL) { + return (AddResult(result::EXIST, old)); + } else { + return (AddResult(result::SUCCESS, NULL)); } } ZoneTable::FindResult ZoneTable::findZone(const Name& name) const { - ZoneTableNode* node(NULL); + const ZoneTableNode* node(NULL); result::Result my_result; // Translate the return codes @@ -132,20 +128,6 @@ ZoneTable::findZone(const Name& name) const { return (FindResult(my_result, node->getData())); } -ZoneTable::FindResult -ZoneTable::setZoneData(const Name& name, ZoneData* data) -{ - ZoneTableNode* node(NULL); - - ZoneTableTree::Result result(zones_->find(name, &node)); - - if (result != ZoneTableTree::EXACTMATCH) { - return (FindResult(result::NOTFOUND, NULL)); - } else { - return (FindResult(result::SUCCESS, node->setData(data))); - } -} - } // end of namespace memory } // end of namespace datasrc } // end of namespace isc diff --git a/src/lib/datasrc/memory/zone_table.h b/src/lib/datasrc/memory/zone_table.h index 8ad6213724..024558eb30 100644 --- a/src/lib/datasrc/memory/zone_table.h +++ b/src/lib/datasrc/memory/zone_table.h @@ -74,23 +74,23 @@ private: typedef DomainTreeNode ZoneTableNode; public: - /// \brief Result data of addZone() method. - struct AddResult { - AddResult(result::Result param_code, ZoneData* param_zone_data) : - code(param_code), zone_data(param_zone_data) - {} - const result::Result code; - ZoneData* const zone_data; - }; + /// \brief Result data of addZone() method. + struct AddResult { + AddResult(result::Result param_code, ZoneData* param_zone_data) : + code(param_code), zone_data(param_zone_data) + {} + const result::Result code; + ZoneData* const zone_data; + }; /// \brief Result data of findZone() method. struct FindResult { FindResult(result::Result param_code, - ZoneData* param_zone_data) : + const ZoneData* param_zone_data) : code(param_code), zone_data(param_zone_data) {} const result::Result code; - ZoneData* const zone_data; + const ZoneData* const zone_data; }; private: @@ -140,30 +140,29 @@ public: /// Add a new zone to the \c ZoneTable. /// - /// This method creates a new \c ZoneData for the given zone name and - /// holds it in the internal table. The newly created zone data will be - /// returned via the \c zone_data member of the return value. If the given - /// zone name already exists in the table, a new data object won't be - /// created; instead, the existing corresponding data will be returned. - /// - /// The zone table keeps the ownership of the created zone data; the - /// caller must not try to destroy it directly. (We'll eventually - /// add an interface to delete specific zone data from the table). + /// This method adds a given zone data to the internal table. /// /// \throw std::bad_alloc Internal resource allocation fails. /// /// \param mem_sgmt The \c MemorySegment to allocate zone data to be - /// created. It must be the same segment that was used to create - /// the zone table at the time of create(). + /// created. It must be the same segment that was used to create + /// the zone table at the time of create(). /// \param zone_name The name of the zone to be added. /// \param zone_class The RR class of the zone. It must be the RR class - /// that is supposed to be associated to the zone table. + /// that is supposed to be associated to the zone table. + /// \param content This one should hold the zone content (the ZoneData). + /// The ownership is passed onto the zone table. Must not be null. + /// Must correspond to the name and class and must be allocated from + /// mem_sgmt. /// \return \c result::SUCCESS If the zone is successfully - /// added to the zone table. - /// \return \c result::EXIST The zone table already contains - /// zone of the same origin. - AddResult addZone(util::MemorySegment& mem_sgmt, dns::RRClass zone_class, - const dns::Name& zone_name); + /// added to the zone table. + /// \return \c result::EXIST The zone table already contained + /// zone of the same origin. The old data is replaced and returned + /// inside the result. + AddResult addZone(util::MemorySegment& mem_sgmt, + dns::RRClass zone_class, + const dns::Name& zone_name, + ZoneData* content); /// Find a zone that best matches the given name in the \c ZoneTable. /// @@ -185,16 +184,6 @@ public: /// \return A \c FindResult object enclosing the search result (see above). FindResult findZone(const isc::dns::Name& name) const; - /// Override the ZoneData for a node (zone) in the zone tree. - /// - /// \throw none - /// - /// \param name A domain name for which the zone data is set. - /// \param data The new zone data to set. - /// \return A \c FindResult object containing the old data if the - /// zone was found. - FindResult setZoneData(const isc::dns::Name& name, ZoneData* data); - private: boost::interprocess::offset_ptr zones_; }; diff --git a/src/lib/datasrc/tests/memory/domaintree_unittest.cc b/src/lib/datasrc/tests/memory/domaintree_unittest.cc index cb16e02c79..45e256a0b4 100644 --- a/src/lib/datasrc/tests/memory/domaintree_unittest.cc +++ b/src/lib/datasrc/tests/memory/domaintree_unittest.cc @@ -256,8 +256,8 @@ TEST_F(DomainTreeTest, subTreeRoot) { // "g.h" is not a subtree root EXPECT_EQ(TestDomainTree::EXACTMATCH, - dtree_expose_empty_node.find(Name("g.h"), &dtnode)); - EXPECT_FALSE(dtnode->getFlag(TestDomainTreeNode::FLAG_SUBTREE_ROOT)); + dtree_expose_empty_node.find(Name("g.h"), &cdtnode)); + EXPECT_FALSE(cdtnode->getFlag(TestDomainTreeNode::FLAG_SUBTREE_ROOT)); // fission the node "g.h" EXPECT_EQ(TestDomainTree::ALREADYEXISTS, @@ -270,8 +270,8 @@ TEST_F(DomainTreeTest, subTreeRoot) { // "g.h" should be a subtree root now. EXPECT_EQ(TestDomainTree::EXACTMATCH, - dtree_expose_empty_node.find(Name("g.h"), &dtnode)); - EXPECT_TRUE(dtnode->getFlag(TestDomainTreeNode::FLAG_SUBTREE_ROOT)); + dtree_expose_empty_node.find(Name("g.h"), &cdtnode)); + EXPECT_TRUE(cdtnode->getFlag(TestDomainTreeNode::FLAG_SUBTREE_ROOT)); } TEST_F(DomainTreeTest, additionalNodeFission) { @@ -286,8 +286,8 @@ TEST_F(DomainTreeTest, additionalNodeFission) { // "t.0" is not a subtree root EXPECT_EQ(TestDomainTree::EXACTMATCH, - dtree_expose_empty_node.find(Name("t.0"), &dtnode)); - EXPECT_FALSE(dtnode->getFlag(TestDomainTreeNode::FLAG_SUBTREE_ROOT)); + dtree_expose_empty_node.find(Name("t.0"), &cdtnode)); + EXPECT_FALSE(cdtnode->getFlag(TestDomainTreeNode::FLAG_SUBTREE_ROOT)); // fission the node "t.0" EXPECT_EQ(TestDomainTree::ALREADYEXISTS, @@ -300,8 +300,8 @@ TEST_F(DomainTreeTest, additionalNodeFission) { // "t.0" should be a subtree root now. EXPECT_EQ(TestDomainTree::EXACTMATCH, - dtree_expose_empty_node.find(Name("t.0"), &dtnode)); - EXPECT_TRUE(dtnode->getFlag(TestDomainTreeNode::FLAG_SUBTREE_ROOT)); + dtree_expose_empty_node.find(Name("t.0"), &cdtnode)); + EXPECT_TRUE(cdtnode->getFlag(TestDomainTreeNode::FLAG_SUBTREE_ROOT)); } TEST_F(DomainTreeTest, findName) { @@ -328,10 +328,10 @@ TEST_F(DomainTreeTest, findName) { EXPECT_EQ(TestDomainTree::PARTIALMATCH, dtree_expose_empty_node.find(Name("m.d.e.f"), &cdtnode)); - // find dtnode + // find cdtnode EXPECT_EQ(TestDomainTree::EXACTMATCH, dtree.find(Name("q.w.y.d.e.f"), - &dtnode)); - EXPECT_EQ(Name("q"), dtnode->getName()); + &cdtnode)); + EXPECT_EQ(Name("q"), cdtnode->getName()); } TEST_F(DomainTreeTest, findError) { @@ -411,11 +411,12 @@ performCallbackTest(TestDomainTree& dtree, Name("example"), &parentdtnode)); // the child/parent nodes shouldn't "inherit" the callback flag. - // "dtnode" may be invalid due to the insertion, so we need to re-find - // it. + // "dtnode" should still validly point to "callback.example", but we + // explicitly confirm it. EXPECT_EQ(TestDomainTree::EXACTMATCH, dtree.find(Name("callback.example"), - &dtnode)); - EXPECT_TRUE(dtnode->getFlag(TestDomainTreeNode::FLAG_CALLBACK)); + &cdtnode)); + ASSERT_EQ(dtnode, cdtnode); + EXPECT_TRUE(cdtnode->getFlag(TestDomainTreeNode::FLAG_CALLBACK)); EXPECT_FALSE(subdtnode->getFlag(TestDomainTreeNode::FLAG_CALLBACK)); EXPECT_FALSE(parentdtnode->getFlag(TestDomainTreeNode::FLAG_CALLBACK)); diff --git a/src/lib/datasrc/tests/memory/memory_client_unittest.cc b/src/lib/datasrc/tests/memory/memory_client_unittest.cc index 58979a4351..004f663c56 100644 --- a/src/lib/datasrc/tests/memory/memory_client_unittest.cc +++ b/src/lib/datasrc/tests/memory/memory_client_unittest.cc @@ -543,29 +543,6 @@ TEST_F(MemoryClientTest, loadRRSIGs) { EXPECT_EQ(1, client_->getZoneCount()); } -TEST_F(MemoryClientTest, loadRRSIGsRdataMixedCoveredTypes) { - client_->load(Name("example.org"), - TEST_DATA_DIR "/example.org-rrsigs.zone"); - - RRsetPtr rrset(new RRset(Name("example.org"), - RRClass::IN(), RRType::A(), RRTTL(3600))); - rrset->addRdata(in::A("192.0.2.1")); - rrset->addRdata(in::A("192.0.2.2")); - - RRsetPtr rrsig(new RRset(Name("example.org"), zclass_, - RRType::RRSIG(), RRTTL(300))); - rrsig->addRdata(generic::RRSIG("A 5 3 3600 20000101000000 20000201000000 " - "12345 example.org. FAKEFAKEFAKE")); - rrsig->addRdata(generic::RRSIG("NS 5 3 3600 20000101000000 20000201000000 " - "54321 example.org. FAKEFAKEFAKEFAKE")); - rrset->addRRsig(rrsig); - - EXPECT_THROW(client_->add(Name("example.org"), rrset), - InMemoryClient::AddError); - - // Teardown checks for memory segment leaks -} - TEST_F(MemoryClientTest, getZoneCount) { EXPECT_EQ(0, client_->getZoneCount()); client_->load(Name("example.org"), TEST_DATA_DIR "/example.org-empty.zone"); @@ -655,75 +632,6 @@ TEST_F(MemoryClientTest, getIteratorGetSOAThrowsNotImplemented) { EXPECT_THROW(iterator->getSOA(), isc::NotImplemented); } -TEST_F(MemoryClientTest, addRRsetToNonExistentZoneThrows) { - // The zone "example.org" doesn't exist, so we can't add an RRset to - // it. - RRsetPtr rrset_a(new RRset(Name("example.org"), RRClass::IN(), RRType::A(), - RRTTL(300))); - rrset_a->addRdata(rdata::in::A("192.0.2.1")); - EXPECT_THROW(client_->add(Name("example.org"), rrset_a), DataSourceError); -} - -TEST_F(MemoryClientTest, addOutOfZoneThrows) { - // Out of zone names should throw. - client_->load(Name("example.org"), - TEST_DATA_DIR "/example.org-empty.zone"); - - RRsetPtr rrset_a(new RRset(Name("a.example.com"), - RRClass::IN(), RRType::A(), RRTTL(300))); - rrset_a->addRdata(rdata::in::A("192.0.2.1")); - - EXPECT_THROW(client_->add(Name("example.org"), rrset_a), - OutOfZone); - // Teardown checks for memory segment leaks -} - -TEST_F(MemoryClientTest, addNullRRsetThrows) { - client_->load(Name("example.org"), - TEST_DATA_DIR "/example.org-rrsigs.zone"); - - EXPECT_THROW(client_->add(Name("example.org"), ConstRRsetPtr()), - InMemoryClient::NullRRset); - - // Teardown checks for memory segment leaks -} - -TEST_F(MemoryClientTest, addEmptyRRsetThrows) { - client_->load(Name("example.org"), - TEST_DATA_DIR "/example.org-rrsigs.zone"); - - RRsetPtr rrset_a(new RRset(Name("example.org"), RRClass::IN(), RRType::A(), - RRTTL(300))); - EXPECT_THROW(client_->add(Name("example.org"), rrset_a), - InMemoryClient::AddError); - - // Teardown checks for memory segment leaks -} - -TEST_F(MemoryClientTest, add) { - client_->load(Name("example.org"), TEST_DATA_DIR "/example.org-empty.zone"); - - // Add another RRset - RRsetPtr rrset_a(new RRset(Name("example.org"), RRClass::IN(), RRType::A(), - RRTTL(300))); - rrset_a->addRdata(rdata::in::A("192.0.2.1")); - client_->add(Name("example.org"), rrset_a); - - ZoneIteratorPtr iterator(client_->getIterator(Name("example.org"))); - - // First we have the SOA - ConstRRsetPtr rrset(iterator->getNextRRset()); - EXPECT_TRUE(rrset); - EXPECT_EQ(RRType::A(), rrset->getType()); - - rrset = iterator->getNextRRset(); - EXPECT_TRUE(rrset); - EXPECT_EQ(RRType::SOA(), rrset->getType()); - - // There's nothing else in this zone - EXPECT_EQ(ConstRRsetPtr(), iterator->getNextRRset()); -} - TEST_F(MemoryClientTest, findZoneData) { client_->load(Name("example.org"), TEST_DATA_DIR "/example.org-rrsigs.zone"); @@ -774,4 +682,16 @@ TEST_F(MemoryClientTest, getJournalReaderNotImplemented) { EXPECT_THROW(client_->getJournalReader(Name("."), 0, 0), isc::NotImplemented); } + +// TODO (upon merge of #2268): Re-add (and modify not to need +// InMemoryClient::add) the tests removed in +// 7a628baa1a158b5837d6f383e10b30542d2ac59b. Maybe some of them +// are really not needed. +// +// * MemoryClientTest::loadRRSIGsRdataMixedCoveredTypes +// * MemoryClientTest::addRRsetToNonExistentZoneThrows +// * MemoryClientTest::addOutOfZoneThrows +// * MemoryClientTest::addNullRRsetThrows +// * MemoryClientTest::addEmptyRRsetThrows +// * MemoryClientTest::add } diff --git a/src/lib/datasrc/tests/memory/zone_data_unittest.cc b/src/lib/datasrc/tests/memory/zone_data_unittest.cc index d15fe8beb2..3c28cec229 100644 --- a/src/lib/datasrc/tests/memory/zone_data_unittest.cc +++ b/src/lib/datasrc/tests/memory/zone_data_unittest.cc @@ -108,7 +108,7 @@ void checkFindRdataSet(const ZoneTree& tree, const Name& name, RRType type, const RdataSet* expected_set) { - ZoneNode* node = NULL; + const ZoneNode* node = NULL; tree.find(name, &node); ASSERT_NE(static_cast(NULL), node); EXPECT_EQ(expected_set, RdataSet::find(node->getData(), type)); diff --git a/src/lib/datasrc/tests/memory/zone_table_unittest.cc b/src/lib/datasrc/tests/memory/zone_table_unittest.cc index 359df4923f..9cf1b3420c 100644 --- a/src/lib/datasrc/tests/memory/zone_table_unittest.cc +++ b/src/lib/datasrc/tests/memory/zone_table_unittest.cc @@ -22,6 +22,7 @@ #include #include #include +#include #include @@ -30,6 +31,7 @@ using namespace isc::dns; using namespace isc::datasrc; using namespace isc::datasrc::memory; +using namespace isc::datasrc::memory::detail; namespace { // Memory segment specified for tests. It normally behaves like a "local" @@ -87,46 +89,89 @@ TEST_F(ZoneTableTest, create) { } TEST_F(ZoneTableTest, addZone) { + // It doesn't accept empty (NULL) zones + EXPECT_THROW(zone_table->addZone(mem_sgmt_, zclass_, zname1, NULL), + isc::BadValue); + + SegmentObjectHolder holder1( + mem_sgmt_, ZoneData::create(mem_sgmt_, zname1), zclass_); + const ZoneData* data1(holder1.get()); // Normal successful case. - const ZoneTable::AddResult result1 = - zone_table->addZone(mem_sgmt_, zclass_, zname1); + const ZoneTable::AddResult result1(zone_table->addZone(mem_sgmt_, zclass_, + zname1, + holder1.release())); EXPECT_EQ(result::SUCCESS, result1.code); + EXPECT_EQ(NULL, result1.zone_data); + // It got released by it + EXPECT_EQ(NULL, holder1.get()); // Duplicate add doesn't replace the existing data. - EXPECT_EQ(result::EXIST, zone_table->addZone(mem_sgmt_, zclass_, - zname1).code); - EXPECT_EQ(result1.zone_data, - zone_table->addZone(mem_sgmt_, zclass_, zname1).zone_data); + SegmentObjectHolder holder2( + mem_sgmt_, ZoneData::create(mem_sgmt_, zname1), zclass_); + const ZoneTable::AddResult result2(zone_table->addZone(mem_sgmt_, zclass_, + zname1, + holder2.release())); + EXPECT_EQ(result::EXIST, result2.code); + // The old one gets out + EXPECT_EQ(data1, result2.zone_data); + // It releases this one even when we replace the old zone + EXPECT_EQ(NULL, holder2.get()); + // We need to release the old one manually + ZoneData::destroy(mem_sgmt_, result2.zone_data, zclass_); + + SegmentObjectHolder holder3( + mem_sgmt_, ZoneData::create(mem_sgmt_, Name("EXAMPLE.COM")), + zclass_); // names are compared in a case insensitive manner. - EXPECT_EQ(result::EXIST, zone_table->addZone(mem_sgmt_, zclass_, - Name("EXAMPLE.COM")).code); + const ZoneTable::AddResult result3(zone_table->addZone(mem_sgmt_, zclass_, + Name("EXAMPLE.COM"), + holder3.release())); + EXPECT_EQ(result::EXIST, result3.code); + ZoneData::destroy(mem_sgmt_, result3.zone_data, zclass_); // Add some more different ones. Should just succeed. + SegmentObjectHolder holder4( + mem_sgmt_, ZoneData::create(mem_sgmt_, zname2), zclass_); EXPECT_EQ(result::SUCCESS, - zone_table->addZone(mem_sgmt_, zclass_, zname2).code); + zone_table->addZone(mem_sgmt_, zclass_, zname2, + holder4.release()).code); + SegmentObjectHolder holder5( + mem_sgmt_, ZoneData::create(mem_sgmt_, zname3), zclass_); EXPECT_EQ(result::SUCCESS, - zone_table->addZone(mem_sgmt_, zclass_, zname3).code); + zone_table->addZone(mem_sgmt_, zclass_, zname3, + holder5.release()).code); // Have the memory segment throw an exception in extending the internal // tree. It still shouldn't cause memory leak (which would be detected // in TearDown()). - mem_sgmt_.setThrowCount(2); - EXPECT_THROW(zone_table->addZone(mem_sgmt_, zclass_, Name("example.org")), + SegmentObjectHolder holder6( + mem_sgmt_, ZoneData::create(mem_sgmt_, Name("example.org")), zclass_); + mem_sgmt_.setThrowCount(1); + EXPECT_THROW(zone_table->addZone(mem_sgmt_, zclass_, Name("example.org"), + holder6.release()), std::bad_alloc); } TEST_F(ZoneTableTest, findZone) { - const ZoneTable::AddResult add_result1 = - zone_table->addZone(mem_sgmt_, zclass_, zname1); - EXPECT_EQ(result::SUCCESS, add_result1.code); + SegmentObjectHolder holder1( + mem_sgmt_, ZoneData::create(mem_sgmt_, zname1), zclass_); + ZoneData* zone_data = holder1.get(); + EXPECT_EQ(result::SUCCESS, zone_table->addZone(mem_sgmt_, zclass_, zname1, + holder1.release()).code); + SegmentObjectHolder holder2( + mem_sgmt_, ZoneData::create(mem_sgmt_, zname2), zclass_); EXPECT_EQ(result::SUCCESS, - zone_table->addZone(mem_sgmt_, zclass_, zname2).code); + zone_table->addZone(mem_sgmt_, zclass_, zname2, + holder2.release()).code); + SegmentObjectHolder holder3( + mem_sgmt_, ZoneData::create(mem_sgmt_, zname3), zclass_); EXPECT_EQ(result::SUCCESS, - zone_table->addZone(mem_sgmt_, zclass_, zname3).code); + zone_table->addZone(mem_sgmt_, zclass_, zname3, + holder3.release()).code); const ZoneTable::FindResult find_result1 = zone_table->findZone(Name("example.com")); EXPECT_EQ(result::SUCCESS, find_result1.code); - EXPECT_EQ(add_result1.zone_data, find_result1.zone_data); + EXPECT_EQ(zone_data, find_result1.zone_data); EXPECT_EQ(result::NOTFOUND, zone_table->findZone(Name("example.org")).code); @@ -137,14 +182,17 @@ TEST_F(ZoneTableTest, findZone) { // and the code should be PARTIALMATCH. EXPECT_EQ(result::PARTIALMATCH, zone_table->findZone(Name("www.example.com")).code); - EXPECT_EQ(add_result1.zone_data, + EXPECT_EQ(zone_data, zone_table->findZone(Name("www.example.com")).zone_data); // make sure the partial match is indeed the longest match by adding // a zone with a shorter origin and query again. + SegmentObjectHolder holder4( + mem_sgmt_, ZoneData::create(mem_sgmt_, Name("com")), zclass_); EXPECT_EQ(result::SUCCESS, zone_table->addZone(mem_sgmt_, zclass_, - Name("com")).code); - EXPECT_EQ(add_result1.zone_data, + Name("com"), + holder4.release()).code); + EXPECT_EQ(zone_data, zone_table->findZone(Name("www.example.com")).zone_data); } }