From b4bdc9b1f5ec444d1bc4002fe7d65a50acc9d897 Mon Sep 17 00:00:00 2001 From: JINMEI Tatuya Date: Tue, 2 Oct 2012 20:24:57 -0700 Subject: [PATCH 1/8] [2203] refactoring 1st step: move session obj outside of datasrc configurator. --- src/bin/auth/datasrc_configurator.h | 24 +++++------------ src/bin/auth/main.cc | 17 ++++++++++-- .../tests/datasrc_configurator_unittest.cc | 27 +++++++++++++------ 3 files changed, 41 insertions(+), 27 deletions(-) diff --git a/src/bin/auth/datasrc_configurator.h b/src/bin/auth/datasrc_configurator.h index 6b2f5826b0..badf62c87c 100644 --- a/src/bin/auth/datasrc_configurator.h +++ b/src/bin/auth/datasrc_configurator.h @@ -54,7 +54,6 @@ private: } } static Server* server_; - static isc::config::ModuleCCSession* session_; typedef boost::shared_ptr ListPtr; public: /// \brief Initializes the class. @@ -74,12 +73,7 @@ public: /// \throw isc::InvalidParameter if any of the parameters is NULL /// \throw isc::config::ModuleCCError if the remote configuration is not /// available for some reason. - static void init(isc::config::ModuleCCSession* session, - Server* server) - { - if (session == NULL) { - isc_throw(isc::InvalidParameter, "The session must not be NULL"); - } + static void init(Server* server) { if (server == NULL) { isc_throw(isc::InvalidParameter, "The server must not be NULL"); } @@ -88,9 +82,8 @@ public: "The configurator is already initialized"); } server_ = server; - session_ = session; - session->addRemoteConfig("data_sources", reconfigureInternal, false); } + /// \brief Deinitializes the class. /// /// This detaches from the session and removes the server from internal @@ -99,12 +92,9 @@ public: /// This can be called even if it is not initialized currently. You /// can initialize it again after this. static void cleanup() { - if (session_ != NULL) { - session_->removeRemoteConfig("data_sources"); - } - session_ = NULL; server_ = NULL; } + /// \brief Reads new configuration and replaces the old one. /// /// It instructs the server to replace the lists with new ones as needed. @@ -210,10 +200,6 @@ public: } }; -template -isc::config::ModuleCCSession* -DataSourceConfiguratorGeneric::session_(NULL); - template Server* DataSourceConfiguratorGeneric::server_(NULL); @@ -224,3 +210,7 @@ typedef DataSourceConfiguratorGenericcontains("classes")) { + DataSourceConfigurator::reconfigure(config->get("classes")); + } +} + void usage() { cerr << "Usage: b10-auth [-v]" @@ -192,7 +202,9 @@ main(int argc, char* argv[]) { auth_server->setTSIGKeyRing(&isc::server_common::keyring); // Start the data source configuration - DataSourceConfigurator::init(config_session, auth_server); + DataSourceConfigurator::init(auth_server); + config_session->addRemoteConfig("data_sources", datasrcConfigHandler, + false); // HACK: The default is not passed to the handler. This one will // get the default (or, current value). Further updates will work // the usual way. @@ -221,7 +233,8 @@ main(int argc, char* argv[]) { xfrin_session->disconnect(); } - DataSourceConfigurator::cleanup(); + //DataSourceConfigurator::cleanup(); + config_session->removeRemoteConfig("data_sources"); delete xfrin_session; delete config_session; delete cc_session; diff --git a/src/bin/auth/tests/datasrc_configurator_unittest.cc b/src/bin/auth/tests/datasrc_configurator_unittest.cc index 254b2380c7..8759a937fb 100644 --- a/src/bin/auth/tests/datasrc_configurator_unittest.cc +++ b/src/bin/auth/tests/datasrc_configurator_unittest.cc @@ -62,6 +62,16 @@ typedef shared_ptr ListPtr; typedef DataSourceConfiguratorGeneric Configurator; +void +datasrcConfigHandler(const std::string&, + isc::data::ConstElementPtr config, + const isc::config::ConfigData&) +{ + if (config->contains("classes")) { + Configurator::reconfigure(config->get("classes")); + } +} + class DatasrcConfiguratorTest : public ::testing::Test { public: // These pretend to be the server @@ -100,6 +110,7 @@ protected: } void TearDown() { // Make sure no matter what we did, it is cleaned up. + mccs->removeRemoteConfig("data_sources"); Configurator::cleanup(); } void init(const ElementPtr& config = ElementPtr()) { @@ -114,7 +125,8 @@ protected: session.getMessages()-> add(createAnswer(0, ElementPtr(new MapElement))); } - Configurator::init(mccs.get(), this); + Configurator::init(this); + mccs->addRemoteConfig("data_sources", datasrcConfigHandler, false); } void SetUp() { init(); @@ -126,10 +138,10 @@ protected: return (external); } void initializeINList() { - const ElementPtr + const ConstElementPtr config(buildConfig("{\"IN\": [{\"type\": \"xxx\"}]}")); - session.addMessage(createCommand("config_update", config), "data_sources", - "*"); + 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. @@ -150,15 +162,14 @@ TEST_F(DatasrcConfiguratorTest, initialization) { EXPECT_THROW(init(), InvalidOperation); EXPECT_TRUE(session.haveSubscription("data_sources", "*")); // Deinitialize to make the tests reasonable + mccs->removeRemoteConfig("data_sources"); Configurator::cleanup(); EXPECT_FALSE(session.haveSubscription("data_sources", "*")); // We can't reconfigure now (not even manually) EXPECT_THROW(Configurator::reconfigure(ElementPtr(new MapElement())), InvalidOperation); - // If one of them is NULL, it does not work - EXPECT_THROW(Configurator::init(NULL, this), InvalidParameter); - EXPECT_FALSE(session.haveSubscription("data_sources", "*")); - EXPECT_THROW(Configurator::init(mccs.get(), NULL), InvalidParameter); + // If the server param is NULL, it does not work + EXPECT_THROW(Configurator::init(NULL), InvalidParameter); EXPECT_FALSE(session.haveSubscription("data_sources", "*")); // But we can initialize it again now EXPECT_NO_THROW(init()); From 4fa694dbee3372b8c310aca2c2dafc3ca7550e80 Mon Sep 17 00:00:00 2001 From: JINMEI Tatuya Date: Tue, 2 Oct 2012 20:38:31 -0700 Subject: [PATCH 2/8] [2203] changed the callback type of addRemoteConfig to boost::function. this will make it more convenient, e.g., by allowing the caller to pass boost::bind encapsulating a class object and a class method. boost::function is upper compatible to function pointer, so it doesn't ensure source-level compatibility. the functor overhead shouldn't matter in this context, and since this module already uses boost::function this change doesn't introduce additional dependency. --- src/lib/config/ccsession.cc | 6 ++---- src/lib/config/ccsession.h | 13 +++++-------- 2 files changed, 7 insertions(+), 12 deletions(-) diff --git a/src/lib/config/ccsession.cc b/src/lib/config/ccsession.cc index daec005dda..8516d6c863 100644 --- a/src/lib/config/ccsession.cc +++ b/src/lib/config/ccsession.cc @@ -338,7 +338,7 @@ getRelatedLoggers(ConstElementPtr loggers) { // Now find the wildcard names (the one that start with "*"). BOOST_FOREACH(ConstElementPtr cur_logger, loggers->listValue()) { - std::string cur_name = cur_logger->get("name")->stringValue(); + const std::string cur_name = cur_logger->get("name")->stringValue(); // If name is '*', or starts with '*.', replace * with root // logger name. if (cur_name == "*" || cur_name.length() > 1 && @@ -671,9 +671,7 @@ ModuleCCSession::fetchRemoteSpec(const std::string& module, bool is_filename) { std::string ModuleCCSession::addRemoteConfig(const std::string& spec_name, - void (*handler)(const std::string& module, - ConstElementPtr, - const ConfigData&), + RemoteHandler handler, bool spec_is_filename) { // First get the module name, specification and default config diff --git a/src/lib/config/ccsession.h b/src/lib/config/ccsession.h index e96a33d44b..4b99a44bd8 100644 --- a/src/lib/config/ccsession.h +++ b/src/lib/config/ccsession.h @@ -283,7 +283,7 @@ public: * the configuration manager must know it). If * spec_is_filename is true (the default), then a * filename is assumed, otherwise a module name. - * \param handler The handler function called whenever there's a change. + * \param handler The handler functor called whenever there's a change. * Called once initally from this function. May be NULL * if you don't want any handler to be called and you're * fine with requesting the data through @@ -296,11 +296,11 @@ public: * \return The name of the module specified in the given specification * file */ + typedef boost::function RemoteHandler; std::string addRemoteConfig(const std::string& spec_name, - void (*handler)(const std::string& module_name, - isc::data::ConstElementPtr - update, - const ConfigData& config_data) = NULL, + RemoteHandler handler = RemoteHandler(), bool spec_is_filename = true); /** @@ -513,9 +513,6 @@ private: const std::string& command, isc::data::ConstElementPtr args); - typedef void (*RemoteHandler)(const std::string&, - isc::data::ConstElementPtr, - const ConfigData&); std::map remote_module_configs_; std::map remote_module_handlers_; From 05136b55f90375a1a7e3a76570cb95bf0590c0ad Mon Sep 17 00:00:00 2001 From: JINMEI Tatuya Date: Wed, 3 Oct 2012 15:24:22 -0700 Subject: [PATCH 3/8] [2203] refactoring 2nd step: configurator can now be a separate object. i.e., it's not a singleton any more. testReconfigure() method isn't needed any more because it doesn't hold CC session internally. DatasrcConfiguratorTest.initialization test currently fails and is disabled for now. The plan is to make the class completely stateless, at which point we don't even have to think about initialization or cleanup, and then the test will be able to be removed. --- src/bin/auth/benchmarks/query_bench.cc | 14 ++-- src/bin/auth/datasrc_configurator.h | 78 ++++--------------- src/bin/auth/main.cc | 45 ++++++----- src/bin/auth/tests/auth_srv_unittest.cc | 59 +++++++------- src/bin/auth/tests/command_unittest.cc | 18 +++-- .../tests/datasrc_configurator_unittest.cc | 36 +++++---- 6 files changed, 112 insertions(+), 138 deletions(-) diff --git a/src/bin/auth/benchmarks/query_bench.cc b/src/bin/auth/benchmarks/query_bench.cc index 93253db8fe..e31668c0cf 100644 --- a/src/bin/auth/benchmarks/query_bench.cc +++ b/src/bin/auth/benchmarks/query_bench.cc @@ -81,6 +81,7 @@ protected: QueryBenchMark(const BenchQueries& queries, Message& query_message, OutputBuffer& buffer) : server_(new AuthSrv(xfrout_client, ddns_forwarder)), + datasrc_configurator_(server_.get()), queries_(queries), query_message_(query_message), buffer_(buffer), @@ -109,6 +110,7 @@ private: MockSocketSessionForwarder ddns_forwarder; protected: AuthSrvPtr server_; + DataSourceConfigurator datasrc_configurator_; private: const BenchQueries& queries_; Message& query_message_; @@ -125,8 +127,7 @@ public: OutputBuffer& buffer) : QueryBenchMark(queries, query_message, buffer) { - DataSourceConfigurator::testReconfigure( - server_.get(), + datasrc_configurator_.reconfigure( Element::fromJSON("{\"IN\":" " [{\"type\": \"sqlite3\"," " \"params\": {" @@ -139,13 +140,12 @@ class MemoryQueryBenchMark : public QueryBenchMark { public: MemoryQueryBenchMark(const char* const zone_file, const char* const zone_origin, - const BenchQueries& queries, - Message& query_message, - OutputBuffer& buffer) : + const BenchQueries& queries, + Message& query_message, + OutputBuffer& buffer) : QueryBenchMark(queries, query_message, buffer) { - DataSourceConfigurator::testReconfigure( - server_.get(), + datasrc_configurator_.reconfigure( Element::fromJSON("{\"IN\":" " [{\"type\": \"MasterFiles\"," " \"cache-enable\": true, " diff --git a/src/bin/auth/datasrc_configurator.h b/src/bin/auth/datasrc_configurator.h index badf62c87c..94a01bd383 100644 --- a/src/bin/auth/datasrc_configurator.h +++ b/src/bin/auth/datasrc_configurator.h @@ -30,32 +30,25 @@ /// keep the local copy of data source clients in the list in the authoritative /// server. /// -/// The class is slightly unusual. Due to some technical limitations, the hook -/// needs to be static method. Therefore it is not possible to create instances -/// of the class. -/// /// Also, the class is a template. This is simply because of easier testing. /// You don't need to pay attention to it, use the DataSourceConfigurator /// type alias instead. template class DataSourceConfiguratorGeneric { private: - /// \brief Disallow creation of instances - DataSourceConfiguratorGeneric(); - /// \brief Internal method to hook into the ModuleCCSession - /// - /// It simply calls reconfigure. - static void reconfigureInternal(const std::string&, - isc::data::ConstElementPtr config, - const isc::config::ConfigData&) - { - if (config->contains("classes")) { - reconfigure(config->get("classes")); - } - } - static Server* server_; + Server* server_; typedef boost::shared_ptr ListPtr; public: + /// \brief Constructor. + /// + /// \throw isc::InvalidParameter if server is NULL + /// \param server The server to configure. + DataSourceConfiguratorGeneric(Server* server) : server_(server) { + if (server == NULL) { + isc_throw(isc::InvalidParameter, "The server must not be NULL"); + } + } + /// \brief Initializes the class. /// /// This configures which session and server should be used. @@ -66,22 +59,13 @@ public: /// session dies, otherwise it might access them after they /// are destroyed. /// - /// \param session The session to hook into and to access the configuration - /// through. /// \param server It is the server to configure. - /// \throw isc::InvalidOperation if this is called when already initialized. + /// \throw isc::InvalidOperation if this is called when already + /// initialized. /// \throw isc::InvalidParameter if any of the parameters is NULL /// \throw isc::config::ModuleCCError if the remote configuration is not /// available for some reason. - static void init(Server* server) { - if (server == NULL) { - isc_throw(isc::InvalidParameter, "The server must not be NULL"); - } - if (server_ != NULL) { - isc_throw(isc::InvalidOperation, - "The configurator is already initialized"); - } - server_ = server; + void init() { } /// \brief Deinitializes the class. @@ -91,7 +75,7 @@ public: /// /// This can be called even if it is not initialized currently. You /// can initialize it again after this. - static void cleanup() { + void cleanup() { server_ = NULL; } @@ -105,7 +89,7 @@ public: /// \param config The configuration value to parse. It is in the form /// as an update from the config manager. /// \throw InvalidOperation if it is called when not initialized. - static void reconfigure(const isc::data::ConstElementPtr& config) { + void reconfigure(const isc::data::ConstElementPtr& config) { if (server_ == NULL) { isc_throw(isc::InvalidOperation, "Can't reconfigure while not initialized by init()"); @@ -171,38 +155,8 @@ public: throw; } } - /// \brief Version of reconfigure for easier testing. - /// - /// This method can be used to reconfigure a server without first - /// initializing the configurator. This does not need a session. - /// Otherwise, it acts the same as reconfigure. - /// - /// This is not meant for production code. Do not use there. - /// - /// \param server The server to configure. - /// \param config The config to use. - /// \throw isc::InvalidOperation if the configurator is initialized. - /// \throw anything that reconfigure does. - static void testReconfigure(Server* server, - const isc::data::ConstElementPtr& config) - { - if (server_ != NULL) { - isc_throw(isc::InvalidOperation, "Currently initialized."); - } - try { - server_ = server; - reconfigure(config); - server_ = NULL; - } catch (...) { - server_ = NULL; - throw; - } - } }; -template -Server* DataSourceConfiguratorGeneric::server_(NULL); - /// \brief Concrete version of DataSourceConfiguratorGeneric for the /// use in authoritative server. typedef DataSourceConfiguratorGeneric -#include -#include -#include -#include -#include -#include -#include - -#include -#include - #include #include @@ -52,6 +41,20 @@ #include #include +#include +#include + +#include +#include +#include +#include +#include +#include +#include + +#include +#include + using namespace std; using namespace isc::asiodns; using namespace isc::asiolink; @@ -84,12 +87,12 @@ my_command_handler(const string& command, ConstElementPtr args) { } void -datasrcConfigHandler(const std::string&, +datasrcConfigHandler(DataSourceConfigurator* configurator, const std::string&, isc::data::ConstElementPtr config, const isc::config::ConfigData&) { if (config->contains("classes")) { - DataSourceConfigurator::reconfigure(config->get("classes")); + configurator->reconfigure(config->get("classes")); } } @@ -137,6 +140,7 @@ main(int argc, char* argv[]) { ModuleCCSession* config_session = NULL; XfroutClient xfrout_client(getXfroutSocketPath()); SocketSessionForwarder ddns_forwarder(getDDNSSocketPath()); + boost::scoped_ptr datasrc_configurator; try { string specfile; if (getenv("B10_FROM_BUILD")) { @@ -202,13 +206,17 @@ main(int argc, char* argv[]) { auth_server->setTSIGKeyRing(&isc::server_common::keyring); // Start the data source configuration - DataSourceConfigurator::init(auth_server); - config_session->addRemoteConfig("data_sources", datasrcConfigHandler, + datasrc_configurator.reset(new DataSourceConfigurator(auth_server)); + config_session->addRemoteConfig("data_sources", + boost::bind(datasrcConfigHandler, + datasrc_configurator.get(), + _1, _2, _3), false); + // HACK: The default is not passed to the handler. This one will // get the default (or, current value). Further updates will work // the usual way. - DataSourceConfigurator::reconfigure( + datasrc_configurator->reconfigure( config_session->getRemoteConfigValue("data_sources", "classes")); // Now start asynchronous read. @@ -233,8 +241,9 @@ main(int argc, char* argv[]) { xfrin_session->disconnect(); } - //DataSourceConfigurator::cleanup(); - config_session->removeRemoteConfig("data_sources"); + if (datasrc_configurator) { + config_session->removeRemoteConfig("data_sources"); + } delete xfrin_session; delete config_session; delete cc_session; diff --git a/src/bin/auth/tests/auth_srv_unittest.cc b/src/bin/auth/tests/auth_srv_unittest.cc index e6e3ef79e6..75a1b8def9 100644 --- a/src/bin/auth/tests/auth_srv_unittest.cc +++ b/src/bin/auth/tests/auth_srv_unittest.cc @@ -98,7 +98,8 @@ protected: // The empty string is expected value of the parameter of // requestSocket, not the app_name (there's no fallback, it checks // the empty string is passed). - sock_requestor_(dnss_, address_store_, 53210, "") + sock_requestor_(dnss_, address_store_, 53210, ""), + datasrc_configurator_(&server) { server.setDNSService(dnss_); server.setXfrinSession(¬ify_session); @@ -183,6 +184,7 @@ protected: vector response_data; AddressList address_store_; TestSocketRequestor sock_requestor_; + DataSourceConfigurator datasrc_configurator_; }; // A helper function that builds a response to version.bind/TXT/CH that @@ -722,17 +724,21 @@ TEST_F(AuthSrvTest, notifyWithSessionMessageError) { } void -updateDatabase(AuthSrv* server, const char* params) { +updateDatabase(DataSourceConfigurator& datasrc_configurator, + const char* params) +{ const ConstElementPtr config(Element::fromJSON("{" "\"IN\": [{" " \"type\": \"sqlite3\"," " \"params\": " + string(params) + "}]}")); - DataSourceConfigurator::testReconfigure(server, config); + datasrc_configurator.reconfigure(config); } void -updateInMemory(AuthSrv* server, const char* origin, const char* filename) { +updateInMemory(DataSourceConfigurator& datasrc_configurator, + const char* origin, const char* filename) +{ const ConstElementPtr config(Element::fromJSON("{" "\"IN\": [{" " \"type\": \"MasterFiles\"," @@ -745,17 +751,17 @@ updateInMemory(AuthSrv* server, const char* origin, const char* filename) { " \"type\": \"static\"," " \"params\": \"" + string(STATIC_DSRC_FILE) + "\"" "}]}")); - DataSourceConfigurator::testReconfigure(server, config); + datasrc_configurator.reconfigure(config); } void -updateBuiltin(AuthSrv* server) { +updateBuiltin(DataSourceConfigurator& datasrc_configurator) { const ConstElementPtr config(Element::fromJSON("{" "\"CH\": [{" " \"type\": \"static\"," " \"params\": \"" + string(STATIC_DSRC_FILE) + "\"" "}]}")); - DataSourceConfigurator::testReconfigure(server, config); + datasrc_configurator.reconfigure(config); } // Try giving the server a TSIG signed request and see it can anwer signed as @@ -766,7 +772,7 @@ TEST_F(AuthSrvTest, DISABLED_TSIGSigned) { // Needs builtin TEST_F(AuthSrvTest, TSIGSigned) { #endif // Prepare key, the client message, etc - updateBuiltin(&server); + updateBuiltin(datasrc_configurator_); const TSIGKey key("key:c2VjcmV0Cg==:hmac-sha1"); TSIGContext context(key); UnitTestUtil::createRequestMessage(request_message, opcode, default_qid, @@ -814,7 +820,7 @@ TEST_F(AuthSrvTest, DISABLED_builtInQueryViaDNSServer) { #else TEST_F(AuthSrvTest, builtInQueryViaDNSServer) { #endif - updateBuiltin(&server); + updateBuiltin(datasrc_configurator_); UnitTestUtil::createRequestMessage(request_message, Opcode::QUERY(), default_qid, Name("VERSION.BIND."), RRClass::CH(), RRType::TXT()); @@ -846,7 +852,7 @@ TEST_F(AuthSrvTest, DISABLED_builtInQuery) { #else TEST_F(AuthSrvTest, builtInQuery) { #endif - updateBuiltin(&server); + updateBuiltin(datasrc_configurator_); UnitTestUtil::createRequestMessage(request_message, Opcode::QUERY(), default_qid, Name("VERSION.BIND."), RRClass::CH(), RRType::TXT()); @@ -867,7 +873,7 @@ TEST_F(AuthSrvTest, DISABLED_iqueryViaDNSServer) { // Needs builtin #else TEST_F(AuthSrvTest, iqueryViaDNSServer) { // Needs builtin #endif - updateBuiltin(&server); + updateBuiltin(datasrc_configurator_); createDataFromFile("iquery_fromWire.wire"); (*server.getDNSLookupProvider())(*io_message, parse_message, response_message, @@ -889,7 +895,7 @@ TEST_F(AuthSrvTest, DISABLED_updateConfig) { #else TEST_F(AuthSrvTest, updateConfig) { #endif - updateDatabase(&server, CONFIG_TESTDB); + updateDatabase(datasrc_configurator_, CONFIG_TESTDB); // query for existent data in the installed data source. The resulting // response should have the AA flag on, and have an RR in each answer @@ -907,7 +913,7 @@ TEST_F(AuthSrvTest, DISABLED_datasourceFail) { #else TEST_F(AuthSrvTest, datasourceFail) { #endif - updateDatabase(&server, CONFIG_TESTDB); + updateDatabase(datasrc_configurator_, CONFIG_TESTDB); // This query will hit a corrupted entry of the data source (the zoneload // tool and the data source itself naively accept it). This will result @@ -927,10 +933,10 @@ TEST_F(AuthSrvTest, DISABLED_updateConfigFail) { TEST_F(AuthSrvTest, updateConfigFail) { #endif // First, load a valid data source. - updateDatabase(&server, CONFIG_TESTDB); + updateDatabase(datasrc_configurator_, CONFIG_TESTDB); // Next, try to update it with a non-existent one. This should fail. - EXPECT_THROW(updateDatabase(&server, BADCONFIG_TESTDB), + EXPECT_THROW(updateDatabase(datasrc_configurator_, BADCONFIG_TESTDB), isc::datasrc::DataSourceError); // The original data source should still exist. @@ -953,7 +959,7 @@ TEST_F(AuthSrvTest, updateWithInMemoryClient) { " \"params\": {}," " \"cache-enable\": true" "}]}")); - DataSourceConfigurator::testReconfigure(&server, config); + datasrc_configurator_.reconfigure(config); // after successful configuration, we should have one (with empty zoneset). // The memory data source is empty, should return REFUSED rcode. @@ -974,7 +980,7 @@ TEST_F(AuthSrvTest, queryWithInMemoryClientNoDNSSEC) { // query handler class, and confirm it returns no error and a non empty // answer section. Detailed examination on the response content // for various types of queries are tested in the query tests. - updateInMemory(&server, "example.", CONFIG_INMEMORY_EXAMPLE); + updateInMemory(datasrc_configurator_, "example.", CONFIG_INMEMORY_EXAMPLE); createDataFromFile("nsec3query_nodnssec_fromWire.wire"); server.processMessage(*io_message, *parse_message, *response_obuffer, @@ -993,7 +999,7 @@ TEST_F(AuthSrvTest, queryWithInMemoryClientDNSSEC) { // Similar to the previous test, but the query has the DO bit on. // The response should contain RRSIGs, and should have more RRs than // the previous case. - updateInMemory(&server, "example.", CONFIG_INMEMORY_EXAMPLE); + updateInMemory(datasrc_configurator_, "example.", CONFIG_INMEMORY_EXAMPLE); createDataFromFile("nsec3query_fromWire.wire"); server.processMessage(*io_message, *parse_message, *response_obuffer, @@ -1013,7 +1019,7 @@ TEST_F(AuthSrvTest, ) { // Set up the in-memory - updateInMemory(&server, "example.", CONFIG_INMEMORY_EXAMPLE); + updateInMemory(datasrc_configurator_, "example.", CONFIG_INMEMORY_EXAMPLE); // This shouldn't affect the result of class CH query UnitTestUtil::createRequestMessage(request_message, Opcode::QUERY(), @@ -1425,7 +1431,7 @@ TEST_F(AuthSrvTest, ) { // Set real inmem client to proxy - updateInMemory(&server, "example.", CONFIG_INMEMORY_EXAMPLE); + updateInMemory(datasrc_configurator_, "example.", CONFIG_INMEMORY_EXAMPLE); { isc::util::thread::Mutex::Locker locker(server.getClientListMutex()); boost::shared_ptr @@ -1450,10 +1456,11 @@ TEST_F(AuthSrvTest, // If non null rrset is given, it will be passed to the proxy so it can // return some faked response. void -setupThrow(AuthSrv* server, ThrowWhen throw_when, bool isc_exception, +setupThrow(AuthSrv* server, DataSourceConfigurator& datasrc_configurator, + ThrowWhen throw_when, bool isc_exception, ConstRRsetPtr rrset = ConstRRsetPtr()) { - updateInMemory(server, "example.", CONFIG_INMEMORY_EXAMPLE); + updateInMemory(datasrc_configurator, "example.", CONFIG_INMEMORY_EXAMPLE); isc::util::thread::Mutex::Locker locker(server->getClientListMutex()); boost::shared_ptr @@ -1482,11 +1489,11 @@ TEST_F(AuthSrvTest, RRClass::IN(), RRType::TXT()); for (ThrowWhen* when(throws); *when != THROW_NEVER; ++when) { createRequestPacket(request_message, IPPROTO_UDP); - setupThrow(&server, *when, true); + setupThrow(&server, datasrc_configurator_, *when, true); processAndCheckSERVFAIL(); // To be sure, check same for non-isc-exceptions createRequestPacket(request_message, IPPROTO_UDP); - setupThrow(&server, *when, false); + setupThrow(&server, datasrc_configurator_, *when, false); processAndCheckSERVFAIL(); } } @@ -1502,7 +1509,7 @@ TEST_F(AuthSrvTest, ) { createDataFromFile("nsec3query_nodnssec_fromWire.wire"); - setupThrow(&server, THROW_AT_GET_CLASS, true); + setupThrow(&server, datasrc_configurator_, THROW_AT_GET_CLASS, true); // getClass is not called so it should just answer server.processMessage(*io_message, *parse_message, *response_obuffer, @@ -1526,7 +1533,7 @@ TEST_F(AuthSrvTest, ConstRRsetPtr empty_rrset(new RRset(Name("foo.example"), RRClass::IN(), RRType::TXT(), RRTTL(0))); - setupThrow(&server, THROW_NEVER, true, empty_rrset); + setupThrow(&server, datasrc_configurator_, THROW_NEVER, true, empty_rrset); // Repeat the query processing two times. Due to the faked RRset, // toWire() should throw, and it should result in SERVFAIL. diff --git a/src/bin/auth/tests/command_unittest.cc b/src/bin/auth/tests/command_unittest.cc index e57de93d67..c5ee8f1e12 100644 --- a/src/bin/auth/tests/command_unittest.cc +++ b/src/bin/auth/tests/command_unittest.cc @@ -64,6 +64,7 @@ class AuthCommandTest : public ::testing::Test { protected: AuthCommandTest() : server_(xfrout_, ddns_forwarder_), + datasrc_configurator_(&server_), rcode_(-1), expect_rcode_(0), itimer_(server_.getIOService()) @@ -77,6 +78,7 @@ protected: MockXfroutClient xfrout_; MockSocketSessionForwarder ddns_forwarder_; AuthSrv server_; + DataSourceConfigurator datasrc_configurator_; ConstElementPtr result_; // The shutdown command parameter ConstElementPtr param_; @@ -190,7 +192,7 @@ zoneChecks(AuthSrv& server) { } void -configureZones(AuthSrv& server) { +configureZones(AuthSrv& server, DataSourceConfigurator& datasrc_configurator) { ASSERT_EQ(0, system(INSTALL_PROG " -c " TEST_DATA_DIR "/test1.zone.in " TEST_DATA_BUILDDIR "/test1.zone.copied")); ASSERT_EQ(0, system(INSTALL_PROG " -c " TEST_DATA_DIR "/test2.zone.in " @@ -208,7 +210,7 @@ configureZones(AuthSrv& server) { " \"cache-enable\": true" "}]}")); - DataSourceConfigurator::testReconfigure(&server, config); + datasrc_configurator.reconfigure(config); zoneChecks(server); } @@ -234,7 +236,7 @@ newZoneChecks(AuthSrv& server) { } TEST_F(AuthCommandTest, loadZone) { - configureZones(server_); + configureZones(server_, datasrc_configurator_); ASSERT_EQ(0, system(INSTALL_PROG " -c " TEST_DATA_DIR "/test1-new.zone.in " @@ -271,7 +273,7 @@ TEST_F(AuthCommandTest, " \"cache-enable\": true," " \"cache-zones\": [\"example.org\"]" "}]}")); - DataSourceConfigurator::testReconfigure(&server_, config); + datasrc_configurator_.reconfigure(config); { isc::util::thread::Mutex::Locker locker(server_.getClientListMutex()); @@ -335,7 +337,7 @@ TEST_F(AuthCommandTest, " \"cache-enable\": true," " \"cache-zones\": [\"example.com\"]" "}]}")); - EXPECT_THROW(DataSourceConfigurator::testReconfigure(&server_, config2), + EXPECT_THROW(datasrc_configurator_.reconfigure(config2), ConfigurableClientList::ConfigurationError); result_ = execAuthServerCommand(server_, "loadzone", @@ -350,7 +352,7 @@ TEST_F(AuthCommandTest, } TEST_F(AuthCommandTest, loadBrokenZone) { - configureZones(server_); + configureZones(server_, datasrc_configurator_); ASSERT_EQ(0, system(INSTALL_PROG " -c " TEST_DATA_DIR "/test1-broken.zone.in " @@ -363,7 +365,7 @@ TEST_F(AuthCommandTest, loadBrokenZone) { } TEST_F(AuthCommandTest, loadUnreadableZone) { - configureZones(server_); + configureZones(server_, datasrc_configurator_); // install the zone file as unreadable ASSERT_EQ(0, system(INSTALL_PROG " -c -m 000 " TEST_DATA_DIR @@ -386,7 +388,7 @@ TEST_F(AuthCommandTest, loadZoneWithoutDataSrc) { } TEST_F(AuthCommandTest, loadZoneInvalidParams) { - configureZones(server_); + configureZones(server_, datasrc_configurator_); // null arg result_ = execAuthServerCommand(server_, "loadzone", ElementPtr()); diff --git a/src/bin/auth/tests/datasrc_configurator_unittest.cc b/src/bin/auth/tests/datasrc_configurator_unittest.cc index 8759a937fb..e5b50cb3ff 100644 --- a/src/bin/auth/tests/datasrc_configurator_unittest.cc +++ b/src/bin/auth/tests/datasrc_configurator_unittest.cc @@ -19,9 +19,12 @@ #include #include -#include + +#include #include +#include + using namespace isc; using namespace isc::cc; using namespace isc::config; @@ -63,12 +66,12 @@ typedef DataSourceConfiguratorGeneric Configurator; void -datasrcConfigHandler(const std::string&, +datasrcConfigHandler(Configurator* configurator, const std::string&, isc::data::ConstElementPtr config, const isc::config::ConfigData&) { if (config->contains("classes")) { - Configurator::reconfigure(config->get("classes")); + configurator->reconfigure(config->get("classes")); } } @@ -99,6 +102,7 @@ protected: DatasrcConfiguratorTest() : session(ElementPtr(new ListElement), ElementPtr(new ListElement), ElementPtr(new ListElement)), + configurator_(this), specfile(string(TEST_OWN_DATA_DIR) + "/spec.spec") { initSession(); @@ -111,7 +115,6 @@ protected: void TearDown() { // Make sure no matter what we did, it is cleaned up. mccs->removeRemoteConfig("data_sources"); - Configurator::cleanup(); } void init(const ElementPtr& config = ElementPtr()) { session.getMessages()-> @@ -125,8 +128,9 @@ protected: session.getMessages()-> add(createAnswer(0, ElementPtr(new MapElement))); } - Configurator::init(this); - mccs->addRemoteConfig("data_sources", datasrcConfigHandler, false); + mccs->addRemoteConfig("data_sources", + boost::bind(datasrcConfigHandler, &configurator_, + _1, _2, _3), false); } void SetUp() { init(); @@ -150,6 +154,7 @@ protected: } FakeSession session; auto_ptr mccs; + Configurator configurator_; const string specfile; std::map lists_; string log_; @@ -157,20 +162,17 @@ protected: }; // Check the initialization (and cleanup) -TEST_F(DatasrcConfiguratorTest, initialization) { +TEST_F(DatasrcConfiguratorTest, DISABLED_initialization) { // It can't be initialized again EXPECT_THROW(init(), InvalidOperation); EXPECT_TRUE(session.haveSubscription("data_sources", "*")); - // Deinitialize to make the tests reasonable - mccs->removeRemoteConfig("data_sources"); - Configurator::cleanup(); EXPECT_FALSE(session.haveSubscription("data_sources", "*")); // We can't reconfigure now (not even manually) - EXPECT_THROW(Configurator::reconfigure(ElementPtr(new MapElement())), + EXPECT_THROW(configurator_.reconfigure(ElementPtr(new MapElement())), InvalidOperation); // If the server param is NULL, it does not work - EXPECT_THROW(Configurator::init(NULL), InvalidParameter); - EXPECT_FALSE(session.haveSubscription("data_sources", "*")); + EXPECT_THROW(Configurator(NULL), InvalidParameter); + EXPECT_FALSE(session.haveSubscription("data_sources", "*")); // TBD // But we can initialize it again now EXPECT_NO_THROW(init()); EXPECT_TRUE(session.haveSubscription("data_sources", "*")); @@ -279,7 +281,7 @@ TEST_F(DatasrcConfiguratorTest, rollbackDeletion) { const ElementPtr config1(Element::fromJSON("{\"IN\": [{\"type\": \"yyy\"}], " "\"CH\": [{\"type\": \"xxx\"}]}")); - Configurator::reconfigure(config1); + configurator_.reconfigure(config1); const ElementPtr config2(Element::fromJSON("{\"IN\": [{\"type\": 13}]}")); // This would delete CH. However, the IN one fails. @@ -287,7 +289,7 @@ TEST_F(DatasrcConfiguratorTest, rollbackDeletion) { // 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. - EXPECT_THROW(Configurator::reconfigure(config2), TypeError); + EXPECT_THROW(configurator_.reconfigure(config2), TypeError); EXPECT_EQ("yyy", lists_[RRClass::IN()]->getConf()); EXPECT_EQ("xxx", lists_[RRClass::CH()]->getConf()); } @@ -300,13 +302,13 @@ TEST_F(DatasrcConfiguratorTest, rollbackConfiguration) { const ElementPtr config1(Element::fromJSON("{\"IN\": [{\"type\": \"yyy\"}], " "\"CH\": [{\"type\": \"xxx\"}]}")); - Configurator::reconfigure(config1); + configurator_.reconfigure(config1); // Now, the CH happens first. But nevertheless, it should be // restored to the previoeus version. const ElementPtr config2(Element::fromJSON("{\"IN\": [{\"type\": 13}], " "\"CH\": [{\"type\": \"yyy\"}]}")); - EXPECT_THROW(Configurator::reconfigure(config2), TypeError); + EXPECT_THROW(configurator_.reconfigure(config2), TypeError); EXPECT_EQ("yyy", lists_[RRClass::IN()]->getConf()); EXPECT_EQ("xxx", lists_[RRClass::CH()]->getConf()); } From 746376902a020e0711bc85fc29a2b339a85b3db2 Mon Sep 17 00:00:00 2001 From: JINMEI Tatuya Date: Wed, 3 Oct 2012 15:44:29 -0700 Subject: [PATCH 4/8] [2203] pass server obj to configurator's reconfigure(). now the configurator class is completely stateless. --- src/bin/auth/benchmarks/query_bench.cc | 3 +- src/bin/auth/datasrc_configurator.h | 71 ++++--------------- src/bin/auth/main.cc | 13 ++-- src/bin/auth/tests/auth_srv_unittest.cc | 65 +++++++++-------- src/bin/auth/tests/command_unittest.cc | 7 +- .../tests/datasrc_configurator_unittest.cc | 22 +++--- 6 files changed, 75 insertions(+), 106 deletions(-) diff --git a/src/bin/auth/benchmarks/query_bench.cc b/src/bin/auth/benchmarks/query_bench.cc index e31668c0cf..fa1c28e16c 100644 --- a/src/bin/auth/benchmarks/query_bench.cc +++ b/src/bin/auth/benchmarks/query_bench.cc @@ -81,7 +81,6 @@ protected: QueryBenchMark(const BenchQueries& queries, Message& query_message, OutputBuffer& buffer) : server_(new AuthSrv(xfrout_client, ddns_forwarder)), - datasrc_configurator_(server_.get()), queries_(queries), query_message_(query_message), buffer_(buffer), @@ -128,6 +127,7 @@ public: QueryBenchMark(queries, query_message, buffer) { datasrc_configurator_.reconfigure( + *server_, Element::fromJSON("{\"IN\":" " [{\"type\": \"sqlite3\"," " \"params\": {" @@ -146,6 +146,7 @@ public: QueryBenchMark(queries, query_message, buffer) { datasrc_configurator_.reconfigure( + *server_, Element::fromJSON("{\"IN\":" " [{\"type\": \"MasterFiles\"," " \"cache-enable\": true, " diff --git a/src/bin/auth/datasrc_configurator.h b/src/bin/auth/datasrc_configurator.h index 94a01bd383..4921d3613f 100644 --- a/src/bin/auth/datasrc_configurator.h +++ b/src/bin/auth/datasrc_configurator.h @@ -18,8 +18,6 @@ #include "auth_srv.h" #include -#include -#include #include #include @@ -36,48 +34,10 @@ template class DataSourceConfiguratorGeneric { private: - Server* server_; typedef boost::shared_ptr ListPtr; public: - /// \brief Constructor. - /// - /// \throw isc::InvalidParameter if server is NULL - /// \param server The server to configure. - DataSourceConfiguratorGeneric(Server* server) : server_(server) { - if (server == NULL) { - isc_throw(isc::InvalidParameter, "The server must not be NULL"); - } - } - - /// \brief Initializes the class. - /// - /// This configures which session and server should be used. - /// It hooks to the session now and downloads the configuration. - /// It is synchronous (it may block for some time). - /// - /// Note that you need to call cleanup before the server or - /// session dies, otherwise it might access them after they - /// are destroyed. - /// - /// \param server It is the server to configure. - /// \throw isc::InvalidOperation if this is called when already - /// initialized. - /// \throw isc::InvalidParameter if any of the parameters is NULL - /// \throw isc::config::ModuleCCError if the remote configuration is not - /// available for some reason. - void init() { - } - - /// \brief Deinitializes the class. - /// - /// This detaches from the session and removes the server from internal - /// storage. The current configuration in the server is preserved. - /// - /// This can be called even if it is not initialized currently. You - /// can initialize it again after this. - void cleanup() { - server_ = NULL; - } + /// \brief Default constructor. + DataSourceConfiguratorGeneric() {} /// \brief Reads new configuration and replaces the old one. /// @@ -86,16 +46,15 @@ public: /// is unknown and it would be questionable at least). It is called /// automatically on normal updates. /// + /// \param server The server for which the data sources are to be + /// configured. /// \param config The configuration value to parse. It is in the form /// as an update from the config manager. - /// \throw InvalidOperation if it is called when not initialized. - void reconfigure(const isc::data::ConstElementPtr& config) { - if (server_ == NULL) { - isc_throw(isc::InvalidOperation, - "Can't reconfigure while not initialized by init()"); - } + void reconfigure(Server& server, + const isc::data::ConstElementPtr& config) + { // Lock the client lists, we're going to manipulate them. - isc::util::thread::Mutex::Locker locker(server_->getClientListMutex()); + isc::util::thread::Mutex::Locker locker(server.getClientListMutex()); typedef std::map Map; typedef std::pair RollbackPair; typedef std::pair @@ -107,14 +66,14 @@ public: // Get the configuration and current state. const Map& map(config->mapValue()); const std::vector - activeVector(server_->getClientListClasses()); + 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) { isc::dns::RRClass rrclass(it->first); active.erase(rrclass); - ListPtr list(server_->getClientList(rrclass)); + ListPtr list(server.getClientList(rrclass)); bool need_set(false); if (list) { rollback_configurations. @@ -127,7 +86,7 @@ public: } list->configure(it->second, true); if (need_set) { - server_->setClientList(rrclass, list); + server.setClientList(rrclass, list); } } // Remove the ones that are not in the configuration. @@ -137,20 +96,20 @@ public: // 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()); + 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); + 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); + server.getClientList(it->first)->configure(it->second, true); } throw; } diff --git a/src/bin/auth/main.cc b/src/bin/auth/main.cc index fa11521898..5a720637ef 100644 --- a/src/bin/auth/main.cc +++ b/src/bin/auth/main.cc @@ -73,7 +73,7 @@ namespace { /* need global var for config/command handlers. * todo: turn this around, and put handlers in the authserver * class itself? */ -AuthSrv *auth_server; +AuthSrv* auth_server; ConstElementPtr my_config_handler(ConstElementPtr new_config) { @@ -87,12 +87,13 @@ my_command_handler(const string& command, ConstElementPtr args) { } void -datasrcConfigHandler(DataSourceConfigurator* configurator, const std::string&, - isc::data::ConstElementPtr config, +datasrcConfigHandler(AuthSrv* server, DataSourceConfigurator* configurator, + const std::string&, isc::data::ConstElementPtr config, const isc::config::ConfigData&) { + assert(server != NULL); if (config->contains("classes")) { - configurator->reconfigure(config->get("classes")); + configurator->reconfigure(*server, config->get("classes")); } } @@ -206,9 +207,10 @@ main(int argc, char* argv[]) { auth_server->setTSIGKeyRing(&isc::server_common::keyring); // Start the data source configuration - datasrc_configurator.reset(new DataSourceConfigurator(auth_server)); + datasrc_configurator.reset(new DataSourceConfigurator); config_session->addRemoteConfig("data_sources", boost::bind(datasrcConfigHandler, + auth_server, datasrc_configurator.get(), _1, _2, _3), false); @@ -217,6 +219,7 @@ main(int argc, char* argv[]) { // get the default (or, current value). Further updates will work // the usual way. datasrc_configurator->reconfigure( + *auth_server, config_session->getRemoteConfigValue("data_sources", "classes")); // Now start asynchronous read. diff --git a/src/bin/auth/tests/auth_srv_unittest.cc b/src/bin/auth/tests/auth_srv_unittest.cc index 75a1b8def9..0bdb9d5acc 100644 --- a/src/bin/auth/tests/auth_srv_unittest.cc +++ b/src/bin/auth/tests/auth_srv_unittest.cc @@ -98,8 +98,7 @@ protected: // The empty string is expected value of the parameter of // requestSocket, not the app_name (there's no fallback, it checks // the empty string is passed). - sock_requestor_(dnss_, address_store_, 53210, ""), - datasrc_configurator_(&server) + sock_requestor_(dnss_, address_store_, 53210, "") { server.setDNSService(dnss_); server.setXfrinSession(¬ify_session); @@ -724,7 +723,7 @@ TEST_F(AuthSrvTest, notifyWithSessionMessageError) { } void -updateDatabase(DataSourceConfigurator& datasrc_configurator, +updateDatabase(AuthSrv& server, DataSourceConfigurator& datasrc_configurator, const char* params) { const ConstElementPtr config(Element::fromJSON("{" @@ -732,11 +731,11 @@ updateDatabase(DataSourceConfigurator& datasrc_configurator, " \"type\": \"sqlite3\"," " \"params\": " + string(params) + "}]}")); - datasrc_configurator.reconfigure(config); + datasrc_configurator.reconfigure(server, config); } void -updateInMemory(DataSourceConfigurator& datasrc_configurator, +updateInMemory(AuthSrv& server, DataSourceConfigurator& datasrc_configurator, const char* origin, const char* filename) { const ConstElementPtr config(Element::fromJSON("{" @@ -751,17 +750,17 @@ updateInMemory(DataSourceConfigurator& datasrc_configurator, " \"type\": \"static\"," " \"params\": \"" + string(STATIC_DSRC_FILE) + "\"" "}]}")); - datasrc_configurator.reconfigure(config); + datasrc_configurator.reconfigure(server, config); } void -updateBuiltin(DataSourceConfigurator& datasrc_configurator) { +updateBuiltin(AuthSrv& server, DataSourceConfigurator& datasrc_configurator) { const ConstElementPtr config(Element::fromJSON("{" "\"CH\": [{" " \"type\": \"static\"," " \"params\": \"" + string(STATIC_DSRC_FILE) + "\"" "}]}")); - datasrc_configurator.reconfigure(config); + datasrc_configurator.reconfigure(server, config); } // Try giving the server a TSIG signed request and see it can anwer signed as @@ -772,7 +771,7 @@ TEST_F(AuthSrvTest, DISABLED_TSIGSigned) { // Needs builtin TEST_F(AuthSrvTest, TSIGSigned) { #endif // Prepare key, the client message, etc - updateBuiltin(datasrc_configurator_); + updateBuiltin(server, datasrc_configurator_); const TSIGKey key("key:c2VjcmV0Cg==:hmac-sha1"); TSIGContext context(key); UnitTestUtil::createRequestMessage(request_message, opcode, default_qid, @@ -820,7 +819,7 @@ TEST_F(AuthSrvTest, DISABLED_builtInQueryViaDNSServer) { #else TEST_F(AuthSrvTest, builtInQueryViaDNSServer) { #endif - updateBuiltin(datasrc_configurator_); + updateBuiltin(server, datasrc_configurator_); UnitTestUtil::createRequestMessage(request_message, Opcode::QUERY(), default_qid, Name("VERSION.BIND."), RRClass::CH(), RRType::TXT()); @@ -852,7 +851,7 @@ TEST_F(AuthSrvTest, DISABLED_builtInQuery) { #else TEST_F(AuthSrvTest, builtInQuery) { #endif - updateBuiltin(datasrc_configurator_); + updateBuiltin(server, datasrc_configurator_); UnitTestUtil::createRequestMessage(request_message, Opcode::QUERY(), default_qid, Name("VERSION.BIND."), RRClass::CH(), RRType::TXT()); @@ -873,7 +872,7 @@ TEST_F(AuthSrvTest, DISABLED_iqueryViaDNSServer) { // Needs builtin #else TEST_F(AuthSrvTest, iqueryViaDNSServer) { // Needs builtin #endif - updateBuiltin(datasrc_configurator_); + updateBuiltin(server, datasrc_configurator_); createDataFromFile("iquery_fromWire.wire"); (*server.getDNSLookupProvider())(*io_message, parse_message, response_message, @@ -895,7 +894,7 @@ TEST_F(AuthSrvTest, DISABLED_updateConfig) { #else TEST_F(AuthSrvTest, updateConfig) { #endif - updateDatabase(datasrc_configurator_, CONFIG_TESTDB); + updateDatabase(server, datasrc_configurator_, CONFIG_TESTDB); // query for existent data in the installed data source. The resulting // response should have the AA flag on, and have an RR in each answer @@ -913,7 +912,7 @@ TEST_F(AuthSrvTest, DISABLED_datasourceFail) { #else TEST_F(AuthSrvTest, datasourceFail) { #endif - updateDatabase(datasrc_configurator_, CONFIG_TESTDB); + updateDatabase(server, datasrc_configurator_, CONFIG_TESTDB); // This query will hit a corrupted entry of the data source (the zoneload // tool and the data source itself naively accept it). This will result @@ -933,10 +932,11 @@ TEST_F(AuthSrvTest, DISABLED_updateConfigFail) { TEST_F(AuthSrvTest, updateConfigFail) { #endif // First, load a valid data source. - updateDatabase(datasrc_configurator_, CONFIG_TESTDB); + updateDatabase(server, datasrc_configurator_, CONFIG_TESTDB); // Next, try to update it with a non-existent one. This should fail. - EXPECT_THROW(updateDatabase(datasrc_configurator_, BADCONFIG_TESTDB), + EXPECT_THROW(updateDatabase(server, datasrc_configurator_, + BADCONFIG_TESTDB), isc::datasrc::DataSourceError); // The original data source should still exist. @@ -959,7 +959,7 @@ TEST_F(AuthSrvTest, updateWithInMemoryClient) { " \"params\": {}," " \"cache-enable\": true" "}]}")); - datasrc_configurator_.reconfigure(config); + datasrc_configurator_.reconfigure(server, config); // after successful configuration, we should have one (with empty zoneset). // The memory data source is empty, should return REFUSED rcode. @@ -980,7 +980,8 @@ TEST_F(AuthSrvTest, queryWithInMemoryClientNoDNSSEC) { // query handler class, and confirm it returns no error and a non empty // answer section. Detailed examination on the response content // for various types of queries are tested in the query tests. - updateInMemory(datasrc_configurator_, "example.", CONFIG_INMEMORY_EXAMPLE); + updateInMemory(server, datasrc_configurator_, "example.", + CONFIG_INMEMORY_EXAMPLE); createDataFromFile("nsec3query_nodnssec_fromWire.wire"); server.processMessage(*io_message, *parse_message, *response_obuffer, @@ -999,7 +1000,8 @@ TEST_F(AuthSrvTest, queryWithInMemoryClientDNSSEC) { // Similar to the previous test, but the query has the DO bit on. // The response should contain RRSIGs, and should have more RRs than // the previous case. - updateInMemory(datasrc_configurator_, "example.", CONFIG_INMEMORY_EXAMPLE); + updateInMemory(server, datasrc_configurator_, "example.", + CONFIG_INMEMORY_EXAMPLE); createDataFromFile("nsec3query_fromWire.wire"); server.processMessage(*io_message, *parse_message, *response_obuffer, @@ -1019,7 +1021,8 @@ TEST_F(AuthSrvTest, ) { // Set up the in-memory - updateInMemory(datasrc_configurator_, "example.", CONFIG_INMEMORY_EXAMPLE); + updateInMemory(server, datasrc_configurator_, "example.", + CONFIG_INMEMORY_EXAMPLE); // This shouldn't affect the result of class CH query UnitTestUtil::createRequestMessage(request_message, Opcode::QUERY(), @@ -1431,7 +1434,8 @@ TEST_F(AuthSrvTest, ) { // Set real inmem client to proxy - updateInMemory(datasrc_configurator_, "example.", CONFIG_INMEMORY_EXAMPLE); + updateInMemory(server, datasrc_configurator_, "example.", + CONFIG_INMEMORY_EXAMPLE); { isc::util::thread::Mutex::Locker locker(server.getClientListMutex()); boost::shared_ptr @@ -1456,17 +1460,18 @@ TEST_F(AuthSrvTest, // If non null rrset is given, it will be passed to the proxy so it can // return some faked response. void -setupThrow(AuthSrv* server, DataSourceConfigurator& datasrc_configurator, +setupThrow(AuthSrv& server, DataSourceConfigurator& datasrc_configurator, ThrowWhen throw_when, bool isc_exception, ConstRRsetPtr rrset = ConstRRsetPtr()) { - updateInMemory(datasrc_configurator, "example.", CONFIG_INMEMORY_EXAMPLE); + updateInMemory(server, datasrc_configurator, "example.", + CONFIG_INMEMORY_EXAMPLE); - isc::util::thread::Mutex::Locker locker(server->getClientListMutex()); + isc::util::thread::Mutex::Locker locker(server.getClientListMutex()); boost::shared_ptr - list(new FakeList(server->getClientList(RRClass::IN()), throw_when, + list(new FakeList(server.getClientList(RRClass::IN()), throw_when, isc_exception, rrset)); - server->setClientList(RRClass::IN(), list); + server.setClientList(RRClass::IN(), list); } TEST_F(AuthSrvTest, @@ -1489,11 +1494,11 @@ TEST_F(AuthSrvTest, RRClass::IN(), RRType::TXT()); for (ThrowWhen* when(throws); *when != THROW_NEVER; ++when) { createRequestPacket(request_message, IPPROTO_UDP); - setupThrow(&server, datasrc_configurator_, *when, true); + setupThrow(server, datasrc_configurator_, *when, true); processAndCheckSERVFAIL(); // To be sure, check same for non-isc-exceptions createRequestPacket(request_message, IPPROTO_UDP); - setupThrow(&server, datasrc_configurator_, *when, false); + setupThrow(server, datasrc_configurator_, *when, false); processAndCheckSERVFAIL(); } } @@ -1509,7 +1514,7 @@ TEST_F(AuthSrvTest, ) { createDataFromFile("nsec3query_nodnssec_fromWire.wire"); - setupThrow(&server, datasrc_configurator_, THROW_AT_GET_CLASS, true); + setupThrow(server, datasrc_configurator_, THROW_AT_GET_CLASS, true); // getClass is not called so it should just answer server.processMessage(*io_message, *parse_message, *response_obuffer, @@ -1533,7 +1538,7 @@ TEST_F(AuthSrvTest, ConstRRsetPtr empty_rrset(new RRset(Name("foo.example"), RRClass::IN(), RRType::TXT(), RRTTL(0))); - setupThrow(&server, datasrc_configurator_, THROW_NEVER, true, empty_rrset); + setupThrow(server, datasrc_configurator_, THROW_NEVER, true, empty_rrset); // Repeat the query processing two times. Due to the faked RRset, // toWire() should throw, and it should result in SERVFAIL. diff --git a/src/bin/auth/tests/command_unittest.cc b/src/bin/auth/tests/command_unittest.cc index c5ee8f1e12..68e1b12e3b 100644 --- a/src/bin/auth/tests/command_unittest.cc +++ b/src/bin/auth/tests/command_unittest.cc @@ -64,7 +64,6 @@ class AuthCommandTest : public ::testing::Test { protected: AuthCommandTest() : server_(xfrout_, ddns_forwarder_), - datasrc_configurator_(&server_), rcode_(-1), expect_rcode_(0), itimer_(server_.getIOService()) @@ -210,7 +209,7 @@ configureZones(AuthSrv& server, DataSourceConfigurator& datasrc_configurator) { " \"cache-enable\": true" "}]}")); - datasrc_configurator.reconfigure(config); + datasrc_configurator.reconfigure(server, config); zoneChecks(server); } @@ -273,7 +272,7 @@ TEST_F(AuthCommandTest, " \"cache-enable\": true," " \"cache-zones\": [\"example.org\"]" "}]}")); - datasrc_configurator_.reconfigure(config); + datasrc_configurator_.reconfigure(server_, config); { isc::util::thread::Mutex::Locker locker(server_.getClientListMutex()); @@ -337,7 +336,7 @@ TEST_F(AuthCommandTest, " \"cache-enable\": true," " \"cache-zones\": [\"example.com\"]" "}]}")); - EXPECT_THROW(datasrc_configurator_.reconfigure(config2), + EXPECT_THROW(datasrc_configurator_.reconfigure(server_, config2), ConfigurableClientList::ConfigurationError); result_ = execAuthServerCommand(server_, "loadzone", diff --git a/src/bin/auth/tests/datasrc_configurator_unittest.cc b/src/bin/auth/tests/datasrc_configurator_unittest.cc index e5b50cb3ff..3821dd4429 100644 --- a/src/bin/auth/tests/datasrc_configurator_unittest.cc +++ b/src/bin/auth/tests/datasrc_configurator_unittest.cc @@ -66,12 +66,13 @@ typedef DataSourceConfiguratorGeneric Configurator; void -datasrcConfigHandler(Configurator* configurator, const std::string&, +datasrcConfigHandler(DatasrcConfiguratorTest* fake_server, + Configurator* configurator, const std::string&, isc::data::ConstElementPtr config, const isc::config::ConfigData&) { if (config->contains("classes")) { - configurator->reconfigure(config->get("classes")); + configurator->reconfigure(*fake_server, config->get("classes")); } } @@ -102,7 +103,6 @@ protected: DatasrcConfiguratorTest() : session(ElementPtr(new ListElement), ElementPtr(new ListElement), ElementPtr(new ListElement)), - configurator_(this), specfile(string(TEST_OWN_DATA_DIR) + "/spec.spec") { initSession(); @@ -129,7 +129,8 @@ protected: add(createAnswer(0, ElementPtr(new MapElement))); } mccs->addRemoteConfig("data_sources", - boost::bind(datasrcConfigHandler, &configurator_, + boost::bind(datasrcConfigHandler, + this, &configurator_, _1, _2, _3), false); } void SetUp() { @@ -168,10 +169,11 @@ TEST_F(DatasrcConfiguratorTest, DISABLED_initialization) { EXPECT_TRUE(session.haveSubscription("data_sources", "*")); EXPECT_FALSE(session.haveSubscription("data_sources", "*")); // We can't reconfigure now (not even manually) - EXPECT_THROW(configurator_.reconfigure(ElementPtr(new MapElement())), + EXPECT_THROW(configurator_.reconfigure(*this, + ElementPtr(new MapElement())), InvalidOperation); // If the server param is NULL, it does not work - EXPECT_THROW(Configurator(NULL), InvalidParameter); + EXPECT_THROW(Configurator configurator, InvalidParameter); EXPECT_FALSE(session.haveSubscription("data_sources", "*")); // TBD // But we can initialize it again now EXPECT_NO_THROW(init()); @@ -281,7 +283,7 @@ TEST_F(DatasrcConfiguratorTest, rollbackDeletion) { const ElementPtr config1(Element::fromJSON("{\"IN\": [{\"type\": \"yyy\"}], " "\"CH\": [{\"type\": \"xxx\"}]}")); - configurator_.reconfigure(config1); + configurator_.reconfigure(*this, config1); const ElementPtr config2(Element::fromJSON("{\"IN\": [{\"type\": 13}]}")); // This would delete CH. However, the IN one fails. @@ -289,7 +291,7 @@ TEST_F(DatasrcConfiguratorTest, rollbackDeletion) { // 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. - EXPECT_THROW(configurator_.reconfigure(config2), TypeError); + EXPECT_THROW(configurator_.reconfigure(*this, config2), TypeError); EXPECT_EQ("yyy", lists_[RRClass::IN()]->getConf()); EXPECT_EQ("xxx", lists_[RRClass::CH()]->getConf()); } @@ -302,13 +304,13 @@ TEST_F(DatasrcConfiguratorTest, rollbackConfiguration) { const ElementPtr config1(Element::fromJSON("{\"IN\": [{\"type\": \"yyy\"}], " "\"CH\": [{\"type\": \"xxx\"}]}")); - configurator_.reconfigure(config1); + configurator_.reconfigure(*this, config1); // Now, the CH happens first. But nevertheless, it should be // restored to the previoeus version. const ElementPtr config2(Element::fromJSON("{\"IN\": [{\"type\": 13}], " "\"CH\": [{\"type\": \"yyy\"}]}")); - EXPECT_THROW(configurator_.reconfigure(config2), TypeError); + EXPECT_THROW(configurator_.reconfigure(*this, config2), TypeError); EXPECT_EQ("yyy", lists_[RRClass::IN()]->getConf()); EXPECT_EQ("xxx", lists_[RRClass::CH()]->getConf()); } From ec6cf17e571f9cf23fd1e7bcd573df55106550b1 Mon Sep 17 00:00:00 2001 From: JINMEI Tatuya Date: Wed, 3 Oct 2012 17:17:36 -0700 Subject: [PATCH 5/8] [2203] changed configurator class to a simple function. as it's now completely stateless and can work independently. the common specialization for the main implementation is defined in a new created .cc file. --- src/bin/auth/Makefile.am | 2 +- src/bin/auth/benchmarks/Makefile.am | 1 + src/bin/auth/benchmarks/query_bench.cc | 5 +- src/bin/auth/datasrc_configurator.cc | 26 +++ src/bin/auth/datasrc_configurator.h | 161 +++++++++--------- src/bin/auth/main.cc | 19 +-- src/bin/auth/tests/Makefile.am | 1 + src/bin/auth/tests/auth_srv_unittest.cc | 62 +++---- src/bin/auth/tests/command_unittest.cc | 17 +- .../tests/datasrc_configurator_unittest.cc | 67 +++----- 10 files changed, 172 insertions(+), 189 deletions(-) create mode 100644 src/bin/auth/datasrc_configurator.cc diff --git a/src/bin/auth/Makefile.am b/src/bin/auth/Makefile.am index 6128a4cf93..df30a428db 100644 --- a/src/bin/auth/Makefile.am +++ b/src/bin/auth/Makefile.am @@ -55,7 +55,7 @@ b10_auth_SOURCES += auth_config.cc auth_config.h b10_auth_SOURCES += command.cc command.h b10_auth_SOURCES += common.h common.cc b10_auth_SOURCES += statistics.cc statistics.h -b10_auth_SOURCES += datasrc_configurator.h +b10_auth_SOURCES += datasrc_configurator.h datasrc_configurator.cc b10_auth_SOURCES += main.cc nodist_b10_auth_SOURCES = auth_messages.h auth_messages.cc diff --git a/src/bin/auth/benchmarks/Makefile.am b/src/bin/auth/benchmarks/Makefile.am index 48e552a502..fcfcb8add4 100644 --- a/src/bin/auth/benchmarks/Makefile.am +++ b/src/bin/auth/benchmarks/Makefile.am @@ -17,6 +17,7 @@ query_bench_SOURCES += ../auth_srv.h ../auth_srv.cc query_bench_SOURCES += ../auth_config.h ../auth_config.cc query_bench_SOURCES += ../statistics.h ../statistics.cc query_bench_SOURCES += ../auth_log.h ../auth_log.cc +query_bench_SOURCES += ../datasrc_configurator.h ../datasrc_configurator.cc nodist_query_bench_SOURCES = ../auth_messages.h ../auth_messages.cc diff --git a/src/bin/auth/benchmarks/query_bench.cc b/src/bin/auth/benchmarks/query_bench.cc index fa1c28e16c..07111a56d4 100644 --- a/src/bin/auth/benchmarks/query_bench.cc +++ b/src/bin/auth/benchmarks/query_bench.cc @@ -109,7 +109,6 @@ private: MockSocketSessionForwarder ddns_forwarder; protected: AuthSrvPtr server_; - DataSourceConfigurator datasrc_configurator_; private: const BenchQueries& queries_; Message& query_message_; @@ -126,7 +125,7 @@ public: OutputBuffer& buffer) : QueryBenchMark(queries, query_message, buffer) { - datasrc_configurator_.reconfigure( + configureDataSource( *server_, Element::fromJSON("{\"IN\":" " [{\"type\": \"sqlite3\"," @@ -145,7 +144,7 @@ public: OutputBuffer& buffer) : QueryBenchMark(queries, query_message, buffer) { - datasrc_configurator_.reconfigure( + configureDataSource( *server_, Element::fromJSON("{\"IN\":" " [{\"type\": \"MasterFiles\"," diff --git a/src/bin/auth/datasrc_configurator.cc b/src/bin/auth/datasrc_configurator.cc new file mode 100644 index 0000000000..5469389a0e --- /dev/null +++ b/src/bin/auth/datasrc_configurator.cc @@ -0,0 +1,26 @@ +// Copyright (C) 2012 Internet Systems Consortium, Inc. ("ISC") +// +// Permission to use, copy, modify, and/or distribute this software for any +// purpose with or without fee is hereby granted, provided that the above +// copyright notice and this permission notice appear in all copies. +// +// THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH +// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY +// AND FITNESS. IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT, +// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM +// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE +// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR +// PERFORMANCE OF THIS SOFTWARE. + +#include +#include "auth_srv.h" +#include "datasrc_configurator.h" + +// 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(server, config)); +} diff --git a/src/bin/auth/datasrc_configurator.h b/src/bin/auth/datasrc_configurator.h index 4921d3613f..b8d21f3e98 100644 --- a/src/bin/auth/datasrc_configurator.h +++ b/src/bin/auth/datasrc_configurator.h @@ -17,110 +17,101 @@ #include "auth_srv.h" +#include #include #include +#include + #include -/// \brief A class to configure the authoritative server's data source lists +/// \brief Configure the authoritative server's data source 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. /// -/// Also, the class is a template. This is simply because of easier testing. -/// You don't need to pay attention to it, use the DataSourceConfigurator -/// type alias instead. +/// 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. +/// \param config The configuration value to parse. It is in the form +/// as an update from the config manager. template -class DataSourceConfiguratorGeneric { -private: +void +configureDataSourceGeneric(Server& server, + const isc::data::ConstElementPtr& config) +{ typedef boost::shared_ptr ListPtr; -public: - /// \brief Default constructor. - DataSourceConfiguratorGeneric() {} + typedef std::map Map; + typedef std::pair RollbackPair; + typedef std::pair + RollbackConfiguration; - /// \brief Reads new configuration and replaces the old one. - /// - /// It instructs the server to replace the lists with new ones as needed. - /// You don't need to call it directly (but you could, though the benefit - /// is unknown and it would be questionable at least). It is called - /// automatically on normal updates. - /// - /// \param server The server for which the data sources are to be - /// configured. - /// \param config The configuration value to parse. It is in the form - /// as an update from the config manager. - void reconfigure(Server& server, - const isc::data::ConstElementPtr& config) - { - // Lock the client lists, we're going to manipulate them. - isc::util::thread::Mutex::Locker locker(server.getClientListMutex()); - typedef std::map Map; - typedef std::pair RollbackPair; - typedef std::pair - RollbackConfiguration; - // 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) { - 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); - } + // Lock the client lists, we're going to manipulate them. + isc::util::thread::Mutex::Locker locker(server.getClientListMutex()); + + // 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())); } - // 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()); + list->configure(it->second, true); + if (need_set) { + server.setClientList(rrclass, list); } - } 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; } + // 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; } -}; +} /// \brief Concrete version of DataSourceConfiguratorGeneric for the -/// use in authoritative server. -typedef DataSourceConfiguratorGeneric - DataSourceConfigurator; +/// use with authoritative server implementation. +void +configureDataSource(AuthSrv& server, const isc::data::ConstElementPtr& config); #endif diff --git a/src/bin/auth/main.cc b/src/bin/auth/main.cc index 5a720637ef..eecc5aa1a6 100644 --- a/src/bin/auth/main.cc +++ b/src/bin/auth/main.cc @@ -87,13 +87,13 @@ my_command_handler(const string& command, ConstElementPtr args) { } void -datasrcConfigHandler(AuthSrv* server, DataSourceConfigurator* configurator, - const std::string&, isc::data::ConstElementPtr config, +datasrcConfigHandler(AuthSrv* server, const std::string&, + isc::data::ConstElementPtr config, const isc::config::ConfigData&) { assert(server != NULL); if (config->contains("classes")) { - configurator->reconfigure(*server, config->get("classes")); + configureDataSource(*server, config->get("classes")); } } @@ -141,7 +141,6 @@ main(int argc, char* argv[]) { ModuleCCSession* config_session = NULL; XfroutClient xfrout_client(getXfroutSocketPath()); SocketSessionForwarder ddns_forwarder(getDDNSSocketPath()); - boost::scoped_ptr datasrc_configurator; try { string specfile; if (getenv("B10_FROM_BUILD")) { @@ -207,19 +206,16 @@ main(int argc, char* argv[]) { auth_server->setTSIGKeyRing(&isc::server_common::keyring); // Start the data source configuration - datasrc_configurator.reset(new DataSourceConfigurator); config_session->addRemoteConfig("data_sources", boost::bind(datasrcConfigHandler, auth_server, - datasrc_configurator.get(), _1, _2, _3), false); // HACK: The default is not passed to the handler. This one will // get the default (or, current value). Further updates will work // the usual way. - datasrc_configurator->reconfigure( - *auth_server, + configureDataSource(*auth_server, config_session->getRemoteConfigValue("data_sources", "classes")); // Now start asynchronous read. @@ -244,9 +240,10 @@ main(int argc, char* argv[]) { xfrin_session->disconnect(); } - if (datasrc_configurator) { - config_session->removeRemoteConfig("data_sources"); - } + // If we haven't registered callback for data sources, this will be just + // no-op. + config_session->removeRemoteConfig("data_sources"); + delete xfrin_session; delete config_session; delete cc_session; diff --git a/src/bin/auth/tests/Makefile.am b/src/bin/auth/tests/Makefile.am index 7c01a6bd3f..3b1873678c 100644 --- a/src/bin/auth/tests/Makefile.am +++ b/src/bin/auth/tests/Makefile.am @@ -42,6 +42,7 @@ run_unittests_SOURCES += ../auth_config.h ../auth_config.cc run_unittests_SOURCES += ../command.h ../command.cc run_unittests_SOURCES += ../common.h ../common.cc run_unittests_SOURCES += ../statistics.h ../statistics.cc +run_unittests_SOURCES += ../datasrc_configurator.h ../datasrc_configurator.cc run_unittests_SOURCES += datasrc_util.h datasrc_util.cc run_unittests_SOURCES += auth_srv_unittest.cc run_unittests_SOURCES += config_unittest.cc diff --git a/src/bin/auth/tests/auth_srv_unittest.cc b/src/bin/auth/tests/auth_srv_unittest.cc index 0bdb9d5acc..00d7350594 100644 --- a/src/bin/auth/tests/auth_srv_unittest.cc +++ b/src/bin/auth/tests/auth_srv_unittest.cc @@ -183,7 +183,6 @@ protected: vector response_data; AddressList address_store_; TestSocketRequestor sock_requestor_; - DataSourceConfigurator datasrc_configurator_; }; // A helper function that builds a response to version.bind/TXT/CH that @@ -723,21 +722,17 @@ TEST_F(AuthSrvTest, notifyWithSessionMessageError) { } void -updateDatabase(AuthSrv& server, DataSourceConfigurator& datasrc_configurator, - const char* params) -{ +updateDatabase(AuthSrv& server, const char* params) { const ConstElementPtr config(Element::fromJSON("{" "\"IN\": [{" " \"type\": \"sqlite3\"," " \"params\": " + string(params) + "}]}")); - datasrc_configurator.reconfigure(server, config); + configureDataSource(server, config); } void -updateInMemory(AuthSrv& server, DataSourceConfigurator& datasrc_configurator, - const char* origin, const char* filename) -{ +updateInMemory(AuthSrv& server, const char* origin, const char* filename) { const ConstElementPtr config(Element::fromJSON("{" "\"IN\": [{" " \"type\": \"MasterFiles\"," @@ -750,17 +745,17 @@ updateInMemory(AuthSrv& server, DataSourceConfigurator& datasrc_configurator, " \"type\": \"static\"," " \"params\": \"" + string(STATIC_DSRC_FILE) + "\"" "}]}")); - datasrc_configurator.reconfigure(server, config); + configureDataSource(server, config); } void -updateBuiltin(AuthSrv& server, DataSourceConfigurator& datasrc_configurator) { +updateBuiltin(AuthSrv& server) { const ConstElementPtr config(Element::fromJSON("{" "\"CH\": [{" " \"type\": \"static\"," " \"params\": \"" + string(STATIC_DSRC_FILE) + "\"" "}]}")); - datasrc_configurator.reconfigure(server, config); + configureDataSource(server, config); } // Try giving the server a TSIG signed request and see it can anwer signed as @@ -771,7 +766,7 @@ TEST_F(AuthSrvTest, DISABLED_TSIGSigned) { // Needs builtin TEST_F(AuthSrvTest, TSIGSigned) { #endif // Prepare key, the client message, etc - updateBuiltin(server, datasrc_configurator_); + updateBuiltin(server); const TSIGKey key("key:c2VjcmV0Cg==:hmac-sha1"); TSIGContext context(key); UnitTestUtil::createRequestMessage(request_message, opcode, default_qid, @@ -819,7 +814,7 @@ TEST_F(AuthSrvTest, DISABLED_builtInQueryViaDNSServer) { #else TEST_F(AuthSrvTest, builtInQueryViaDNSServer) { #endif - updateBuiltin(server, datasrc_configurator_); + updateBuiltin(server); UnitTestUtil::createRequestMessage(request_message, Opcode::QUERY(), default_qid, Name("VERSION.BIND."), RRClass::CH(), RRType::TXT()); @@ -851,7 +846,7 @@ TEST_F(AuthSrvTest, DISABLED_builtInQuery) { #else TEST_F(AuthSrvTest, builtInQuery) { #endif - updateBuiltin(server, datasrc_configurator_); + updateBuiltin(server); UnitTestUtil::createRequestMessage(request_message, Opcode::QUERY(), default_qid, Name("VERSION.BIND."), RRClass::CH(), RRType::TXT()); @@ -872,7 +867,7 @@ TEST_F(AuthSrvTest, DISABLED_iqueryViaDNSServer) { // Needs builtin #else TEST_F(AuthSrvTest, iqueryViaDNSServer) { // Needs builtin #endif - updateBuiltin(server, datasrc_configurator_); + updateBuiltin(server); createDataFromFile("iquery_fromWire.wire"); (*server.getDNSLookupProvider())(*io_message, parse_message, response_message, @@ -894,7 +889,7 @@ TEST_F(AuthSrvTest, DISABLED_updateConfig) { #else TEST_F(AuthSrvTest, updateConfig) { #endif - updateDatabase(server, datasrc_configurator_, CONFIG_TESTDB); + updateDatabase(server, CONFIG_TESTDB); // query for existent data in the installed data source. The resulting // response should have the AA flag on, and have an RR in each answer @@ -912,7 +907,7 @@ TEST_F(AuthSrvTest, DISABLED_datasourceFail) { #else TEST_F(AuthSrvTest, datasourceFail) { #endif - updateDatabase(server, datasrc_configurator_, CONFIG_TESTDB); + updateDatabase(server, CONFIG_TESTDB); // This query will hit a corrupted entry of the data source (the zoneload // tool and the data source itself naively accept it). This will result @@ -932,11 +927,10 @@ TEST_F(AuthSrvTest, DISABLED_updateConfigFail) { TEST_F(AuthSrvTest, updateConfigFail) { #endif // First, load a valid data source. - updateDatabase(server, datasrc_configurator_, CONFIG_TESTDB); + updateDatabase(server, CONFIG_TESTDB); // Next, try to update it with a non-existent one. This should fail. - EXPECT_THROW(updateDatabase(server, datasrc_configurator_, - BADCONFIG_TESTDB), + EXPECT_THROW(updateDatabase(server, BADCONFIG_TESTDB), isc::datasrc::DataSourceError); // The original data source should still exist. @@ -959,7 +953,7 @@ TEST_F(AuthSrvTest, updateWithInMemoryClient) { " \"params\": {}," " \"cache-enable\": true" "}]}")); - datasrc_configurator_.reconfigure(server, config); + configureDataSource(server, config); // after successful configuration, we should have one (with empty zoneset). // The memory data source is empty, should return REFUSED rcode. @@ -980,8 +974,7 @@ TEST_F(AuthSrvTest, queryWithInMemoryClientNoDNSSEC) { // query handler class, and confirm it returns no error and a non empty // answer section. Detailed examination on the response content // for various types of queries are tested in the query tests. - updateInMemory(server, datasrc_configurator_, "example.", - CONFIG_INMEMORY_EXAMPLE); + updateInMemory(server, "example.", CONFIG_INMEMORY_EXAMPLE); createDataFromFile("nsec3query_nodnssec_fromWire.wire"); server.processMessage(*io_message, *parse_message, *response_obuffer, @@ -1000,8 +993,7 @@ TEST_F(AuthSrvTest, queryWithInMemoryClientDNSSEC) { // Similar to the previous test, but the query has the DO bit on. // The response should contain RRSIGs, and should have more RRs than // the previous case. - updateInMemory(server, datasrc_configurator_, "example.", - CONFIG_INMEMORY_EXAMPLE); + updateInMemory(server, "example.", CONFIG_INMEMORY_EXAMPLE); createDataFromFile("nsec3query_fromWire.wire"); server.processMessage(*io_message, *parse_message, *response_obuffer, @@ -1021,8 +1013,7 @@ TEST_F(AuthSrvTest, ) { // Set up the in-memory - updateInMemory(server, datasrc_configurator_, "example.", - CONFIG_INMEMORY_EXAMPLE); + updateInMemory(server, "example.", CONFIG_INMEMORY_EXAMPLE); // This shouldn't affect the result of class CH query UnitTestUtil::createRequestMessage(request_message, Opcode::QUERY(), @@ -1434,8 +1425,7 @@ TEST_F(AuthSrvTest, ) { // Set real inmem client to proxy - updateInMemory(server, datasrc_configurator_, "example.", - CONFIG_INMEMORY_EXAMPLE); + updateInMemory(server, "example.", CONFIG_INMEMORY_EXAMPLE); { isc::util::thread::Mutex::Locker locker(server.getClientListMutex()); boost::shared_ptr @@ -1460,12 +1450,10 @@ TEST_F(AuthSrvTest, // If non null rrset is given, it will be passed to the proxy so it can // return some faked response. void -setupThrow(AuthSrv& server, DataSourceConfigurator& datasrc_configurator, - ThrowWhen throw_when, bool isc_exception, +setupThrow(AuthSrv& server, ThrowWhen throw_when, bool isc_exception, ConstRRsetPtr rrset = ConstRRsetPtr()) { - updateInMemory(server, datasrc_configurator, "example.", - CONFIG_INMEMORY_EXAMPLE); + updateInMemory(server, "example.", CONFIG_INMEMORY_EXAMPLE); isc::util::thread::Mutex::Locker locker(server.getClientListMutex()); boost::shared_ptr @@ -1494,11 +1482,11 @@ TEST_F(AuthSrvTest, RRClass::IN(), RRType::TXT()); for (ThrowWhen* when(throws); *when != THROW_NEVER; ++when) { createRequestPacket(request_message, IPPROTO_UDP); - setupThrow(server, datasrc_configurator_, *when, true); + setupThrow(server, *when, true); processAndCheckSERVFAIL(); // To be sure, check same for non-isc-exceptions createRequestPacket(request_message, IPPROTO_UDP); - setupThrow(server, datasrc_configurator_, *when, false); + setupThrow(server, *when, false); processAndCheckSERVFAIL(); } } @@ -1514,7 +1502,7 @@ TEST_F(AuthSrvTest, ) { createDataFromFile("nsec3query_nodnssec_fromWire.wire"); - setupThrow(server, datasrc_configurator_, THROW_AT_GET_CLASS, true); + setupThrow(server, THROW_AT_GET_CLASS, true); // getClass is not called so it should just answer server.processMessage(*io_message, *parse_message, *response_obuffer, @@ -1538,7 +1526,7 @@ TEST_F(AuthSrvTest, ConstRRsetPtr empty_rrset(new RRset(Name("foo.example"), RRClass::IN(), RRType::TXT(), RRTTL(0))); - setupThrow(server, datasrc_configurator_, THROW_NEVER, true, empty_rrset); + setupThrow(server, THROW_NEVER, true, empty_rrset); // Repeat the query processing two times. Due to the faked RRset, // toWire() should throw, and it should result in SERVFAIL. diff --git a/src/bin/auth/tests/command_unittest.cc b/src/bin/auth/tests/command_unittest.cc index 68e1b12e3b..c7465263ea 100644 --- a/src/bin/auth/tests/command_unittest.cc +++ b/src/bin/auth/tests/command_unittest.cc @@ -77,7 +77,6 @@ protected: MockXfroutClient xfrout_; MockSocketSessionForwarder ddns_forwarder_; AuthSrv server_; - DataSourceConfigurator datasrc_configurator_; ConstElementPtr result_; // The shutdown command parameter ConstElementPtr param_; @@ -191,7 +190,7 @@ zoneChecks(AuthSrv& server) { } void -configureZones(AuthSrv& server, DataSourceConfigurator& datasrc_configurator) { +configureZones(AuthSrv& server) { ASSERT_EQ(0, system(INSTALL_PROG " -c " TEST_DATA_DIR "/test1.zone.in " TEST_DATA_BUILDDIR "/test1.zone.copied")); ASSERT_EQ(0, system(INSTALL_PROG " -c " TEST_DATA_DIR "/test2.zone.in " @@ -209,7 +208,7 @@ configureZones(AuthSrv& server, DataSourceConfigurator& datasrc_configurator) { " \"cache-enable\": true" "}]}")); - datasrc_configurator.reconfigure(server, config); + configureDataSource(server, config); zoneChecks(server); } @@ -235,7 +234,7 @@ newZoneChecks(AuthSrv& server) { } TEST_F(AuthCommandTest, loadZone) { - configureZones(server_, datasrc_configurator_); + configureZones(server_); ASSERT_EQ(0, system(INSTALL_PROG " -c " TEST_DATA_DIR "/test1-new.zone.in " @@ -272,7 +271,7 @@ TEST_F(AuthCommandTest, " \"cache-enable\": true," " \"cache-zones\": [\"example.org\"]" "}]}")); - datasrc_configurator_.reconfigure(server_, config); + configureDataSource(server_, config); { isc::util::thread::Mutex::Locker locker(server_.getClientListMutex()); @@ -336,7 +335,7 @@ TEST_F(AuthCommandTest, " \"cache-enable\": true," " \"cache-zones\": [\"example.com\"]" "}]}")); - EXPECT_THROW(datasrc_configurator_.reconfigure(server_, config2), + EXPECT_THROW(configureDataSource(server_, config2), ConfigurableClientList::ConfigurationError); result_ = execAuthServerCommand(server_, "loadzone", @@ -351,7 +350,7 @@ TEST_F(AuthCommandTest, } TEST_F(AuthCommandTest, loadBrokenZone) { - configureZones(server_, datasrc_configurator_); + configureZones(server_); ASSERT_EQ(0, system(INSTALL_PROG " -c " TEST_DATA_DIR "/test1-broken.zone.in " @@ -364,7 +363,7 @@ TEST_F(AuthCommandTest, loadBrokenZone) { } TEST_F(AuthCommandTest, loadUnreadableZone) { - configureZones(server_, datasrc_configurator_); + configureZones(server_); // install the zone file as unreadable ASSERT_EQ(0, system(INSTALL_PROG " -c -m 000 " TEST_DATA_DIR @@ -387,7 +386,7 @@ TEST_F(AuthCommandTest, loadZoneWithoutDataSrc) { } TEST_F(AuthCommandTest, loadZoneInvalidParams) { - configureZones(server_, datasrc_configurator_); + configureZones(server_); // null arg result_ = execAuthServerCommand(server_, "loadzone", ElementPtr()); diff --git a/src/bin/auth/tests/datasrc_configurator_unittest.cc b/src/bin/auth/tests/datasrc_configurator_unittest.cc index 3821dd4429..f294536475 100644 --- a/src/bin/auth/tests/datasrc_configurator_unittest.cc +++ b/src/bin/auth/tests/datasrc_configurator_unittest.cc @@ -60,19 +60,23 @@ private: typedef shared_ptr ListPtr; -// We use the test fixture as both parameters, this makes it possible -// to easily fake all needed methods and look that they were called. -typedef DataSourceConfiguratorGeneric Configurator; +void +testConfigureDataSource(DatasrcConfiguratorTest& 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(test, + config); +} void -datasrcConfigHandler(DatasrcConfiguratorTest* fake_server, - Configurator* configurator, const std::string&, +datasrcConfigHandler(DatasrcConfiguratorTest* fake_server, const std::string&, isc::data::ConstElementPtr config, const isc::config::ConfigData&) { if (config->contains("classes")) { - configurator->reconfigure(*fake_server, config->get("classes")); + testConfigureDataSource(*fake_server, config->get("classes")); } } @@ -113,28 +117,24 @@ protected: false)); } void TearDown() { - // Make sure no matter what we did, it is cleaned up. + // Make sure no matter what we did, it is cleaned up. Also check + // we really have subscribed to the configuration, and after removing + // it we actually cancel it. + EXPECT_TRUE(session.haveSubscription("data_sources", "*")); mccs->removeRemoteConfig("data_sources"); + EXPECT_FALSE(session.haveSubscription("data_sources", "*")); } - void init(const ElementPtr& config = ElementPtr()) { + void SetUp() { session.getMessages()-> add(createAnswer(0, moduleSpecFromFile(string(PLUGIN_DATA_PATH) + "/datasrc.spec"). getFullSpec())); - if (config) { - session.getMessages()->add(createAnswer(0, config)); - } else { - session.getMessages()-> - add(createAnswer(0, ElementPtr(new MapElement))); - } + session.getMessages()->add(createAnswer(0, + ElementPtr(new MapElement))); mccs->addRemoteConfig("data_sources", boost::bind(datasrcConfigHandler, - this, &configurator_, - _1, _2, _3), false); - } - void SetUp() { - init(); + this, _1, _2, _3), false); } ElementPtr buildConfig(const string& config) const { const ElementPtr internal(Element::fromJSON(config)); @@ -155,31 +155,12 @@ protected: } FakeSession session; auto_ptr mccs; - Configurator configurator_; const string specfile; std::map lists_; string log_; mutable isc::util::thread::Mutex mutex_; }; -// Check the initialization (and cleanup) -TEST_F(DatasrcConfiguratorTest, DISABLED_initialization) { - // It can't be initialized again - EXPECT_THROW(init(), InvalidOperation); - EXPECT_TRUE(session.haveSubscription("data_sources", "*")); - EXPECT_FALSE(session.haveSubscription("data_sources", "*")); - // We can't reconfigure now (not even manually) - EXPECT_THROW(configurator_.reconfigure(*this, - ElementPtr(new MapElement())), - InvalidOperation); - // If the server param is NULL, it does not work - EXPECT_THROW(Configurator configurator, InvalidParameter); - EXPECT_FALSE(session.haveSubscription("data_sources", "*")); // TBD - // But we can initialize it again now - EXPECT_NO_THROW(init()); - EXPECT_TRUE(session.haveSubscription("data_sources", "*")); -} - // Push there a configuration with a single list. TEST_F(DatasrcConfiguratorTest, createList) { initializeINList(); @@ -283,7 +264,7 @@ TEST_F(DatasrcConfiguratorTest, rollbackDeletion) { const ElementPtr config1(Element::fromJSON("{\"IN\": [{\"type\": \"yyy\"}], " "\"CH\": [{\"type\": \"xxx\"}]}")); - configurator_.reconfigure(*this, config1); + testConfigureDataSource(*this, config1); const ElementPtr config2(Element::fromJSON("{\"IN\": [{\"type\": 13}]}")); // This would delete CH. However, the IN one fails. @@ -291,7 +272,7 @@ TEST_F(DatasrcConfiguratorTest, rollbackDeletion) { // 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. - EXPECT_THROW(configurator_.reconfigure(*this, config2), TypeError); + EXPECT_THROW(testConfigureDataSource(*this, config2), TypeError); EXPECT_EQ("yyy", lists_[RRClass::IN()]->getConf()); EXPECT_EQ("xxx", lists_[RRClass::CH()]->getConf()); } @@ -304,13 +285,13 @@ TEST_F(DatasrcConfiguratorTest, rollbackConfiguration) { const ElementPtr config1(Element::fromJSON("{\"IN\": [{\"type\": \"yyy\"}], " "\"CH\": [{\"type\": \"xxx\"}]}")); - configurator_.reconfigure(*this, config1); + testConfigureDataSource(*this, config1); // Now, the CH happens first. But nevertheless, it should be // restored to the previoeus version. const ElementPtr config2(Element::fromJSON("{\"IN\": [{\"type\": 13}], " "\"CH\": [{\"type\": \"yyy\"}]}")); - EXPECT_THROW(configurator_.reconfigure(*this, config2), TypeError); + EXPECT_THROW(testConfigureDataSource(*this, config2), TypeError); EXPECT_EQ("yyy", lists_[RRClass::IN()]->getConf()); EXPECT_EQ("xxx", lists_[RRClass::CH()]->getConf()); } From c91bffdd00deb8b7f3ceda9954a84e31c0cfb9f1 Mon Sep 17 00:00:00 2001 From: JINMEI Tatuya Date: Wed, 3 Oct 2012 17:28:41 -0700 Subject: [PATCH 6/8] [2203] a piggy back fix: prevent redundant initial data configuration. this addresses the issue described in #2291. Still not really clean, but thanks to boost::bind we can centralize all the code logic in the callback, so I think it's now less likely that we forget cleaning it up when the hack is not necessary. --- src/bin/auth/main.cc | 29 +++++++++++++++++++---------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/src/bin/auth/main.cc b/src/bin/auth/main.cc index eecc5aa1a6..d0ff8f48d5 100644 --- a/src/bin/auth/main.cc +++ b/src/bin/auth/main.cc @@ -87,13 +87,25 @@ my_command_handler(const string& command, ConstElementPtr args) { } void -datasrcConfigHandler(AuthSrv* server, const std::string&, +datasrcConfigHandler(AuthSrv* server, bool* first_time, + ModuleCCSession* config_session, const std::string&, isc::data::ConstElementPtr config, const isc::config::ConfigData&) { assert(server != NULL); if (config->contains("classes")) { - configureDataSource(*server, config->get("classes")); + 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")); + } else { + configureDataSource(*server, config->get("classes")); + } } } @@ -205,19 +217,16 @@ main(int argc, char* argv[]) { isc::server_common::initKeyring(*config_session); auth_server->setTSIGKeyRing(&isc::server_common::keyring); - // Start the data source configuration + // Start the data source configuration. We pass first_time and + // config_session for the hack described in datasrcConfigHandler. + bool first_time = true; config_session->addRemoteConfig("data_sources", boost::bind(datasrcConfigHandler, - auth_server, + auth_server, &first_time, + config_session, _1, _2, _3), false); - // HACK: The default is not passed to the handler. This one will - // get the default (or, current value). Further updates will work - // the usual way. - configureDataSource(*auth_server, - config_session->getRemoteConfigValue("data_sources", "classes")); - // Now start asynchronous read. config_session->start(); LOG_DEBUG(auth_logger, DBG_AUTH_START, AUTH_CONFIG_CHANNEL_STARTED); From 6b99a39f0ab27939c29a05b0332d5ec255bc0764 Mon Sep 17 00:00:00 2001 From: JINMEI Tatuya Date: Wed, 3 Oct 2012 22:58:06 -0700 Subject: [PATCH 7/8] [2203] forotten cleanup: removed now-unused header file. --- src/bin/auth/main.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/src/bin/auth/main.cc b/src/bin/auth/main.cc index d0ff8f48d5..8431a488a2 100644 --- a/src/bin/auth/main.cc +++ b/src/bin/auth/main.cc @@ -42,7 +42,6 @@ #include #include -#include #include #include From 44d9dfa8aad85d583b7b90fa01c50613a9d9d1a5 Mon Sep 17 00:00:00 2001 From: JINMEI Tatuya Date: Fri, 5 Oct 2012 10:12:42 -0700 Subject: [PATCH 8/8] [2203] renamed datasrc_configurator to datasrc_config; it represents it better. --- src/bin/auth/Makefile.am | 2 +- src/bin/auth/benchmarks/Makefile.am | 2 +- src/bin/auth/benchmarks/query_bench.cc | 2 +- ...asrc_configurator.cc => datasrc_config.cc} | 2 +- ...atasrc_configurator.h => datasrc_config.h} | 8 ++--- src/bin/auth/main.cc | 2 +- src/bin/auth/tests/Makefile.am | 4 +-- src/bin/auth/tests/auth_srv_unittest.cc | 2 +- src/bin/auth/tests/command_unittest.cc | 2 +- ...unittest.cc => datasrc_config_unittest.cc} | 31 +++++++++---------- 10 files changed, 28 insertions(+), 29 deletions(-) rename src/bin/auth/{datasrc_configurator.cc => datasrc_config.cc} (97%) rename src/bin/auth/{datasrc_configurator.h => datasrc_config.h} (97%) rename src/bin/auth/tests/{datasrc_configurator_unittest.cc => datasrc_config_unittest.cc} (92%) diff --git a/src/bin/auth/Makefile.am b/src/bin/auth/Makefile.am index df30a428db..9eee9d4d54 100644 --- a/src/bin/auth/Makefile.am +++ b/src/bin/auth/Makefile.am @@ -55,7 +55,7 @@ b10_auth_SOURCES += auth_config.cc auth_config.h b10_auth_SOURCES += command.cc command.h b10_auth_SOURCES += common.h common.cc b10_auth_SOURCES += statistics.cc statistics.h -b10_auth_SOURCES += datasrc_configurator.h datasrc_configurator.cc +b10_auth_SOURCES += datasrc_config.h datasrc_config.cc b10_auth_SOURCES += main.cc nodist_b10_auth_SOURCES = auth_messages.h auth_messages.cc diff --git a/src/bin/auth/benchmarks/Makefile.am b/src/bin/auth/benchmarks/Makefile.am index fcfcb8add4..c525b665d3 100644 --- a/src/bin/auth/benchmarks/Makefile.am +++ b/src/bin/auth/benchmarks/Makefile.am @@ -17,7 +17,7 @@ query_bench_SOURCES += ../auth_srv.h ../auth_srv.cc query_bench_SOURCES += ../auth_config.h ../auth_config.cc query_bench_SOURCES += ../statistics.h ../statistics.cc query_bench_SOURCES += ../auth_log.h ../auth_log.cc -query_bench_SOURCES += ../datasrc_configurator.h ../datasrc_configurator.cc +query_bench_SOURCES += ../datasrc_config.h ../datasrc_config.cc nodist_query_bench_SOURCES = ../auth_messages.h ../auth_messages.cc diff --git a/src/bin/auth/benchmarks/query_bench.cc b/src/bin/auth/benchmarks/query_bench.cc index 07111a56d4..73f702baff 100644 --- a/src/bin/auth/benchmarks/query_bench.cc +++ b/src/bin/auth/benchmarks/query_bench.cc @@ -30,7 +30,7 @@ #include #include -#include +#include #include #include diff --git a/src/bin/auth/datasrc_configurator.cc b/src/bin/auth/datasrc_config.cc similarity index 97% rename from src/bin/auth/datasrc_configurator.cc rename to src/bin/auth/datasrc_config.cc index 5469389a0e..73fb5190c6 100644 --- a/src/bin/auth/datasrc_configurator.cc +++ b/src/bin/auth/datasrc_config.cc @@ -14,7 +14,7 @@ #include #include "auth_srv.h" -#include "datasrc_configurator.h" +#include "datasrc_config.h" // This is a trivial specialization for the commonly used version. // Defined in .cc to avoid accidental creation of multiple copies. diff --git a/src/bin/auth/datasrc_configurator.h b/src/bin/auth/datasrc_config.h similarity index 97% rename from src/bin/auth/datasrc_configurator.h rename to src/bin/auth/datasrc_config.h index b8d21f3e98..79ace2811a 100644 --- a/src/bin/auth/datasrc_configurator.h +++ b/src/bin/auth/datasrc_config.h @@ -12,8 +12,8 @@ // OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR // PERFORMANCE OF THIS SOFTWARE. -#ifndef DATASRC_CONFIGURATOR_H -#define DATASRC_CONFIGURATOR_H +#ifndef DATASRC_CONFIG_H +#define DATASRC_CONFIG_H #include "auth_srv.h" @@ -108,12 +108,12 @@ configureDataSourceGeneric(Server& server, } } -/// \brief Concrete version of DataSourceConfiguratorGeneric for the +/// \brief Concrete version of configureDataSource() for the /// use with authoritative server implementation. void configureDataSource(AuthSrv& server, const isc::data::ConstElementPtr& config); -#endif +#endif // DATASRC_CONFIG_H // Local Variables: // mode: c++ diff --git a/src/bin/auth/main.cc b/src/bin/auth/main.cc index 8431a488a2..b425813006 100644 --- a/src/bin/auth/main.cc +++ b/src/bin/auth/main.cc @@ -34,7 +34,7 @@ #include #include #include -#include +#include #include #include #include diff --git a/src/bin/auth/tests/Makefile.am b/src/bin/auth/tests/Makefile.am index 3b1873678c..6b9d385298 100644 --- a/src/bin/auth/tests/Makefile.am +++ b/src/bin/auth/tests/Makefile.am @@ -42,7 +42,7 @@ run_unittests_SOURCES += ../auth_config.h ../auth_config.cc run_unittests_SOURCES += ../command.h ../command.cc run_unittests_SOURCES += ../common.h ../common.cc run_unittests_SOURCES += ../statistics.h ../statistics.cc -run_unittests_SOURCES += ../datasrc_configurator.h ../datasrc_configurator.cc +run_unittests_SOURCES += ../datasrc_config.h ../datasrc_config.cc run_unittests_SOURCES += datasrc_util.h datasrc_util.cc run_unittests_SOURCES += auth_srv_unittest.cc run_unittests_SOURCES += config_unittest.cc @@ -51,7 +51,7 @@ run_unittests_SOURCES += command_unittest.cc run_unittests_SOURCES += common_unittest.cc run_unittests_SOURCES += query_unittest.cc run_unittests_SOURCES += statistics_unittest.cc -run_unittests_SOURCES += datasrc_configurator_unittest.cc +run_unittests_SOURCES += datasrc_config_unittest.cc run_unittests_SOURCES += run_unittests.cc nodist_run_unittests_SOURCES = ../auth_messages.h ../auth_messages.cc diff --git a/src/bin/auth/tests/auth_srv_unittest.cc b/src/bin/auth/tests/auth_srv_unittest.cc index 00d7350594..8cf514688e 100644 --- a/src/bin/auth/tests/auth_srv_unittest.cc +++ b/src/bin/auth/tests/auth_srv_unittest.cc @@ -36,7 +36,7 @@ #include #include #include -#include +#include #include #include diff --git a/src/bin/auth/tests/command_unittest.cc b/src/bin/auth/tests/command_unittest.cc index c7465263ea..c7994831a5 100644 --- a/src/bin/auth/tests/command_unittest.cc +++ b/src/bin/auth/tests/command_unittest.cc @@ -19,7 +19,7 @@ #include #include #include -#include +#include #include #include diff --git a/src/bin/auth/tests/datasrc_configurator_unittest.cc b/src/bin/auth/tests/datasrc_config_unittest.cc similarity index 92% rename from src/bin/auth/tests/datasrc_configurator_unittest.cc rename to src/bin/auth/tests/datasrc_config_unittest.cc index f294536475..3d3aa58b91 100644 --- a/src/bin/auth/tests/datasrc_configurator_unittest.cc +++ b/src/bin/auth/tests/datasrc_config_unittest.cc @@ -12,7 +12,7 @@ // OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR // PERFORMANCE OF THIS SOFTWARE. -#include +#include #include #include @@ -35,7 +35,7 @@ using namespace boost; namespace { -class DatasrcConfiguratorTest; +class DatasrcConfigTest; class FakeList { public: @@ -61,17 +61,16 @@ private: typedef shared_ptr ListPtr; void -testConfigureDataSource(DatasrcConfiguratorTest& test, +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(test, - config); + configureDataSourceGeneric(test, config); } void -datasrcConfigHandler(DatasrcConfiguratorTest* fake_server, const std::string&, +datasrcConfigHandler(DatasrcConfigTest* fake_server, const std::string&, isc::data::ConstElementPtr config, const isc::config::ConfigData&) { @@ -80,7 +79,7 @@ datasrcConfigHandler(DatasrcConfiguratorTest* fake_server, const std::string&, } } -class DatasrcConfiguratorTest : public ::testing::Test { +class DatasrcConfigTest : public ::testing::Test { public: // These pretend to be the server ListPtr getClientList(const RRClass& rrclass) { @@ -104,7 +103,7 @@ public: return (mutex_); } protected: - DatasrcConfiguratorTest() : + DatasrcConfigTest() : session(ElementPtr(new ListElement), ElementPtr(new ListElement), ElementPtr(new ListElement)), specfile(string(TEST_OWN_DATA_DIR) + "/spec.spec") @@ -162,11 +161,11 @@ protected: }; // Push there a configuration with a single list. -TEST_F(DatasrcConfiguratorTest, createList) { +TEST_F(DatasrcConfigTest, createList) { initializeINList(); } -TEST_F(DatasrcConfiguratorTest, modifyList) { +TEST_F(DatasrcConfigTest, modifyList) { // First, initialize the list initializeINList(); // And now change the configuration of the list @@ -184,7 +183,7 @@ TEST_F(DatasrcConfiguratorTest, modifyList) { } // Check we can have multiple lists at once -TEST_F(DatasrcConfiguratorTest, multiple) { +TEST_F(DatasrcConfigTest, multiple) { const ElementPtr config(buildConfig("{\"IN\": [{\"type\": \"yyy\"}], " "\"CH\": [{\"type\": \"xxx\"}]}")); @@ -204,7 +203,7 @@ TEST_F(DatasrcConfiguratorTest, multiple) { // // It's almost like above, but we initialize first with single-list // config. -TEST_F(DatasrcConfiguratorTest, updateAdd) { +TEST_F(DatasrcConfigTest, updateAdd) { initializeINList(); const ElementPtr config(buildConfig("{\"IN\": [{\"type\": \"yyy\"}], " @@ -222,7 +221,7 @@ TEST_F(DatasrcConfiguratorTest, updateAdd) { } // We delete a class list in this test. -TEST_F(DatasrcConfiguratorTest, updateDelete) { +TEST_F(DatasrcConfigTest, updateDelete) { initializeINList(); const ElementPtr config(buildConfig("{}")); @@ -239,7 +238,7 @@ TEST_F(DatasrcConfiguratorTest, updateDelete) { } // Check that we can rollback an addition if something else fails -TEST_F(DatasrcConfiguratorTest, rollbackAddition) { +TEST_F(DatasrcConfigTest, rollbackAddition) { initializeINList(); // The configuration is wrong. However, the CH one will get done first. const ElementPtr @@ -258,7 +257,7 @@ TEST_F(DatasrcConfiguratorTest, rollbackAddition) { } // Check that we can rollback a deletion if something else fails -TEST_F(DatasrcConfiguratorTest, rollbackDeletion) { +TEST_F(DatasrcConfigTest, rollbackDeletion) { initializeINList(); // Put the CH there const ElementPtr @@ -279,7 +278,7 @@ TEST_F(DatasrcConfiguratorTest, rollbackDeletion) { // Check that we can roll back configuration change if something // fails later on. -TEST_F(DatasrcConfiguratorTest, rollbackConfiguration) { +TEST_F(DatasrcConfigTest, rollbackConfiguration) { initializeINList(); // Put the CH there const ElementPtr