Improve exception handle when running actions

- Don't use depricated exception.message. Instead using exception.args
- Pass stdout output along with stderr in exception
- Handle possible exceptions such as ValueError, OSError in all cases
- Log all command executions and their errors

actions.py

@@ -72,6 +72,7 @@ Actions run commands with this contract (version 1.1):
 
 """
 
+import logging
 import os
 import pipes
 import subprocess
@@ -80,6 +81,9 @@ import cfg
 from errors import ActionError
 
 
+LOGGER = logging.getLogger(__name__)
+
+
 def run(action, options=None, async=False):
     """Safely run a specific action as the current user.
     See actions._run for more information.
@@ -107,7 +111,6 @@ def _run(action, options=None, async=False, run_as_root=False):
 
     # contract 3A and 3B: don't call anything outside of the actions directory.
     if os.sep in action:
-        # TODO: perhaps we should raise an ActionError instead of ValueError
         raise ValueError("Action can't contain:" + os.sep)
 
     cmd = cfg.actions_dir + os.sep + action
@@ -131,6 +134,8 @@ def _run(action, options=None, async=False, run_as_root=False):
     if run_as_root:
         cmd = ["sudo", "-n"] + cmd
 
+    LOGGER.info('Executing command - %s', cmd)
+
     # contract 3C: don't interpret shell escape sequences.
     # contract 5 (and 6-ish).
     proc = subprocess.Popen(
@@ -142,5 +147,8 @@ def _run(action, options=None, async=False, run_as_root=False):
     if not async:
         output, error = proc.communicate()
         if proc.returncode != 0:
-            raise ActionError('Running action %s failed: %s' % (action, error))
+            LOGGER.error('Error executing command - %s, %s, %s', cmd, output,
+                         error)
+            raise ActionError(action, output, error)
+
         return output

modules/config/config.py

@@ -32,7 +32,6 @@ import socket
 import actions
 import cfg
 import util
-from errors import ActionError
 
 
 LOGGER = logging.getLogger(__name__)
@@ -136,9 +135,9 @@ def _apply_changes(request, old_status, new_status):
     if old_status['hostname'] != new_status['hostname']:
         try:
             set_hostname(new_status['hostname'])
-        except (ActionError, ValueError) as err:
+        except Exception as exception:
             messages.error(request, _('Error setting hostname: %s') %
-                           err.message)
+                           str(exception))
         else:
             messages.success(request, _('Hostname set'))
     else:
@@ -147,9 +146,9 @@ def _apply_changes(request, old_status, new_status):
     if old_status['time_zone'] != new_status['time_zone']:
         try:
             actions.superuser_run('timezone-change', [new_status['time_zone']])
-        except (ActionError, ValueError) as err:
+        except Exception as exception:
             messages.error(request, _('Error setting time zone: %s') %
-                           err.message)
+                           str(exception))
         else:
             messages.success(request, _('Time zone set'))
     else:
@@ -163,9 +162,6 @@ def set_hostname(hostname):
     hostname = str(hostname)
 
     LOGGER.info('Changing hostname to - %s', hostname)
-    try:
     actions.superuser_run('xmpp-pre-hostname-change')
     actions.superuser_run('hostname-change', hostname)
     actions.superuser_run('xmpp-hostname-change', hostname, async=True)
-    except OSError as err:
-        raise ActionError(err.message)

modules/diagnostics/diagnostics.py

@@ -44,12 +44,15 @@ def index(request):
 @login_required
 def test(request):
     """Run diagnostics and the output page"""
-    output = ""
+    output = ''
-    error = ""
+    error = ''
     try:
         output = actions.superuser_run("diagnostic-test")
-    except ActionError as err:
+    except ActionError as exception:
-        error = err.message
+        output, error = exception.args[1:]
+    except Exception as exception:
+        error = str(exception)
+
     return TemplateResponse(request, 'diagnostics_test.html',
                             {'title': _('Diagnostic Test'),
                              'diagnostics_output': output,

modules/firewall/firewall.py

@@ -140,11 +140,8 @@ def on_service_enabled(sender, service_id, enabled, **kwargs):
 def _run(arguments, superuser=False):
     """Run an given command and raise exception if there was an error"""
     command = 'firewall'
-    LOGGER.info('Running command - %s, %s, %s', command, arguments, superuser)
 
     if superuser:
-        output = actions.superuser_run(command, arguments)
+        return actions.superuser_run(command, arguments)
     else:
-        output = actions.run(command, arguments)
+        return actions.run(command, arguments)
-
-    return output

modules/packages/packages.py

@@ -6,7 +6,6 @@ from gettext import gettext as _
 
 import actions
 import cfg
-from errors import ActionError
 
 
 def get_modules_available():
@@ -86,7 +85,7 @@ def _apply_changes(request, old_status, new_status):
             try:
                 actions.superuser_run(
                     'module-manager', ['enable', cfg.python_root, module])
-            except ActionError:
+            except Exception:
                 # TODO: need to get plinth to load the module we just
                 # enabled
                 messages.error(
@@ -100,7 +99,7 @@ def _apply_changes(request, old_status, new_status):
             try:
                 actions.superuser_run(
                     'module-manager', ['disable', cfg.python_root, module])
-            except ActionError:
+            except Exception:
                 # TODO: need a smoother way for plinth to unload the
                 # module
                 messages.error(

modules/pagekite/pagekite.py

@@ -197,10 +197,8 @@ def _apply_changes(request, old_status, new_status):
 def _run(arguments, superuser=True):
     """Run an given command and raise exception if there was an error"""
     command = 'pagekite-configure'
-    LOGGER.info('Running command - %s, %s, %s', command, arguments, superuser)
 
     if superuser:
-        output = actions.superuser_run(command, arguments)
+        return actions.superuser_run(command, arguments)
     else:
-        output = actions.run(command, arguments)
+        return actions.run(command, arguments)
-    return output