From: Michal 'vorner' Vaner Date: Sun, 7 Oct 2012 14:13:56 +0000 (+0200) Subject: [2292] Remove the setZoneData method X-Git-Tag: trac2351_base~1^2~9^2~10 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=76243c3ccd7a30484da8478f7457e18fce485492;p=thirdparty%2Fkea.git [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. --- 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); } }