From: Jelte Jansen Date: Mon, 22 Oct 2012 12:55:33 +0000 (+0200) Subject: [2210] Address review comments X-Git-Tag: trac2402_base~4^2~2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=3a5e410f451b3254df6bf454e50af98b80ce4f16;p=thirdparty%2Fkea.git [2210] Address review comments as discussed in the ticket comments --- diff --git a/src/bin/auth/auth_messages.mes b/src/bin/auth/auth_messages.mes index 55682d2202..39d0283ea7 100644 --- a/src/bin/auth/auth_messages.mes +++ b/src/bin/auth/auth_messages.mes @@ -75,28 +75,48 @@ exception type is even more unexpected. This may rather indicate some run time failure than program errors, but in any case the server needs to be restarted by hand. -% AUTH_DATASRC_CLIENTS_BUILDER_RECONFIGURE_CONFIG_ERROR Error in configuration: %1 +% AUTH_DATASRC_CLIENTS_BUILDER_RECONFIGURE_CONFIG_ERROR Error in data source configuration: %1 The thread for maintaining data source clients has received a command to -reconfigure, but the parameter data is not in the expected format, or contains -another error. This data should have been validated, but may still contain -an error. The system is still running with the data sources from before -the reconfigure command, and the configuration data needs to be checked. +reconfigure, but the parameter data contains an error. This data should have +been validated, but may still contain an error. The system is still running +with the data sources that were previously configured (i.e. as if the +configuration has not changed), and the configuration data needs to be +checked. The specific problem is printed in the log message. -% AUTH_DATASRC_CLIENTS_BUILDER_RECONFIGURE_ERROR Error in configuration: %1 +% AUTH_DATASRC_CLIENTS_BUILDER_RECONFIGURE_ERROR Error in data source configuration: %1 The thread for maintaining data source clients has received a command to reconfigure, but raised an exception while reconfiguring. This is most likely a bug in the data source, but it is probably a good idea to verify the configuration itself. The system is still running with the data sources -from before the reconfigure command, and the configuration data needs to be -checked. The specific problem is printed in the log message. +that were previously configured (i.e. as if the configuration has not +changed), and the configuration data needs to be checked. +The specific problem is printed in the log message. + +% AUTH_DATASRC_CLIENTS_BUILDER_RECONFIGURE_FORMAT_ERROR Error in data source configuration format: %1 +The thread for maintaining data source clients has received a command to +reconfigure, but the parameter data is not in the expected format (a list of +maps). This should have been validated, and this error is most likely to be +a bug in the system, but it is probably a good idea to verify the +configuration itself. The system is still running with the data sources that +were previously configured (i.e. as if the configuration has not changed), +and the configuration data needs to be checked. +The specific problem is printed in the log message. -% AUTH_DATASRC_CLIENTS_BUILDER_RECONFIGURE_UNKNOWN_ERROR Unknown uncaught exception in reconfigure +% AUTH_DATASRC_CLIENTS_BUILDER_RECONFIGURE_STARTED data source reconfiguration started +The thread for maintaining data source clients has received a command to +reconfigure, and has now started this process. + +% AUTH_DATASRC_CLIENTS_BUILDER_RECONFIGURE_SUCCESS data source reconfiguration completed succesfully +The thread for maintaining data source clients has finished reconfiguring +the data source clients, and is now running with the new configuration. + +% AUTH_DATASRC_CLIENTS_BUILDER_RECONFIGURE_UNKNOWN_ERROR unknown uncaught exception in data source reconfigure The thread for maintaining data source clients has received a command to reconfigure, but raised an unknown exception while reconfiguring. This is a bug in the data source. The system is still running with the data sources -from before the reconfigure command, and the configuration data needs to be -checked. +that were previously configured (i.e. as if the configuration has not +changed), and the configuration data needs to be checked. % AUTH_DATASRC_CLIENTS_BUILDER_STARTED data source builder thread started A separate thread for maintaining data source clients has been started. diff --git a/src/bin/auth/datasrc_clients_mgr.h b/src/bin/auth/datasrc_clients_mgr.h index 8fa7d344f1..ec2188d207 100644 --- a/src/bin/auth/datasrc_clients_mgr.h +++ b/src/bin/auth/datasrc_clients_mgr.h @@ -47,7 +47,9 @@ namespace datasrc_clientmgr_internal { /// \brief ID of commands from the DataSrcClientsMgr to DataSrcClientsBuilder. enum CommandID { NOOP, ///< Do nothing. Only useful for tests; no argument - RECONFIGURE, ///< Reconfigure the datasource client lists, configuration argument (TODO: describe here?) + RECONFIGURE, ///< Reconfigure the datasource client lists, + /// the argument to the command is the full new + /// datasources configuration. SHUTDOWN, ///< Shutdown the builder; no argument NUM_COMMANDS }; @@ -167,8 +169,9 @@ private: std::list command_queue_; CondVarType cond_; // condition variable for queue operations MutexType queue_mutex_; // mutex to protect the queue - isc::datasrc::DataSrcClientListsPtr clients_map_; - MutexType map_mutex_; + datasrc::DataSrcClientListsPtr clients_map_; + // map of actual data source client objects + MutexType map_mutex_; // mutex to protect the clients map BuilderType builder_; ThreadType builder_thread_; // for safety this should be placed last @@ -204,7 +207,7 @@ public: /// \throw None DataSrcClientsBuilderBase(std::list* command_queue, CondVarType* cond, MutexType* queue_mutex, - isc::datasrc::DataSrcClientListsPtr* clients_map, + datasrc::DataSrcClientListsPtr* clients_map, MutexType* map_mutex ) : command_queue_(command_queue), cond_(cond), queue_mutex_(queue_mutex), @@ -229,17 +232,32 @@ private: // implementation really does nothing. void doNoop() {} - void doReconfigure(const isc::data::ConstElementPtr& config) { + void doReconfigure(const data::ConstElementPtr& config) { if (config) { + LOG_INFO(auth_logger, + AUTH_DATASRC_CLIENTS_BUILDER_RECONFIGURE_STARTED); try { - isc::datasrc::DataSrcClientListsPtr new_clients_map = + // Define new_clients_map outside of the block that + // has the lock scope; this way, after the swap, + // the lock is guaranteed to be released before + // the old data is destroyed, minimizing the lock + // duration. + datasrc::DataSrcClientListsPtr new_clients_map = configureDataSource(config); - typename MutexType::Locker locker(*map_mutex_); - std::swap(new_clients_map, *clients_map_); - // lock is released by leaving scope - } catch (const isc::data::TypeError& type_error) { + { + typename MutexType::Locker locker(*map_mutex_); + new_clients_map.swap(*clients_map_); + } // lock is released by leaving scope + LOG_INFO(auth_logger, + AUTH_DATASRC_CLIENTS_BUILDER_RECONFIGURE_SUCCESS); + } catch (const datasrc::ConfigurableClientList::ConfigurationError& + config_error) { LOG_ERROR(auth_logger, AUTH_DATASRC_CLIENTS_BUILDER_RECONFIGURE_CONFIG_ERROR). + arg(config_error.what()); + } catch (const data::TypeError& type_error) { + LOG_ERROR(auth_logger, + AUTH_DATASRC_CLIENTS_BUILDER_RECONFIGURE_FORMAT_ERROR). arg(type_error.what()); } catch (const std::exception& exc) { LOG_ERROR(auth_logger, @@ -249,6 +267,7 @@ private: LOG_ERROR(auth_logger, AUTH_DATASRC_CLIENTS_BUILDER_RECONFIGURE_UNKNOWN_ERROR); } + // old clients_map_ data is released by leaving scope } } @@ -256,7 +275,7 @@ private: std::list* command_queue_; CondVarType* cond_; MutexType* queue_mutex_; - isc::datasrc::DataSrcClientListsPtr* clients_map_; + datasrc::DataSrcClientListsPtr* clients_map_; MutexType* map_mutex_; }; @@ -282,7 +301,7 @@ DataSrcClientsBuilderBase::run() { } current_commands.splice(current_commands.end(), *command_queue_); - } // the lock is release here. + } // the lock is released here. while (keep_running && !current_commands.empty()) { keep_running = handleCommand(current_commands.front()); diff --git a/src/bin/auth/datasrc_config.h b/src/bin/auth/datasrc_config.h index 68c7358e71..c33a4b0f08 100644 --- a/src/bin/auth/datasrc_config.h +++ b/src/bin/auth/datasrc_config.h @@ -48,6 +48,8 @@ /// \param config The configuration value to parse. It is in the form /// as an update from the config manager. /// \return A map from RR classes to configured lists. +/// \throw ConfigurationError if the config element is not in the expected +/// format (A map of lists) template boost::shared_ptr > > // = ListMap below @@ -58,7 +60,6 @@ configureDataSourceGeneric(const isc::data::ConstElementPtr& config) { boost::shared_ptr new_lists(new ListMap); - // Go through the configuration and create corresponding list. const Map& map(config->mapValue()); for (Map::const_iterator it(map.begin()); it != map.end(); ++it) { const isc::dns::RRClass rrclass(it->first); diff --git a/src/bin/auth/tests/datasrc_clients_builder_unittest.cc b/src/bin/auth/tests/datasrc_clients_builder_unittest.cc index ff39b95227..71f4407cae 100644 --- a/src/bin/auth/tests/datasrc_clients_builder_unittest.cc +++ b/src/bin/auth/tests/datasrc_clients_builder_unittest.cc @@ -117,6 +117,18 @@ TEST_F(DataSrcClientsBuilderTest, reconfigure) { "}" ); + // A configuration that is 'correct' in the top-level, but contains + // bad data for the type it specifies + ConstElementPtr bad_config = isc::data::Element::fromJSON( + "{" + "\"IN\": [{" + " \"type\": \"MasterFiles\"," + " \"params\": { \"foo\": [ 1, 2, 3, 4 ]}," + " \"cache-enable\": true" + "}]" + "}" + ); + reconfig_cmd.second = good_config; EXPECT_TRUE(builder.handleCommand(reconfig_cmd)); EXPECT_EQ(1, clients_map->size()); @@ -134,6 +146,15 @@ TEST_F(DataSrcClientsBuilderTest, reconfigure) { // Building failed, so map mutex should not have been locked again EXPECT_EQ(1, map_mutex.lock_count); + // The same for a configuration that has bad data for the type it + // specifies + reconfig_cmd.second = bad_config; + builder.handleCommand(reconfig_cmd); + EXPECT_TRUE(builder.handleCommand(reconfig_cmd)); + EXPECT_EQ(working_config_clients, clients_map); + // Building failed, so map mutex should not have been locked again + EXPECT_EQ(1, map_mutex.lock_count); + // The same goes for an empty parameter (it should at least be // an empty map) reconfig_cmd.second = ConstElementPtr();