[2204] completely replaced setClientList with swapDataSrcClientLists.

the test cases using setClientList were updated so they use
swapDataSrcClientLists (some of them work as a test for the "swap" itself).
now we don't need setClientList, so it was removed.

5 files changed, 76 insertions, 60 deletions

diff --git a/src/bin/auth/auth_srv.cc b/src/bin/auth/auth_srv.cc
index 391416b82e..1102ae3ed3 100644
--- a/src/bin/auth/auth_srv.cc
+++ b/src/bin/auth/auth_srv.cc
@@ -931,29 +931,13 @@ AuthSrv::destroyDDNSForwarder() {
     }
 }
 
-void
-AuthSrv::setClientList(const RRClass& rrclass,
-                       const shared_ptr<ConfigurableClientList>& list)
-{
-    // TODO: Debug-build only check
-    if (!impl_->mutex_.locked()) {
-        isc_throw(isc::Unexpected, "Not locked");
-    }
-
-    if (list) {
-        (*impl_->datasrc_client_lists_)[rrclass] = list;
-    } else {
-        impl_->datasrc_client_lists_->erase(rrclass);
-    }
-}
-
 AuthSrv::DataSrcClientListsPtr
 AuthSrv::swapDataSrcClientLists(DataSrcClientListsPtr new_lists) {
-    {
-        thread::Mutex::Locker locker(impl_->mutex_);
-        std::swap(new_lists, impl_->datasrc_client_lists_);
+    // TODO: Debug-build only check
+    if (!impl_->mutex_.locked()) {
+        isc_throw(isc::Unexpected, "Not locked!");
     }
-
+    std::swap(new_lists, impl_->datasrc_client_lists_);
     return (new_lists);
 }
 
diff --git a/src/bin/auth/auth_srv.h b/src/bin/auth/auth_srv.h
index 48aac067b9..f1e73c1158 100644
--- a/src/bin/auth/auth_srv.h
+++ b/src/bin/auth/auth_srv.h
@@ -302,24 +302,36 @@ public:
     /// If there was no forwarder yet, this method does nothing.
     void destroyDDNSForwarder();
 
-    /// \brief Sets the currently used list for data sources of given
-    ///     class.
-    ///
-    /// Replaces the internally used client list with a new one. Other
-    /// classes are not changed.
-    ///
-    /// \param rrclass The class to modify.
-    /// \param list Shared pointer to the client list. If it is NULL,
-    ///     the list is removed instead.
-    void setClientList(const isc::dns::RRClass& rrclass, const
-                       boost::shared_ptr<isc::datasrc::ConfigurableClientList>&
-                       list);
-
     typedef boost::shared_ptr<std::map<
         isc::dns::RRClass, boost::shared_ptr<
                                isc::datasrc::ConfigurableClientList> > >
     DataSrcClientListsPtr;
 
+    /// \brief Swap the currently used set of data source client lists with
+    /// given one.
+    ///
+    /// The "set" of lists is actually given in the form of map from
+    /// RRClasses to shared pointers to isc::datasrc::ConfigurableClientList.
+    ///
+    /// This method returns the swapped set of lists, which was previously
+    /// used by the server.
+    ///
+    /// This method is intended to be used by a separate method to update
+    /// the data source configuration "at once".  The caller must hold
+    /// a lock for the mutex object returned by  \c getClientListMutex()
+    /// before calling this method.
+    ///
+    /// The ownership of the returned pointer is transferred to the caller.
+    /// The caller is generally expected to release the resources used in
+    /// the old lists.  Note that it could take longer time if some of the
+    /// data source clients contain a large size of in-memory data.
+    ///
+    /// The caller can pass a NULL pointer.  This effectively disables
+    /// any data source for the server.
+    ///
+    /// \param new_lists Shared pointer to a new set of data source client
+    /// lists.
+    /// \return The previous set of lists.  It can be NULL.
     DataSrcClientListsPtr swapDataSrcClientLists(DataSrcClientListsPtr
                                                  new_lists);
 
diff --git a/src/bin/auth/datasrc_config.h b/src/bin/auth/datasrc_config.h
index 02354ffa26..532aa34a2f 100644
--- a/src/bin/auth/datasrc_config.h
+++ b/src/bin/auth/datasrc_config.h
@@ -17,6 +17,8 @@
 
 #include "auth_srv.h"
 
+#include <util/threads/lock.h>
+
 #include <cc/data.h>
 #include <datasrc/client_list.h>
 
@@ -62,8 +64,12 @@ configureDataSourceGeneric(Server& server,
     }
 
     // Replace the server's lists.  By ignoring the return value we let the
-    // old lists be destroyed.
-    server.swapDataSrcClientLists(new_lists);
+    // old lists be destroyed.  Lock will be released immediately after the
+    // swap.
+    {
+        isc::util::thread::Mutex::Locker locker(server.getClientListMutex());
+        server.swapDataSrcClientLists(new_lists);
+    }
 }
 
 /// \brief Concrete version of configureDataSource() for the
diff --git a/src/bin/auth/tests/auth_srv_unittest.cc b/src/bin/auth/tests/auth_srv_unittest.cc
index 60a9a2ac38..dd60d896ad 100644
--- a/src/bin/auth/tests/auth_srv_unittest.cc
+++ b/src/bin/auth/tests/auth_srv_unittest.cc
@@ -63,6 +63,7 @@
 using namespace std;
 using namespace isc::cc;
 using namespace isc::dns;
+using namespace isc::datasrc;
 using namespace isc::util;
 using namespace isc::util::io::internal;
 using namespace isc::util::unittests;
@@ -90,6 +91,9 @@ const char* const STATIC_DSRC_FILE = DSRC_DIR "/static.zone";
 // a signed example zone.
 const char* const CONFIG_INMEMORY_EXAMPLE = TEST_DATA_DIR "/rfc5155-example.zone.signed";
 
+// shortcut commonly used in tests
+typedef boost::shared_ptr<ConfigurableClientList> ListPtr;
+
 class AuthSrvTest : public SrvTestBase {
 protected:
     AuthSrvTest() :
@@ -1431,7 +1435,9 @@ TEST_F(AuthSrvTest,
         boost::shared_ptr<isc::datasrc::ConfigurableClientList>
             list(new FakeList(server.getClientList(RRClass::IN()), THROW_NEVER,
                               false));
-        server.setClientList(RRClass::IN(), list);
+        AuthSrv::DataSrcClientListsPtr lists(new std::map<RRClass, ListPtr>);
+        lists->insert(pair<RRClass, ListPtr>(RRClass::IN(), list));
+        server.swapDataSrcClientLists(lists);
     }
 
     createDataFromFile("nsec3query_nodnssec_fromWire.wire");
@@ -1459,7 +1465,9 @@ setupThrow(AuthSrv& server, ThrowWhen throw_when, bool isc_exception,
     boost::shared_ptr<isc::datasrc::ConfigurableClientList>
         list(new FakeList(server.getClientList(RRClass::IN()), throw_when,
                           isc_exception, rrset));
-    server.setClientList(RRClass::IN(), list);
+    AuthSrv::DataSrcClientListsPtr lists(new std::map<RRClass, ListPtr>);
+    lists->insert(pair<RRClass, ListPtr>(RRClass::IN(), list));
+    server.swapDataSrcClientLists(lists);
 }
 
 TEST_F(AuthSrvTest,
@@ -1772,34 +1780,37 @@ TEST_F(AuthSrvTest, clientList) {
     // There's a debug-build only check in them to make sure everything
     // locks them and we call them directly here.
     isc::util::thread::Mutex::Locker locker(server.getClientListMutex());
+
+    AuthSrv::DataSrcClientListsPtr lists; // initially empty
+
     // The lists don't exist. Therefore, the list of RRClasses is empty.
-    // We also have no IN list.
-    EXPECT_TRUE(server.getClientListClasses().empty());
-    EXPECT_EQ(boost::shared_ptr<const isc::datasrc::ClientList>(),
-              server.getClientList(RRClass::IN()));
+    EXPECT_TRUE(server.swapDataSrcClientLists(lists)->empty());
+
     // Put something in.
-    const boost::shared_ptr<isc::datasrc::ConfigurableClientList>
-        list(new isc::datasrc::ConfigurableClientList(RRClass::IN()));
-    const boost::shared_ptr<isc::datasrc::ConfigurableClientList>
-        list2(new isc::datasrc::ConfigurableClientList(RRClass::CH()));
-    server.setClientList(RRClass::IN(), list);
-    server.setClientList(RRClass::CH(), list2);
-    // There are two things in the list and they are IN and CH
-    vector<RRClass> classes(server.getClientListClasses());
-    ASSERT_EQ(2, classes.size());
-    EXPECT_EQ(RRClass::IN(), classes[0]);
-    EXPECT_EQ(RRClass::CH(), classes[1]);
+    const ListPtr list(new ConfigurableClientList(RRClass::IN()));
+    const ListPtr list2(new ConfigurableClientList(RRClass::CH()));
+
+    lists.reset(new std::map<RRClass, ListPtr>);
+    lists->insert(pair<RRClass, ListPtr>(RRClass::IN(), list));
+    lists->insert(pair<RRClass, ListPtr>(RRClass::CH(), list2));
+    server.swapDataSrcClientLists(lists);
+
     // And the lists can be retrieved.
     EXPECT_EQ(list, server.getClientList(RRClass::IN()));
     EXPECT_EQ(list2, server.getClientList(RRClass::CH()));
-    // Remove one of them
-    server.setClientList(RRClass::CH(),
-        boost::shared_ptr<isc::datasrc::ConfigurableClientList>());
-    // This really got deleted, including the class.
-    classes = server.getClientListClasses();
-    ASSERT_EQ(1, classes.size());
-    EXPECT_EQ(RRClass::IN(), classes[0]);
+
+    // Replace the lists with new lists containing only one list.
+    lists.reset(new std::map<RRClass, ListPtr>);
+    lists->insert(pair<RRClass, ListPtr>(RRClass::IN(), list));
+    lists = server.swapDataSrcClientLists(lists);
+
+    // Old one had two lists.  That confirms our swap for IN and CH classes
+    // (i.e., no other entries were there).
+    EXPECT_EQ(2, lists->size());
+
+    // The CH list really got deleted.
     EXPECT_EQ(list, server.getClientList(RRClass::IN()));
+    EXPECT_FALSE(server.getClientList(RRClass::CH()));
 }
 
 // We just test the mutex can be locked (exactly once).
diff --git a/src/bin/auth/tests/datasrc_config_unittest.cc b/src/bin/auth/tests/datasrc_config_unittest.cc
index c0c9b5ce99..329c5d11be 100644
--- a/src/bin/auth/tests/datasrc_config_unittest.cc
+++ b/src/bin/auth/tests/datasrc_config_unittest.cc
@@ -81,7 +81,10 @@ datasrcConfigHandler(DatasrcConfigTest* fake_server, const std::string&,
 
 class DatasrcConfigTest : public ::testing::Test {
 public:
-    // To pretend to be the server:
+    // These pretend to be the server
+    isc::util::thread::Mutex& getClientListMutex() const {
+        return (mutex_);
+    }
     void swapDataSrcClientLists(shared_ptr<std::map<dns::RRClass, ListPtr> >
                                 new_lists)
     {