[2210] Address review comments

as discussed in the ticket comments

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.

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::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>* 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>* command_queue_;
     CondVarType* cond_;
     MutexType* queue_mutex_;
-    isc::datasrc::DataSrcClientListsPtr* clients_map_;
+    datasrc::DataSrcClientListsPtr* 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());

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);

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();