diff --git a/actions.py b/actions.py index a24d9ebd5..87eec5129 100644 --- a/actions.py +++ b/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 diff --git a/modules/config/config.py b/modules/config/config.py index 6ba411a24..1d11a9008 100644 --- a/modules/config/config.py +++ b/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) + actions.superuser_run('xmpp-pre-hostname-change') + actions.superuser_run('hostname-change', hostname) + actions.superuser_run('xmpp-hostname-change', hostname, async=True) diff --git a/modules/diagnostics/diagnostics.py b/modules/diagnostics/diagnostics.py index 09031b259..32b66310c 100644 --- a/modules/diagnostics/diagnostics.py +++ b/modules/diagnostics/diagnostics.py @@ -44,12 +44,15 @@ def index(request): @login_required def test(request): """Run diagnostics and the output page""" - output = "" - error = "" + output = '' + error = '' try: output = actions.superuser_run("diagnostic-test") - except ActionError as err: - error = err.message + except ActionError as exception: + output, error = exception.args[1:] + except Exception as exception: + error = str(exception) + return TemplateResponse(request, 'diagnostics_test.html', {'title': _('Diagnostic Test'), 'diagnostics_output': output, diff --git a/modules/firewall/firewall.py b/modules/firewall/firewall.py index a1fadf0f1..760e980e6 100644 --- a/modules/firewall/firewall.py +++ b/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 output + return actions.run(command, arguments) diff --git a/modules/packages/packages.py b/modules/packages/packages.py index bc2c787f5..ee4c52717 100644 --- a/modules/packages/packages.py +++ b/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( diff --git a/modules/pagekite/pagekite.py b/modules/pagekite/pagekite.py index 827cde439..f6e90ced0 100644 --- a/modules/pagekite/pagekite.py +++ b/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 output + return actions.run(command, arguments)