[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

@@ -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 {

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));
     }
 }
 

src/lib/datasrc/memory/zone_table.h

@@ -74,6 +74,14 @@ private:
     typedef DomainTreeNode<ZoneData> 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.
     ///

src/lib/datasrc/tests/memory/zone_table_unittest.cc

@@ -95,38 +95,50 @@ TEST_F(ZoneTableTest, addZone) {
 
     SegmentObjectHolder<ZoneData, RRClass> 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<ZoneData, RRClass> 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<ZoneData, RRClass> 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<ZoneData, RRClass> 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<ZoneData, RRClass> 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<ZoneData, RRClass> 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<ZoneData, RRClass> 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);
 }