[1451] addressed review comments

src/bin/ddns/b10-ddns.8

@@ -68,7 +68,7 @@ The configurable settings are:
 .PP
 
 \fIzones\fR
-The zones option is a named set of zones that can be updated with DDNS\&. Each entry has one element called update_acls, which itself is a list of ACLs that define update permissions\&. By default this is empty; DDNS must be explicitely enabled per zone\&.
+The zones option is a named set of zones that can be updated with DDNS\&. Each entry has one element called update_acl, which is is a list of access control rules that define update permissions\&. By default this is empty; DDNS must be explicitely enabled per zone\&.
 .PP
 The module commands are:
 .PP

src/bin/ddns/b10-ddns.xml

@@ -106,8 +106,8 @@
     <para>
       <varname>zones</varname>
       The zones option is a named set of zones that can be updated with
-      DDNS. Each entry has one element called update_acls, which itself
+      DDNS. Each entry has one element called update_acl, which is
-      is a list of ACLs that define update permissions.
+      is a list of access control rules that define update permissions.
       By default this is empty; DDNS must be explicitely enabled per zone.
     </para>
 

src/bin/ddns/ddns.py.in

@@ -63,15 +63,6 @@ class DDNSSession:
 
     def __init__(self):
         '''Initialize a DDNS Session'''
-        self._handle()
-
-    def _handle(self):
-        '''
-        Handle a DDNS update.
-        This should be called from the initializer, and should contain the
-        logic for doing the checks, handling the update, and responding with
-        the result.
-        '''
         pass
 
 class DDNSServer:
@@ -120,6 +111,7 @@ class DDNSServer:
         so the main loop in run() stops.
         '''
         self._shutdown = True
+        logger.info(DDNS_SHUTDOWN)
 
     def run(self):
         '''
@@ -128,7 +120,13 @@ class DDNSServer:
         '''
         logger.info(DDNS_RUNNING)
         while not self._shutdown:
+            # We do not catch any exceptions here right now, but this would
+            # be a good place to catch any exceptions that b10-ddns can
+            # recover from. We currently have no exception hierarchy to
+            # make such a distinction easily, but once we do, this would
+            # be the place to catch.
             self._cc.check_command(False)
+        logger.info(DDNS_STOPPED)
 
 def create_signal_handler(ddns_server):
     '''
@@ -142,8 +140,7 @@ def create_signal_handler(ddns_server):
         here, the actual signal is not checked and the server is simply shut
         down.
         '''
-        if ddns_server is not None:
+        ddns_server.shutdown()
-            ddns_server.shutdown()
     return signal_handler
 
 def set_signal_handler(signal_handler):
@@ -160,7 +157,17 @@ def set_cmd_options(parser):
     parser.add_option("-v", "--verbose", dest="verbose", action="store_true",
             help="display more about what is going on")
 
-if '__main__' == __name__:
+def main(ddns_server=None):
+    '''
+    The main function.
+    Parameters:
+    ddns_server: If None (default), a DDNSServer object is initialized.
+                 If specified, the given DDNSServer will be used. This is
+                 mainly used for testing.
+    cc_session: If None (default), a new ModuleCCSession will be set up.
+                If specified, the given session will be used. This is
+                mainly used for testing.
+    '''
     try:
         parser = OptionParser()
         set_cmd_options(parser)
@@ -168,8 +175,8 @@ if '__main__' == __name__:
         if options.verbose:
             print("[b10-ddns] Warning: -v verbose option is ignored at this point.")
 
-        ddns_server = DDNSServer()
+        if ddns_server is None:
-        logger.debug(10, DDNS_STOPPED_BY_KEYBOARD)
+            ddns_server = DDNSServer()
         set_signal_handler(create_signal_handler(ddns_server))
         ddns_server.run()
     except KeyboardInterrupt:
@@ -182,3 +189,8 @@ if '__main__' == __name__:
         logger.error(DDNS_CONFIG_ERROR, str(e))
     except SessionTimeout as e:
         logger.error(DDNS_CC_SESSION_TIMEOUT_ERROR)
+    except Exception as e:
+        logger.error(DDNS_UNCAUGHT_EXCEPTION, type(e).__name__, str(e))
+
+if '__main__' == __name__:
+    main()

src/bin/ddns/ddns.spec

@@ -12,12 +12,12 @@
           "item_type": "map",
           "item_optional": true,
           "item_default": {
-            "update_acls": [{"action": "ACCEPT", "from": "127.0.0.1"},
+            "update_acl": [{"action": "ACCEPT", "from": "127.0.0.1"},
-                            {"action": "ACCEPT", "from": "::1"}]
+                           {"action": "ACCEPT", "from": "::1"}]
           },
           "map_item_spec": [
             {
-              "item_name": "update_acls",
+              "item_name": "update_acl",
               "item_type": "list",
               "item_optional": false,
               "list_item_spec": {

src/bin/ddns/ddns_messages.mes

@@ -15,9 +15,13 @@
 # No namespace declaration - these constants go in the global namespace
 # of the ddns messages python module.
 
+# When you add a message to this file, it is a good idea to run
+# <topsrcdir>/tools/reorder_message_file.py to make sure the
+# messages are in the correct order.
+
 % DDNS_CC_SESSION_ERROR error reading from cc channel: %1
 There was a problem reading from the command and control channel. The
-most likely cause is that the msgq daemon is not running.
+most likely cause is that the msgq process is not running.
 
 % DDNS_CC_SESSION_TIMEOUT_ERROR timeout waiting for cc response
 There was a problem reading a response from another module over the
@@ -36,13 +40,27 @@ failed to start at initialization.  A detailed error message from the module
 will also be displayed.
 
 % DDNS_RECEIVED_SHUTDOWN_COMMAND shutdown command received
-The ddns daemon received a shutdown command from the command channel
+The ddns process received a shutdown command from the command channel
 and will now shut down.
 
 % DDNS_RUNNING ddns server is running and listening for updates
-The ddns daemon has successfully started and is now ready to receive commands
+The ddns process has successfully started and is now ready to receive commands
 and updates.
 
+% DDNS_SHUTDOWN ddns server shutting down
+The ddns process is shutting down. It will no longer listen for new commands
+or updates. Any command or update that is being addressed at this moment will
+be completed, after which the process will exit.
+
+% DDNS_STOPPED ddns server has stopped
+The ddns process has successfully stopped and is no longer listening for or
+handling commands or updates, and will now exit.
+
 % DDNS_STOPPED_BY_KEYBOARD keyboard interrupt, shutting down
-There was a keyboard interrupt signal to stop the ddns daemon. The
+There was a keyboard interrupt signal to stop the ddns process. The
-daemon will now shut down.
+process will now shut down.
+
+% DDNS_UNCAUGHT_EXCEPTION uncaught exception of type %1: %2
+The b10-ddns process encountered an uncaught exception and will now shut
+down. This is indicative of a programming error and should not happen under
+normal circumstances. The exception type and message are printed.

src/bin/ddns/tests/ddns_test.in

@@ -1,27 +0,0 @@
-#! /bin/sh
-
-# Copyright (C) 2011  Internet Systems Consortium.
-#
-# Permission to use, copy, modify, and 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 INTERNET SYSTEMS CONSORTIUM
-# DISCLAIMS ALL WARRANTIES WITH REGARD TO THIS SOFTWARE INCLUDING ALL
-# IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL
-# INTERNET SYSTEMS CONSORTIUM 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.
-
-PYTHON_EXEC=${PYTHON_EXEC:-@PYTHON@}
-export PYTHON_EXEC
-
-TEST_PATH=@abs_top_srcdir@/src/bin/ddns/tests
-PYTHONPATH=@abs_top_srcdir@/src/bin/ddns:@abs_top_srcdir@/src/lib/python
-export PYTHONPATH
-
-cd ${TEST_PATH}
-exec ${PYTHON_EXEC} -O ddns_test.py $*
-

src/bin/ddns/tests/ddns_test.py

@@ -32,6 +32,32 @@ class MyCCSession(isc.config.ConfigData):
         '''Called by DDNSServer initialization, but not used in tests'''
         self._started = True
 
+class MyDDNSServer():
+    '''Fake DDNS server used to test the main() function'''
+    def __init__(self):
+        self.reset()
+
+    def run(self):
+        '''
+        Fake the run() method of the DDNS server. This will set
+        self._run_called to True.
+        If self._exception is not None, this is raised as an exception
+        '''
+        self.run_called = True
+        if self._exception is not None:
+            self.exception_raised = True
+            raise self._exception
+
+    def set_exception(self, exception):
+        '''Set an exception to be raised when run() is called'''
+        self._exception = exception
+
+    def reset(self):
+        '''(Re)set to initial values'''
+        self.run_called = False
+        self.exception_raised = False
+        self._exception = None
+
 class TestDDNSServer(unittest.TestCase):
     def setUp(self):
         cc_session = MyCCSession()
@@ -67,6 +93,65 @@ class TestDDNSServer(unittest.TestCase):
         signal_handler(None, None)
         self.assertTrue(self.ddns_server._shutdown)
 
+class TestMain(unittest.TestCase):
+    def setUp(self):
+        self._server = MyDDNSServer()
+
+    def test_main(self):
+        self.assertFalse(self._server.run_called)
+        ddns.main(self._server)
+        self.assertTrue(self._server.run_called)
+
+    def test_exceptions(self):
+        '''
+        Test whether exceptions are caught in main()
+        These exceptions should not bubble up.
+        '''
+        self._server.set_exception(KeyboardInterrupt())
+        self.assertFalse(self._server.exception_raised)
+        ddns.main(self._server)
+        self.assertTrue(self._server.exception_raised)
+
+        # Should technically not be necessary, but reset server to be sure
+        self._server.reset()
+        self.assertFalse(self._server.exception_raised)
+        self._server.set_exception(isc.cc.SessionError("error"))
+        ddns.main(self._server)
+        self.assertTrue(self._server.exception_raised)
+
+        self._server.reset()
+        self.assertFalse(self._server.exception_raised)
+        self._server.set_exception(isc.config.ModuleCCSessionError("error"))
+        ddns.main(self._server)
+        self.assertTrue(self._server.exception_raised)
+
+        self._server.reset()
+        self.assertFalse(self._server.exception_raised)
+        self._server.set_exception(ddns.DDNSConfigError("error"))
+        ddns.main(self._server)
+        self.assertTrue(self._server.exception_raised)
+
+        self._server.reset()
+        self.assertFalse(self._server.exception_raised)
+        self._server.set_exception(isc.cc.SessionTimeout("error"))
+        ddns.main(self._server)
+        self.assertTrue(self._server.exception_raised)
+
+        self._server.reset()
+        self.assertFalse(self._server.exception_raised)
+        self._server.set_exception(Exception("error"))
+        ddns.main(self._server)
+        self.assertTrue(self._server.exception_raised)
+
+        # Add one that is not a subclass of Exception, and hence not
+        # caught. Misuse BaseException for that.
+        self._server.reset()
+        self.assertFalse(self._server.exception_raised)
+        self._server.set_exception(BaseException("error"))
+        self.assertRaises(BaseException, ddns.main, self._server)
+        self.assertTrue(self._server.exception_raised)
+        
+
 if __name__== "__main__":
     isc.log.resetUnitTestRootLogger()
     unittest.main()