BIND 10 trac2210, updated. 20fdec048d51fa193425ebb284278ffeec37b93f [2210] rename DataSrcClientListsPtr to ClientListMapPtr
BIND 10 source code commits
bind10-changes at lists.isc.org
Mon Oct 22 13:09:04 UTC 2012
The branch, trac2210 has been updated
via 20fdec048d51fa193425ebb284278ffeec37b93f (commit)
via 3a5e410f451b3254df6bf454e50af98b80ce4f16 (commit)
from 213bd813ebe0eacec4bda6d11301f81315494364 (commit)
Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.
- Log -----------------------------------------------------------------
commit 20fdec048d51fa193425ebb284278ffeec37b93f
Author: Jelte Jansen <jelte at isc.org>
Date: Mon Oct 22 15:08:34 2012 +0200
[2210] rename DataSrcClientListsPtr to ClientListMapPtr
commit 3a5e410f451b3254df6bf454e50af98b80ce4f16
Author: Jelte Jansen <jelte at isc.org>
Date: Mon Oct 22 14:55:33 2012 +0200
[2210] Address review comments
as discussed in the ticket comments
-----------------------------------------------------------------------
Summary of changes:
src/bin/auth/auth_messages.mes | 42 ++++++++++++++-----
src/bin/auth/auth_srv.cc | 6 +--
src/bin/auth/auth_srv.h | 4 +-
src/bin/auth/datasrc_clients_mgr.h | 43 ++++++++++++++------
src/bin/auth/datasrc_config.cc | 2 +-
src/bin/auth/datasrc_config.h | 5 ++-
src/bin/auth/main.cc | 2 +-
src/bin/auth/tests/auth_srv_unittest.cc | 8 ++--
src/bin/auth/tests/command_unittest.cc | 2 +-
.../auth/tests/datasrc_clients_builder_unittest.cc | 27 ++++++++++--
src/bin/auth/tests/test_datasrc_clients_mgr.cc | 2 +-
src/bin/auth/tests/test_datasrc_clients_mgr.h | 4 +-
src/lib/datasrc/client_list.h | 2 +-
13 files changed, 105 insertions(+), 44 deletions(-)
-----------------------------------------------------------------------
diff --git a/src/bin/auth/auth_messages.mes b/src/bin/auth/auth_messages.mes
index 55682d2..39d0283 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/auth_srv.cc b/src/bin/auth/auth_srv.cc
index 917ef48..b555ea5 100644
--- a/src/bin/auth/auth_srv.cc
+++ b/src/bin/auth/auth_srv.cc
@@ -269,7 +269,7 @@ public:
const shared_ptr<TSIGKeyRing>* keyring_;
/// The data source client list
- DataSrcClientListsPtr datasrc_client_lists_;
+ ClientListMapPtr datasrc_client_lists_;
shared_ptr<ConfigurableClientList> getDataSrcClientList(
const RRClass& rrclass)
@@ -933,8 +933,8 @@ AuthSrv::destroyDDNSForwarder() {
}
}
-DataSrcClientListsPtr
-AuthSrv::swapDataSrcClientLists(DataSrcClientListsPtr new_lists) {
+ClientListMapPtr
+AuthSrv::swapDataSrcClientLists(ClientListMapPtr new_lists) {
// TODO: Debug-build only check
if (!impl_->mutex_.locked()) {
isc_throw(isc::Unexpected, "Not locked!");
diff --git a/src/bin/auth/auth_srv.h b/src/bin/auth/auth_srv.h
index a70d51c..94efd08 100644
--- a/src/bin/auth/auth_srv.h
+++ b/src/bin/auth/auth_srv.h
@@ -328,8 +328,8 @@ public:
/// \param new_lists Shared pointer to a new set of data source client
/// lists.
/// \return The previous set of lists. It can be NULL.
- isc::datasrc::DataSrcClientListsPtr
- swapDataSrcClientLists(isc::datasrc::DataSrcClientListsPtr new_lists);
+ isc::datasrc::ClientListMapPtr
+ swapDataSrcClientLists(isc::datasrc::ClientListMapPtr new_lists);
/// \brief Returns the currently used client list for the class.
///
diff --git a/src/bin/auth/datasrc_clients_mgr.h b/src/bin/auth/datasrc_clients_mgr.h
index 8fa7d34..0fdfa81 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<datasrc_clientmgr_internal::Command> 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::ClientListMapPtr 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>* command_queue,
CondVarType* cond, MutexType* queue_mutex,
- isc::datasrc::DataSrcClientListsPtr* clients_map,
+ datasrc::ClientListMapPtr* 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::ClientListMapPtr 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>* command_queue_;
CondVarType* cond_;
MutexType* queue_mutex_;
- isc::datasrc::DataSrcClientListsPtr* clients_map_;
+ datasrc::ClientListMapPtr* clients_map_;
MutexType* map_mutex_;
};
@@ -282,7 +301,7 @@ DataSrcClientsBuilderBase<MutexType, CondVarType>::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.cc b/src/bin/auth/datasrc_config.cc
index 6452f95..4869050 100644
--- a/src/bin/auth/datasrc_config.cc
+++ b/src/bin/auth/datasrc_config.cc
@@ -17,7 +17,7 @@
// This is a trivial specialization for the commonly used version.
// Defined in .cc to avoid accidental creation of multiple copies.
-isc::datasrc::DataSrcClientListsPtr
+isc::datasrc::ClientListMapPtr
configureDataSource(const isc::data::ConstElementPtr& config) {
return (configureDataSourceGeneric<
isc::datasrc::ConfigurableClientList>(config));
diff --git a/src/bin/auth/datasrc_config.h b/src/bin/auth/datasrc_config.h
index 68c7358..4b67e86 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<class List>
boost::shared_ptr<std::map<isc::dns::RRClass,
boost::shared_ptr<List> > > // = ListMap below
@@ -58,7 +60,6 @@ configureDataSourceGeneric(const isc::data::ConstElementPtr& config) {
boost::shared_ptr<ListMap> 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);
@@ -73,7 +74,7 @@ configureDataSourceGeneric(const isc::data::ConstElementPtr& config) {
/// \brief Concrete version of configureDataSource() for the
/// use with authoritative server implementation.
-isc::datasrc::DataSrcClientListsPtr
+isc::datasrc::ClientListMapPtr
configureDataSource(const isc::data::ConstElementPtr& config);
#endif // DATASRC_CONFIG_H
diff --git a/src/bin/auth/main.cc b/src/bin/auth/main.cc
index 47131b1..5d68d16 100644
--- a/src/bin/auth/main.cc
+++ b/src/bin/auth/main.cc
@@ -96,7 +96,7 @@ datasrcConfigHandler(AuthSrv* server, bool* first_time,
{
assert(server != NULL);
if (config->contains("classes")) {
- isc::datasrc::DataSrcClientListsPtr lists;
+ isc::datasrc::ClientListMapPtr lists;
if (*first_time) {
// HACK: The default is not passed to the handler in the first
diff --git a/src/bin/auth/tests/auth_srv_unittest.cc b/src/bin/auth/tests/auth_srv_unittest.cc
index 11a5450..732cdcc 100644
--- a/src/bin/auth/tests/auth_srv_unittest.cc
+++ b/src/bin/auth/tests/auth_srv_unittest.cc
@@ -727,7 +727,7 @@ TEST_F(AuthSrvTest, notifyWithSessionMessageError) {
void
installDataSrcClientLists(AuthSrv& server,
- DataSrcClientListsPtr lists)
+ ClientListMapPtr lists)
{
thread::Mutex::Locker locker(server.getDataSrcClientListMutex());
server.swapDataSrcClientLists(lists);
@@ -1444,7 +1444,7 @@ TEST_F(AuthSrvTest,
boost::shared_ptr<isc::datasrc::ConfigurableClientList>
list(new FakeList(server.getDataSrcClientList(RRClass::IN()),
THROW_NEVER, false));
- DataSrcClientListsPtr lists(new std::map<RRClass, ListPtr>);
+ ClientListMapPtr lists(new std::map<RRClass, ListPtr>);
lists->insert(pair<RRClass, ListPtr>(RRClass::IN(), list));
server.swapDataSrcClientLists(lists);
}
@@ -1475,7 +1475,7 @@ setupThrow(AuthSrv& server, ThrowWhen throw_when, bool isc_exception,
boost::shared_ptr<isc::datasrc::ConfigurableClientList>
list(new FakeList(server.getDataSrcClientList(RRClass::IN()),
throw_when, isc_exception, rrset));
- DataSrcClientListsPtr lists(new std::map<RRClass, ListPtr>);
+ ClientListMapPtr lists(new std::map<RRClass, ListPtr>);
lists->insert(pair<RRClass, ListPtr>(RRClass::IN(), list));
server.swapDataSrcClientLists(lists);
}
@@ -1792,7 +1792,7 @@ TEST_F(AuthSrvTest, clientList) {
isc::util::thread::Mutex::Locker locker(
server.getDataSrcClientListMutex());
- DataSrcClientListsPtr lists; // initially empty
+ ClientListMapPtr lists; // initially empty
// The lists don't exist. Therefore, the list of RRClasses is empty.
EXPECT_TRUE(server.swapDataSrcClientLists(lists)->empty());
diff --git a/src/bin/auth/tests/command_unittest.cc b/src/bin/auth/tests/command_unittest.cc
index 764ab11..15bd662 100644
--- a/src/bin/auth/tests/command_unittest.cc
+++ b/src/bin/auth/tests/command_unittest.cc
@@ -191,7 +191,7 @@ zoneChecks(AuthSrv& server) {
}
void
-installDataSrcClientLists(AuthSrv& server, DataSrcClientListsPtr lists) {
+installDataSrcClientLists(AuthSrv& server, ClientListMapPtr lists) {
isc::util::thread::Mutex::Locker locker(
server.getDataSrcClientListMutex());
server.swapDataSrcClientLists(lists);
diff --git a/src/bin/auth/tests/datasrc_clients_builder_unittest.cc b/src/bin/auth/tests/datasrc_clients_builder_unittest.cc
index ff39b95..74216f6 100644
--- a/src/bin/auth/tests/datasrc_clients_builder_unittest.cc
+++ b/src/bin/auth/tests/datasrc_clients_builder_unittest.cc
@@ -38,7 +38,7 @@ protected:
TestDataSrcClientsBuilder builder;
std::list<Command> command_queue; // test command queue
std::list<Command> delayed_command_queue; // commands available after wait
- DataSrcClientListsPtr clients_map; // configured clients
+ ClientListMapPtr clients_map; // configured clients
TestCondVar cond;
TestMutex queue_mutex;
TestMutex map_mutex;
@@ -104,7 +104,7 @@ TEST_F(DataSrcClientsBuilderTest, reconfigure) {
Command reconfig_cmd(RECONFIGURE, ConstElementPtr());
// Initially, no clients should be there
- EXPECT_EQ(DataSrcClientListsPtr(), clients_map);
+ EXPECT_EQ(ClientListMapPtr(), clients_map);
// A config that doesn't do much except be accepted
ConstElementPtr good_config = isc::data::Element::fromJSON(
@@ -117,13 +117,25 @@ 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());
EXPECT_EQ(1, map_mutex.lock_count);
// Store the nonempty clients map we now have
- DataSrcClientListsPtr working_config_clients(clients_map);
+ ClientListMapPtr working_config_clients(clients_map);
// If a 'bad' command argument got here, the config validation should
// have failed already, but still, the handler should return true,
@@ -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();
diff --git a/src/bin/auth/tests/test_datasrc_clients_mgr.cc b/src/bin/auth/tests/test_datasrc_clients_mgr.cc
index a06c596..37c225e 100644
--- a/src/bin/auth/tests/test_datasrc_clients_mgr.cc
+++ b/src/bin/auth/tests/test_datasrc_clients_mgr.cc
@@ -28,7 +28,7 @@ std::list<Command> FakeDataSrcClientsBuilder::command_queue_copy;
TestCondVar* FakeDataSrcClientsBuilder::cond = NULL;
TestCondVar FakeDataSrcClientsBuilder::cond_copy;
TestMutex* FakeDataSrcClientsBuilder::queue_mutex = NULL;
-isc::datasrc::DataSrcClientListsPtr*
+isc::datasrc::ClientListMapPtr*
FakeDataSrcClientsBuilder::clients_map = NULL;
TestMutex* FakeDataSrcClientsBuilder::map_mutex = NULL;
TestMutex FakeDataSrcClientsBuilder::queue_mutex_copy;
diff --git a/src/bin/auth/tests/test_datasrc_clients_mgr.h b/src/bin/auth/tests/test_datasrc_clients_mgr.h
index a5e015e..98c7682 100644
--- a/src/bin/auth/tests/test_datasrc_clients_mgr.h
+++ b/src/bin/auth/tests/test_datasrc_clients_mgr.h
@@ -131,7 +131,7 @@ public:
static std::list<Command>* command_queue;
static TestCondVar* cond;
static TestMutex* queue_mutex;
- static isc::datasrc::DataSrcClientListsPtr* clients_map;
+ static isc::datasrc::ClientListMapPtr* clients_map;
static TestMutex* map_mutex;
static std::list<Command> command_queue_copy;
static TestCondVar cond_copy;
@@ -149,7 +149,7 @@ public:
std::list<Command>* command_queue,
TestCondVar* cond,
TestMutex* queue_mutex,
- isc::datasrc::DataSrcClientListsPtr* clients_map,
+ isc::datasrc::ClientListMapPtr* clients_map,
TestMutex* map_mutex)
{
FakeDataSrcClientsBuilder::started = false;
diff --git a/src/lib/datasrc/client_list.h b/src/lib/datasrc/client_list.h
index 97d7483..8694b4a 100644
--- a/src/lib/datasrc/client_list.h
+++ b/src/lib/datasrc/client_list.h
@@ -393,7 +393,7 @@ protected:
/// \brief Shortcut typedef for maps of client_lists.
typedef boost::shared_ptr<std::map<
isc::dns::RRClass, boost::shared_ptr<ConfigurableClientList> > >
- DataSrcClientListsPtr;
+ ClientListMapPtr;
} // namespace datasrc
} // namespace isc
More information about the bind10-changes
mailing list