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