[2207] Deprecate InstallAction

As we want to directly modify the ZoneTable in ZoneReloaderLocal
(because it is memory-type dependant), we don't need the InstallAction
for anything. If we don't need it, we don't pass it as constructor
parameter -- it'll be called from a factory anyway and the factory may
know we don't need it.

Also adding a setTable() method to ZoneTableHeader. The header had no
way to specify the table currently and we need it for the tests. The
method is currently tentative, we'll probably replace it by something.

src/lib/datasrc/memory/zone_reloader.cc

@@ -26,11 +26,11 @@ namespace memory {
 
 ZoneReloaderLocal::ZoneReloaderLocal(ZoneTableSegment* segment,
                                      const LoadAction& load_action,
-                                     const InstallAction& install_action,
+                                     const dns::Name& origin,
                                      const dns::RRClass& rrclass) :
     segment_(segment),
     load_action_(load_action),
-    install_action_(install_action),
+    origin_(origin),
     rrclass_(rrclass),
     zone_data_(NULL),
     loaded_(false),
@@ -67,12 +67,15 @@ ZoneReloaderLocal::install() {
     }
 
     data_ready_ = false;
-    auto_ptr<ZoneSegment> zone_segment(new ZoneSegment(zone_data_));
 
-    zone_data_ = install_action_(ZoneSegmentID(), zone_segment.get());
+    ZoneTable* table(segment_->getHeader().getTable());
+    if (table == NULL) {
+        isc_throw(isc::Unexpected, "No zone table present");
+    }
+    ZoneTable::AddResult result(table->addZone(segment_->getMemorySegment(),
+                                               rrclass_, origin_, zone_data_));
 
-    // The ownership was passed to the callback, no need to clear it now.
-    zone_segment.release();
+    zone_data_ = result.zone_data;
 }
 
 void

src/lib/datasrc/memory/zone_reloader.h

@@ -13,6 +13,7 @@
 // PERFORMANCE OF THIS SOFTWARE.
 
 #include <dns/rrclass.h>
+#include <dns/name.h>
 
 #include <boost/function.hpp>
 
@@ -80,29 +81,6 @@ public:
     virtual void cleanup() = 0;
 };
 
-// TODO: Fully define this. It is supposed to be passed to the install_action
-// callback, but what does it actually represent? Is it the actuall zone data
-// there?
-//
-// The current interface is temporary, so the tests work. It will probably
-// change (and we may even fold this class to some other, because there
-// seem to be too many classes around holding zone already).
-//
-// FIXME: Who is responsible for releasing of the segment itself?
-class ZoneSegment {
-public:
-    explicit ZoneSegment(ZoneData* data) :
-        data_(data)
-    {}
-    ZoneData* getZoneData() {
-        return (data_);
-    }
-private:
-    ZoneData* data_;
-};
-// TODO: Somehow specify what the ID is
-class ZoneSegmentID {};
-
 /// \brief Callback to load data into the memory
 ///
 /// This callback should create new ZoneData (allocated from the passed
@@ -111,17 +89,6 @@ class ZoneSegmentID {};
 ///
 /// It must not return NULL.
 typedef boost::function<ZoneData*(util::MemorySegment&)> LoadAction;
-/// \brief Install the zone somewhere.
-///
-/// The goal of the callback is to take the zone data (contained in the
-/// ZoneSegment and identified by ZoneSegmentID) and put it somewhere
-/// to use. The return value should contain the old copy of the zone, if
-/// there was any (it may be NULL). The updater will then destroy it.
-///
-/// Upon successful completion, the ownership of the new zone is passed
-/// to the callback and the old to the updater.
-typedef boost::function<ZoneData* (const ZoneSegmentID&,
-                                   ZoneSegment*)> InstallAction;
 
 /// \brief Reloader implementation which loads data locally.
 ///
@@ -138,8 +105,7 @@ public:
     /// \param install_action The callback used to install the loaded zone.
     /// \param rrclass The class of the zone.
     ZoneReloaderLocal(ZoneTableSegment* segment, const LoadAction& load_action,
-                      const InstallAction& install_action,
-                      const dns::RRClass& rrclass);
+                      const dns::Name& name, const dns::RRClass& rrclass);
     /// \brief Destructor
     ~ZoneReloaderLocal();
     /// \brief Loads the data.
@@ -169,7 +135,7 @@ public:
 private:
     ZoneTableSegment* segment_;
     LoadAction load_action_;
-    InstallAction install_action_;
+    dns::Name origin_;
     dns::RRClass rrclass_;
     ZoneData* zone_data_;
     // The load was performed

src/lib/datasrc/memory/zone_table_segment.h

@@ -45,6 +45,26 @@ public:
         return (table.get());
     }
 
+    /// \brief Method to set the internal table
+    ///
+    /// The interface is tentative, we don't know if this is the correct place
+    /// and way to set the data. But for now, we need something to be there
+    /// at least for the tests. So we have this. For this reason, there are
+    /// no tests for this method directly. Do not use in actuall
+    /// implementation.
+    ///
+    /// It can be used only once, to initially set it. It can't replace the
+    /// one already there.
+    ///
+    /// \param table Pointer to the table to use.
+    /// \throw isc::Unexpected if called the second time.
+    void setTable(ZoneTable* table) {
+        if (this->table.get() != NULL) {
+            isc_throw(isc::Unexpected, "Replacing table");
+        }
+        this->table = table;
+    }
+
 private:
     boost::interprocess::offset_ptr<ZoneTable> table;
 };

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

@@ -46,18 +46,22 @@ public:
             ZoneReloaderLocal(segment_.get(),
                               bind(&ZoneReloaderLocalTest::loadAction, this,
                                    _1),
-                              bind(&ZoneReloaderLocalTest::installAction, this,
-                                   _1, _2),
-                              RRClass::IN())),
+                              Name("example.org"), RRClass::IN())),
         load_called_(false),
-        install_called_(false),
         load_throw_(false),
-        install_throw_(false),
         load_null_(false)
-    {}
+    {
+        // TODO: The setTable is only a temporary interface
+        segment_->getHeader().
+            setTable(ZoneTable::create(segment_->getMemorySegment(),
+                                       RRClass::IN()));
+    }
     void TearDown() {
         // Release the reloader
         reloader_.reset();
+        // Release the table we used
+        ZoneTable::destroy(segment_->getMemorySegment(),
+                           segment_->getHeader().getTable(), RRClass::IN());
         // And check we freed all memory
         EXPECT_TRUE(segment_->getMemorySegment().allMemoryDeallocated());
     }
@@ -65,9 +69,7 @@ protected:
     scoped_ptr<ZoneTableSegment> segment_;
     scoped_ptr<ZoneReloaderLocal> reloader_;
     bool load_called_;
-    bool install_called_;
     bool load_throw_;
-    bool install_throw_;
     bool load_null_;
 private:
     ZoneData* loadAction(isc::util::MemorySegment& segment) {
@@ -88,32 +90,6 @@ private:
         // goes inside.
         return (ZoneData::create(segment, Name("example.org")));
     }
-    ZoneData* installAction(const ZoneSegmentID&, ZoneSegment* segment) {
-        install_called_ = true;
-
-        // Check we got something
-        if (segment == NULL) {
-            ADD_FAILURE() << "Zone segment is NULL in install action";
-            return (NULL);
-        }
-        EXPECT_NE(static_cast<const ZoneData*>(NULL), segment->getZoneData());
-
-        if (install_throw_) {
-            // In case we throw, we do so before releasing the memory there,
-            // as in that case we don't claim the ownership of the data.
-            throw TestException();
-        }
-
-        // We received ownership of the parameters here. And we don't really need
-        // them in the tests, so we just release them.
-        ZoneData::destroy(segment_->getMemorySegment(), segment->getZoneData(),
-                          RRClass::IN());
-        delete segment;
-
-        // And we are supposed to pass the old version back. So we create
-        // new zone data and pass it.
-        return (ZoneData::create(segment_->getMemorySegment(), Name("exmaple.org")));
-    }
 };
 
 // We call it the way we are supposed to, check every callback is called in the
@@ -121,17 +97,14 @@ private:
 TEST_F(ZoneReloaderLocalTest, correctCall) {
     // Nothing called before we call it
     EXPECT_FALSE(load_called_);
-    EXPECT_FALSE(install_called_);
 
     // Just the load gets called now
     EXPECT_NO_THROW(reloader_->load());
     EXPECT_TRUE(load_called_);
-    EXPECT_FALSE(install_called_);
     load_called_ = false;
 
     EXPECT_NO_THROW(reloader_->install());
     EXPECT_FALSE(load_called_);
-    EXPECT_TRUE(install_called_);
 
     // We don't check explicitly how this works, but call it to free memory. If
     // everything is freed should be checked inside the TearDown.
@@ -142,18 +115,15 @@ TEST_F(ZoneReloaderLocalTest, loadTwice) {
     // Load it the first time
     EXPECT_NO_THROW(reloader_->load());
     EXPECT_TRUE(load_called_);
-    EXPECT_FALSE(install_called_);
     load_called_ = false;
 
     // The second time, it should not be possible
     EXPECT_THROW(reloader_->load(), isc::Unexpected);
     EXPECT_FALSE(load_called_);
-    EXPECT_FALSE(install_called_);
 
     // The object should not be damaged, try installing and clearing now
     EXPECT_NO_THROW(reloader_->install());
     EXPECT_FALSE(load_called_);
-    EXPECT_TRUE(install_called_);
 
     // We don't check explicitly how this works, but call it to free memory. If
     // everything is freed should be checked inside the TearDown.
@@ -167,18 +137,16 @@ TEST_F(ZoneReloaderLocalTest, loadLater) {
     EXPECT_NO_THROW(reloader_->load());
     EXPECT_NO_THROW(reloader_->install());
     // Reset so we see nothing is called now
-    install_called_ = load_called_ = false;
+    load_called_ = false;
 
     EXPECT_THROW(reloader_->load(), isc::Unexpected);
     EXPECT_FALSE(load_called_);
-    EXPECT_FALSE(install_called_);
 
     // Cleanup and try loading again. Still shouldn't work.
     EXPECT_NO_THROW(reloader_->cleanup());
 
     EXPECT_THROW(reloader_->load(), isc::Unexpected);
     EXPECT_FALSE(load_called_);
-    EXPECT_FALSE(install_called_);
 }
 
 // Try calling install at various bad times
@@ -186,17 +154,14 @@ TEST_F(ZoneReloaderLocalTest, invalidInstall) {
     // Nothing loaded yet
     EXPECT_THROW(reloader_->install(), isc::Unexpected);
     EXPECT_FALSE(load_called_);
-    EXPECT_FALSE(install_called_);
 
     EXPECT_NO_THROW(reloader_->load());
     load_called_ = false;
     // This install is OK
     EXPECT_NO_THROW(reloader_->install());
-    install_called_ = false;
     // But we can't call it second time now
     EXPECT_THROW(reloader_->install(), isc::Unexpected);
     EXPECT_FALSE(load_called_);
-    EXPECT_FALSE(install_called_);
 }
 
 // We check we can clean without installing first and nothing bad
@@ -207,11 +172,9 @@ TEST_F(ZoneReloaderLocalTest, cleanWithoutInstall) {
     EXPECT_NO_THROW(reloader_->cleanup());
 
     EXPECT_TRUE(load_called_);
-    EXPECT_FALSE(install_called_);
 
     // We cleaned up, no way to install now
     EXPECT_THROW(reloader_->install(), isc::Unexpected);
-    EXPECT_FALSE(install_called_);
 }
 
 // Test the case when load callback throws
@@ -222,29 +185,11 @@ TEST_F(ZoneReloaderLocalTest, loadThrows) {
     // We can't install now
     EXPECT_THROW(reloader_->install(), isc::Unexpected);
     EXPECT_TRUE(load_called_);
-    EXPECT_FALSE(install_called_);
 
     // But we can cleanup
     EXPECT_NO_THROW(reloader_->cleanup());
 }
 
-// Test we free all our memory even when we throw from install
-TEST_F(ZoneReloaderLocalTest, installThrows) {
-    install_throw_ = true;
-    EXPECT_NO_THROW(reloader_->load());
-
-    EXPECT_THROW(reloader_->install(), TestException);
-    EXPECT_TRUE(load_called_);
-    EXPECT_TRUE(install_called_);
-
-    // We can't try again
-    install_throw_ = false;
-    EXPECT_THROW(reloader_->install(), isc::Unexpected);
-
-    // But it is not completely broken
-    EXPECT_NO_THROW(reloader_->cleanup());
-}
-
 // Check the reloader defends itsefl when load action returns NULL
 TEST_F(ZoneReloaderLocalTest, loadNull) {
     load_null_ = true;