[trac615] Code cleanups

 - Less hardcoding of values
 - Tests of missing argument values
 - "args" instead of "opts" for argument values
 - Some documentation updates

src/bin/bind10/bind10.py.in

@@ -204,7 +204,11 @@ class BoB:
             msgq process listens on.  If verbose is True, then the boss reports
             what it is doing.
 
-            Data path and config filename are passed trough to config manager.
+            Data path and config filename are passed trough to config manager
+            (if provided) and specify the config file to be used.
+
+            The cmdctl_port is passed to cmdctl and specify on which port it
+            should listen.
         """
         self.cc_session = None
         self.ccs = None
@@ -396,12 +400,12 @@ class BoB:
             Starts the configuration manager process
         """
         self.log_starting("b10-cfgmgr")
-        opts = ["b10-cfgmgr"]
+        args = ["b10-cfgmgr"]
         if self.data_path is not None:
-            opts.append("--data-path=" + self.data_path)
+            args.append("--data-path=" + self.data_path)
         if self.config_filename is not None:
-            opts.append("--config-filename=" + self.config_filename)
+            args.append("--config-filename=" + self.config_filename)
-        bind_cfgd = ProcessInfo("b10-cfgmgr", opts,
+        bind_cfgd = ProcessInfo("b10-cfgmgr", args,
                                 c_channel_env, uid=self.uid,
                                 username=self.username)
         self.processes[bind_cfgd.pid] = bind_cfgd
@@ -514,10 +518,10 @@ class BoB:
         """
             Starts the command control process
         """
-        opts = ["b10-cmdctl"]
+        args = ["b10-cmdctl"]
         if self.cmdctl_port is not None:
-            opts.append("--port=" + str(self.cmdctl_port))
+            args.append("--port=" + str(self.cmdctl_port))
-        self.start_process("b10-cmdctl", opts, c_channel_env, self.cmdctl_port)
+        self.start_process("b10-cmdctl", args, c_channel_env, self.cmdctl_port)
 
     def start_all_processes(self):
         """

src/bin/bind10/tests/bind10_test.py

@@ -431,6 +431,9 @@ class TestParseArgs(unittest.TestCase):
         """
         Test it can parse the data path.
         """
+        self.assertRaises(OptsError, parse_args, ['-p'], TestOptParser)
+        self.assertRaises(OptsError, parse_args, ['--data-path'],
+                          TestOptParser)
         options = parse_args(['-p', '/data/path'], TestOptParser)
         self.assertEqual('/data/path', options.data_path)
         options = parse_args(['--data-path=/data/path'], TestOptParser)
@@ -440,6 +443,9 @@ class TestParseArgs(unittest.TestCase):
         """
         Test it can parse the config switch.
         """
+        self.assertRaises(OptsError, parse_args, ['-c'], TestOptParser)
+        self.assertRaises(OptsError, parse_args, ['--config-file'],
+                          TestOptParser)
         options = parse_args(['-c', 'config-file'], TestOptParser)
         self.assertEqual('config-file', options.config_file)
         options = parse_args(['--config-file=config-file'], TestOptParser)
@@ -453,6 +459,8 @@ class TestParseArgs(unittest.TestCase):
                                                 TestOptParser)
         self.assertRaises(OptsError, parse_args, ['--cmdctl-port=100000000'],
                                                 TestOptParser)
+        self.assertRaises(OptsError, parse_args, ['--cmdctl-port'],
+                          TestOptParser)
         options = parse_args(['--cmdctl-port=1234'], TestOptParser)
         self.assertEqual(1234, options.cmdctl_port)
 

src/bin/cfgmgr/b10-cfgmgr.py.in

@@ -34,6 +34,7 @@ if "B10_FROM_SOURCE" in os.environ:
 else:
     PREFIX = "@prefix@"
     DATA_PATH = "@localstatedir@/@PACKAGE@".replace("${prefix}", PREFIX)
+DEFAULT_CONFIG_FILE = "b10-config.db"
 
 cm = None
 
@@ -41,10 +42,11 @@ def parse_options(args=sys.argv[1:], Parser=OptionParser):
     parser = Parser()
     parser.add_option("-p", "--data-path", dest="data_path",
                       help="Directory to search for configuration files " +
-                      "(default="+DATA_PATH+")", default=DATA_PATH)
+                      "(default=" + DATA_PATH + ")", default=DATA_PATH)
     parser.add_option("-c", "--config-filename", dest="file_name",
                       help="Configuration database filename " +
-                      "(default=b10-config.db)", default="b10-config.db")
+                      "(default=" + DEFAULT_CONFIG_FILE + ")",
+                      default=DEFAULT_CONFIG_FILE)
     (options, args) = parser.parse_args(args)
     if args:
         parser.error("No non-option arguments allowed")

src/bin/cfgmgr/tests/b10-cfgmgr_test.py.in

@@ -103,14 +103,14 @@ class TestParseArgs(unittest.TestCase):
         b = __import__("b10-cfgmgr")
         parsed = b.parse_options([], TestOptParser)
         self.assertEqual(b.DATA_PATH, parsed.data_path)
-        self.assertEqual("b10-config.db", parsed.file_name)
+        self.assertEqual(b.DEFAULT_CONFIG_FILE, parsed.file_name)
 
     def test_wrong_args(self):
         """
         Test it fails when we pass invalid option.
         """
         b = __import__("b10-cfgmgr")
-        self.assertRaises(OptsError, b.parse_options, (['--wrong-option']),
+        self.assertRaises(OptsError, b.parse_options, ['--wrong-option'],
                           TestOptParser)
 
     def test_not_arg(self):
@@ -119,7 +119,7 @@ class TestParseArgs(unittest.TestCase):
         (eg. without -- at the beginning).
         """
         b = __import__("b10-cfgmgr")
-        self.assertRaises(OptsError, b.parse_options, (['not-option']),
+        self.assertRaises(OptsError, b.parse_options, ['not-option'],
                           TestOptParser)
 
     def test_datapath(self):
@@ -129,10 +129,13 @@ class TestParseArgs(unittest.TestCase):
         b = __import__("b10-cfgmgr")
         parsed = b.parse_options(['--data-path=/path'], TestOptParser)
         self.assertEqual('/path', parsed.data_path)
-        self.assertEqual("b10-config.db", parsed.file_name)
+        self.assertEqual(b.DEFAULT_CONFIG_FILE, parsed.file_name)
         parsed = b.parse_options(['-p', '/path'], TestOptParser)
         self.assertEqual('/path', parsed.data_path)
-        self.assertEqual("b10-config.db", parsed.file_name)
+        self.assertEqual(b.DEFAULT_CONFIG_FILE, parsed.file_name)
+        self.assertRaises(OptsError, b.parse_options, ['-p'], TestOptParser)
+        self.assertRaises(OptsError, b.parse_options, ['--data-path'],
+                          TestOptParser)
 
     def test_db_filename(self):
         """
@@ -146,6 +149,9 @@ class TestParseArgs(unittest.TestCase):
         parsed = b.parse_options(['-c', 'filename'], TestOptParser)
         self.assertEqual(b.DATA_PATH, parsed.data_path)
         self.assertEqual("filename", parsed.file_name)
+        self.assertRaises(OptsError, b.parse_options, ['-c'], TestOptParser)
+        self.assertRaises(OptsError, b.parse_options, ['--config-filename'],
+                          TestOptParser)
 
 if __name__ == '__main__':
     unittest.main()

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

@@ -44,7 +44,7 @@ class ConfigManagerData:
     """This class hold the actual configuration information, and
        reads it from and writes it to persistent storage"""
 
-    def __init__(self, data_path, file_name = "b10-config.db"):
+    def __init__(self, data_path, file_name="b10-config.db"):
         """Initialize the data for the configuration manager, and
            set the version and path for the data store. Initializing
            this does not yet read the database, a call to
@@ -62,7 +62,7 @@ class ConfigManagerData:
             self.db_filename = data_path + os.sep + file_name
             self.data_path = data_path
 
-    def read_from_file(data_path, file_name = "b10-config.db"):
+    def read_from_file(data_path, file_name):
         """Read the current configuration found in the file file_name.
            If file_name is absolute, data_path is ignored. Otherwise
            we look for the file_name in data_path directory.
@@ -153,7 +153,8 @@ class ConfigManagerData:
 
 class ConfigManager:
     """Creates a configuration manager. The data_path is the path
-       to the directory containing the b10-config.db file.
+       to the directory containing the configuraton file,
+       database_filename points to the configuration file.
        If session is set, this will be used as the communication
        channel session. If not, a new session will be created.
        The ability to specify a custom session is for testing purposes
@@ -162,7 +163,8 @@ class ConfigManager:
                  session=None):
         """Initialize the configuration manager. The data_path string
            is the path to the directory where the configuration is
-           stored (in <data_path>/b10-config.db). The dabase_filename
+           stored (in <data_path>/<database_filename> or in
+           <database_filename>, if it is absolute). The dabase_filename
            is the config file to load. Session is an optional
            cc-channel session. If this is not given, a new one is
            created."""
@@ -237,8 +239,7 @@ class ConfigManager:
         return commands
 
     def read_config(self):
-        """Read the current configuration from the b10-config.db file
+        """Read the current configuration from the file specificied at init()"""
-           at the path specificied at init()"""
         try:
             self.config = ConfigManagerData.read_from_file(self.data_path,
                                                            self.\
@@ -249,8 +250,7 @@ class ConfigManager:
                                             self.database_filename)
         
     def write_config(self):
-        """Write the current configuration to the b10-config.db file
+        """Write the current configuration to the file specificied at init()"""
-           at the path specificied at init()"""
         self.config.write_to_file()
 
     def _handle_get_module_spec(self, cmd):

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

@@ -49,10 +49,10 @@ class TestConfigManagerData(unittest.TestCase):
                          self.writable_data_path + os.sep + "b10-config.db")
 
     def test_read_from_file(self):
-        ConfigManagerData.read_from_file(self.writable_data_path)
+        ConfigManagerData.read_from_file(self.writable_data_path, "b10-config.db")
         self.assertRaises(ConfigManagerDataEmpty,
                           ConfigManagerData.read_from_file,
-                          "doesnotexist")
+                          "doesnotexist", "b10-config.db")
         self.assertRaises(ConfigManagerDataReadError,
                           ConfigManagerData.read_from_file,
                           self.data_path, "b10-config-bad1.db")
@@ -105,11 +105,13 @@ class TestConfigManager(unittest.TestCase):
         Test data_path and database filename is passed trough to
         underlying ConfigManagerData.
         """
-        cm = ConfigManager("/data/path", "filename", self.fake_session)
+        cm = ConfigManager("datapath", "filename", self.fake_session)
-        self.assertEqual("/data/path/filename", cm.config.db_filename)
+        self.assertEqual("datapath" + os.sep + "filename",
+                         cm.config.db_filename)
         # It should preserve it while reading
         cm.read_config()
-        self.assertEqual("/data/path/filename", cm.config.db_filename)
+        self.assertEqual("datapath" + os.sep + "filename",
+                         cm.config.db_filename)
 
     def test_init(self):
         self.assert_(self.cm.module_specs == {})