[640] addressed new review comments

src/bin/cmdctl/cmdctl.py.in

@@ -313,7 +313,7 @@ class CommandControl():
             # The 'value' of a specification update can be either
             # a specification, or None. In the first case, simply
             # set it. If it is None, delete the module if it is
-            # known (and ignore otherwise).
+            # known.
             with self._lock:
                 if args[1] is None:
                     if args[0] in self.modules_spec:

src/bin/xfrout/xfrout.py.in

@@ -974,13 +974,18 @@ class XfroutServer:
         self._notifier.shutdown()
         if self._unix_socket_server:
             self._unix_socket_server.shutdown()
-
-            # Wait for all threads to terminate
-            main_thread = threading.currentThread()
-            for th in threading.enumerate():
-                if th is main_thread:
-                    continue
-                th.join()
+        self._wait_for_threads()
+
+    def _wait_for_threads(self):
+        # Wait for all threads to terminate. this is a call that is only used
+        # in shutdown(), but it has its own method, so we can test shutdown
+        # without involving thread operations (the test would override this
+        # method)
+        main_thread = threading.currentThread()
+        for th in threading.enumerate():
+            if th is main_thread:
+                continue
+            th.join()
 
     def command_handler(self, cmd, args):
         if cmd == "shutdown":

src/lib/config/ccsession.cc

@@ -490,20 +490,14 @@ ModuleCCSession::ModuleCCSession(
 }
 
 ModuleCCSession::~ModuleCCSession() {
-    // Logging itself can possibly throw too
-    // TODO: add exception-free logging calls
     try {
-        try {
-            sendStopping();
-        } catch (const std::exception& exc) {
-            LOG_ERROR(config_logger,
-                      CONFIG_CCSESSION_STOPPING).arg(exc.what());
-        } catch (...) {
-            LOG_ERROR(config_logger,
-                      CONFIG_CCSESSION_STOPPING_UNKNOWN);
-        }
+        sendStopping();
+    } catch (const std::exception& exc) {
+        LOG_ERROR(config_logger,
+                  CONFIG_CCSESSION_STOPPING).arg(exc.what());
     } catch (...) {
-        // Can't really do anything at this point...
+        LOG_ERROR(config_logger,
+                  CONFIG_CCSESSION_STOPPING_UNKNOWN);
     }
 };
 

src/lib/python/isc/config/ccsession.py

@@ -438,9 +438,6 @@ class UIModuleCCSession(MultiConfigData):
             be run after each interaction (at this moment). It is usually
             also combined with request_current_config(). For that reason,
             we provide update_specs_and_config() which calls both."""
-        # this step should be unnecessary but is the current way cmdctl returns stuff
-        # so changes are needed there to make this clean (we need a command to simply get the
-        # full specs for everything, including commands etc, not separate gets for that)
         specs = self._conn.send_GET('/module_spec')
         self.clear_specifications()
         for module in specs.keys():
@@ -612,7 +609,6 @@ class UIModuleCCSession(MultiConfigData):
             # answer is either an empty dict (on success), or one
             # containing errors
             if answer == {}:
-                self.update_specs_and_config()
                 self.clear_local_changes()
             elif "error" in answer:
                 raise ModuleCCSessionError("Error: " + str(answer["error"]) + "\n" + "Configuration not committed")

src/lib/python/isc/config/tests/cfgmgr_test.py

@@ -337,8 +337,10 @@ class TestConfigManager(unittest.TestCase):
         # Send the 'ok' that cfgmgr expects back to the fake queue first
         self.fake_session.group_sendmsg(my_ok_answer, "ConfigManager")
 
+        config_version = config_data.BIND10_CONFIG_DATA_VERSION
         self._handle_msg_helper({ "command": [ "set_config",
-                                               [self.name, new_config] ] },
+                                               [ { "version": config_version,
+                                                 self.name: new_config } ] ] },
                                 my_ok_answer)
 
         # The cfgmgr should have eaten the ok message, and sent out an update
@@ -349,7 +351,7 @@ class TestConfigManager(unittest.TestCase):
 
         # Config should have been updated
         self.assertEqual(self.cm.config.data, {self.name: new_config,
-                            'version': config_data.BIND10_CONFIG_DATA_VERSION})
+                            'version': config_version})
 
         # and the queue should now be empty again
         self.assertEqual(len(self.fake_session.message_queue), 0)
@@ -439,13 +441,6 @@ class TestConfigManager(unittest.TestCase):
                                 None)
         self.assertEqual(len(self.fake_session.message_queue), 0)
 
-        # If the module does not exist, or is not seen as 'running', the
-        # same message should not cause new messages to be sent
-        self._handle_msg_helper({ "command": [ "stopping",
-                                               { "module_name": "Foo"}] },
-                                None)
-        self.assertEqual(len(self.fake_session.message_queue), 0)
-
     def test_set_config_virtual(self):
         """Test that if the module is virtual, we don't send it over the
            message bus, but call the checking function.