Code cleanup.

The code was mostly functional but hard to follow and not quite up to the best current practice.

What has been done:

Exception hierarchy cleanup. Now it is VyOSError > ConfigError > CommitError > ConfigLocked
VyOSError (base class) is raised when the user does something obviously wrong, like trying to
commit before entering conf mode. These are programming errors and not recoverable without
editing the code.
ConfigError other than CommitError is raised on set/delete command failures. These are usually
user input errors and cannot be recovered from without interacting with the user.
CommitErrors include commit failures due to user errors, and ConfigLocked condition.
ConfigLocked is raised when anoher commit is in progress. This is the only condition that can be
recovered from programatically (wait and try again).
This way people can write try/catch sequences that go from more specific down to VyOSError
and use different recovery strategies for different error classes.
These names are also more self-explanatory than MaintenanceError etc.

Eliminate the last instances of functions that return error messages as strings instead of raising
exceptions, and especially those that extract error messages from exceptions caught within and return them.
Exceptions are easy to recognize and handle programmatically, string messages are not. Writing a
try/catch statement is quick, knowing what the message is and using re.search every time is inefficient
for both the user and the computer.

Replace inefficient and functionally limited "foo in bar" substring checks with regex matching.

Add handling of timeout errors, which was entirely absent (prompt() returns false in those cases,
easy to check).

Replace the __status dict with individual instance variables. There are few of them, and they are
always checked individually, so a dict is unwarranted. I kept the status() method that now wraps
those attributes in a dict. I also made it protected (single _) as it's debug stuff and not something
end users would normally want to touch.

Give attributes descriptive names (logged_in, conf_mode, session_modified, session_saved).
All attributes are now boolean. It both makes it more efficient than comparing strings,
and easier to understand what values they can have. It would be very hard to deduce that "status"
can be "login" or "logout" (or None, but only before the user logins) and "save" can be "Yes" or None
without reading the whole thing, and even harder to remember.

Simplify the error handling. Eliminate the has_error being set to "Type1", "Type2" etc.. This is very
hard to follow especially when the mapping of those strings to actual error strings is partially in different file.
This is affectionately known as "spaghetti code". ;)
It's been replaced with simple conditionals that raise appropriate exceptions as they go.

Eliminate unnecessary attribute checking and setting. If the state machine model cuts off erroneous paths
early and sets the state properly, there is simply no way the conf_mode flag can be True while logged_in is False,
for instance. There is also no need to unset say session_modified in save() because there is no way it can be True
if save() got to that state without throwing an error. It's (now) easy to see, and, if needed, formally prove.

Always reuse the pxssh connection object created by __init__, never make a new one, as it's redundant.

Replace all occurences of type() with isinstance() so that it also works for all subclases of the type/class in question.

Eliminate inconsistent use of stuff like """ foo is "Yes" """, """ foo == None """ and so on. Boolean checks all should be
like """ if foo: something() """ or """ if not foo: """, None checks should be """ foo is None """,
and == should be reserved for values that are neither None nor boolean.

Pass user and password to the constructor as separate arguments for convenience, rather than have the user join them just
to split them back.

Eliminate the "basic_string" dict. Command prefixes like set and delete always stay the same anyway, and they are only used
in corresponding functions.

7 files changed, 144 insertions, 470 deletions

diff --git a/vymgmt/base_exceptions/__init__.py b/vymgmt/base_exceptions/__init__.py
deleted file mode 100644
index e69de29..0000000
--- a/vymgmt/base_exceptions/__init__.py
+++ /dev/null
diff --git a/vymgmt/base_exceptions/base.py b/vymgmt/base_exceptions/base.py
deleted file mode 100644
index 839224b..0000000
--- a/vymgmt/base_exceptions/base.py
+++ /dev/null
@@ -1,23 +0,0 @@
-# Copyright (c) 2016 Hochikong
-
-prefix = "\r\n"
-
-
-class Error(Exception):
-    pass
-
-
-class CommonError(Error):
-    def __init__(self, message):
-        self.error_message = message
-
-    def __str__(self):
-        return prefix + self.error_message
-
-
-class MaintenanceError(Error):
-    def __init__(self, message):
-        self.error_message = message
-
-    def __str__(self):
-        return prefix + self.error_message
diff --git a/vymgmt/base_exceptions/exceptions_for_commit.py b/vymgmt/base_exceptions/exceptions_for_commit.py
deleted file mode 100644
index 26c14c4..0000000
--- a/vymgmt/base_exceptions/exceptions_for_commit.py
+++ /dev/null
@@ -1,21 +0,0 @@
-# Copyright (c) 2016 Hochikong
-
-from .base import Error
-
-prefix = "\r\n"
-
-
-class CommitFailed(Error):
-    def __init__(self, message):
-        self.error_message = message
-
-    def __str__(self):
-        return prefix + self.error_message
-
-
-class CommitConflict(Error):
-    def __init__(self, message):
-        self.error_message = message
-
-    def __str__(self):
-        return prefix + self.error_message
diff --git a/vymgmt/base_exceptions/exceptions_for_set_and_delete.py b/vymgmt/base_exceptions/exceptions_for_set_and_delete.py
deleted file mode 100644
index 0f65c88..0000000
--- a/vymgmt/base_exceptions/exceptions_for_set_and_delete.py
+++ /dev/null
@@ -1,22 +0,0 @@
-# Copyright (c) 2016 Hochikong
-
-
-from .base import Error
-
-prefix = "\r\n"
-
-
-class ConfigPathError(Error):
-    def __init__(self, message):
-        self.error_message = message
-
-    def __str__(self):
-        return prefix + self.error_message
-
-
-class ConfigValueError(Error):
-    def __init__(self, message):
-        self.error_message = message
-
-    def __str__(self):
-        return prefix + self.error_message
diff --git a/vymgmt/error_distinguish.py b/vymgmt/error_distinguish.py
deleted file mode 100644
index 1f889f8..0000000
--- a/vymgmt/error_distinguish.py
+++ /dev/null
@@ -1,65 +0,0 @@
-# Copyright (c) 2016 Hochikong
-
-
-def distinguish_for_set(message):
-    """Distinguish the error type,PathError or ValueError
-
-    :param message: A error message string from VyOS
-    :return: The type of error
-    """
-    path_error_string = ['Configuration path:', 'is not valid', 'already exists']
-    value_error_string = ['Value validation failed']
-    all_strings = [path_error_string, value_error_string]
-    condition = 0
-    for i in all_strings:
-        for x in i:
-            if x in message:
-                condition += 1
-
-    if condition == 2:
-        return "ConfigPathError"
-    elif condition == 1:
-        return "ConfigValueError"
-    else:
-        return "NonsupportButError"
-
-
-def distinguish_for_delete(message):
-    """Distinguish the error type,PathError or ValueError
-
-    :param message: A error message string from VyOS
-    :return: The type of error
-    """
-    path_error_string = ['Configuration path:', 'is not valid', 'Delete failed']
-    value_error_string = ['Nothing to delete', 'the specified value does not exist',
-                          "the specified node does not exist"]
-    all_strings = [path_error_string, value_error_string]
-    condition = 0
-
-    for i in all_strings:
-        for x in i:
-            if x in message:
-                condition += 1
-
-    if condition == 3:
-        return "ConfigPathError"
-    elif condition == 2:
-        return "ConfigValueError"
-    else:
-        return "NonsupportButError"
-
-
-def distinguish_for_commit(message):
-    """Distinguish the error type
-
-    :param message: A error message string from VyOS
-    :return: The type of error
-    """
-    all_strings = ['Commit failed', 'due to another commit in progress']
-
-    for i in all_strings:
-        if i in message:
-            if i == all_strings[0]:
-                return "CommitFailed"
-            if i == all_strings[1]:
-                return "CommitConflict"
diff --git a/vymgmt/mgmt_common.py b/vymgmt/mgmt_common.py
deleted file mode 100644
index 1153c54..0000000
--- a/vymgmt/mgmt_common.py
+++ /dev/null
@@ -1,62 +0,0 @@
-# Copyright (c) 2016 Hochikong
-
-CODEC = 'utf8'
-
-
-def messenger(obj, config):
-    """This method used for sending configuration to VyOS
-
-    :param obj: A connection object
-    :param config: A configuration string
-    :return: A message or an error
-    """
-    try:
-        obj.sendline(config)
-        obj.prompt()
-        result = decodetool(obj.before, CODEC)
-        if len(result) > result.index('\r\n') + 2:
-            return result
-        else:
-            return "Result : Configured successfully"
-    except Exception as e:
-        return e
-
-
-def committer(obj, config):
-    """This method used for sending commit task to VyOS
-
-    :param obj: A connection object
-    :param config: A configuration string
-    :return: A message or an error
-    """
-    exception_string = "enhanced syslogd: rsyslogd"
-    try:
-        obj.sendline(config)
-        obj.prompt()
-        result = decodetool(obj.before, CODEC)
-        if len(result) > result.index('\r\n') + 2:
-            if exception_string in result:
-                return "Result : Commit successfully"
-            else:
-                return result
-        else:
-            return "Result : Commit successfully"
-    except Exception as e:
-        return e
-
-
-def decodetool(target, codec):
-    """This method is used for decoding obj.before to string when run this
-    library under python3
-
-    :param target: The obj.before
-    :param codec: The codec use to decode
-    :return:
-    """
-    try:
-        if type(target) == str:
-            return target
-        if type(target) == bytes:
-            return target.decode(codec)
-    except Exception as e:
-        return e
diff --git a/vymgmt/router.py b/vymgmt/router.py
index d2b35d5..99e9f8f 100644
--- a/vymgmt/router.py
+++ b/vymgmt/router.py
@@ -1,342 +1,209 @@
 # Copyright (c) 2016 Hochikong
 
+import re
 
 from pexpect import pxssh
-from .mgmt_common import messenger, committer
-from .base_exceptions.exceptions_for_set_and_delete import ConfigPathError, ConfigValueError
-from .base_exceptions.exceptions_for_commit import CommitFailed, CommitConflict
-from .base_exceptions.base import CommonError, MaintenanceError
-from .error_distinguish import distinguish_for_set, distinguish_for_delete, distinguish_for_commit
+
+
+class VyOSError(Exception):
+    pass
+
+
+class ConfigError(VyOSError):
+    pass
+
+
+class CommitError(ConfigError):
+    pass
+
+
+class ConfigLocked(CommitError):
+    pass
 
 
 class Router(object):
-    def __init__(self, address, cred):
+    def __init__(self, address, user, password):
         """Initial a router object
 
         :param address: Router address,example:'192.168.10.10'
         :param cred: Router user and password,example:'vyos:vyos'
         """
         self.__address = address
-        self.__cred = list(cred)
-        self.__divi = self.__cred.index(":")
-        self.__username = ''.join(self.__cred[:self.__divi])
-        self.__passwd = ''.join(self.__cred[self.__divi+1:])
+        self.__user = user
+        self.__password = password
+
+        # Session flags
+        self.__logged_in = False
+        self.__session_modified = False
+        self.__session_saved = True
+        self.__conf_mode = False
+
         self.__conn = pxssh.pxssh()
-        self.__status = {"status": None, "commit": None, "save": None, "configure": None}
-        self.__basic_string = {0: 'set ', 1: 'delete '}
 
-    def status(self):
+        # String codec, hardcoded for now
+        self.__codec = "utf8"
+
+    def __execute_command(self, command):
+        """This method used for sending configuration to VyOS
+
+        :param obj: A connection object
+        :param config: A configuration string
+        :return: A message or an error
+        """
+        self.__conn.sendline(command)
+
+        if not self.__conn.prompt():
+            raise VyOSError("Connection timed out")
+
+        output = self.__conn.before
+
+        # XXX: In python3 it's bytes rather than str
+        if isinstance(output, bytes):
+            output = output.decode(self.__codec)
+        return output
+
+    def _status(self):
         """Check the router object inner status
 
         :return: A python dictionary include the status of the router object
-        """
-        return self.__status
+       	"""
+        return { "logged_in": self.__logged_in,
+                 "session_modified": self.__session_modified,
+                 "session_saved": self.__session_saved,
+                 "conf_mode": self.__conf_mode }
 
     def login(self):
         """Login the router
 
-        :return: A message or an error
         """
-        has_error = None
-        try:
-            if self.__conn.login(self.__address, self.__username, self.__passwd) is True:
-                self.__status["status"] = "login"
-            else:
-                has_error = 'Type1'
-        except Exception as e:
-            return e
-
-        if has_error == 'Type1':
-            raise MaintenanceError("Error : Connect Failed.")
+        self.__conn.login(self.__address, self.__user, self.__password)
+        self.__logged_in = True
 
     def logout(self):
         """Logout the router
 
         :return: A message or an error
         """
-        has_error = None
-        try:
-            if self.__status["status"] == "login":
-                if self.__status["configure"] == "No":
-                    self.__conn.close()
-                    self.__status["status"] = "logout"
-                    self.__status["configure"] = None
-                    self.__conn = pxssh.pxssh()
-                elif self.__status["configure"] is None:
-                    self.__conn.close()
-                    self.__status["status"] = "logout"
-                    self.__conn = pxssh.pxssh()
-                else:
-                    if self.__status["save"] == "Yes":
-                        has_error = 'Type3'
-                    elif self.__status["save"] is None:
-                        has_error = 'Type3'
-                    else:
-                        if self.__status["commit"] == "Yes":
-                            has_error = 'Type3'
-                        elif self.__status["commit"] is None:
-                            has_error = 'Type3'
-                        else:
-                            has_error = 'Type1'
+
+        if not self.__logged_in:
+            raise VyOSError("Not logged in")
+        else:
+            if self.__conf_mode:
+                raise VyOSError("Cannot logout before exiting configuration mode")
             else:
-                has_error = 'Type4'
-        except Exception as e:
-            return e
-
-        if has_error == 'Type1':
-            raise MaintenanceError("Error : You should commit and exit configure mode first.")
-#        if has_error == 'Type2':
-#            raise MaintenanceError("Error : You should save and exit configure mode first.")
-        if has_error == 'Type3':
-            raise MaintenanceError("Error : You should exit configure mode first.")
-        if has_error == 'Type4':
-            raise MaintenanceError("Error : Router object not connect to a router.")
+                self.__conn.close()
+                self.__logged_in = False
 
     def configure(self):
         """Enter the VyOS configure mode
 
-        :return: A message or an error
         """
-        has_error = None
-        try:
-            if self.__status["status"] == "login":
-                if self.__status["configure"] is not "Yes":
-                    self.__conn.sendline("configure")
-                    self.__conn.prompt(0)
-                    self.__conn.set_unique_prompt()
-                    self.__status["configure"] = "Yes"
-                else:
-                    has_error = 'Type1'
+        if not self.__logged_in:
+            raise VyOSError("Cannot enter configuration mode when not logged in")
+        else:
+            if self.__conf_mode:
+                raise VyOSError("Session is already in configuration mode")
             else:
-                has_error = 'Type2'
-        except Exception as e:
-            return e
+                # configure changes the prompt (from $ to #), so this is
+                # a bit of a special case, and we use pxssh directly instead
+                # of the __execute_command wrapper...
+                self.__conn.sendline("configure")
+
+                # XXX: set_unique_prompt() after this breaks things, for some reason
+                # We should find out why.
+                self.__conn.PROMPT = "[#$]"
+
+                if not self.__conn.prompt():
+                    raise VyOSError("Entering configure mode failed (possibly due to timeout)")
+
+                #self.__conn.set_unique_prompt()
+                self.__conf_mode = True
 
-        if has_error == 'Type1':
-            raise MaintenanceError("Error : In configure mode now!")
-        if has_error == 'Type2':
-            raise MaintenanceError("Error : Router object not connect to a router.")
+                # XXX: There should be a check for operator vs. admin
+                # mode and appropriate exception, but pexpect doesn't work
+                # with operator's overly restricted shell...
 
     def commit(self):
         """Commit the configuration changes
 
-        :return: A message or an error
         """
-        has_error = None
-        result = None
-        res = None
-        try:
-            if self.__status["status"] == "login":
-                if self.__status["configure"] == "Yes":
-                    if self.__status["commit"] is None:
-                        has_error = 'Type1'
-                    if self.__status["commit"] == "No":
-                        res = committer(self.__conn, "commit")
-                        if "Result" in res:
-                            self.__status["commit"] = "Yes"
-                        else:
-                            result = distinguish_for_commit(res)
-                    else:
-                        has_error = 'Type2'
-                else:
-                    has_error = 'Type3'
+        if not self.__conf_mode:
+            raise VyOSError("Cannot commit without entering configuration mode")
+        else:
+            if not self.__session_modified:
+                raise ConfigError("No configuration changes to commit")
             else:
-                has_error = 'Type4'
-        except Exception as e:
-            return e
-
-        if has_error == 'Type1':
-            raise MaintenanceError("Error : You don't need to commit.")
-        if has_error == 'Type2':
-            raise MaintenanceError("Error : You have commit!")
-        if has_error == 'Type3':
-            raise MaintenanceError("Error : Router not in configure mode!")
-        if has_error == 'Type4':
-            raise MaintenanceError("Error : Router object not connect to a router.")
-
-        if result == "CommitFailed":
-            raise CommitFailed(res)
-        elif result == "CommitConflict":
-            raise CommitConflict(res)
+                output = self.__execute_command("commit")
+
+                if re.search(r"Commit\s+failed", output):
+                    raise CommitError(output)
+                if re.search(r"another\s+commit\s+in\s+progress", output):
+                    raise ConfigLocked("Configuration is locked due to another commit in progress")
+
+                self.__session_modified = False
+                self.__session_saved = False
 
     def save(self):
         """Save the configuration after commit
 
-        :return: A message or an error
         """
-        has_error = None
-        try:
-            if self.__status["status"] == "login":
-                if self.__status["configure"] == "Yes":
-                    if self.__status["commit"] == "Yes":
-                        if self.__status["save"] is None:
-                            has_error = 'Type1'
-                        if self.__status["save"] == "No":
-                            self.__conn.sendline("save")
-                            self.__conn.prompt(0)
-                            self.__status["save"] = "Yes"
-                        else:
-                            has_error = 'Type2'
-                    elif self.__status["commit"] is None:
-                        has_error = 'Type3'
-                    else:
-                        has_error = 'Type4'
-                else:
-                    has_error = 'Type5'
-            else:
-                has_error = 'Type6'
-        except Exception as e:
-            return e
-
-        if has_error == 'Type1':
-            raise MaintenanceError("Error : You don't need to save.")
-        if has_error == 'Type2':
-            raise MaintenanceError("Error : You have saved!")
-        if has_error == 'Type3':
-            raise MaintenanceError("Error : You don't need to save.")
-        if has_error == 'Type4':
-            raise MaintenanceError("Error : You need to commit first!")
-        if has_error == 'Type5':
-            raise MaintenanceError("Error : Router not in configure mode!")
-        if has_error == 'Type6':
-            raise MaintenanceError("Error : Router object not connect to a router.")
+        if not self.__conf_mode:
+            raise VyOSError("Cannot save when not in configuration mode")
+        elif self.__session_modified:
+            raise VyOSError("Cannot save when there are uncommited changes")
+        else:
+            self.__execute_command("save")
+            self.__session_saved = True
+
 
     def exit(self, force=False):
         """Exit VyOS configure mode
 
         :param force: True or False
-        :return: A message or an error
         """
-        has_error = None
-        try:
-            if self.__status["status"] == "login":
-                if self.__status["configure"] == "Yes":
-                    if force is True:
-                        self.__conn.sendline("exit discard")
-                        self.__conn.prompt()
-                        self.__status["configure"] = "No"
-                        self.__status["save"] = None
-                        self.__status["commit"] = None
-                    else:
-                        if self.__status["commit"] == "Yes":
-                            if self.__status["save"] == "Yes":
-                                self.__conn.sendline("exit")
-                                self.__conn.prompt()
-                                self.__status["configure"] = "No"
-                                self.__status["save"] = None
-                                self.__status["commit"] = None
-                            elif self.__status["save"] == "No":
-                                self.__conn.sendline("exit")
-                                self.__conn.prompt()
-                                self.__status["configure"] = "No"
-                                self.__status["save"] = None
-                                self.__status["commit"] = None
-                        elif self.__status["commit"] is None:
-                            self.__conn.sendline("exit")
-                            self.__conn.prompt()
-                            self.__status['configure'] = "No"
-                        else:
-                            has_error = 'Type2'
+        if not self.__conf_mode:
+            pass
+        else:
+            # XXX: would be nice to simplify these conditionals
+            if self.__session_modified:
+                if not force:
+                    raise VyOSError("Cannot exit a session with uncommited changes, use force flag to discard")
                 else:
-                    has_error = 'Type3'
+                    self.__execute_command("exit discard")
+                    self.__conf_mode = False
+                    return
+            elif (not self.__session_saved) and (not force):
+                raise VyOSError("Cannot exit a session with unsaved changes, use force flag to ignore")
             else:
-                has_error = 'Type4'
-        except Exception as e:
-            return e
-
-        if has_error == 'Type2':
-            raise MaintenanceError("Error : You should commit first.")
-        if has_error == 'Type3':
-            raise MaintenanceError("Error : You are not in configure mode,no need to exit.")
-        if has_error == 'Type4':
-            raise MaintenanceError("Error : Router object not connect to a router.")
-
-    def set(self, config):
+                self.__execute_command("exit")
+                self.__conf_mode = False
+
+    def set(self, path):
         """Basic 'set' method,execute the set command in VyOS
 
         :param config: A configuration string.
                        e.g. 'protocols static route ... next-hop ... distance ...'
-        :return: A message or an error
         """
-        has_error = None
-        result = None
-        res = None
-        full_config = self.__basic_string[0] + config
-        try:
-            if self.__status["status"] == "login":
-                if self.__status["configure"] == "Yes":
-                    res = messenger(self.__conn, full_config)
-                    if "Result" in res:
-                        if self.__status["commit"] == "No":
-                            pass
-                        else:
-                            self.__status["commit"] = "No"
-                        if self.__status["save"] == "No":
-                            pass
-                        else:
-                            self.__status["save"] = "No"
-                    else:
-                        result = distinguish_for_set(res)
-                else:
-                    has_error = 'Type1'
-            else:
-                has_error = 'Type2'
-        except Exception as e:
-            return e
-
-        if has_error == 'Type1':
-            raise MaintenanceError("Error : You are not in configure mode.")
-        if has_error == 'Type2':
-            raise MaintenanceError("Error : Router object not connect to a router.")
-
-        if result == "ConfigPathError":
-            raise ConfigPathError(res)
-        elif result == "ConfigValueError":
-            raise ConfigValueError(res)
-        elif result == "NonsupportButError":
-            raise CommonError(res)
-
-    def delete(self, config):
+        if not self.__conf_mode:
+            raise ConfigError("Cannot execute set commands when not in configuration mode")
+        else:
+            output = self.__execute_command("{0} {1}". format("set", path))
+            if re.search(r"Set\s+failed", output):
+                raise ConfigError(output)
+            elif re.search(r"already exists", output):
+                raise ConfigError("Configuration path already exists")
+            self.__session_modified = True
+
+    def delete(self, path):
         """Basic 'delete' method,execute the delete command in VyOS
 
-        :param config: A configuration string.
+        :param path: A configuration string.
                                e.g. 'protocols static route ... next-hop ... distance ...'
-        :return: A message or an error
         """
-        has_error = None
-        result = None
-        res = None
-        full_config = self.__basic_string[1] + config
-        try:
-            if self.__status["status"] == "login":
-                if self.__status["configure"] == "Yes":
-                    res = messenger(self.__conn, full_config)
-                    if "Result" in res:
-                        if self.__status["commit"] == "No":
-                            pass
-                        else:
-                            self.__status["commit"] = "No"
-                        if self.__status["save"] == "No":
-                            pass
-                        else:
-                            self.__status["save"] = "No"
-                    else:
-                        result = distinguish_for_delete(res)
-                else:
-                    has_error = 'Type1'
-            else:
-                has_error = 'Type2'
-        except Exception as e:
-            return e
-
-        if has_error == 'Type1':
-            raise MaintenanceError("Error : You are not in configure mode.")
-        if has_error == 'Type2':
-            raise MaintenanceError("Error : Router object not connect to a router.")
-
-        if result == "ConfigPathError":
-            raise ConfigPathError(res)
-        elif result == "ConfigValueError":
-            raise ConfigValueError(res)
-        elif result == "NonsupportButError":
-            raise CommonError(res)
+        if not self.__conf_mode:
+            raise ConfigError("Cannot execute delete commands when not in configuration mode")
+        else:
+            output = self.__execute_command("{0} {1}". format("delete", path))
+            if re.search(r"Nothing\s+to\s+delete", output):
+                raise ConfigError(output)
+            self.__session_modified = True