From 0d5636a90044ecf7e4629dbd2b040a467df34fc8 Mon Sep 17 00:00:00 2001 From: fonfon Date: Thu, 10 Jul 2014 18:27:49 +0300 Subject: [PATCH] refactored actions to use exceptions instead of return-values --- actions.py | 6 ++++- errors.py | 10 ++++++++ modules/config/config.py | 25 ++++++++++---------- modules/diagnostics/diagnostics.py | 8 ++++++- modules/firewall/firewall.py | 9 ++------ modules/owncloud/owncloud.py | 5 +--- modules/packages/packages.py | 37 ++++++++++++------------------ modules/pagekite/pagekite.py | 10 ++------ modules/tor/tor.py | 3 +-- modules/xmpp/xmpp.py | 15 +++--------- tests/actions_test.py | 4 ++-- views.py | 2 +- 12 files changed, 62 insertions(+), 72 deletions(-) create mode 100644 errors.py diff --git a/actions.py b/actions.py index e4bd54bc3..a24d9ebd5 100644 --- a/actions.py +++ b/actions.py @@ -77,6 +77,7 @@ import pipes import subprocess import cfg +from errors import ActionError def run(action, options=None, async=False): @@ -106,6 +107,7 @@ 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 @@ -139,4 +141,6 @@ def _run(action, options=None, async=False, run_as_root=False): if not async: output, error = proc.communicate() - return output, error + if proc.returncode != 0: + raise ActionError('Running action %s failed: %s' % (action, error)) + return output diff --git a/errors.py b/errors.py new file mode 100644 index 000000000..e6b320084 --- /dev/null +++ b/errors.py @@ -0,0 +1,10 @@ +""" Plinth project specific errors """ + + +class PlinthError(Exception): + pass + + +class ActionError(PlinthError): + """ Use this error for exceptions when executing an action """ + pass diff --git a/modules/config/config.py b/modules/config/config.py index 4aa6f299e..f24523588 100644 --- a/modules/config/config.py +++ b/modules/config/config.py @@ -32,6 +32,7 @@ import socket import actions import cfg import util +from errors import ActionError LOGGER = logging.getLogger(__name__) @@ -133,20 +134,22 @@ def get_status(): def _apply_changes(request, old_status, new_status): """Apply the form changes""" if old_status['hostname'] != new_status['hostname']: - if not set_hostname(new_status['hostname']): - messages.error(request, _('Setting hostname failed')) + try: + set_hostname(new_status['hostname']) + except (ActionError, ValueError) as err: + messages.error(request, _('Error setting hostname: %s') % + err.message) else: messages.success(request, _('Hostname set')) else: messages.info(request, _('Hostname is unchanged')) if old_status['time_zone'] != new_status['time_zone']: - output, error = actions.superuser_run('timezone-change', - [new_status['time_zone']]) - del output # Unused - if error: - messages.error(request, - _('Error setting time zone - %s') % error) + try: + actions.superuser_run('timezone-change', [new_status['time_zone']]) + except (ActionError, ValueError) as err: + messages.error(request, _('Error setting time zone: %s') % + err.message) else: messages.success(request, _('Time zone set')) else: @@ -164,7 +167,5 @@ def set_hostname(hostname): actions.superuser_run('xmpp-pre-hostname-change') actions.superuser_run('hostname-change', hostname) actions.superuser_run('xmpp-hostname-change', hostname, async=True) - except OSError: - return False - - return True + except OSError as err: + raise ActionError(err.message) diff --git a/modules/diagnostics/diagnostics.py b/modules/diagnostics/diagnostics.py index 854bdf2ed..d1370c818 100644 --- a/modules/diagnostics/diagnostics.py +++ b/modules/diagnostics/diagnostics.py @@ -25,6 +25,7 @@ from gettext import gettext as _ import actions import cfg +from errors import ActionError def init(): @@ -43,7 +44,12 @@ def index(request): @login_required def test(request): """Run diagnostics and the output page""" - output, error = actions.superuser_run("diagnostic-test") + output = "" + error = "" + try: + output = actions.superuser_run("diagnostic-test") + except ActionError as err: + error = err.message 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 e75b9fdb2..21b3ac399 100644 --- a/modules/firewall/firewall.py +++ b/modules/firewall/firewall.py @@ -140,16 +140,11 @@ 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, error = actions.superuser_run(command, arguments) + output = actions.superuser_run(command, arguments) else: - output, error = actions.run(command, arguments) - - if error: - raise Exception('Error setting/getting firewalld confguration - %s' - % error) + output = actions.run(command, arguments) return output diff --git a/modules/owncloud/owncloud.py b/modules/owncloud/owncloud.py index 1507569b6..8b53bcb09 100644 --- a/modules/owncloud/owncloud.py +++ b/modules/owncloud/owncloud.py @@ -53,10 +53,7 @@ def index(request): def get_status(): """Return the current status""" - output, error = actions.run('owncloud-setup', 'status') - if error: - raise Exception('Error getting ownCloud status: %s' % error) - + output = actions.run('owncloud-setup', 'status') return {'enabled': 'enable' in output.split()} diff --git a/modules/packages/packages.py b/modules/packages/packages.py index cbae9198a..292653eff 100644 --- a/modules/packages/packages.py +++ b/modules/packages/packages.py @@ -6,23 +6,18 @@ from gettext import gettext as _ import actions import cfg +from errors import ActionError def get_modules_available(): """Return list of all modules""" - output, error = actions.run('module-manager', ['list-available']) - if error: - raise Exception('Error getting modules: %s' % error) - + output = actions.run('module-manager', ['list-available']) return output.split() def get_modules_enabled(): """Return list of all modules""" - output, error = actions.run('module-manager', ['list-enabled']) - if error: - raise Exception('Error getting enabled modules - %s' % error) - + output = actions.run('module-manager', ['list-enabled']) return output.split() @@ -88,13 +83,12 @@ def _apply_changes(request, old_status, new_status): module = field.split('_enabled')[0] if enabled: - output, error = actions.superuser_run( - 'module-manager', ['enable', cfg.python_root, module]) - del output # Unused - - # TODO: need to get plinth to load the module we just - # enabled - if error: + try: + actions.superuser_run( + 'module-manager', ['enable', cfg.python_root, module]) + except ActionError: + # TODO: need to get plinth to load the module we just + # enabled messages.error( request, _('Error enabling module - {module}').format( module=module)) @@ -103,13 +97,12 @@ def _apply_changes(request, old_status, new_status): request, _('Module enabled - {module}').format( module=module)) else: - output, error = actions.superuser_run( - 'module-manager', ['disable', cfg.python_root, module]) - del output # Unused - - # TODO: need a smoother way for plinth to unload the - # module - if error: + try: + actions.superuser_run( + 'module-manager', ['disable', cfg.python_root, module]) + except ActionError: + # TODO: need a smoother way for plinth to unload the + # module messages.error( request, _('Error disabling module - {module}').format( module=module)) diff --git a/modules/pagekite/pagekite.py b/modules/pagekite/pagekite.py index 9856f9da1..9a252c6b9 100644 --- a/modules/pagekite/pagekite.py +++ b/modules/pagekite/pagekite.py @@ -196,16 +196,10 @@ 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, error = actions.superuser_run(command, arguments) + output = actions.superuser_run(command, arguments) else: - output, error = actions.run(command, arguments) - - if error: - raise Exception('Error setting/getting PageKite confguration - %s' - % error) - + output = actions.run(command, arguments) return output diff --git a/modules/tor/tor.py b/modules/tor/tor.py index ee3cdfdb9..45fa32298 100644 --- a/modules/tor/tor.py +++ b/modules/tor/tor.py @@ -36,8 +36,7 @@ def init(): @login_required def index(request): """Service the index page""" - output, error = actions.superuser_run("tor-get-ports") - del error # Unused + output = actions.superuser_run("tor-get-ports") port_info = output.split("\n") tor_ports = {} diff --git a/modules/xmpp/xmpp.py b/modules/xmpp/xmpp.py index 592dd3b01..5e9af1e3f 100644 --- a/modules/xmpp/xmpp.py +++ b/modules/xmpp/xmpp.py @@ -88,10 +88,7 @@ def configure(request): def get_status(): """Return the current status""" - output, error = actions.run('xmpp-setup', 'status') - if error: - raise Exception('Error getting status: %s' % error) - + output = actions.run('xmpp-setup', 'status') return {'inband_enabled': 'inband_enable' in output.split()} @@ -111,11 +108,7 @@ def _apply_changes(request, old_status, new_status): option = 'noinband_enable' LOGGER.info('Option - %s', option) - - _output, error = actions.superuser_run('xmpp-setup', [option]) - del _output # Unused - if error: - raise Exception('Error running command - %s' % error) + actions.superuser_run('xmpp-setup', [option]) class RegisterForm(forms.Form): # pylint: disable-msg=W0232 @@ -151,10 +144,8 @@ def register(request): def _register_user(request, data): """Register a new XMPP user""" - output, error = actions.superuser_run( + output = actions.superuser_run( 'xmpp-register', [data['username'], data['password']]) - if error: - raise Exception('Error registering user - %s' % error) if 'successfully registered' in output: messages.success(request, _('Registered account for %s') % diff --git a/tests/actions_test.py b/tests/actions_test.py index 09fbae144..420ade662 100644 --- a/tests/actions_test.py +++ b/tests/actions_test.py @@ -90,7 +90,7 @@ class TestPrivileged(unittest.TestCase): # counting is safer than actual badness. options = "good; echo $((1+1))" - output, error = run(action, options) + output = run(action, options) self.assertFalse("2" in output) @@ -105,7 +105,7 @@ class TestPrivileged(unittest.TestCase): # counting is safer than actual badness. options = ["good", ";", "echo $((1+1))"] - output, error = run(action, options) + output = run(action, options) # we'd better not evaluate the data. self.assertFalse("2" in output) diff --git a/views.py b/views.py index 776758c55..a27abd70a 100644 --- a/views.py +++ b/views.py @@ -48,4 +48,4 @@ def index(request): if request.user.is_authenticated(): return HttpResponseRedirect(reverse('apps:index')) - return HttpResponseRedirect(reverse('apps:about')) + return HttpResponseRedirect(reverse('help:about'))