From: JINMEI Tatuya Date: Fri, 5 Oct 2012 22:15:18 +0000 (-0700) Subject: [2204] simplify configureDataSource by always creating a new lists and swap. X-Git-Tag: trac2402_base~15^2~56 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=919fe74ccbeecf5d546b60f017c3e9e1b073655a;p=thirdparty%2Fkea.git [2204] simplify configureDataSource by always creating a new lists and swap. so we don't have to worry about what are in the current lists or rollback operations. swapDataSrcClientLists() is newly introduced for AuthSrv. No direc tests yet (technically bad in terms TDD but the definition is very simple), which will be provided in the next step. the lock is now moved inside swapDataSrcClientLists(). note: even though this version builds everything, the amount of work should be mostly the same because the only save is to create the empty ClientList when the new and old have the same class of client. The expensive part is ClientList::configure(). This version doesn't need any more call to configure() than the old version. --- diff --git a/src/bin/auth/auth_srv.cc b/src/bin/auth/auth_srv.cc index 880a4ac1e8..391416b82e 100644 --- a/src/bin/auth/auth_srv.cc +++ b/src/bin/auth/auth_srv.cc @@ -269,8 +269,7 @@ public: const shared_ptr* keyring_; /// The data source client list - shared_ptr > > - datasrc_client_lists_; + AuthSrv::DataSrcClientListsPtr datasrc_client_lists_; shared_ptr getClientList(const RRClass& rrclass) { // TODO: Debug-build only check @@ -948,6 +947,16 @@ AuthSrv::setClientList(const RRClass& rrclass, } } +AuthSrv::DataSrcClientListsPtr +AuthSrv::swapDataSrcClientLists(DataSrcClientListsPtr new_lists) { + { + thread::Mutex::Locker locker(impl_->mutex_); + std::swap(new_lists, impl_->datasrc_client_lists_); + } + + return (new_lists); +} + shared_ptr AuthSrv::getClientList(const RRClass& rrclass) { return (impl_->getClientList(rrclass)); diff --git a/src/bin/auth/auth_srv.h b/src/bin/auth/auth_srv.h index ee7bd52bb2..48aac067b9 100644 --- a/src/bin/auth/auth_srv.h +++ b/src/bin/auth/auth_srv.h @@ -15,10 +15,11 @@ #ifndef __AUTH_SRV_H #define __AUTH_SRV_H 1 -#include - #include + #include +#include + #include #include #include @@ -35,6 +36,11 @@ #include #include +#include + +#include +#include + namespace isc { namespace util { namespace io { @@ -309,6 +315,14 @@ public: boost::shared_ptr& list); + typedef boost::shared_ptr > > + DataSrcClientListsPtr; + + DataSrcClientListsPtr swapDataSrcClientLists(DataSrcClientListsPtr + new_lists); + /// \brief Returns the currently used client list for the class. /// /// \param rrclass The class for which to get the list. diff --git a/src/bin/auth/datasrc_config.h b/src/bin/auth/datasrc_config.h index 79ace2811a..02354ffa26 100644 --- a/src/bin/auth/datasrc_config.h +++ b/src/bin/auth/datasrc_config.h @@ -19,10 +19,10 @@ #include #include -#include #include +#include #include /// \brief Configure the authoritative server's data source lists @@ -45,67 +45,25 @@ configureDataSourceGeneric(Server& server, { typedef boost::shared_ptr ListPtr; typedef std::map Map; - typedef std::pair RollbackPair; - typedef std::pair - RollbackConfiguration; + typedef std::map ListMap; - // Lock the client lists, we're going to manipulate them. - isc::util::thread::Mutex::Locker locker(server.getClientListMutex()); + boost::shared_ptr new_lists(new ListMap); - // Some structures to be able to perform a rollback - std::vector rollback_sets; - std::vector rollback_configurations; - try { - // Get the configuration and current state. - const Map& map(config->mapValue()); - const std::vector - activeVector(server.getClientListClasses()); - std::set active(activeVector.begin(), - activeVector.end()); - // Go through the configuration and change everything. - for (Map::const_iterator it(map.begin()); it != map.end(); ++it) { - const isc::dns::RRClass rrclass(it->first); - active.erase(rrclass); - ListPtr list(server.getClientList(rrclass)); - bool need_set(false); - if (list) { - rollback_configurations. - push_back(RollbackConfiguration(rrclass, - list->getConfiguration())); - } else { - list.reset(new List(rrclass)); - need_set = true; - rollback_sets.push_back(RollbackPair(rrclass, ListPtr())); - } - list->configure(it->second, true); - if (need_set) { - server.setClientList(rrclass, list); - } - } - // Remove the ones that are not in the configuration. - for (std::set::iterator it(active.begin()); - it != active.end(); ++it) { - // There seems to be no way the setClientList could throw. - // But this is just to make sure in case it did to restore - // the original. - rollback_sets.push_back( - RollbackPair(*it, server.getClientList(*it))); - server.setClientList(*it, ListPtr()); - } - } catch (...) { - // Perform a rollback of the changes. The old configuration should - // work. - for (typename std::vector::const_iterator - it(rollback_sets.begin()); it != rollback_sets.end(); ++it) { - server.setClientList(it->first, it->second); - } - for (typename std::vector::const_iterator - it(rollback_configurations.begin()); - it != rollback_configurations.end(); ++it) { - server.getClientList(it->first)->configure(it->second, true); - } - throw; + // Get the configuration and current state. + const Map& map(config->mapValue()); + + // Go through the configuration and create corresponding list. + for (Map::const_iterator it(map.begin()); it != map.end(); ++it) { + const isc::dns::RRClass rrclass(it->first); + ListPtr list(new List(rrclass)); + list->configure(it->second, true); + new_lists->insert(std::pair(rrclass, + list)); } + + // Replace the server's lists. By ignoring the return value we let the + // old lists be destroyed. + server.swapDataSrcClientLists(new_lists); } /// \brief Concrete version of configureDataSource() for the diff --git a/src/bin/auth/tests/command_unittest.cc b/src/bin/auth/tests/command_unittest.cc index c7994831a5..b5e43edda2 100644 --- a/src/bin/auth/tests/command_unittest.cc +++ b/src/bin/auth/tests/command_unittest.cc @@ -16,6 +16,8 @@ #include "datasrc_util.h" +#include + #include #include #include diff --git a/src/bin/auth/tests/datasrc_config_unittest.cc b/src/bin/auth/tests/datasrc_config_unittest.cc index 3d3aa58b91..c0c9b5ce99 100644 --- a/src/bin/auth/tests/datasrc_config_unittest.cc +++ b/src/bin/auth/tests/datasrc_config_unittest.cc @@ -81,27 +81,27 @@ datasrcConfigHandler(DatasrcConfigTest* fake_server, const std::string&, class DatasrcConfigTest : public ::testing::Test { public: - // These pretend to be the server - ListPtr getClientList(const RRClass& rrclass) { - log_ += "get " + rrclass.toText() + "\n"; - return (lists_[rrclass]); - } - void setClientList(const RRClass& rrclass, const ListPtr& list) { - log_ += "set " + rrclass.toText() + " " + - (list ? list->getConf() : "") + "\n"; - lists_[rrclass] = list; - } - vector getClientListClasses() const { - vector result; - for (std::map::const_iterator it(lists_.begin()); - it != lists_.end(); ++it) { - result.push_back(it->first); + // To pretend to be the server: + void swapDataSrcClientLists(shared_ptr > + new_lists) + { + lists_.clear(); // first empty it + + // Record the operation and results. Note that map elements are + // sorted by RRClass, so the ordering should be predictable. + for (std::map::const_iterator it = + new_lists->begin(); + it != new_lists->end(); + ++it) + { + const RRClass rrclass = it->first; + ListPtr list = it->second; + log_ += "set " + rrclass.toText() + " " + + (list ? list->getConf() : "") + "\n"; + lists_[rrclass] = list; } - return (result); - } - isc::util::thread::Mutex& getClientListMutex() const { - return (mutex_); } + protected: DatasrcConfigTest() : session(ElementPtr(new ListElement), ElementPtr(new ListElement), @@ -147,9 +147,8 @@ protected: session.addMessage(createCommand("config_update", config), "data_sources", "*"); mccs->checkCommand(); - // Check it called the correct things (check that there's no IN yet and - // set a new one. - EXPECT_EQ("get IN\nset IN xxx\n", log_); + // Check that the passed config is stored. + EXPECT_EQ("set IN xxx\n", log_); EXPECT_EQ(1, lists_.size()); } FakeSession session; @@ -166,8 +165,10 @@ TEST_F(DatasrcConfigTest, createList) { } TEST_F(DatasrcConfigTest, modifyList) { - // First, initialize the list + // First, initialize the list, and confirm the current config initializeINList(); + EXPECT_EQ("xxx", lists_[RRClass::IN()]->getConf()); + // And now change the configuration of the list const ElementPtr config(buildConfig("{\"IN\": [{\"type\": \"yyy\"}]}")); @@ -175,9 +176,7 @@ TEST_F(DatasrcConfigTest, modifyList) { "*"); log_ = ""; mccs->checkCommand(); - // This one does not set - EXPECT_EQ("get IN\n", log_); - // But this should contain the yyy configuration + // Now the new one should be installed. EXPECT_EQ("yyy", lists_[RRClass::IN()]->getConf()); EXPECT_EQ(1, lists_.size()); } @@ -191,7 +190,7 @@ TEST_F(DatasrcConfigTest, multiple) { "*"); mccs->checkCommand(); // We have set commands for both classes. - EXPECT_EQ("get CH\nset CH xxx\nget IN\nset IN yyy\n", log_); + EXPECT_EQ("set IN yyy\nset CH xxx\n", log_); // We should have both there EXPECT_EQ("yyy", lists_[RRClass::IN()]->getConf()); EXPECT_EQ("xxx", lists_[RRClass::CH()]->getConf()); @@ -212,9 +211,7 @@ TEST_F(DatasrcConfigTest, updateAdd) { "*"); log_ = ""; mccs->checkCommand(); - // The CH is set, IN not - EXPECT_EQ("get CH\nset CH xxx\nget IN\n", log_); - // But this should contain the yyy configuration + EXPECT_EQ("set IN yyy\nset CH xxx\n", log_); EXPECT_EQ("xxx", lists_[RRClass::CH()]->getConf()); EXPECT_EQ("yyy", lists_[RRClass::IN()]->getConf()); EXPECT_EQ(2, lists_.size()); @@ -229,12 +226,11 @@ TEST_F(DatasrcConfigTest, updateDelete) { "*"); log_ = ""; mccs->checkCommand(); - EXPECT_EQ("get IN\nset IN \n", log_); - EXPECT_FALSE(lists_[RRClass::IN()]); - // In real auth server, the NULL one would be removed. However, we just - // store it, so the IN bucket is still in there. This checks there's nothing - // else. - EXPECT_EQ(1, lists_.size()); + + // No operation takes place in the configuration, and the old one is + // just dropped + EXPECT_EQ("", log_); + EXPECT_TRUE(lists_.empty()); } // Check that we can rollback an addition if something else fails