From: Michal 'vorner' Vaner Date: Tue, 9 Oct 2012 12:07:13 +0000 (+0200) Subject: [2292] Pass the old zone back X-Git-Tag: trac2402_base~15^2~60 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=a3953a556b472084d669dcb629fb801dec67c5dd;p=thirdparty%2Fkea.git [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. --- 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); }