1. Change bindctl's code according the suggestion in trac220.

   Command parser only do minimal check. now parameter value can be a sequence of non-space characters, or a string surrounded by quotation marks(these marks can be a part of the value string in escaped form)
   Make error message be more friendly.(if there is some error in parameter's value, the parameter name will be provided)
2. Refactor function login_to_cmdctl() in class BindCmdInterpreter.
   Avoid using Exception to catch all exceptions.

git-svn-id: svn://bind10.isc.org/svn/bind10/branches/trac220@2108 e5f2f494-b856-4b98-b285-d166d9295462

src/bin/bindctl/bindcmd.py

@@ -49,6 +49,12 @@ try:
 except ImportError:
     my_readline = sys.stdin.readline
 
+CSV_FILE_DIR = None 
+if ('HOME' in os.environ):
+    CSV_FILE_DIR = os.environ['HOME']
+    CSV_FILE_DIR += os.sep + '.bind10' + os.sep
+CSV_FILE_NAME = 'default_user.csv'
+FAIL_TO_CONNEC_WITH_CMDCTL = "Fail to connect with b10-cmdctl module, is it running?"
 CONFIG_MODULE_NAME = 'config'
 CONST_BINDCTL_HELP = """
 usage: <module name> <command name> [param1 = value1 [, param2 = value2]]
@@ -96,14 +102,64 @@ class BindCmdInterpreter(Cmd):
         '''Parse commands inputted from user and send them to cmdctl. '''
         try:
             if not self.login_to_cmdctl():
-                return False
+                return 
 
             # Get all module information from cmd-ctrld
             self.config_data = isc.config.UIModuleCCSession(self)
             self._update_commands()
             self.cmdloop()
+        except FailToLogin as err:
+            print(err)
+            print(FAIL_TO_CONNEC_WITH_CMDCTL)
         except KeyboardInterrupt:
-            return True
+            print('\nExit from bindctl')
+
+    def _get_saved_user_info(self, dir, file_name):
+        ''' Read all the available username and password pairs saved in 
+        file(path is "dir + file_name"), Return value is one list of elements
+        ['name', 'password'], If get information failed, empty list will be 
+        returned.'''
+        csvfile = None
+        try:
+            if (not dir) or (not os.path.exists(dir)):
+                return []
+
+            csvfile = open(dir + file_name)
+            users_info = csv.reader(csvfile)
+            users = []
+            for row in users_info:
+                users.append([row[0], row[1]])
+            
+            return users
+        except (IOError, IndexError) as e:
+            pass
+        finally:
+            if csvfile:
+                csvfile.close()
+            return []
+
+    def _save_user_info(self, username, passwd, dir, file_name):
+        ''' Save username and password in file "dir + file_name"
+        If it's saved properly, return True, or else return False. '''
+        try:
+            if dir:
+                if not os.path.exists(dir):
+                    os.mkdir(dir, 0o700)
+            else:
+                print("Cannot determine location of $HOME. Not storing default user")
+                return False
+            csvfilepath = dir + file_name 
+            csvfile = open(csvfilepath, 'w')
+            os.chmod(csvfilepath, 0o600)
+            writer = csv.writer(csvfile)
+            writer.writerow([username, passwd])
+            csvfile.close()
+        except Exception as e:
+            # just not store it
+            print(e, "\nCannot write ~/.bind10/default_user.csv; default user is not stored")
+            return False
+
+        return True
 
     def login_to_cmdctl(self):
         '''Login to cmdctl with the username and password inputted 
@@ -112,33 +168,18 @@ class BindCmdInterpreter(Cmd):
         time, username and password saved in 'default_user.csv' will be
         used first.
         '''
-        csvfile = None
-        bsuccess = False
-        try:
-            cvsfilepath = ""
-            if ('HOME' in os.environ):
-                cvsfilepath = os.environ['HOME']
-                cvsfilepath += os.sep + '.bind10' + os.sep
-            cvsfilepath += 'default_user.csv'
-            csvfile = open(cvsfilepath)
-            users = csv.reader(csvfile)
-            for row in users:
-                param = {'username': row[0], 'password' : row[1]}
+        users = self._get_saved_user_info(CSV_FILE_DIR, CSV_FILE_NAME)
+        for row in users:
+            param = {'username': row[0], 'password' : row[1]}
+            try:
                 response = self.send_POST('/login', param)
                 data = response.read().decode()
-                if response.status == http.client.OK:
-                    print(data + ' login as ' + row[0] )
-                    bsuccess = True
-                    break
-        except IOError as e:
-            pass
-        except Exception as e:
-            print(e)
-        finally:
-            if csvfile:
-                csvfile.close()
-            if bsuccess:
-                return True
+            except socket.error:
+                raise FailToLogin()
+
+            if response.status == http.client.OK:
+                print(data + ' login as ' + row[0] )
+                return True 
 
         count = 0
         print("[TEMP MESSAGE]: username :root  password :bind10")
@@ -151,34 +192,17 @@ class BindCmdInterpreter(Cmd):
             username = input("Username:")
             passwd = getpass.getpass()
             param = {'username': username, 'password' : passwd}
-            response = self.send_POST('/login', param)
-            data = response.read().decode()
-            print(data)
-            
+            try:
+                response = self.send_POST('/login', param)
+                data = response.read().decode()
+                print(data)
+            except socket.error as e:
+                raise FailToLogin()
+
             if response.status == http.client.OK:
-                cvsfilepath = ""
-                try:
-                    if ('HOME' in os.environ):
-                        cvsfilepath = os.environ['HOME']
-                        cvsfilepath += os.sep + '.bind10' + os.sep
-                        if not os.path.exists(cvsfilepath):
-                                os.mkdir(cvsfilepath, 0o700)
-                    else:
-                        print("Cannot determine location of $HOME. Not storing default user")
-                        return True
-                    cvsfilepath += 'default_user.csv'
-                    csvfile = open(cvsfilepath, 'w')
-                    os.chmod(cvsfilepath, 0o600)
-                    writer = csv.writer(csvfile)
-                    writer.writerow([username, passwd])
-                    csvfile.close()
-                except Exception as e:
-                    # just not store it
-                    print("Cannot write ~/.bind10/default_user.csv; default user is not stored")
-                    print(e)
+                self._save_user_info(username, passwd, CSV_FILE_DIR, CSV_FILE_NAME)
                 return True
 
-
     def _update_commands(self):
         '''Update the commands of all modules. '''
         for module_name in self.config_data.get_config_item_list():
@@ -308,8 +332,11 @@ class BindCmdInterpreter(Cmd):
         if cmd.module != CONFIG_MODULE_NAME:
             for param_name in cmd.params:
                 param_spec = command_info.get_param_with_name(param_name).param_spec
-                cmd.params[param_name] = isc.config.config_data.convert_type(param_spec, cmd.params[param_name])
-
+                try:
+                    cmd.params[param_name] = isc.config.config_data.convert_type(param_spec, cmd.params[param_name])
+                except isc.cc.data.DataTypeError as e:
+                    raise isc.cc.data.DataTypeError('Invalid parameter value for \"%s\", the type should be \"%s\" \n' 
+                                                     % (param_name, param_spec['item_type']) + str(e))
     
     def _handle_cmd(self, cmd):
         '''Handle a command entered by the user'''
@@ -441,13 +468,14 @@ class BindCmdInterpreter(Cmd):
             cmd = BindCmdParse(line)
             self._validate_cmd(cmd)
             self._handle_cmd(cmd)
-        except BindCtlException as e:
-            print("Error! ", e)
-            self._print_correct_usage(e)
-        except isc.cc.data.DataTypeError as e:
-            print("Error! ", e)
-            self._print_correct_usage(e)
-            
+        except (IOError, http.client.HTTPException) as err:
+            print('Error!', err)
+            print(FAIL_TO_CONNEC_WITH_CMDCTL)
+        except BindCtlException as err:
+            print("Error! ", err)
+            self._print_correct_usage(err)
+        except isc.cc.data.DataTypeError as err:
+            print("Error! ", err)
             
     def _print_correct_usage(self, ept):        
         if isinstance(ept, CmdUnknownModuleSyntaxError):
@@ -556,7 +584,7 @@ class BindCmdInterpreter(Cmd):
         if (len(cmd.params) != 0):
             cmd_params = json.dumps(cmd.params)
 
-        print("send the message to cmd-ctrld")        
+        print("send the command to cmd-ctrld")        
         reply = self.send_POST(url, cmd.params)
         data = reply.read().decode()
         print("received reply:", data)

src/bin/bindctl/cmdparse.py

@@ -24,15 +24,19 @@ except ImportError:
     from bindctl.mycollections import OrderedDict
 
 param_name_str = "^\s*(?P<param_name>[\w]+)\s*=\s*"
-param_value_str = "(?P<param_value>[\w\.:/-]+)"
-param_value_with_quota_str = "[\"\'](?P<param_value>[\w\.:, /-]+)[\"\']"
+
+# The value string can be a sequence without space or comma 
+# characters, or a string surroundedby quotation marks(such marks
+# can be part of string in an escaped form)
+#param_value_str  = "(?P<param_value>[\"\'].+?(?<!\\\)[\"\']|[^\'\"][^, ]+)"
+param_value_str  = "(?P<param_value>[^\'\" ][^, ]+)"
+param_value_with_quota_str  = "[\"\'](?P<param_value>.+?)(?<!\\\)[\"\']"
 next_params_str = "(?P<blank>\s*)(?P<comma>,?)(?P<next_params>.*)$"
 
 PARAM_WITH_QUOTA_PATTERN = re.compile(param_name_str + 
-                                      param_value_with_quota_str +
+                                      param_value_with_quota_str + 
                                       next_params_str)
 PARAM_PATTERN = re.compile(param_name_str + param_value_str + next_params_str)
-                           
 # Used for module and command name
 NAME_PATTERN = re.compile("^\s*(?P<name>[\w]+)(?P<blank>\s*)(?P<others>.*)$")
 
@@ -98,7 +102,6 @@ class BindCmdParse:
                 
             groups = PARAM_PATTERN.match(param_text) or \
                      PARAM_WITH_QUOTA_PATTERN.match(param_text)
-            
             if not groups:
                 # ok, fill in the params in the order entered
                 params = re.findall("([^\" ]+|\".*\")", param_text)

src/bin/bindctl/exception.py

@@ -115,3 +115,10 @@ class CmdMissParamSyntaxError(CmdSyntaxError):
     def __str__(self):
         return str("Parameter '%s' is missed for command '%s' of module '%s'" % 
                    (self.param, self.command, self.module))
+
+
+class FailToLogin(BindCtlException):
+    def __str__(self):
+        return "Fail to login to cmdctl"
+
+

src/bin/bindctl/tests/bindctl_test.py

@@ -16,6 +16,7 @@
 
 import unittest
 import isc.cc.data
+import os
 from bindctl import cmdparse
 from bindctl import bindcmd
 from bindctl.moduleinfo import *
@@ -50,12 +51,31 @@ class TestCmdLex(unittest.TestCase):
             assert cmd.params["zone_name"] == "cnnic.cn"
             assert cmd.params["file"] == "cnnic.cn.file"
             assert cmd.params["master"] == '1.1.1.1'
+
+    def testCommandWithParamters_2(self):
+        '''Test whether the parameters in key=value can be parsed properly.'''
+        cmd = cmdparse.BindCmdParse('zone cmd name = 1:34::2')
+        self.assertEqual(cmd.params['name'], '1:34::2')
+
+        cmd = cmdparse.BindCmdParse('zone cmd name = 1\"\'34**&2 value=44\"\'\"')
+        self.assertEqual(cmd.params['name'], '1\"\'34**&2')
+        self.assertEqual(cmd.params['value'], '44\"\'\"')
+
+        cmd = cmdparse.BindCmdParse('zone cmd name = 1\"\'34**&2 ,value=  44\"\'\"')
+        self.assertEqual(cmd.params['name'], '1\"\'34**&2')
+        self.assertEqual(cmd.params['value'], '44\"\'\"')
             
+        cmd = cmdparse.BindCmdParse('zone cmd name =  1\'34**&2value=44\"\'\" value = \"==============\'')
+        self.assertEqual(cmd.params['name'], '1\'34**&2value=44\"\'\"')
+        self.assertEqual(cmd.params['value'], '==============')
+
+        cmd = cmdparse.BindCmdParse('zone cmd name =    \"1234, 567890 \" value ==&*/')
+        self.assertEqual(cmd.params['name'], '1234, 567890 ')
+        self.assertEqual(cmd.params['value'], '=&*/')
             
     def testCommandWithListParam(self):
-            cmd = cmdparse.BindCmdParse("zone set zone_name='cnnic.cn', master='1.1.1.1, 2.2.2.2'")
-            assert cmd.params["master"] == '1.1.1.1, 2.2.2.2'            
-        
+        cmd = cmdparse.BindCmdParse("zone set zone_name='cnnic.cn', master='1.1.1.1, 2.2.2.2'")
+        assert cmd.params["master"] == '1.1.1.1, 2.2.2.2'            
         
     def testCommandWithHelpParam(self):
         cmd = cmdparse.BindCmdParse("zone add help")
@@ -217,8 +237,32 @@ class TestNameSequence(unittest.TestCase):
             assert self.random_names[i] == cmd_names[i+1]
             assert self.random_names[i] == module_names[i+1]
             i = i + 1
-        
     
+class FakeBindCmdInterpreter(bindcmd.BindCmdInterpreter):
+    def __init__(self):
+        pass
+
+class TestBindCmdInterpreter(unittest.TestCase):
+
+    def _create_invalid_csv_file(self, csvfilename):
+        import csv
+        csvfile = open(csvfilename, 'w')
+        writer = csv.writer(csvfile)
+        writer.writerow(['name1'])
+        writer.writerow(['name2'])
+        csvfile.close()
+
+    def test_get_saved_user_info(self):
+        cmd = FakeBindCmdInterpreter()
+        users = cmd._get_saved_user_info('/notexist', 'cvs_file.cvs')
+        self.assertEqual([], users)
+        
+        csvfilename = 'csv_file.csv'
+        self._create_invalid_csv_file(csvfilename)
+        users = cmd._get_saved_user_info('./', csvfilename)
+        self.assertEqual([], users)
+        os.remove(csvfilename)
+
 if __name__== "__main__":
     unittest.main()