From: Michal 'vorner' Vaner Date: Sat, 13 Oct 2012 11:03:26 +0000 (+0200) Subject: [2207] Deprecate InstallAction X-Git-Tag: trac2402_base~12^2~26 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=c0e090f453d3a1627ccd0591e0ec0abc3e80a01b;p=thirdparty%2Fkea.git [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. --- diff --git a/src/lib/datasrc/memory/zone_reloader.cc b/src/lib/datasrc/memory/zone_reloader.cc index 3027a70f8c..7a3bd544ab 100644 --- a/src/lib/datasrc/memory/zone_reloader.cc +++ b/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 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 diff --git a/src/lib/datasrc/memory/zone_reloader.h b/src/lib/datasrc/memory/zone_reloader.h index 904b603b44..70572f7180 100644 --- a/src/lib/datasrc/memory/zone_reloader.h +++ b/src/lib/datasrc/memory/zone_reloader.h @@ -13,6 +13,7 @@ // PERFORMANCE OF THIS SOFTWARE. #include +#include #include @@ -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 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 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 diff --git a/src/lib/datasrc/memory/zone_table_segment.h b/src/lib/datasrc/memory/zone_table_segment.h index 7fd1310229..3c927e7944 100644 --- a/src/lib/datasrc/memory/zone_table_segment.h +++ b/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 table; }; diff --git a/src/lib/datasrc/tests/memory/zone_reloader_unittest.cc b/src/lib/datasrc/tests/memory/zone_reloader_unittest.cc index ba9d5c22e4..a841f29ace 100644 --- a/src/lib/datasrc/tests/memory/zone_reloader_unittest.cc +++ b/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 segment_; scoped_ptr 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(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;