[master] Merge branch 'trac2204'

src/bin/auth/auth_srv.cc

@@ -69,6 +69,8 @@
 
 using namespace std;
 
+using boost::shared_ptr;
+
 using namespace isc;
 using namespace isc::cc;
 using namespace isc::datasrc;
@@ -264,23 +266,22 @@ public:
     AddressList listen_addresses_;
 
     /// The TSIG keyring
-    const boost::shared_ptr<TSIGKeyRing>* keyring_;
+    const shared_ptr<TSIGKeyRing>* keyring_;
 
-    /// The client list
-    std::map<RRClass, boost::shared_ptr<ConfigurableClientList> >
-        client_lists_;
+    /// The data source client list
+    AuthSrv::DataSrcClientListsPtr datasrc_client_lists_;
 
-    boost::shared_ptr<ConfigurableClientList> getClientList(const RRClass&
-                                                            rrclass)
+    shared_ptr<ConfigurableClientList> getDataSrcClientList(
+        const RRClass& rrclass)
     {
         // TODO: Debug-build only check
         if (!mutex_.locked()) {
             isc_throw(isc::Unexpected, "Not locked!");
         }
-        const std::map<RRClass, boost::shared_ptr<ConfigurableClientList> >::
-            const_iterator it(client_lists_.find(rrclass));
-        if (it == client_lists_.end()) {
-            return (boost::shared_ptr<ConfigurableClientList>());
+        const std::map<RRClass, shared_ptr<ConfigurableClientList> >::
+            const_iterator it(datasrc_client_lists_->find(rrclass));
+        if (it == datasrc_client_lists_->end()) {
+            return (shared_ptr<ConfigurableClientList>());
         } else {
             return (it->second);
         }
@@ -335,6 +336,8 @@ AuthSrvImpl::AuthSrvImpl(AbstractXfroutClient& xfrout_client,
     xfrin_session_(NULL),
     counters_(),
     keyring_(NULL),
+    datasrc_client_lists_(new std::map<RRClass,
+                          shared_ptr<ConfigurableClientList> >()),
     ddns_base_forwarder_(ddns_forwarder),
     ddns_forwarder_(NULL),
     xfrout_connected_(false),
@@ -645,13 +648,13 @@ AuthSrvImpl::processNormalQuery(const IOMessage& io_message, Message& message,
     }
     // Lock the client lists and keep them under the lock until the processing
     // and rendering is done (this is the same mutex as from
-    // AuthSrv::getClientListMutex()).
+    // AuthSrv::getDataSrcClientListMutex()).
     isc::util::thread::Mutex::Locker locker(mutex_);
 
     try {
         const ConstQuestionPtr question = *message.beginQuestion();
-        const boost::shared_ptr<datasrc::ClientList>
-            list(getClientList(question->getClass()));
+        const shared_ptr<datasrc::ClientList>
+            list(getDataSrcClientList(question->getClass()));
         if (list) {
             const RRType& qtype = question->getType();
             const Name& qname = question->getName();
@@ -911,7 +914,7 @@ AuthSrv::setDNSService(isc::asiodns::DNSServiceBase& dnss) {
 }
 
 void
-AuthSrv::setTSIGKeyRing(const boost::shared_ptr<TSIGKeyRing>* keyring) {
+AuthSrv::setTSIGKeyRing(const shared_ptr<TSIGKeyRing>* keyring) {
     impl_->keyring_ = keyring;
 }
 
@@ -930,43 +933,23 @@ AuthSrv::destroyDDNSForwarder() {
     }
 }
 
-void
-AuthSrv::setClientList(const RRClass& rrclass,
-                       const boost::shared_ptr<ConfigurableClientList>& list) {
+AuthSrv::DataSrcClientListsPtr
+AuthSrv::swapDataSrcClientLists(DataSrcClientListsPtr new_lists) {
     // TODO: Debug-build only check
     if (!impl_->mutex_.locked()) {
-        isc_throw(isc::Unexpected, "Not locked");
-    }
-
-    if (list) {
-        impl_->client_lists_[rrclass] = list;
-    } else {
-        impl_->client_lists_.erase(rrclass);
+        isc_throw(isc::Unexpected, "Not locked!");
     }
-}
-boost::shared_ptr<ConfigurableClientList>
-AuthSrv::getClientList(const RRClass& rrclass) {
-    return (impl_->getClientList(rrclass));
+    std::swap(new_lists, impl_->datasrc_client_lists_);
+    return (new_lists);
 }
 
-vector<RRClass>
-AuthSrv::getClientListClasses() const {
-    // TODO: Debug-build only check
-    if (!impl_->mutex_.locked()) {
-        isc_throw(isc::Unexpected, "Not locked");
-    }
-
-    vector<RRClass> result;
-    for (std::map<RRClass, boost::shared_ptr<ConfigurableClientList> >::
-         const_iterator it(impl_->client_lists_.begin());
-         it != impl_->client_lists_.end(); ++it) {
-        result.push_back(it->first);
-    }
-    return (result);
+shared_ptr<ConfigurableClientList>
+AuthSrv::getDataSrcClientList(const RRClass& rrclass) {
+    return (impl_->getDataSrcClientList(rrclass));
 }
 
 util::thread::Mutex&
-AuthSrv::getClientListMutex() const {
+AuthSrv::getDataSrcClientListMutex() const {
     return (impl_->mutex_);
 }
 

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 {
@@ -296,31 +302,46 @@ 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.
+    /// \brief Shortcut typedef used for swapDataSrcClientLists().
+    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 getDataSrcClientListMutex()
+    /// before calling this method.
     ///
-    /// Replaces the internally used client list with a new one. Other
-    /// classes are not changed.
+    /// 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.
     ///
-    /// \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);
+    /// 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);
 
     /// \brief Returns the currently used client list for the class.
     ///
     /// \param rrclass The class for which to get the list.
     /// \return The list, or NULL if no list is set for the class.
     boost::shared_ptr<isc::datasrc::ConfigurableClientList>
-        getClientList(const isc::dns::RRClass& rrclass);
-
-    /// \brief Returns a list of classes that have a client list.
-    ///
-    /// \return List of classes for which a non-NULL client list
-    ///     has been set by setClientList.
-    std::vector<isc::dns::RRClass> getClientListClasses() const;
+        getDataSrcClientList(const isc::dns::RRClass& rrclass);
 
     /// \brief Return a mutex for the client lists.
     ///
@@ -331,9 +352,9 @@ public:
     /// is correct:
     /// \code
     /// {
-    ///  Mutex::Locker locker(auth->getClientListMutex());
+    ///  Mutex::Locker locker(auth->getDataSrcClientListMutex());
     ///  boost::shared_ptr<isc::datasrc::ConfigurableClientList>
-    ///    list(auth->getClientList(RRClass::IN()));
+    ///    list(auth->getDataSrcClientList(RRClass::IN()));
     ///  // Do some processing here
     /// }
     /// \endcode
@@ -342,8 +363,8 @@ public:
     /// \code
     /// boost::shared_ptr<isc::datasrc::ConfigurableClientList> list;
     /// {
-    ///     Mutex::Locker locker(auth->getClientListMutex());
-    ///     list = auth->getClientList(RRClass::IN()));
+    ///     Mutex::Locker locker(auth->getDataSrcClientListMutex());
+    ///     list = auth->getDataSrcClientList(RRClass::IN()));
     /// }
     /// // Do some processing here
     /// \endcode
@@ -352,7 +373,7 @@ public:
     ///    (lock) the mutex. It's because locking of the mutex is not really
     ///    a modification of the server object and it is needed to protect the
     ///    lists even on read-only operations.
-    isc::util::thread::Mutex& getClientListMutex() const;
+    isc::util::thread::Mutex& getDataSrcClientListMutex() const;
 
     /// \brief Sets the timeout for incoming TCP connections
     ///

src/bin/auth/benchmarks/query_bench.cc

@@ -18,6 +18,8 @@
 #include <bench/benchmark_util.h>
 
 #include <util/buffer.h>
+#include <util/threads/lock.h>
+
 #include <dns/message.h>
 #include <dns/name.h>
 #include <dns/question.h>
@@ -125,13 +127,15 @@ public:
                           OutputBuffer& buffer) :
         QueryBenchMark(queries, query_message, buffer)
     {
-        configureDataSource(
-            *server_,
-            Element::fromJSON("{\"IN\":"
-                              "  [{\"type\": \"sqlite3\","
-                              "    \"params\": {"
-                              "      \"database_file\": \"" +
-                              string(datasrc_file) + "\"}}]}"));
+        isc::util::thread::Mutex::Locker locker(
+                server_->getDataSrcClientListMutex());
+        server_->swapDataSrcClientLists(
+            configureDataSource(
+                Element::fromJSON("{\"IN\":"
+                                  "  [{\"type\": \"sqlite3\","
+                                  "    \"params\": {"
+                                  "      \"database_file\": \"" +
+                                  string(datasrc_file) + "\"}}]}")));
     }
 };
 
@@ -144,14 +148,16 @@ public:
                          OutputBuffer& buffer) :
         QueryBenchMark(queries, query_message, buffer)
     {
-        configureDataSource(
-            *server_,
-            Element::fromJSON("{\"IN\":"
-                              "  [{\"type\": \"MasterFiles\","
-                              "    \"cache-enable\": true, "
-                              "    \"params\": {\"" +
-                              string(zone_origin) + "\": \"" +
-                              string(zone_file) + "\"}}]}"));
+        isc::util::thread::Mutex::Locker locker(
+                server_->getDataSrcClientListMutex());
+        server_->swapDataSrcClientLists(
+            configureDataSource(
+                Element::fromJSON("{\"IN\":"
+                                  "  [{\"type\": \"MasterFiles\","
+                                  "    \"cache-enable\": true, "
+                                  "    \"params\": {\"" +
+                                  string(zone_origin) + "\": \"" +
+                                  string(zone_file) + "\"}}]}")));
     }
 };
 

src/bin/auth/command.cc

@@ -192,9 +192,10 @@ public:
 
         // We're going to work with the client lists. They may be used
         // from a different thread too, protect them.
-        isc::util::thread::Mutex::Locker locker(server.getClientListMutex());
+        isc::util::thread::Mutex::Locker locker(
+            server.getDataSrcClientListMutex());
         const boost::shared_ptr<isc::datasrc::ConfigurableClientList>
-            list(server.getClientList(zone_class));
+            list(server.getDataSrcClientList(zone_class));
 
         if (!list) {
             isc_throw(AuthCommandError, "There's no client list for "

src/bin/auth/datasrc_config.cc

@@ -18,9 +18,8 @@
 
 // This is a trivial specialization for the commonly used version.
 // Defined in .cc to avoid accidental creation of multiple copies.
-void
-configureDataSource(AuthSrv& server, const isc::data::ConstElementPtr& config)
-{
-    return (configureDataSourceGeneric<AuthSrv,
-            isc::datasrc::ConfigurableClientList>(server, config));
+AuthSrv::DataSrcClientListsPtr
+configureDataSource(const isc::data::ConstElementPtr& config) {
+    return (configureDataSourceGeneric<
+            isc::datasrc::ConfigurableClientList>(config));
 }

src/bin/auth/datasrc_config.h

@@ -19,99 +19,62 @@
 
 #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
+/// \brief Configure data source client lists
 ///
 /// This will hook into the data_sources module configuration and it will
-/// keep the local copy of data source clients in the list in the authoritative
-/// server.
+/// return a new set (in the form of a shared pointer to map) of data source
+/// client lists corresponding to the configuration.
 ///
 /// This function is templated. This is simply because of easier testing.
 /// You don't need to pay attention to it, use the configureDataSource
 /// specialization instead.
 ///
-/// \param server It is the server to configure.
+/// \note In future we may want to make the reconfiguration more efficient
+/// by only creating newly configured data and just moving the rest from
+/// the running configuration if they are used in the new configuration
+/// without any parameter change.  We could probably do it by passing
+/// the old lists in addition to the new config, but further details are
+/// still to be defined yet.  It will surely require changes in the
+/// data source library, too.  So, right now, we don't introduce the
+/// possibility in the function interface.  If and when we decide to introduce
+/// the optimization, we'll extend the interface.
+///
 /// \param config The configuration value to parse. It is in the form
 ///     as an update from the config manager.
-template<class Server, class List>
-void
-configureDataSourceGeneric(Server& server,
-                           const isc::data::ConstElementPtr& config)
-{
+/// \return A map from RR classes to configured lists.
+template<class List>
+boost::shared_ptr<std::map<isc::dns::RRClass,
+                           boost::shared_ptr<List> > > // = ListMap below
+configureDataSourceGeneric(const isc::data::ConstElementPtr& config) {
     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::pair<isc::dns::RRClass, isc::data::ConstElementPtr>
-        RollbackConfiguration;
+    typedef std::map<isc::dns::RRClass, ListPtr> ListMap;
 
-    // Lock the client lists, we're going to manipulate them.
-    isc::util::thread::Mutex::Locker locker(server.getClientListMutex());
+    boost::shared_ptr<ListMap> new_lists(new ListMap);
 
-    // Some structures to be able to perform a rollback
-    std::vector<RollbackPair> rollback_sets;
-    std::vector<RollbackConfiguration> rollback_configurations;
-    try {
-        // Get the configuration and current state.
-        const Map& map(config->mapValue());
-        const std::vector<isc::dns::RRClass>
-            activeVector(server.getClientListClasses());
-        std::set<isc::dns::RRClass> 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<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;
+    // Go through the configuration and create corresponding list.
+    const Map& map(config->mapValue());
+    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<isc::dns::RRClass, ListPtr>(rrclass,
+                                                                list));
     }
+
+    return (new_lists);
 }
 
 /// \brief Concrete version of configureDataSource() for the
 ///     use with authoritative server implementation.
-void
-configureDataSource(AuthSrv& server, const isc::data::ConstElementPtr& config);
+AuthSrv::DataSrcClientListsPtr
+configureDataSource(const isc::data::ConstElementPtr& config);
 
 #endif  // DATASRC_CONFIG_H
 

src/bin/auth/main.cc

@@ -18,6 +18,7 @@
 
 #include <util/buffer.h>
 #include <util/io/socketsession.h>
+#include <util/threads/lock.h>
 
 #include <dns/message.h>
 #include <dns/messagerenderer.h>
@@ -93,18 +94,31 @@ datasrcConfigHandler(AuthSrv* server, bool* first_time,
 {
     assert(server != NULL);
     if (config->contains("classes")) {
+        AuthSrv::DataSrcClientListsPtr lists;
+
         if (*first_time) {
             // HACK: The default is not passed to the handler in the first
             // callback. This one will get the default (or, current value).
             // Further updates will work the usual way.
             assert(config_session != NULL);
             *first_time = false;
-            configureDataSource(*auth_server,
-                                config_session->getRemoteConfigValue(
-                                    "data_sources", "classes"));
+            lists = configureDataSource(
+                config_session->getRemoteConfigValue("data_sources",
+                                                     "classes"));
         } else {
-            configureDataSource(*server, config->get("classes"));
+            lists = configureDataSource(config->get("classes"));
+        }
+
+        // Replace the server's lists.  The returned lists will be stored
+        // in a local variable 'lists', and will be destroyed outside of
+        // the temporary block for the lock scope.  That way we can minimize
+        // the range of the critical section.
+        {
+            isc::util::thread::Mutex::Locker locker(
+                server->getDataSrcClientListMutex());
+            lists = server->swapDataSrcClientLists(lists);
         }
+        // The previous lists are destroyed here.
     }
 }
 

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() :
@@ -722,13 +726,21 @@ TEST_F(AuthSrvTest, notifyWithSessionMessageError) {
 }
 
 void
+installDataSrcClientLists(AuthSrv& server,
+                          AuthSrv::DataSrcClientListsPtr lists)
+{
+    thread::Mutex::Locker locker(server.getDataSrcClientListMutex());
+    server.swapDataSrcClientLists(lists);
+}
+
+void
 updateDatabase(AuthSrv& server, const char* params) {
     const ConstElementPtr config(Element::fromJSON("{"
         "\"IN\": [{"
         "    \"type\": \"sqlite3\","
         "    \"params\": " + string(params) +
         "}]}"));
-    configureDataSource(server, config);
+    installDataSrcClientLists(server, configureDataSource(config));
 }
 
 void
@@ -745,7 +757,7 @@ updateInMemory(AuthSrv& server, const char* origin, const char* filename) {
         "   \"type\": \"static\","
         "   \"params\": \"" + string(STATIC_DSRC_FILE) + "\""
         "}]}"));
-    configureDataSource(server, config);
+    installDataSrcClientLists(server, configureDataSource(config));
 }
 
 void
@@ -755,7 +767,7 @@ updateBuiltin(AuthSrv& server) {
         "   \"type\": \"static\","
         "   \"params\": \"" + string(STATIC_DSRC_FILE) + "\""
         "}]}"));
-    configureDataSource(server, config);
+    installDataSrcClientLists(server, configureDataSource(config));
 }
 
 // Try giving the server a TSIG signed request and see it can anwer signed as
@@ -953,7 +965,7 @@ TEST_F(AuthSrvTest, updateWithInMemoryClient) {
         "   \"params\": {},"
         "   \"cache-enable\": true"
         "}]}"));
-    configureDataSource(server, config);
+    installDataSrcClientLists(server, configureDataSource(config));
     // after successful configuration, we should have one (with empty zoneset).
 
     // The memory data source is empty, should return REFUSED rcode.
@@ -1427,11 +1439,14 @@ TEST_F(AuthSrvTest,
     // Set real inmem client to proxy
     updateInMemory(server, "example.", CONFIG_INMEMORY_EXAMPLE);
     {
-        isc::util::thread::Mutex::Locker locker(server.getClientListMutex());
+        isc::util::thread::Mutex::Locker locker(
+            server.getDataSrcClientListMutex());
         boost::shared_ptr<isc::datasrc::ConfigurableClientList>
-            list(new FakeList(server.getClientList(RRClass::IN()), THROW_NEVER,
-                              false));
-        server.setClientList(RRClass::IN(), list);
+            list(new FakeList(server.getDataSrcClientList(RRClass::IN()),
+                              THROW_NEVER, false));
+        AuthSrv::DataSrcClientListsPtr lists(new std::map<RRClass, ListPtr>);
+        lists->insert(pair<RRClass, ListPtr>(RRClass::IN(), list));
+        server.swapDataSrcClientLists(lists);
     }
 
     createDataFromFile("nsec3query_nodnssec_fromWire.wire");
@@ -1455,11 +1470,14 @@ setupThrow(AuthSrv& server, ThrowWhen throw_when, bool isc_exception,
 {
     updateInMemory(server, "example.", CONFIG_INMEMORY_EXAMPLE);
 
-    isc::util::thread::Mutex::Locker locker(server.getClientListMutex());
+    isc::util::thread::Mutex::Locker locker(
+        server.getDataSrcClientListMutex());
     boost::shared_ptr<isc::datasrc::ConfigurableClientList>
-        list(new FakeList(server.getClientList(RRClass::IN()), throw_when,
-                          isc_exception, rrset));
-    server.setClientList(RRClass::IN(), list);
+        list(new FakeList(server.getDataSrcClientList(RRClass::IN()),
+                          throw_when, isc_exception, rrset));
+    AuthSrv::DataSrcClientListsPtr lists(new std::map<RRClass, ListPtr>);
+    lists->insert(pair<RRClass, ListPtr>(RRClass::IN(), list));
+    server.swapDataSrcClientLists(lists);
 }
 
 TEST_F(AuthSrvTest,
@@ -1771,46 +1789,51 @@ TEST_F(AuthSrvTest, clientList) {
     // We need to lock the mutex to make the (get|set)ClientList happy.
     // 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());
+    isc::util::thread::Mutex::Locker locker(
+        server.getDataSrcClientListMutex());
+
+    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]);
-    EXPECT_EQ(list, server.getClientList(RRClass::IN()));
+    EXPECT_EQ(list, server.getDataSrcClientList(RRClass::IN()));
+    EXPECT_EQ(list2, server.getDataSrcClientList(RRClass::CH()));
+
+    // 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.getDataSrcClientList(RRClass::IN()));
+    EXPECT_FALSE(server.getDataSrcClientList(RRClass::CH()));
 }
 
 // We just test the mutex can be locked (exactly once).
 TEST_F(AuthSrvTest, mutex) {
-    isc::util::thread::Mutex::Locker l1(server.getClientListMutex());
+    isc::util::thread::Mutex::Locker l1(server.getDataSrcClientListMutex());
     // TODO: Once we have non-debug build, this one will not work, since
     // we currently use the fact that we can't lock twice from the same
     // thread. In the non-debug mode, this would deadlock.
     // Skip then.
     EXPECT_THROW({
-        isc::util::thread::Mutex::Locker l2(server.getClientListMutex());
+        isc::util::thread::Mutex::Locker l2(
+            server.getDataSrcClientListMutex());
     }, isc::InvalidOperation);
 }
 

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>
@@ -174,22 +176,32 @@ TEST_F(AuthCommandTest, shutdownIncorrectPID) {
 // zones, and checks the zones are correctly loaded.
 void
 zoneChecks(AuthSrv& server) {
-    isc::util::thread::Mutex::Locker locker(server.getClientListMutex());
-    EXPECT_EQ(ZoneFinder::SUCCESS, server.getClientList(RRClass::IN())->
+    isc::util::thread::Mutex::Locker locker(
+        server.getDataSrcClientListMutex());
+    EXPECT_EQ(ZoneFinder::SUCCESS, server.getDataSrcClientList(RRClass::IN())->
               find(Name("ns.test1.example")).finder_->
               find(Name("ns.test1.example"), RRType::A())->code);
-    EXPECT_EQ(ZoneFinder::NXRRSET, server.getClientList(RRClass::IN())->
+    EXPECT_EQ(ZoneFinder::NXRRSET, server.getDataSrcClientList(RRClass::IN())->
               find(Name("ns.test1.example")).finder_->
               find(Name("ns.test1.example"), RRType::AAAA())->code);
-    EXPECT_EQ(ZoneFinder::SUCCESS, server.getClientList(RRClass::IN())->
+    EXPECT_EQ(ZoneFinder::SUCCESS, server.getDataSrcClientList(RRClass::IN())->
               find(Name("ns.test2.example")).finder_->
               find(Name("ns.test2.example"), RRType::A())->code);
-    EXPECT_EQ(ZoneFinder::NXRRSET, server.getClientList(RRClass::IN())->
+    EXPECT_EQ(ZoneFinder::NXRRSET, server.getDataSrcClientList(RRClass::IN())->
               find(Name("ns.test2.example")).finder_->
               find(Name("ns.test2.example"), RRType::AAAA())->code);
 }
 
 void
+installDataSrcClientLists(AuthSrv& server,
+                          AuthSrv::DataSrcClientListsPtr lists)
+{
+    isc::util::thread::Mutex::Locker locker(
+        server.getDataSrcClientListMutex());
+    server.swapDataSrcClientLists(lists);
+}
+
+void
 configureZones(AuthSrv& server) {
     ASSERT_EQ(0, system(INSTALL_PROG " -c " TEST_DATA_DIR "/test1.zone.in "
                         TEST_DATA_BUILDDIR "/test1.zone.copied"));
@@ -208,27 +220,29 @@ configureZones(AuthSrv& server) {
         "   \"cache-enable\": true"
         "}]}"));
 
-    configureDataSource(server, config);
+    installDataSrcClientLists(server, configureDataSource(config));
 
     zoneChecks(server);
 }
 
 void
 newZoneChecks(AuthSrv& server) {
-    isc::util::thread::Mutex::Locker locker(server.getClientListMutex());
-    EXPECT_EQ(ZoneFinder::SUCCESS, server.getClientList(RRClass::IN())->
+    isc::util::thread::Mutex::Locker locker(
+        server.getDataSrcClientListMutex());
+    EXPECT_EQ(ZoneFinder::SUCCESS, server.getDataSrcClientList(RRClass::IN())->
               find(Name("ns.test1.example")).finder_->
               find(Name("ns.test1.example"), RRType::A())->code);
     // now test1.example should have ns/AAAA
-    EXPECT_EQ(ZoneFinder::SUCCESS, server.getClientList(RRClass::IN())->
+    EXPECT_EQ(ZoneFinder::SUCCESS, server.getDataSrcClientList(RRClass::IN())->
               find(Name("ns.test1.example")).finder_->
               find(Name("ns.test1.example"), RRType::AAAA())->code);
 
     // test2.example shouldn't change
-    EXPECT_EQ(ZoneFinder::SUCCESS, server.getClientList(RRClass::IN())->
+    EXPECT_EQ(ZoneFinder::SUCCESS, server.getDataSrcClientList(RRClass::IN())->
               find(Name("ns.test2.example")).finder_->
               find(Name("ns.test2.example"), RRType::A())->code);
-    EXPECT_EQ(ZoneFinder::NXRRSET, server.getClientList(RRClass::IN())->
+    EXPECT_EQ(ZoneFinder::NXRRSET,
+              server.getDataSrcClientList(RRClass::IN())->
               find(Name("ns.test2.example")).finder_->
               find(Name("ns.test2.example"), RRType::AAAA())->code);
 }
@@ -271,12 +285,14 @@ TEST_F(AuthCommandTest,
         "    \"cache-enable\": true,"
         "    \"cache-zones\": [\"example.org\"]"
         "}]}"));
-    configureDataSource(server_, config);
+    installDataSrcClientLists(server_, configureDataSource(config));
 
     {
-        isc::util::thread::Mutex::Locker locker(server_.getClientListMutex());
+        isc::util::thread::Mutex::Locker locker(
+            server_.getDataSrcClientListMutex());
         // Check that the A record at www.example.org does not exist
-        EXPECT_EQ(ZoneFinder::NXDOMAIN, server_.getClientList(RRClass::IN())->
+        EXPECT_EQ(ZoneFinder::NXDOMAIN,
+                  server_.getDataSrcClientList(RRClass::IN())->
                   find(Name("example.org")).finder_->
                   find(Name("www.example.org"), RRType::A())->code);
 
@@ -296,7 +312,8 @@ TEST_F(AuthCommandTest,
         sql_updater->addRRset(*rrset);
         sql_updater->commit();
 
-        EXPECT_EQ(ZoneFinder::NXDOMAIN, server_.getClientList(RRClass::IN())->
+        EXPECT_EQ(ZoneFinder::NXDOMAIN,
+                  server_.getDataSrcClientList(RRClass::IN())->
                   find(Name("example.org")).finder_->
                   find(Name("www.example.org"), RRType::A())->code);
     }
@@ -308,9 +325,11 @@ TEST_F(AuthCommandTest,
     checkAnswer(0, "Successful load");
 
     {
-        isc::util::thread::Mutex::Locker locker(server_.getClientListMutex());
+        isc::util::thread::Mutex::Locker locker(
+            server_.getDataSrcClientListMutex());
         // And now it should be present too.
-        EXPECT_EQ(ZoneFinder::SUCCESS, server_.getClientList(RRClass::IN())->
+        EXPECT_EQ(ZoneFinder::SUCCESS,
+                  server_.getDataSrcClientList(RRClass::IN())->
                   find(Name("example.org")).finder_->
                   find(Name("www.example.org"), RRType::A())->code);
     }
@@ -321,9 +340,11 @@ TEST_F(AuthCommandTest,
     checkAnswer(1, "example.com");
 
     {
-        isc::util::thread::Mutex::Locker locker(server_.getClientListMutex());
+        isc::util::thread::Mutex::Locker locker(
+            server_.getDataSrcClientListMutex());
         // The previous zone is not hurt in any way
-        EXPECT_EQ(ZoneFinder::SUCCESS, server_.getClientList(RRClass::IN())->
+        EXPECT_EQ(ZoneFinder::SUCCESS,
+                  server_.getDataSrcClientList(RRClass::IN())->
                   find(Name("example.org")).finder_->
                   find(Name("example.org"), RRType::SOA())->code);
     }
@@ -335,16 +356,18 @@ TEST_F(AuthCommandTest,
         "    \"cache-enable\": true,"
         "    \"cache-zones\": [\"example.com\"]"
         "}]}"));
-    EXPECT_THROW(configureDataSource(server_, config2),
+    EXPECT_THROW(configureDataSource(config2),
                  ConfigurableClientList::ConfigurationError);
 
     result_ = execAuthServerCommand(server_, "loadzone",
         Element::fromJSON("{\"origin\": \"example.com\"}"));
     checkAnswer(1, "Unreadable");
 
-    isc::util::thread::Mutex::Locker locker(server_.getClientListMutex());
+    isc::util::thread::Mutex::Locker locker(
+        server_.getDataSrcClientListMutex());
     // The previous zone is not hurt in any way
-    EXPECT_EQ(ZoneFinder::SUCCESS, server_.getClientList(RRClass::IN())->
+    EXPECT_EQ(ZoneFinder::SUCCESS,
+              server_.getDataSrcClientList(RRClass::IN())->
               find(Name("example.org")).finder_->
               find(Name("example.org"), RRType::SOA())->code);
 }

src/bin/auth/tests/datasrc_config_unittest.cc

@@ -60,14 +60,11 @@ private:
 
 typedef shared_ptr<FakeList> ListPtr;
 
+// Forward declaration.  We need precise definition of DatasrcConfigTest
+// to complete this function.
 void
 testConfigureDataSource(DatasrcConfigTest& test,
-                        const isc::data::ConstElementPtr& config)
-{
-    // We use the test fixture for the Server type.  This makes it possible
-    // to easily fake all needed methods and look that they were called.
-    configureDataSourceGeneric<DatasrcConfigTest, FakeList>(test, config);
-}
+                        const isc::data::ConstElementPtr& config);
 
 void
 datasrcConfigHandler(DatasrcConfigTest* fake_server, const std::string&,
@@ -82,26 +79,29 @@ 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;
+    isc::util::thread::Mutex& getDataSrcClientListMutex() const {
+        return (mutex_);
     }
-    vector<RRClass> getClientListClasses() const {
-        vector<RRClass> result;
-        for (std::map<RRClass, ListPtr>::const_iterator it(lists_.begin());
-             it != lists_.end(); ++it) {
-            result.push_back(it->first);
+    void swapDataSrcClientLists(shared_ptr<std::map<dns::RRClass, ListPtr> >
+                                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<dns::RRClass, ListPtr>::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;
@@ -160,14 +159,27 @@ protected:
     mutable isc::util::thread::Mutex mutex_;
 };
 
+void
+testConfigureDataSource(DatasrcConfigTest& test,
+                        const isc::data::ConstElementPtr& config)
+{
+    // We use customized (faked lists) for the List type.  This makes it
+    // possible to easily look that they were called.
+    shared_ptr<std::map<dns::RRClass, ListPtr> > lists =
+        configureDataSourceGeneric<FakeList>(config);
+    test.swapDataSrcClientLists(lists);
+}
+
 // Push there a configuration with a single list.
 TEST_F(DatasrcConfigTest, createList) {
     initializeINList();
 }
 
 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 +187,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 +201,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 +222,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,18 +237,18 @@ 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
-TEST_F(DatasrcConfigTest, rollbackAddition) {
+// Check that broken new configuration doesn't break the running configuration.
+TEST_F(DatasrcConfigTest, brokenConfigForAdd) {
     initializeINList();
-    // The configuration is wrong. However, the CH one will get done first.
+    // The configuration is wrong. However, the CH one will be handled
+    // without an error first.
     const ElementPtr
         config(buildConfig("{\"IN\": [{\"type\": 13}], "
                            "\"CH\": [{\"type\": \"xxx\"}]}"));
@@ -256,8 +264,9 @@ TEST_F(DatasrcConfigTest, rollbackAddition) {
     EXPECT_FALSE(lists_[RRClass::CH()]);
 }
 
-// Check that we can rollback a deletion if something else fails
-TEST_F(DatasrcConfigTest, rollbackDeletion) {
+// Similar to the previous one, but the broken config would delete part of
+// the running config.
+TEST_F(DatasrcConfigTest, brokenConfigForDelete) {
     initializeINList();
     // Put the CH there
     const ElementPtr
@@ -266,27 +275,25 @@ TEST_F(DatasrcConfigTest, rollbackDeletion) {
     testConfigureDataSource(*this, config1);
     const ElementPtr
         config2(Element::fromJSON("{\"IN\": [{\"type\": 13}]}"));
-    // This would delete CH. However, the IN one fails.
-    // As the deletions happen after the additions/settings
-    // and there's no known way to cause an exception during the
-    // deletions, it is not a true rollback, but the result should
-    // be the same.
+    // This would delete CH. However, the new config is broken, so it won't
+    // actually apply.
     EXPECT_THROW(testConfigureDataSource(*this, config2), TypeError);
     EXPECT_EQ("yyy", lists_[RRClass::IN()]->getConf());
     EXPECT_EQ("xxx", lists_[RRClass::CH()]->getConf());
 }
 
-// Check that we can roll back configuration change if something
-// fails later on.
-TEST_F(DatasrcConfigTest, rollbackConfiguration) {
+// Similar to the previous cases, but the broken config would modify the
+// running config of one of the classes.
+TEST_F(DatasrcConfigTest, brokenConfigForModify) {
     initializeINList();
     // Put the CH there
     const ElementPtr
         config1(Element::fromJSON("{\"IN\": [{\"type\": \"yyy\"}], "
                                   "\"CH\": [{\"type\": \"xxx\"}]}"));
     testConfigureDataSource(*this, config1);
-    // Now, the CH happens first. But nevertheless, it should be
-    // restored to the previoeus version.
+    // Now, the CH change will be handled first without an error, then
+    // the change to the IN class will fail, and the none of the changes
+    // will apply.
     const ElementPtr
         config2(Element::fromJSON("{\"IN\": [{\"type\": 13}], "
                                   "\"CH\": [{\"type\": \"yyy\"}]}"));