[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.

src/bin/auth/auth_srv.cc

@@ -269,8 +269,7 @@ public:
     const shared_ptr<TSIGKeyRing>* keyring_;
 
     /// The data source client list
-    shared_ptr<std::map<RRClass, shared_ptr<ConfigurableClientList> > >
+    AuthSrv::DataSrcClientListsPtr datasrc_client_lists_;
-        datasrc_client_lists_;
 
     shared_ptr<ConfigurableClientList> 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<ConfigurableClientList>
 AuthSrv::getClientList(const RRClass& rrclass) {
     return (impl_->getClientList(rrclass));

src/bin/auth/auth_srv.h

@@ -15,10 +15,11 @@
 #ifndef __AUTH_SRV_H
 #define __AUTH_SRV_H 1
 
-#include <string>
-
 #include <config/ccsession.h>
+
 #include <datasrc/factory.h>
+#include <datasrc/client_list.h>
+
 #include <dns/message.h>
 #include <dns/opcode.h>
 #include <util/buffer.h>
@@ -35,6 +36,11 @@
 #include <server_common/portconfig.h>
 #include <auth/statistics.h>
 
+#include <boost/shared_ptr.hpp>
+
+#include <map>
+#include <string>
+
 namespace isc {
 namespace util {
 namespace io {
@@ -309,6 +315,14 @@ public:
                        boost::shared_ptr<isc::datasrc::ConfigurableClientList>&
                        list);
 
+    typedef boost::shared_ptr<std::map<
+        isc::dns::RRClass, boost::shared_ptr<
+                               isc::datasrc::ConfigurableClientList> > >
+    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.

src/bin/auth/datasrc_config.h

@@ -19,10 +19,10 @@
 
 #include <cc/data.h>
 #include <datasrc/client_list.h>
-#include <util/threads/lock.h>
 
 #include <boost/shared_ptr.hpp>
 
+#include <utility>
 #include <set>
 
 /// \brief Configure the authoritative server's data source lists
@@ -45,67 +45,25 @@ configureDataSourceGeneric(Server& server,
 {
     typedef boost::shared_ptr<List> ListPtr;
     typedef std::map<std::string, isc::data::ConstElementPtr> Map;
-    typedef std::pair<isc::dns::RRClass, ListPtr> RollbackPair;
+    typedef std::map<isc::dns::RRClass, ListPtr> ListMap;
-    typedef std::pair<isc::dns::RRClass, isc::data::ConstElementPtr>
-        RollbackConfiguration;
 
-    // Lock the client lists, we're going to manipulate them.
+    boost::shared_ptr<ListMap> new_lists(new ListMap);
-    isc::util::thread::Mutex::Locker locker(server.getClientListMutex());
 
-    // Some structures to be able to perform a rollback
+    // Get the configuration and current state.
-    std::vector<RollbackPair> rollback_sets;
+    const Map& map(config->mapValue());
-    std::vector<RollbackConfiguration> rollback_configurations;
+
-    try {
+    // Go through the configuration and create corresponding list.
-        // Get the configuration and current state.
+    for (Map::const_iterator it(map.begin()); it != map.end(); ++it) {
-        const Map& map(config->mapValue());
+        const isc::dns::RRClass rrclass(it->first);
-        const std::vector<isc::dns::RRClass>
+        ListPtr list(new List(rrclass));
-            activeVector(server.getClientListClasses());
+        list->configure(it->second, true);
-        std::set<isc::dns::RRClass> active(activeVector.begin(),
+        new_lists->insert(std::pair<isc::dns::RRClass, ListPtr>(rrclass,
-                                           activeVector.end());
+                                                                list));
-        // 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<isc::dns::RRClass>::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<RollbackPair>::const_iterator
-                 it(rollback_sets.begin()); it != rollback_sets.end(); ++it) {
-            server.setClientList(it->first, it->second);
-        }
-        for (typename std::vector<RollbackConfiguration>::const_iterator
-                 it(rollback_configurations.begin());
-             it != rollback_configurations.end(); ++it) {
-            server.getClientList(it->first)->configure(it->second, true);
-        }
-        throw;
     }
+
+    // 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

src/bin/auth/tests/command_unittest.cc

@@ -16,6 +16,8 @@
 
 #include "datasrc_util.h"
 
+#include <util/threads/lock.h>
+
 #include <auth/auth_srv.h>
 #include <auth/auth_config.h>
 #include <auth/command.h>

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
+    // To pretend to be the server:
-    ListPtr getClientList(const RRClass& rrclass) {
+    void swapDataSrcClientLists(shared_ptr<std::map<dns::RRClass, ListPtr> >
-        log_ += "get " + rrclass.toText() + "\n";
+                                new_lists)
-        return (lists_[rrclass]);
+    {
-    }
+        lists_.clear();         // first empty it
-    void setClientList(const RRClass& rrclass, const ListPtr& list) {
+
-        log_ += "set " + rrclass.toText() + " " +
+        // Record the operation and results.  Note that map elements are
-            (list ? list->getConf() : "") + "\n";
+        // sorted by RRClass, so the ordering should be predictable.
-        lists_[rrclass] = list;
+        for (std::map<dns::RRClass, ListPtr>::const_iterator it =
-    }
+                 new_lists->begin();
-    vector<RRClass> getClientListClasses() const {
+             it != new_lists->end();
-        vector<RRClass> result;
+             ++it)
-        for (std::map<RRClass, ListPtr>::const_iterator it(lists_.begin());
+        {
-             it != lists_.end(); ++it) {
+            const RRClass rrclass = it->first;
-            result.push_back(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
+        // Check that the passed config is stored.
-        // set a new one.
+        EXPECT_EQ("set IN xxx\n", log_);
-        EXPECT_EQ("get IN\nset 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
+    // Now the new one should be installed.
-    EXPECT_EQ("get IN\n", log_);
-    // But this should contain the yyy configuration
     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("set IN yyy\nset CH xxx\n", log_);
-    EXPECT_EQ("get CH\nset CH xxx\nget IN\n", log_);
-    // But this should contain the yyy configuration
     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()]);
+    // No operation takes place in the configuration, and the old one is
-    // In real auth server, the NULL one would be removed. However, we just
+    // just dropped
-    // store it, so the IN bucket is still in there. This checks there's nothing
+    EXPECT_EQ("", log_);
-    // else.
+    EXPECT_TRUE(lists_.empty());
-    EXPECT_EQ(1, lists_.size());
 }
 
 // Check that we can rollback an addition if something else fails