more generic error handler in EipConductorAppMixin

documentation of the Exception Hierarchy and attributes.

also a bit of general cleanup around error handling in conductor.
Hopefully to be polished an abstracted to leap.base with time.

not all errors are converted (and the old with_errors/ignoring
errors) are still there, but we should be using this style of handlers
from now on.

wrapping up with this pseudo-feature for now. as we work on individual
features we can mimick the exceptions that are working.

9 files changed, 190 insertions, 112 deletions

diff --git a/src/leap/baseapp/constants.py b/src/leap/baseapp/constants.py
index 763df23b..e312be21 100644
--- a/src/leap/baseapp/constants.py
+++ b/src/leap/baseapp/constants.py
@@ -1 +1,6 @@
+# This timer used for polling vpn manager state.
+
+# XXX what is an optimum polling interval?
+# too little will be overkill, too much will
+# miss transition states.
 TIMER_MILLISECONDS = 250.0
diff --git a/src/leap/baseapp/dialogs.py b/src/leap/baseapp/dialogs.py
index d37a234c..d4acb09d 100644
--- a/src/leap/baseapp/dialogs.py
+++ b/src/leap/baseapp/dialogs.py
@@ -1,14 +1,25 @@
+import logging
+
 from PyQt4.QtGui import (QDialog, QFrame, QPushButton, QLabel, QMessageBox)
 
+logger = logging.getLogger(name=__name__)
+
 
 class ErrorDialog(QDialog):
-    def __init__(self, parent=None):
+    def __init__(self, parent=None, errtype=None, msg=None, label=None):
         super(ErrorDialog, self).__init__(parent)
         frameStyle = QFrame.Sunken | QFrame.Panel
         self.warningLabel = QLabel()
         self.warningLabel.setFrameStyle(frameStyle)
         self.warningButton = QPushButton("QMessageBox.&warning()")
 
+        if msg is not None:
+            self.msg = msg
+        if label is not None:
+            self.label = label
+        if errtype == "critical":
+            self.criticalMessage(self.msg, self.label)
+
     def warningMessage(self, msg, label):
         msgBox = QMessageBox(QMessageBox.Warning,
                              "QMessageBox.warning()", msg,
@@ -26,5 +37,11 @@ class ErrorDialog(QDialog):
                              QMessageBox.NoButton, self)
         msgBox.addButton("&Ok", QMessageBox.AcceptRole)
         msgBox.exec_()
+
+        # It's critical, so we exit.
+        # We should better emit a signal and connect it
+        # with the proper shutdownAndQuit method, but
+        # this suffices for now.
+        logger.info('Quitting')
         import sys
         sys.exit()
diff --git a/src/leap/baseapp/eip.py b/src/leap/baseapp/eip.py
index dd88b7f5..afdb7adc 100644
--- a/src/leap/baseapp/eip.py
+++ b/src/leap/baseapp/eip.py
@@ -11,8 +11,7 @@ from leap.eip.eipconnection import EIPConnection
 logger = logging.getLogger(name=__name__)
 
 
-class EIPConductorApp(object):
-    # XXX EIPConductorMixin ?
+class EIPConductorAppMixin(object):
     """
     initializes an instance of EIPConnection,
     gathers errors, and passes status-change signals
@@ -52,86 +51,90 @@ class EIPConductorApp(object):
                 lambda: self.start_or_stopVPN())
 
     def error_check(self):
+        """
+        consumes the conductor error queue.
+        pops errors, and acts accordingly (launching user dialogs).
+        """
         logger.debug('error check')
 
         #####################################
         # XXX refactor in progress (by #504)
+
         errq = self.conductor.error_queue
         while errq.qsize() != 0:
             logger.debug('%s errors left in conductor queue', errq.qsize())
             error = errq.get()
+
+            # redundant log, debugging the loop.
             logger.error('%s: %s', error.__class__.__name__, error.message)
 
             if issubclass(error.__class__, eip_exceptions.EIPClientError):
-                if error.critical:
-                    logger.critical(error.message)
-                    logger.error('quitting')
-
-                    # XXX
-                    # check headless = False before
-                    # launching dialog.
-                    # (for Qt tests)
-
-                    dialog = ErrorDialog()
-                    if getattr(error, 'usermessage', None):
-                        message = error.usermessage
-                    else:
-                        message = error.message
-                    dialog.criticalMessage(message, 'error')
-                else:
-                    logger.exception(error.message)
+                self.handle_eip_error(error)
+
             else:
+                # This is not quite working. FIXME
                 import traceback
                 traceback.print_exc()
                 raise error
 
-        if self.conductor.missing_definition is True:
-            dialog = ErrorDialog()
-            dialog.criticalMessage(
-                'The default '
-                'definition.json file cannot be found',
-                'error')
+            if error.failfirst is True:
+                break
 
-        if self.conductor.missing_provider is True:
-            dialog = ErrorDialog()
-            dialog.criticalMessage(
-                'Missing provider. Add a remote_ip entry '
-                'under section [provider] in eip.cfg',
-                'error')
+        #############################################
+        # old errors to check
+        # write test for them and them remove
+        # their corpses from here.
 
-        if self.conductor.missing_vpn_keyfile is True:
-            dialog = ErrorDialog()
-            dialog.criticalMessage(
-                'Could not find the vpn keys file',
-                'error')
+        #if self.conductor.missing_vpn_keyfile is True:
+            #dialog = ErrorDialog()
+            #dialog.criticalMessage(
+                #'Could not find the vpn keys file',
+                #'error')
 
-        # ... btw, review pending.
-        # os.kill of subprocess fails if we have
-        # some of this errors.
+        #if self.conductor.bad_keyfile_perms is True:
+            #dialog = ErrorDialog()
+            #dialog.criticalMessage(
+                #'The vpn keys file has bad permissions',
+                #'error')
 
-        # deprecated.
-        # get something alike.
-        #if self.conductor.bad_provider is True:
+        # deprecated. configchecker takes care of that.
+        #if self.conductor.missing_definition is True:
             #dialog = ErrorDialog()
             #dialog.criticalMessage(
-                #'Bad provider entry. Check that remote_ip entry '
-                #'has an IP under section [provider] in eip.cfg',
+                #'The default '
+                #'definition.json file cannot be found',
                 #'error')
 
-        if self.conductor.bad_keyfile_perms is True:
-            dialog = ErrorDialog()
-            dialog.criticalMessage(
-                'The vpn keys file has bad permissions',
-                'error')
+    def handle_eip_error(self, error):
+        """
+        check severity and launches
+        dialogs informing user about the errors.
+        in the future we plan to derive errors to
+        our log viewer.
+        """
 
-        if self.conductor.missing_pkexec is True:
+        if getattr(error, 'usermessage', None):
+            message = error.usermessage
+        else:
+            message = error.message
+
+        # XXX
+        # check headless = False before
+        # launching dialog.
+        # (so Qt tests can assert stuff)
+
+        if error.critical:
+            logger.critical(error.message)
+            #critical error (non recoverable),
+            #we give user some info and quit.
+            #(critical error dialog will exit app)
+            ErrorDialog(errtype="critical",
+                        msg=message,
+                        label="critical error")
+
+        else:
             dialog = ErrorDialog()
-            dialog.warningMessage(
-                'We could not find <b>pkexec</b> in your '
-                'system.<br/> Do you want to try '
-                '<b>setuid workaround</b>? '
-                '(<i>DOES NOTHING YET</i>)',
-                'error')
+            dialog.warningMessage(message, 'error')
 
     @QtCore.pyqtSlot()
     def statusUpdate(self):
@@ -188,29 +191,30 @@ class EIPConductorApp(object):
         """
         stub for running child process with vpn
         """
+        if self.conductor.has_errors():
+            logger.debug('not starting vpn; conductor has errors')
+
         if self.eip_service_started is False:
             try:
                 self.conductor.connect()
-                # XXX move this to error queue
-            except eip_exceptions.EIPNoCommandError:
-                dialog = ErrorDialog()
-                dialog.warningMessage(
-                    'No suitable openvpn command found. '
-                    '<br/>(Might be a permissions problem)',
-                    'error')
-            if self.debugmode:
-                self.startStopButton.setText('&Disconnect')
-            self.eip_service_started = True
 
-            # XXX what is optimum polling interval?
-            # too little is overkill, too much
-            # will miss transition states..
+            except eip_exceptions.EIPNoCommandError as exc:
+                self.handle_eip_error(exc)
+
+            except Exception as err:
+                # raise generic exception (Bad Thing Happened?)
+                logger.exception(err)
+            else:
+                # no errors, so go on.
+                if self.debugmode:
+                    self.startStopButton.setText('&Disconnect')
+                self.eip_service_started = True
 
-            # XXX decouple! (timer is init by icons class).
-            # should bring it here?
-            # to its own class?
+                # XXX decouple! (timer is init by icons class).
+                # we could bring Timer Init to this Mixin
+                # or to its own Mixin.
+                self.timer.start(constants.TIMER_MILLISECONDS)
 
-            self.timer.start(constants.TIMER_MILLISECONDS)
             return
 
         if self.eip_service_started is True:
diff --git a/src/leap/baseapp/leap_app.py b/src/leap/baseapp/leap_app.py
index 85644360..f91b2329 100644
--- a/src/leap/baseapp/leap_app.py
+++ b/src/leap/baseapp/leap_app.py
@@ -7,7 +7,7 @@ from leap.gui import mainwindow_rc
 logger = logging.getLogger(name=__name__)
 
 
-class MainWindow(object):
+class MainWindowMixin(object):
     """
     create the main window
     for leap app
diff --git a/src/leap/baseapp/log.py b/src/leap/baseapp/log.py
index 3580e987..8a7f81c3 100644
--- a/src/leap/baseapp/log.py
+++ b/src/leap/baseapp/log.py
@@ -6,7 +6,7 @@ from PyQt4 import QtCore
 vpnlogger = logging.getLogger('leap.openvpn')
 
 
-class LogPane(object):
+class LogPaneMixin(object):
     """
     a simple log pane
     that writes new lines as they come
@@ -22,7 +22,6 @@ class LogPane(object):
         self.logbrowser = QtGui.QTextBrowser()
 
         startStopButton = QtGui.QPushButton("&Connect")
-        #startStopButton.clicked.connect(self.start_or_stopVPN)
         self.startStopButton = startStopButton
 
         logging_layout.addWidget(self.logbrowser)
diff --git a/src/leap/baseapp/mainwindow.py b/src/leap/baseapp/mainwindow.py
index e87f5844..10b23d9a 100644
--- a/src/leap/baseapp/mainwindow.py
+++ b/src/leap/baseapp/mainwindow.py
@@ -5,18 +5,18 @@ import logging
 from PyQt4 import QtCore
 from PyQt4 import QtGui
 
-from leap.baseapp.eip import EIPConductorApp
-from leap.baseapp.log import LogPane
-from leap.baseapp.systray import StatusAwareTrayIcon
-from leap.baseapp.leap_app import MainWindow
+from leap.baseapp.eip import EIPConductorAppMixin
+from leap.baseapp.log import LogPaneMixin
+from leap.baseapp.systray import StatusAwareTrayIconMixin
+from leap.baseapp.leap_app import MainWindowMixin
 
 logger = logging.getLogger(name=__name__)
 
 
 class LeapWindow(QtGui.QMainWindow,
-                 MainWindow, EIPConductorApp,
-                 StatusAwareTrayIcon,
-                 LogPane):
+                 MainWindowMixin, EIPConductorAppMixin,
+                 StatusAwareTrayIconMixin,
+                 LogPaneMixin):
     """
     main window for the leap app.
     Initializes all of its base classes
@@ -34,9 +34,9 @@ class LeapWindow(QtGui.QMainWindow,
         super(LeapWindow, self).__init__()
         if self.debugmode:
             self.createLogBrowser()
-        EIPConductorApp.__init__(self, opts=opts)
-        StatusAwareTrayIcon.__init__(self)
-        MainWindow.__init__(self)
+        EIPConductorAppMixin.__init__(self, opts=opts)
+        StatusAwareTrayIconMixin.__init__(self)
+        MainWindowMixin.__init__(self)
 
         # bind signals
         # XXX move to parent classes init??
diff --git a/src/leap/baseapp/systray.py b/src/leap/baseapp/systray.py
index f3832473..c696ee74 100644
--- a/src/leap/baseapp/systray.py
+++ b/src/leap/baseapp/systray.py
@@ -4,7 +4,7 @@ from PyQt4 import QtGui
 from leap.gui import mainwindow_rc
 
 
-class StatusAwareTrayIcon(object):
+class StatusAwareTrayIconMixin(object):
     """
     a mix of several functions needed
     to create a systray and make it
diff --git a/src/leap/eip/eipconnection.py b/src/leap/eip/eipconnection.py
index e090f9a7..5c54a986 100644
--- a/src/leap/eip/eipconnection.py
+++ b/src/leap/eip/eipconnection.py
@@ -24,7 +24,6 @@ class EIPConnection(OpenVPNConnection):
         self.settingsfile = kwargs.get('settingsfile', None)
         self.logfile = kwargs.get('logfile', None)
 
-        # XXX USE THIS
         self.error_queue = Queue.Queue()
 
         status_signals = kwargs.pop('status_signals', None)
@@ -33,6 +32,9 @@ class EIPConnection(OpenVPNConnection):
 
         super(EIPConnection, self).__init__(*args, **kwargs)
 
+    def has_errors(self):
+        return True if self.error_queue.qsize != 0 else True
+
     def run_checks(self, skip_download=False):
         """
         run all eip checks previous to attempting a connection
diff --git a/src/leap/eip/exceptions.py b/src/leap/eip/exceptions.py
index a30cd2a6..3c8f6afb 100644
--- a/src/leap/eip/exceptions.py
+++ b/src/leap/eip/exceptions.py
@@ -1,23 +1,60 @@
+"""
+Generic error hierarchy
+Leap/EIP exceptions used for exception handling,
+logging, and notifying user of errors
+during leap operation.
+
+Exception hierarchy
+-------------------
+All EIP Errors must inherit from EIPClientError (note: move that to
+a more generic LEAPClientBaseError).
+
+Exception attributes and their meaning/uses
+-------------------------------------------
+
+* critical:    if True, will abort execution prematurely,
+               after attempting any cleaning
+               action.
+
+* failfirst:   breaks any error_check loop that is examining
+               the error queue.
+
+* message:     the message that will be used in the __repr__ of the exception.
+
+* usermessage: the message that will be passed to user in ErrorDialogs
+               in Qt-land.
+
+TODO:
+
+* EIPClientError:
+  Should inherit from LeapException
+  and move basic attrs there
+
+* gettext / i18n for user messages.
+
+"""
+
+
 class EIPClientError(Exception):
     """
     base EIPClient exception
     """
-    # Should inherit from LeapException
-    # and move basic attrs there
     critical = False
 
-    #def __str__(self):
-        #if len(self.args) >= 1:
-            #return repr(self.args[0])
-        #else:
-            #return ConnectionError
-
 
 class CriticalError(EIPClientError):
     """
     we cannot do anything about it, sorry
     """
     critical = True
+    failfirst = True
+
+
+class Warning(EIPClientError):
+    """
+    just that, warnings
+    """
+    pass
 
 
 class EIPNoPolkitAuthAgentAvailable(CriticalError):
@@ -28,33 +65,53 @@ class EIPNoPolkitAuthAgentAvailable(CriticalError):
                    "<b>polkit-gnome-authentication-agent-1</b> "
                    "running and try again.")
 
-# Errors needing some work
 
+class EIPNoPkexecAvailable(Warning):
+    message = "No pkexec binary found"
+    usermessage = ("We could not find <b>pkexec</b> in your "
+                   "system.<br/> Do you want to try "
+                   "<b>setuid workaround</b>? "
+                   "(<i>DOES NOTHING YET</i>)")
+    failfirst = True
 
-class EIPNoPkexecAvailable(Exception):
-    pass
 
+class EIPNoCommandError(EIPClientError):
+    message = "no suitable openvpn command found"
+    usermessage = ("No suitable openvpn command found. "
+                   "<br/>(Might be a permissions problem)")
 
-class EIPInitNoProviderError(Exception):
-    pass
 
+#
+# errors still needing some love
+#
+
+class EIPInitNoKeyFileError(CriticalError):
+    message = "No vpn keys found in the expected path"
+    usermessage = "We could not find your eip certs in the expected path"
 
-class EIPInitBadProviderError(Exception):
+
+class EIPInitBadKeyFilePermError(Warning):
+    # I don't know if we should be telling user or not,
+    # we try to fix permissions and should only re-raise
+    # if permission check failed.
     pass
 
 
-class EIPInitNoKeyFileError(Exception):
+class EIPInitNoProviderError(EIPClientError):
     pass
 
 
-class EIPInitBadKeyFilePermError(Exception):
+class EIPInitBadProviderError(EIPClientError):
     pass
 
 
-class EIPNoCommandError(Exception):
+class EIPConfigurationError(EIPClientError):
     pass
 
+#
 # Errors that probably we don't need anymore
+# chase down for them and check.
+#
 
 
 class MissingSocketError(Exception):
@@ -65,11 +122,5 @@ class ConnectionRefusedError(Exception):
     pass
 
 
-
-
 class EIPMissingDefaultProvider(Exception):
     pass
-
-
-class EIPConfigurationError(Exception):
-    pass