From 4babe763de5b20437e32829024f25a1b902df82f Mon Sep 17 00:00:00 2001 From: Michal 'vorner' Vaner Date: Tue, 2 Oct 2012 20:22:23 +0200 Subject: [PATCH 01/13] [2292] Parametrize constness of the chain --- src/lib/datasrc/memory/domaintree.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/lib/datasrc/memory/domaintree.h b/src/lib/datasrc/memory/domaintree.h index 20f46936e7..4f1d94aca7 100644 --- a/src/lib/datasrc/memory/domaintree.h +++ b/src/lib/datasrc/memory/domaintree.h @@ -684,7 +684,7 @@ DomainTreeNode::predecessor() const { /// DomainTree. /// This is the reason why manipulation methods such as \c push() and \c pop() /// are private (and not shown in the doxygen document). -template +template > class DomainTreeNodeChain { /// DomainTreeNodeChain is initialized by DomainTree, only DomainTree has /// knowledge to manipulate it. @@ -817,7 +817,7 @@ private: /// root node of DomainTree /// /// \exception None - const DomainTreeNode* top() const { + NodeType* top() const { assert(!isEmpty()); return (nodes_[level_count_ - 1]); } @@ -840,7 +840,7 @@ private: /// otherwise the node should be the root node of DomainTree. /// /// \exception None - void push(const DomainTreeNode* node) { + void push(NodeType* node) { assert(level_count_ < RBT_MAX_LEVEL); nodes_[level_count_++] = node; } @@ -852,7 +852,7 @@ private: const static int RBT_MAX_LEVEL = isc::dns::Name::MAX_LABELS; size_t level_count_; - const DomainTreeNode* nodes_[RBT_MAX_LEVEL]; + NodeType* nodes_[RBT_MAX_LEVEL]; const DomainTreeNode* last_compared_; isc::dns::NameComparisonResult last_comparison_; }; From 5c74085e2f4c16655e77e1bcbc308d4fb0a694f8 Mon Sep 17 00:00:00 2001 From: Michal 'vorner' Vaner Date: Tue, 2 Oct 2012 21:05:33 +0200 Subject: [PATCH 02/13] [2292] Get rid of the const_cast It was needed when extracting data from a domain tree chain. The chain now can hold mutable pointers too, so we use that (and some amount of template bureaucracy) to avoid the cast. While the interface changed (on the core find function, it is not possible to pass const node chain and have a mutable node get out), it doesn't seem to influence the current code. Also, it is a private interface anyway, so it should be safe. --- src/lib/datasrc/memory/domaintree.h | 46 ++++++++++++++++----------- src/lib/datasrc/memory/zone_finder.cc | 2 +- 2 files changed, 28 insertions(+), 20 deletions(-) diff --git a/src/lib/datasrc/memory/domaintree.h b/src/lib/datasrc/memory/domaintree.h index 4f1d94aca7..e54d8eaaed 100644 --- a/src/lib/datasrc/memory/domaintree.h +++ b/src/lib/datasrc/memory/domaintree.h @@ -1085,9 +1085,10 @@ public: /// Acts as described in the \ref find section. Result find(const isc::dns::Name& name, DomainTreeNode** node) const { - DomainTreeNodeChain node_path; + DomainTreeNodeChain > node_path; const isc::dns::LabelSequence ls(name); - return (find(ls, node, node_path, NULL, NULL)); + return (find >(ls, node, node_path, NULL, + NULL)); } /// \brief Simple find returning immutable node. @@ -1097,9 +1098,11 @@ public: Result find(const isc::dns::Name& name, const DomainTreeNode** node) const { DomainTreeNodeChain node_path; - DomainTreeNode *target_node = NULL; + const DomainTreeNode *target_node = NULL; const isc::dns::LabelSequence ls(name); - Result ret = (find(ls, &target_node, node_path, NULL, NULL)); + Result ret = (find >(ls, &target_node, + node_path, NULL, + NULL)); if (ret != NOTFOUND) { *node = target_node; } @@ -1113,7 +1116,8 @@ public: DomainTreeNodeChain& node_path) const { const isc::dns::LabelSequence ls(name); - return (find(ls, node, node_path, NULL, NULL)); + return (find >(ls, node, node_path, + NULL, NULL)); } /// \brief Simple find returning immutable node, with node_path tracking @@ -1123,9 +1127,11 @@ public: Result find(const isc::dns::Name& name, const DomainTreeNode** node, DomainTreeNodeChain& node_path) const { - DomainTreeNode *target_node = NULL; + const DomainTreeNode *target_node = NULL; const isc::dns::LabelSequence ls(name); - Result ret = (find(ls, &target_node, node_path, NULL, NULL)); + Result ret = (find >(ls, &target_node, + node_path, NULL, + NULL)); if (ret != NOTFOUND) { *node = target_node; } @@ -1143,7 +1149,7 @@ public: bool (*callback)(const DomainTreeNode&, CBARG), CBARG callback_arg) const { - DomainTreeNode* target_node = NULL; + const DomainTreeNode* target_node = NULL; const isc::dns::LabelSequence ls(name); Result ret = find(ls, &target_node, node_path, callback, callback_arg); @@ -1227,10 +1233,10 @@ public: /// /// \return As in the description, but in case of callback returning /// \c true, it returns immediately with the current node. - template + template Result find(const isc::dns::LabelSequence& target_labels_orig, - DomainTreeNode** node, - DomainTreeNodeChain& node_path, + NodeType** node, + DomainTreeNodeChain& node_path, bool (*callback)(const DomainTreeNode&, CBARG), CBARG callback_arg) const; @@ -1245,9 +1251,11 @@ public: 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); + const DomainTreeNode* target_node = NULL; + Result ret = find >(target_labels, + &target_node, + node_path, callback, + callback_arg); if (ret != NOTFOUND) { *node = target_node; } @@ -1512,11 +1520,11 @@ DomainTree::deleteHelper(util::MemorySegment& mem_sgmt, } template -template +template typename DomainTree::Result DomainTree::find(const isc::dns::LabelSequence& target_labels_orig, - DomainTreeNode** target, - DomainTreeNodeChain& node_path, + NodeType** target, + DomainTreeNodeChain& node_path, bool (*callback)(const DomainTreeNode&, CBARG), CBARG callback_arg) const { @@ -1526,11 +1534,11 @@ DomainTree::find(const isc::dns::LabelSequence& target_labels_orig, " and label sequence"); } - DomainTreeNode* node; + NodeType* 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/zone_finder.cc b/src/lib/datasrc/memory/zone_finder.cc index 9403c4fec8..31a58d930e 100644 --- a/src/lib/datasrc/memory/zone_finder.cc +++ b/src/lib/datasrc/memory/zone_finder.cc @@ -417,7 +417,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()); From 76243c3ccd7a30484da8478f7457e18fce485492 Mon Sep 17 00:00:00 2001 From: Michal 'vorner' Vaner Date: Sun, 7 Oct 2012 16:13:56 +0200 Subject: [PATCH 03/13] [2292] Remove the setZoneData method Currently, the content of zone is passed to the addZone method. No way to change it later, but the addZone overwrites the old value if called again. This should hopefully allow for removal of mutable find methods from the tree. --- src/lib/datasrc/memory/memory_client.cc | 15 ++--- src/lib/datasrc/memory/zone_table.cc | 41 +++--------- src/lib/datasrc/memory/zone_table.h | 54 ++++++--------- .../tests/memory/zone_table_unittest.cc | 67 +++++++++++++------ 4 files changed, 80 insertions(+), 97 deletions(-) diff --git a/src/lib/datasrc/memory/memory_client.cc b/src/lib/datasrc/memory/memory_client.cc index 5f6f510306..5eaf58b547 100644 --- a/src/lib/datasrc/memory/memory_client.cc +++ b/src/lib/datasrc/memory/memory_client.cc @@ -630,22 +630,15 @@ InMemoryClient::InMemoryClientImpl::load( std::string* tstr = node->setData(new std::string(filename)); delete tstr; - ZoneTable::AddResult result(zone_table_->addZone(mem_sgmt_, rrclass_, - zone_name)); - if (result.code == result::SUCCESS) { + result::Result result(zone_table_->addZone(mem_sgmt_, rrclass_, + zone_name, holder)); + if (result == 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_); - } - - return (result.code); + return (result); } namespace { diff --git a/src/lib/datasrc/memory/zone_table.cc b/src/lib/datasrc/memory/zone_table.cc index a74a61dd27..68c73ad712 100644 --- a/src/lib/datasrc/memory/zone_table.cc +++ b/src/lib/datasrc/memory/zone_table.cc @@ -67,19 +67,11 @@ ZoneTable::destroy(util::MemorySegment& mem_sgmt, ZoneTable* ztable, mem_sgmt.deallocate(ztable, sizeof(ZoneTable)); } -ZoneTable::AddResult +result::Result ZoneTable::addZone(util::MemorySegment& mem_sgmt, RRClass zone_class, - const Name& zone_name) + const Name& zone_name, + SegmentObjectHolder& 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); - // Get the node where we put the zone ZoneTableNode* node(NULL); switch (zones_->insert(mem_sgmt, zone_name, &node)) { @@ -94,12 +86,13 @@ 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(content.release()); + if (old != NULL) { + ZoneData::destroy(mem_sgmt, old, zone_class); + return (result::EXIST); + } else { + return (result::SUCCESS); } } @@ -132,20 +125,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..aebd4d0d0f 100644 --- a/src/lib/datasrc/memory/zone_table.h +++ b/src/lib/datasrc/memory/zone_table.h @@ -36,6 +36,10 @@ namespace memory { // forward declaration: in this header it's mostly an opaque type. class ZoneData; +namespace detail { +template class SegmentObjectHolder; +} + /// \brief A conceptual table of authoritative zones. /// /// This class is actually a simple wrapper for a \c DomainTree whose data is @@ -74,14 +78,6 @@ 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 findZone() method. struct FindResult { @@ -140,30 +136,28 @@ 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). + /// When it is added successfully, it is released from the holder. /// \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 released and replaced + /// by the new one. + result::Result addZone(util::MemorySegment& mem_sgmt, + dns::RRClass zone_class, + const dns::Name& zone_name, + detail::SegmentObjectHolder& content); /// Find a zone that best matches the given name in the \c ZoneTable. /// @@ -185,16 +179,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/zone_table_unittest.cc b/src/lib/datasrc/tests/memory/zone_table_unittest.cc index 359df4923f..b9ee3d9441 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,69 @@ TEST_F(ZoneTableTest, create) { } TEST_F(ZoneTableTest, addZone) { + SegmentObjectHolder holder1( + mem_sgmt_, ZoneData::create(mem_sgmt_, zname1), zclass_); // Normal successful case. - const ZoneTable::AddResult result1 = - zone_table->addZone(mem_sgmt_, zclass_, zname1); - EXPECT_EQ(result::SUCCESS, result1.code); + EXPECT_EQ(result::SUCCESS, zone_table->addZone(mem_sgmt_, zclass_, + zname1, holder1)); + // It got released by it + EXPECT_EQ(NULL, holder1.get()); // Duplicate add doesn't replace the existing data. + SegmentObjectHolder holder2( + mem_sgmt_, ZoneData::create(mem_sgmt_, zname1), zclass_); 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); + zname1, holder2)); + // It releases this one even when we replace the old zone + EXPECT_EQ(NULL, holder2.get()); + + 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); + Name("EXAMPLE.COM"), + holder3)); // 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)); + 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)); // 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), 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)); + 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)); + 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)); 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 +162,16 @@ 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)); + EXPECT_EQ(zone_data, zone_table->findZone(Name("www.example.com")).zone_data); } } From e1b4bf1cb50160c436f2e5440fd774680b91b7a2 Mon Sep 17 00:00:00 2001 From: Michal 'vorner' Vaner Date: Sun, 7 Oct 2012 16:38:23 +0200 Subject: [PATCH 04/13] [2292] Remove mutable find methods They are currently used in test code only (so we'll need to update the tests to compile). The real code seams clean now. --- src/lib/datasrc/memory/domaintree.h | 47 ++----------------------- src/lib/datasrc/memory/memory_client.cc | 20 +---------- src/lib/datasrc/memory/memory_client.h | 29 --------------- src/lib/datasrc/memory/zone_table.cc | 2 +- src/lib/datasrc/memory/zone_table.h | 4 +-- 5 files changed, 6 insertions(+), 96 deletions(-) diff --git a/src/lib/datasrc/memory/domaintree.h b/src/lib/datasrc/memory/domaintree.h index e54d8eaaed..50f5d40cf6 100644 --- a/src/lib/datasrc/memory/domaintree.h +++ b/src/lib/datasrc/memory/domaintree.h @@ -1080,17 +1080,6 @@ public: /// of it. In that case, node parameter is left intact. //@{ - /// \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 @@ -1109,17 +1098,6 @@ public: 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 @@ -1138,27 +1116,6 @@ public: return (ret); } - /// \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::Name& name, - const DomainTreeNode** node, - DomainTreeNodeChain& node_path, - bool (*callback)(const DomainTreeNode&, CBARG), - CBARG callback_arg) const - { - const DomainTreeNode* target_node = NULL; - const isc::dns::LabelSequence ls(name); - Result ret = find(ls, &target_node, node_path, callback, - callback_arg); - if (ret != NOTFOUND) { - *node = target_node; - } - return (ret); - } - /// \brief Find with callback and node chain /// \anchor callback /// @@ -1235,7 +1192,7 @@ public: /// \c true, it returns immediately with the current node. template Result find(const isc::dns::LabelSequence& target_labels_orig, - NodeType** node, + const NodeType** node, DomainTreeNodeChain& node_path, bool (*callback)(const DomainTreeNode&, CBARG), CBARG callback_arg) const; @@ -1523,7 +1480,7 @@ template template typename DomainTree::Result DomainTree::find(const isc::dns::LabelSequence& target_labels_orig, - NodeType** target, + const NodeType** target, DomainTreeNodeChain& node_path, bool (*callback)(const DomainTreeNode&, CBARG), CBARG callback_arg) const diff --git a/src/lib/datasrc/memory/memory_client.cc b/src/lib/datasrc/memory/memory_client.cc index 5eaf58b547..de953ca107 100644 --- a/src/lib/datasrc/memory/memory_client.cc +++ b/src/lib/datasrc/memory/memory_client.cc @@ -725,7 +725,7 @@ InMemoryClient::load(const isc::dns::Name& zone_name, const std::string InMemoryClient::getFileName(const isc::dns::Name& zone_name) const { - FileNameNode* node(NULL); + const FileNameNode* node(NULL); FileNameTree::Result result = impl_->file_name_tree_->find(zone_name, &node); if (result == FileNameTree::EXACTMATCH) { @@ -735,24 +735,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_table.cc b/src/lib/datasrc/memory/zone_table.cc index 68c73ad712..0e6ed3c4ed 100644 --- a/src/lib/datasrc/memory/zone_table.cc +++ b/src/lib/datasrc/memory/zone_table.cc @@ -98,7 +98,7 @@ ZoneTable::addZone(util::MemorySegment& mem_sgmt, RRClass zone_class, ZoneTable::FindResult ZoneTable::findZone(const Name& name) const { - ZoneTableNode* node(NULL); + const ZoneTableNode* node(NULL); result::Result my_result; // Translate the return codes diff --git a/src/lib/datasrc/memory/zone_table.h b/src/lib/datasrc/memory/zone_table.h index aebd4d0d0f..4f309b7c2e 100644 --- a/src/lib/datasrc/memory/zone_table.h +++ b/src/lib/datasrc/memory/zone_table.h @@ -82,11 +82,11 @@ public: /// \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: From 915576b6aa27d020faadf7fbfc4e4d9bb1df129e Mon Sep 17 00:00:00 2001 From: Michal 'vorner' Vaner Date: Sun, 7 Oct 2012 16:50:50 +0200 Subject: [PATCH 05/13] [2292] Remove unused template parameters The domain tree no longer supports the mutable find versions, so we don't need the template parameters allowing for them. --- src/lib/datasrc/memory/domaintree.h | 68 +++++++---------------------- 1 file changed, 16 insertions(+), 52 deletions(-) diff --git a/src/lib/datasrc/memory/domaintree.h b/src/lib/datasrc/memory/domaintree.h index 50f5d40cf6..5101c65635 100644 --- a/src/lib/datasrc/memory/domaintree.h +++ b/src/lib/datasrc/memory/domaintree.h @@ -684,7 +684,7 @@ DomainTreeNode::predecessor() const { /// DomainTree. /// This is the reason why manipulation methods such as \c push() and \c pop() /// are private (and not shown in the doxygen document). -template > +template class DomainTreeNodeChain { /// DomainTreeNodeChain is initialized by DomainTree, only DomainTree has /// knowledge to manipulate it. @@ -817,7 +817,7 @@ private: /// root node of DomainTree /// /// \exception None - NodeType* top() const { + const DomainTreeNode* top() const { assert(!isEmpty()); return (nodes_[level_count_ - 1]); } @@ -840,7 +840,7 @@ private: /// otherwise the node should be the root node of DomainTree. /// /// \exception None - void push(NodeType* node) { + void push(const DomainTreeNode* node) { assert(level_count_ < RBT_MAX_LEVEL); nodes_[level_count_++] = node; } @@ -852,7 +852,7 @@ private: const static int RBT_MAX_LEVEL = isc::dns::Name::MAX_LABELS; size_t level_count_; - NodeType* nodes_[RBT_MAX_LEVEL]; + const DomainTreeNode* nodes_[RBT_MAX_LEVEL]; const DomainTreeNode* last_compared_; isc::dns::NameComparisonResult last_comparison_; }; @@ -1080,39 +1080,25 @@ public: /// of it. In that case, node parameter is left intact. //@{ - /// \brief Simple find returning immutable node. + /// \brief Simple find /// - /// Acts as described in the \ref find section, but returns immutable node - /// pointer. + /// Acts as described in the \ref find section. Result find(const isc::dns::Name& name, const DomainTreeNode** node) const { 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); } - /// \brief Simple find returning immutable node, with node_path tracking + /// \brief Simple find, with node_path tracking /// - /// Acts as described in the \ref find section, but returns immutable node - /// pointer. + /// Acts as described in the \ref find section. Result find(const isc::dns::Name& name, const DomainTreeNode** node, DomainTreeNodeChain& node_path) const { - 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); } @@ -1190,34 +1176,12 @@ public: /// /// \return As in the description, but in case of callback returning /// \c true, it returns immediately with the current node. - template - Result find(const isc::dns::LabelSequence& target_labels_orig, - const NodeType** 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, + Result find(const isc::dns::LabelSequence& target_labels_orig, const DomainTreeNode** node, DomainTreeNodeChain& node_path, bool (*callback)(const DomainTreeNode&, CBARG), - CBARG callback_arg) const - { - 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 @@ -1477,11 +1441,11 @@ DomainTree::deleteHelper(util::MemorySegment& mem_sgmt, } template -template +template typename DomainTree::Result DomainTree::find(const isc::dns::LabelSequence& target_labels_orig, - const NodeType** target, - DomainTreeNodeChain& node_path, + const DomainTreeNode** target, + DomainTreeNodeChain& node_path, bool (*callback)(const DomainTreeNode&, CBARG), CBARG callback_arg) const { @@ -1491,7 +1455,7 @@ DomainTree::find(const isc::dns::LabelSequence& target_labels_orig, " and label sequence"); } - NodeType* node; + const DomainTreeNode* node; if (!node_path.isEmpty()) { // Get the top node in the node chain From 7a628baa1a158b5837d6f383e10b30542d2ac59b Mon Sep 17 00:00:00 2001 From: Michal 'vorner' Vaner Date: Sun, 7 Oct 2012 17:03:18 +0200 Subject: [PATCH 06/13] [2292] Drop tests for InMemoryClient::add As the method was removed. --- .../tests/memory/memory_client_unittest.cc | 92 ------------------- 1 file changed, 92 deletions(-) diff --git a/src/lib/datasrc/tests/memory/memory_client_unittest.cc b/src/lib/datasrc/tests/memory/memory_client_unittest.cc index 58979a4351..6d3f05505c 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"); From 73900f62a74bf769b5f6ea7b3cf43428baa19842 Mon Sep 17 00:00:00 2001 From: Michal 'vorner' Vaner Date: Sun, 7 Oct 2012 17:18:33 +0200 Subject: [PATCH 07/13] [2292] Re-add version of find removed by accident This one was not mutable, it just took Name as an argument. --- src/lib/datasrc/memory/domaintree.h | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/src/lib/datasrc/memory/domaintree.h b/src/lib/datasrc/memory/domaintree.h index 5101c65635..7e68f53bf0 100644 --- a/src/lib/datasrc/memory/domaintree.h +++ b/src/lib/datasrc/memory/domaintree.h @@ -1102,6 +1102,23 @@ public: return (ret); } + /// \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::Name& name, + const DomainTreeNode** node, + DomainTreeNodeChain& node_path, + bool (*callback)(const DomainTreeNode&, CBARG), + CBARG callback_arg) const + { + const isc::dns::LabelSequence ls(name); + Result ret = find(ls, node, node_path, callback, + callback_arg); + return (ret); + } + /// \brief Find with callback and node chain /// \anchor callback /// From 00d3de1af86efe6b4df1310a039adad8f6a719ae Mon Sep 17 00:00:00 2001 From: Michal 'vorner' Vaner Date: Sun, 7 Oct 2012 17:19:12 +0200 Subject: [PATCH 08/13] [2292] Update tests They don't need the mutable versions to work, but they used them anyway. --- .../tests/memory/domaintree_unittest.cc | 28 +++++++++---------- .../tests/memory/zone_data_unittest.cc | 2 +- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/src/lib/datasrc/tests/memory/domaintree_unittest.cc b/src/lib/datasrc/tests/memory/domaintree_unittest.cc index cb16e02c79..3754139620 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,11 @@ 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 + // "cdtnode" may be invalid due to the insertion, so we need to re-find // it. EXPECT_EQ(TestDomainTree::EXACTMATCH, dtree.find(Name("callback.example"), - &dtnode)); - EXPECT_TRUE(dtnode->getFlag(TestDomainTreeNode::FLAG_CALLBACK)); + &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/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)); From 869d14d7056ee7b0c3cf1dd0db1b375be755bba7 Mon Sep 17 00:00:00 2001 From: JINMEI Tatuya Date: Mon, 8 Oct 2012 11:32:18 -0700 Subject: [PATCH 09/13] [2292] minor cleanups: constify, indentation --- src/lib/datasrc/memory/memory_client.cc | 10 +++++----- src/lib/datasrc/tests/memory/domaintree_unittest.cc | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/lib/datasrc/memory/memory_client.cc b/src/lib/datasrc/memory/memory_client.cc index de953ca107..a5824fa63b 100644 --- a/src/lib/datasrc/memory/memory_client.cc +++ b/src/lib/datasrc/memory/memory_client.cc @@ -627,11 +627,11 @@ 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; - result::Result result(zone_table_->addZone(mem_sgmt_, rrclass_, - zone_name, holder)); + const result::Result result(zone_table_->addZone(mem_sgmt_, rrclass_, + zone_name, holder)); if (result == result::SUCCESS) { // Only increment the zone count if the zone doesn't already // exist. @@ -726,8 +726,8 @@ InMemoryClient::load(const isc::dns::Name& zone_name, const std::string InMemoryClient::getFileName(const isc::dns::Name& zone_name) const { const FileNameNode* node(NULL); - FileNameTree::Result result = impl_->file_name_tree_->find(zone_name, - &node); + const FileNameTree::Result result = impl_->file_name_tree_->find(zone_name, + &node); if (result == FileNameTree::EXACTMATCH) { return (*node->getData()); } else { diff --git a/src/lib/datasrc/tests/memory/domaintree_unittest.cc b/src/lib/datasrc/tests/memory/domaintree_unittest.cc index 3754139620..81fa576b21 100644 --- a/src/lib/datasrc/tests/memory/domaintree_unittest.cc +++ b/src/lib/datasrc/tests/memory/domaintree_unittest.cc @@ -414,7 +414,7 @@ performCallbackTest(TestDomainTree& dtree, // "cdtnode" may be invalid due to the insertion, so we need to re-find // it. EXPECT_EQ(TestDomainTree::EXACTMATCH, dtree.find(Name("callback.example"), - &cdtnode)); + &cdtnode)); EXPECT_TRUE(cdtnode->getFlag(TestDomainTreeNode::FLAG_CALLBACK)); EXPECT_FALSE(subdtnode->getFlag(TestDomainTreeNode::FLAG_CALLBACK)); EXPECT_FALSE(parentdtnode->getFlag(TestDomainTreeNode::FLAG_CALLBACK)); From 7888a2127db48e004cb1cce24552db230cb3d685 Mon Sep 17 00:00:00 2001 From: Michal 'vorner' Vaner Date: Tue, 9 Oct 2012 13:39:30 +0200 Subject: [PATCH 10/13] [2292] Don't pass SegmentObjectHolder It is supposed to be mostly private. Use it internally only. --- src/lib/datasrc/memory/memory_client.cc | 3 +- src/lib/datasrc/memory/zone_table.cc | 10 +++++-- src/lib/datasrc/memory/zone_table.h | 11 +++---- .../tests/memory/zone_table_unittest.cc | 29 ++++++++++++------- 4 files changed, 32 insertions(+), 21 deletions(-) diff --git a/src/lib/datasrc/memory/memory_client.cc b/src/lib/datasrc/memory/memory_client.cc index a5824fa63b..e33a0f32e9 100644 --- a/src/lib/datasrc/memory/memory_client.cc +++ b/src/lib/datasrc/memory/memory_client.cc @@ -631,7 +631,8 @@ InMemoryClient::InMemoryClientImpl::load( delete tstr; const result::Result result(zone_table_->addZone(mem_sgmt_, rrclass_, - zone_name, holder)); + zone_name, + holder.release())); if (result == result::SUCCESS) { // Only increment the zone count if the zone doesn't already // exist. diff --git a/src/lib/datasrc/memory/zone_table.cc b/src/lib/datasrc/memory/zone_table.cc index 0e6ed3c4ed..4a1f184974 100644 --- a/src/lib/datasrc/memory/zone_table.cc +++ b/src/lib/datasrc/memory/zone_table.cc @@ -69,9 +69,13 @@ ZoneTable::destroy(util::MemorySegment& mem_sgmt, ZoneTable* ztable, result::Result ZoneTable::addZone(util::MemorySegment& mem_sgmt, RRClass zone_class, - const Name& zone_name, - SegmentObjectHolder& content) + const Name& zone_name, ZoneData* content) { + 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)) { @@ -87,7 +91,7 @@ ZoneTable::addZone(util::MemorySegment& mem_sgmt, RRClass zone_class, assert(node != NULL); // We can release now, setData never throws - ZoneData* old = node->setData(content.release()); + ZoneData* old = node->setData(holder.release()); if (old != NULL) { ZoneData::destroy(mem_sgmt, old, zone_class); return (result::EXIST); diff --git a/src/lib/datasrc/memory/zone_table.h b/src/lib/datasrc/memory/zone_table.h index 4f309b7c2e..5ade3ac963 100644 --- a/src/lib/datasrc/memory/zone_table.h +++ b/src/lib/datasrc/memory/zone_table.h @@ -36,10 +36,6 @@ namespace memory { // forward declaration: in this header it's mostly an opaque type. class ZoneData; -namespace detail { -template class SegmentObjectHolder; -} - /// \brief A conceptual table of authoritative zones. /// /// This class is actually a simple wrapper for a \c DomainTree whose data is @@ -147,7 +143,9 @@ public: /// \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. /// \param content This one should hold the zone content (the ZoneData). - /// When it is added successfully, it is released from the holder. + /// 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 contained @@ -156,8 +154,7 @@ public: result::Result addZone(util::MemorySegment& mem_sgmt, dns::RRClass zone_class, const dns::Name& zone_name, - detail::SegmentObjectHolder& content); + ZoneData* content); /// Find a zone that best matches the given name in the \c ZoneTable. /// diff --git a/src/lib/datasrc/tests/memory/zone_table_unittest.cc b/src/lib/datasrc/tests/memory/zone_table_unittest.cc index b9ee3d9441..64ec312c02 100644 --- a/src/lib/datasrc/tests/memory/zone_table_unittest.cc +++ b/src/lib/datasrc/tests/memory/zone_table_unittest.cc @@ -89,11 +89,15 @@ 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_); // Normal successful case. EXPECT_EQ(result::SUCCESS, zone_table->addZone(mem_sgmt_, zclass_, - zname1, holder1)); + zname1, holder1.release())); // It got released by it EXPECT_EQ(NULL, holder1.get()); @@ -101,7 +105,7 @@ TEST_F(ZoneTableTest, addZone) { SegmentObjectHolder holder2( mem_sgmt_, ZoneData::create(mem_sgmt_, zname1), zclass_); EXPECT_EQ(result::EXIST, zone_table->addZone(mem_sgmt_, zclass_, - zname1, holder2)); + zname1, holder2.release())); // It releases this one even when we replace the old zone EXPECT_EQ(NULL, holder2.get()); @@ -111,16 +115,18 @@ TEST_F(ZoneTableTest, addZone) { // names are compared in a case insensitive manner. EXPECT_EQ(result::EXIST, zone_table->addZone(mem_sgmt_, zclass_, Name("EXAMPLE.COM"), - holder3)); + holder3.release())); // 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, holder4)); + zone_table->addZone(mem_sgmt_, zclass_, zname2, + holder4.release())); SegmentObjectHolder holder5( mem_sgmt_, ZoneData::create(mem_sgmt_, zname3), zclass_); EXPECT_EQ(result::SUCCESS, - zone_table->addZone(mem_sgmt_, zclass_, zname3, holder5)); + zone_table->addZone(mem_sgmt_, zclass_, zname3, + holder5.release())); // Have the memory segment throw an exception in extending the internal // tree. It still shouldn't cause memory leak (which would be detected @@ -129,7 +135,7 @@ TEST_F(ZoneTableTest, addZone) { 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), + holder6.release()), std::bad_alloc); } @@ -138,15 +144,17 @@ TEST_F(ZoneTableTest, findZone) { 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)); + holder1.release())); SegmentObjectHolder holder2( mem_sgmt_, ZoneData::create(mem_sgmt_, zname2), zclass_); EXPECT_EQ(result::SUCCESS, - zone_table->addZone(mem_sgmt_, zclass_, zname2, holder2)); + zone_table->addZone(mem_sgmt_, zclass_, zname2, + holder2.release())); SegmentObjectHolder holder3( mem_sgmt_, ZoneData::create(mem_sgmt_, zname3), zclass_); EXPECT_EQ(result::SUCCESS, - zone_table->addZone(mem_sgmt_, zclass_, zname3, holder3)); + zone_table->addZone(mem_sgmt_, zclass_, zname3, + holder3.release())); const ZoneTable::FindResult find_result1 = zone_table->findZone(Name("example.com")); @@ -170,7 +178,8 @@ TEST_F(ZoneTableTest, findZone) { SegmentObjectHolder holder4( mem_sgmt_, ZoneData::create(mem_sgmt_, Name("com")), zclass_); EXPECT_EQ(result::SUCCESS, zone_table->addZone(mem_sgmt_, zclass_, - Name("com"), holder4)); + Name("com"), + holder4.release())); EXPECT_EQ(zone_data, zone_table->findZone(Name("www.example.com")).zone_data); } From 3a6ee0d12d06c1f9e5b9f261b3fcaab444a9ae80 Mon Sep 17 00:00:00 2001 From: Michal 'vorner' Vaner Date: Tue, 9 Oct 2012 14:07:13 +0200 Subject: [PATCH 11/13] [2292] Pass the old zone back Instead of releasing it directly. While the internal release was more convenient, it didn't allow for swapping things fast under a mutex and then spending the time releasing it unlocked. --- src/lib/datasrc/memory/memory_client.cc | 14 ++++--- src/lib/datasrc/memory/zone_table.cc | 7 ++-- src/lib/datasrc/memory/zone_table.h | 20 +++++++--- .../tests/memory/zone_table_unittest.cc | 38 ++++++++++++------- 4 files changed, 51 insertions(+), 28 deletions(-) diff --git a/src/lib/datasrc/memory/memory_client.cc b/src/lib/datasrc/memory/memory_client.cc index e33a0f32e9..16096114fc 100644 --- a/src/lib/datasrc/memory/memory_client.cc +++ b/src/lib/datasrc/memory/memory_client.cc @@ -630,16 +630,20 @@ InMemoryClient::InMemoryClientImpl::load( const std::string* tstr = node->setData(new std::string(filename)); delete tstr; - const result::Result result(zone_table_->addZone(mem_sgmt_, rrclass_, - zone_name, - holder.release())); - if (result == result::SUCCESS) { + 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_; } + // 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); + return (result.code); } namespace { diff --git a/src/lib/datasrc/memory/zone_table.cc b/src/lib/datasrc/memory/zone_table.cc index 4a1f184974..836b020088 100644 --- a/src/lib/datasrc/memory/zone_table.cc +++ b/src/lib/datasrc/memory/zone_table.cc @@ -67,7 +67,7 @@ ZoneTable::destroy(util::MemorySegment& mem_sgmt, ZoneTable* ztable, mem_sgmt.deallocate(ztable, sizeof(ZoneTable)); } -result::Result +ZoneTable::AddResult ZoneTable::addZone(util::MemorySegment& mem_sgmt, RRClass zone_class, const Name& zone_name, ZoneData* content) { @@ -93,10 +93,9 @@ ZoneTable::addZone(util::MemorySegment& mem_sgmt, RRClass zone_class, // We can release now, setData never throws ZoneData* old = node->setData(holder.release()); if (old != NULL) { - ZoneData::destroy(mem_sgmt, old, zone_class); - return (result::EXIST); + return (AddResult(result::EXIST, old)); } else { - return (result::SUCCESS); + return (AddResult(result::SUCCESS, NULL)); } } diff --git a/src/lib/datasrc/memory/zone_table.h b/src/lib/datasrc/memory/zone_table.h index 5ade3ac963..024558eb30 100644 --- a/src/lib/datasrc/memory/zone_table.h +++ b/src/lib/datasrc/memory/zone_table.h @@ -74,6 +74,14 @@ 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 findZone() method. struct FindResult { @@ -149,12 +157,12 @@ public: /// \return \c result::SUCCESS If the zone is successfully /// added to the zone table. /// \return \c result::EXIST The zone table already contained - /// zone of the same origin. The old data is released and replaced - /// by the new one. - result::Result addZone(util::MemorySegment& mem_sgmt, - dns::RRClass zone_class, - const dns::Name& zone_name, - ZoneData* content); + /// 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. /// diff --git a/src/lib/datasrc/tests/memory/zone_table_unittest.cc b/src/lib/datasrc/tests/memory/zone_table_unittest.cc index 64ec312c02..9cf1b3420c 100644 --- a/src/lib/datasrc/tests/memory/zone_table_unittest.cc +++ b/src/lib/datasrc/tests/memory/zone_table_unittest.cc @@ -95,38 +95,50 @@ TEST_F(ZoneTableTest, addZone) { SegmentObjectHolder holder1( mem_sgmt_, ZoneData::create(mem_sgmt_, zname1), zclass_); + const ZoneData* data1(holder1.get()); // Normal successful case. - EXPECT_EQ(result::SUCCESS, zone_table->addZone(mem_sgmt_, zclass_, - zname1, holder1.release())); + 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. SegmentObjectHolder holder2( mem_sgmt_, ZoneData::create(mem_sgmt_, zname1), zclass_); - EXPECT_EQ(result::EXIST, zone_table->addZone(mem_sgmt_, zclass_, - zname1, holder2.release())); + 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"), - holder3.release())); + 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, - holder4.release())); + holder4.release()).code); SegmentObjectHolder holder5( mem_sgmt_, ZoneData::create(mem_sgmt_, zname3), zclass_); EXPECT_EQ(result::SUCCESS, zone_table->addZone(mem_sgmt_, zclass_, zname3, - holder5.release())); + 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 @@ -144,17 +156,17 @@ TEST_F(ZoneTableTest, findZone) { 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())); + holder1.release()).code); SegmentObjectHolder holder2( mem_sgmt_, ZoneData::create(mem_sgmt_, zname2), zclass_); EXPECT_EQ(result::SUCCESS, zone_table->addZone(mem_sgmt_, zclass_, zname2, - holder2.release())); + holder2.release()).code); SegmentObjectHolder holder3( mem_sgmt_, ZoneData::create(mem_sgmt_, zname3), zclass_); EXPECT_EQ(result::SUCCESS, zone_table->addZone(mem_sgmt_, zclass_, zname3, - holder3.release())); + holder3.release()).code); const ZoneTable::FindResult find_result1 = zone_table->findZone(Name("example.com")); @@ -179,7 +191,7 @@ TEST_F(ZoneTableTest, findZone) { mem_sgmt_, ZoneData::create(mem_sgmt_, Name("com")), zclass_); EXPECT_EQ(result::SUCCESS, zone_table->addZone(mem_sgmt_, zclass_, Name("com"), - holder4.release())); + holder4.release()).code); EXPECT_EQ(zone_data, zone_table->findZone(Name("www.example.com")).zone_data); } From d78dd497ecb846ab2545ce1cb7b6e41341ff5995 Mon Sep 17 00:00:00 2001 From: Michal 'vorner' Vaner Date: Tue, 9 Oct 2012 14:11:25 +0200 Subject: [PATCH 12/13] [2292] Clarify test And make it slightly stronger (it now checks something that should be mostly obvious too, but who knows, with software). --- src/lib/datasrc/tests/memory/domaintree_unittest.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/lib/datasrc/tests/memory/domaintree_unittest.cc b/src/lib/datasrc/tests/memory/domaintree_unittest.cc index 81fa576b21..45e256a0b4 100644 --- a/src/lib/datasrc/tests/memory/domaintree_unittest.cc +++ b/src/lib/datasrc/tests/memory/domaintree_unittest.cc @@ -411,10 +411,11 @@ performCallbackTest(TestDomainTree& dtree, Name("example"), &parentdtnode)); // the child/parent nodes shouldn't "inherit" the callback flag. - // "cdtnode" 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"), &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)); From c6391ad769265a1f5c2d5594dcdb113420c7eb4d Mon Sep 17 00:00:00 2001 From: Michal 'vorner' Vaner Date: Wed, 10 Oct 2012 11:58:17 +0200 Subject: [PATCH 13/13] [2292] Add a TODO about re-adding tests Planned for merge #2268 --- .../datasrc/tests/memory/memory_client_unittest.cc | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/lib/datasrc/tests/memory/memory_client_unittest.cc b/src/lib/datasrc/tests/memory/memory_client_unittest.cc index 6d3f05505c..004f663c56 100644 --- a/src/lib/datasrc/tests/memory/memory_client_unittest.cc +++ b/src/lib/datasrc/tests/memory/memory_client_unittest.cc @@ -682,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 }