[2100] revised addZone() iface: now it only takes name, creates data in it.

also completed the zone table deleter.

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

@@ -35,58 +35,47 @@ protected:
     ZoneTableTest() : zname1(Name("example.com")),
                       zname2(Name("example.net")),
                       zname3(Name("example")),
-                      zone1(new ZoneData(RRClass::IN(), zname1)),
-                      zone2(new ZoneData(RRClass::IN(), zname2)),
-                      zone3(new ZoneData(RRClass::IN(), zname3)),
                       zone_table(ZoneTable::create(mem_sgmt_))
     {}
-
     ~ZoneTableTest() {
+        if (zone_table != NULL) {
+            ZoneTable::destroy(mem_sgmt_, zone_table);
+        }
+    }
+    void TearDown() {
         ZoneTable::destroy(mem_sgmt_, zone_table);
+        zone_table = NULL;
+        EXPECT_TRUE(mem_sgmt_.allMemoryDeallocated());
     }
     const Name zname1, zname2, zname3;
-    ZoneData* zone1;
-    ZoneData* zone2;
-    ZoneData* zone3;
     isc::util::MemorySegmentLocal mem_sgmt_;
     ZoneTable* zone_table;
 };
 
 TEST_F(ZoneTableTest, addZone) {
-    EXPECT_EQ(result::SUCCESS, zone_table->addZone(mem_sgmt_, zname1, zone1));
-    EXPECT_EQ(result::EXIST, zone_table->addZone(mem_sgmt_, zname1, zone1));
+    EXPECT_EQ(result::SUCCESS, zone_table->addZone(mem_sgmt_, zname1));
+    EXPECT_EQ(result::EXIST, zone_table->addZone(mem_sgmt_, zname1));
     // names are compared in a case insensitive manner.
-    EXPECT_EQ(result::EXIST, zone_table->addZone(
-                  mem_sgmt_, Name("EXAMPLE.COM"),
-                  new ZoneData(RRClass::IN(), Name("EXAMPLE.COM"))));
-
-    EXPECT_EQ(result::SUCCESS, zone_table->addZone(mem_sgmt_, zname2, zone2));
-    EXPECT_EQ(result::SUCCESS, zone_table->addZone(mem_sgmt_, zname3, zone3));
-
-    // Zone table is indexed only by name.  Duplicate origin name with
-    // different zone class isn't allowed.
-    EXPECT_EQ(result::EXIST, zone_table->addZone(
-                  mem_sgmt_, Name("example.com"),
-                  new ZoneData(RRClass::CH(), Name("example.com"))));
-
-    /// Bogus zone (NULL)
-    EXPECT_THROW(zone_table->addZone(mem_sgmt_, zname1, NULL),
-                 isc::InvalidParameter);
+    EXPECT_EQ(result::EXIST, zone_table->addZone(mem_sgmt_,
+                                                 Name("EXAMPLE.COM")));
+    // Add some more different ones.  Should just succeed.
+    EXPECT_EQ(result::SUCCESS, zone_table->addZone(mem_sgmt_, zname2));
+    EXPECT_EQ(result::SUCCESS, zone_table->addZone(mem_sgmt_, zname3));
 }
 
 TEST_F(ZoneTableTest, DISABLED_removeZone) {
-    EXPECT_EQ(result::SUCCESS, zone_table->addZone(mem_sgmt_, zname1, zone1));
-    EXPECT_EQ(result::SUCCESS, zone_table->addZone(mem_sgmt_, zname2, zone2));
-    EXPECT_EQ(result::SUCCESS, zone_table->addZone(mem_sgmt_, zname3, zone3));
+    EXPECT_EQ(result::SUCCESS, zone_table->addZone(mem_sgmt_, zname1));
+    EXPECT_EQ(result::SUCCESS, zone_table->addZone(mem_sgmt_, zname2));
+    EXPECT_EQ(result::SUCCESS, zone_table->addZone(mem_sgmt_, zname3));
 
     EXPECT_EQ(result::SUCCESS, zone_table->removeZone(Name("example.net")));
     EXPECT_EQ(result::NOTFOUND, zone_table->removeZone(Name("example.net")));
 }
 
 TEST_F(ZoneTableTest, DISABLED_findZone) {
-    EXPECT_EQ(result::SUCCESS, zone_table->addZone(mem_sgmt_, zname1, zone1));
-    EXPECT_EQ(result::SUCCESS, zone_table->addZone(mem_sgmt_, zname2, zone2));
-    EXPECT_EQ(result::SUCCESS, zone_table->addZone(mem_sgmt_, zname3, zone3));
+    EXPECT_EQ(result::SUCCESS, zone_table->addZone(mem_sgmt_, zname1));
+    EXPECT_EQ(result::SUCCESS, zone_table->addZone(mem_sgmt_, zname2));
+    EXPECT_EQ(result::SUCCESS, zone_table->addZone(mem_sgmt_, zname3));
 
     EXPECT_EQ(result::SUCCESS, zone_table->findZone(Name("example.com")).code);
     EXPECT_EQ(Name("example.com"),
@@ -106,10 +95,7 @@ TEST_F(ZoneTableTest, DISABLED_findZone) {
 
     // make sure the partial match is indeed the longest match by adding
     // a zone with a shorter origin and query again.
-    EXPECT_EQ(result::SUCCESS, zone_table->addZone(mem_sgmt_,
-                                                   Name("com"),
-                                                   new ZoneData(RRClass::IN(),
-                                                                Name("com"))));
+    EXPECT_EQ(result::SUCCESS, zone_table->addZone(mem_sgmt_, Name("com")));
     EXPECT_EQ(Name("example.com"),
               zone_table->findZone(Name("www.example.com")).zone->getOrigin());
 }

src/lib/datasrc/memory/zone_data.h

@@ -23,12 +23,11 @@ namespace datasrc {
 namespace memory {
 class ZoneData {
 public:
-    ZoneData(dns::RRClass zone_class, const dns::Name& zone_name) :
-        zone_class_(zone_class), zone_name_(zone_name)
+    ZoneData(const dns::Name& zone_name) :
+        zone_name_(zone_name)
     {}
 
 private:
-    const dns::RRClass zone_class_;
     const dns::Name zone_name_;
 };
 } // namespace memory

src/lib/datasrc/memory/zone_table.cc

@@ -68,24 +68,24 @@ ZoneTable::destroy(util::MemorySegment& mem_sgmt, ZoneTable* ztable) {
 }
 
 result::Result
-ZoneTable::addZone(util::MemorySegment& mem_sgmt, const Name& zone_name,
-                   ZoneData* zone_data)
-{
-    // Sanity check
-    if (zone_data == NULL) {
-        isc_throw(InvalidParameter,
-                  "Null pointer is passed to ZoneTable::addZone()");
-    }
+ZoneTable::addZone(util::MemorySegment& mem_sgmt, const Name& zone_name) {
+    // Create a new ZoneData instance first.  If it already exists the new
+    // one 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.
+    void* p = mem_sgmt.allocate(sizeof(ZoneData)); // XXX safety
+    ZoneData* zone_data = new(p) ZoneData(zone_name);
 
     // Get the node where we put the zone
     ZoneTableNode* node(NULL);
     switch (zones_->insert(mem_sgmt, zone_name, &node)) {
-        // This is OK
     case ZoneTableTree::SUCCESS:
     case ZoneTableTree::ALREADYEXISTS:
+        // These are OK
         break;
-        // Can Not Happen
     default:
+        // Can Not Happen
         assert(false);
     }
     // Can Not Happen
@@ -96,6 +96,7 @@ ZoneTable::addZone(util::MemorySegment& mem_sgmt, const Name& zone_name,
         node->setData(mem_sgmt, zone_data);
         return (result::SUCCESS);
     } else { // There's something there already
+        mem_sgmt.deallocate(p, sizeof(ZoneData));
         return (result::EXIST);
     }
 }

src/lib/datasrc/memory/zone_table.h

@@ -99,21 +99,17 @@ public:
     /// is undefined if this condition isn't met).
     static void destroy(util::MemorySegment& mem_sgmt, ZoneTable* ztable);
 
-    /// Add a \c Zone to the \c ZoneTable.
+    /// Add a new zone to the \c ZoneTable.
     ///
-    /// \c Zone must not be associated with a NULL pointer; otherwise
-    /// an exception of class \c InvalidParameter will be thrown.
-    /// If internal resource allocation fails, a corresponding standard
-    /// exception will be thrown.
-    /// This method never throws an exception otherwise.
+    /// \throw std::bad_alloc Internal resource allocation fails.
     ///
-    /// \param zone A \c Zone object to be added.
+    /// \param zone_name The name of the zone to be added.
     /// \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.
     result::Result addZone(util::MemorySegment& mem_sgmt,
-                           const dns::Name& zone_name, ZoneData* zone_data);
+                           const dns::Name& zone_name);
 
     /// Remove a \c Zone of the given origin name from the \c ZoneTable.
     ///
@@ -149,7 +145,11 @@ public:
 private:
     struct ZoneDataDeleter {
         ZoneDataDeleter() {}
-        void operator()(util::MemorySegment&, ZoneData*) const {}
+        void operator()(util::MemorySegment& mem_sgmt,
+                        ZoneData* zone_data) const
+        {
+            mem_sgmt.deallocate(zone_data, sizeof(ZoneData));
+        }
     };
 
     // Type aliases to make it shorter