From 9c84f2b4354ef3ebbe82f5886c8a762c48f3a71d Mon Sep 17 00:00:00 2001 From: Ad Schellevis Date: Mon, 1 Feb 2016 21:51:23 +0100 Subject: [PATCH] (configd) code style cleanups --- src/opnsense/service/modules/__init__.py | 5 + .../modules/addons/template_helpers.py | 4 +- src/opnsense/service/modules/config.py | 19 ++-- src/opnsense/service/modules/daemonize.py | 1 + .../service/modules/ph_inline_actions.py | 4 +- .../service/modules/processhandler.py | 93 +++++++++++-------- src/opnsense/service/modules/template.py | 34 +++---- 7 files changed, 93 insertions(+), 67 deletions(-) diff --git a/src/opnsense/service/modules/__init__.py b/src/opnsense/service/modules/__init__.py index d1b3ed313..481b12bd6 100644 --- a/src/opnsense/service/modules/__init__.py +++ b/src/opnsense/service/modules/__init__.py @@ -24,12 +24,17 @@ POSSIBILITY OF SUCH DAMAGE. """ + def singleton(cls, *args, **kwargs): """ singleton pattern, use ad decorator + :param cls: """ instances = {} + + # noinspection PyShadowingNames def getinstance(*args, **kwargs): if cls not in instances: instances[cls] = cls(*args, **kwargs) return instances[cls] + return getinstance diff --git a/src/opnsense/service/modules/addons/template_helpers.py b/src/opnsense/service/modules/addons/template_helpers.py index 4962bc5ca..99ccd890c 100644 --- a/src/opnsense/service/modules/addons/template_helpers.py +++ b/src/opnsense/service/modules/addons/template_helpers.py @@ -30,6 +30,8 @@ from operator import itemgetter + +# noinspection PyPep8Naming class Helpers(object): def __init__(self, template_in_data): """ initialize template helpers @@ -82,7 +84,7 @@ class Helpers(object): return result else: # resort list by tag - return sorted(result,key=itemgetter(sortBy)) + return sorted(result, key=itemgetter(sortBy)) def getUUIDtag(self, uuid): """ retrieve tag name of registered uuid, returns __not_found__ if none available diff --git a/src/opnsense/service/modules/config.py b/src/opnsense/service/modules/config.py index dbb590c6c..ea41ea164 100644 --- a/src/opnsense/service/modules/config.py +++ b/src/opnsense/service/modules/config.py @@ -28,14 +28,14 @@ package : configd function: config handler """ -__author__ = 'Ad Schellevis' - import os import stat import collections import copy import xml.etree.cElementTree as ElementTree +__author__ = 'Ad Schellevis' + class Config(object): def __init__(self, filename): @@ -62,14 +62,14 @@ class Config(object): self._config_data['__uuid_tags__'] = self.__uuid_tags self._file_mod = mod_time - def _traverse(self, xmlNode): + def _traverse(self, xml_node): """ traverse xml node and return ordered dictionary structure - :param xmlNode: ElementTree node + :param xml_node: ElementTree node :return: collections.OrderedDict """ this_item = collections.OrderedDict() - if len(list(xmlNode)) > 0: - for item in list(xmlNode): + if len(list(xml_node)) > 0: + for item in list(xml_node): item_content = self._traverse(item) if 'uuid' in item.attrib: self.__uuid_data[item.attrib['uuid']] = item_content @@ -88,7 +88,7 @@ class Config(object): this_item[item.tag] = self._traverse(item) else: # last node, return text - return xmlNode.text + return xml_node.text return this_item @@ -98,14 +98,15 @@ class Config(object): @param elem: cElementTree @param level: Currentlevel """ - i = "\n" + level*" " + i = "\n" + level * " " if len(elem): if not elem.text or not elem.text.strip(): elem.text = i + " " for e in elem: - self.indent(e, level+1) + self.indent(e, level + 1) if not e.tail or not e.tail.strip(): e.tail = i + " " + # noinspection PyUnboundLocalVariable if not e.tail or not e.tail.strip(): e.tail = i else: diff --git a/src/opnsense/service/modules/daemonize.py b/src/opnsense/service/modules/daemonize.py index 077a34eac..a360880cb 100644 --- a/src/opnsense/service/modules/daemonize.py +++ b/src/opnsense/service/modules/daemonize.py @@ -60,6 +60,7 @@ class Daemonize(object): def sigterm(self, signum, frame): """ These actions will be done after SIGTERM. + :param frame: """ self.logger.warn("Caught signal %s. Stopping daemon." % signum) os.remove(self.pid) diff --git a/src/opnsense/service/modules/ph_inline_actions.py b/src/opnsense/service/modules/ph_inline_actions.py index c8b68753a..cfd98935a 100644 --- a/src/opnsense/service/modules/ph_inline_actions.py +++ b/src/opnsense/service/modules/ph_inline_actions.py @@ -48,7 +48,7 @@ def execute(action, parameters): # generate template tmpl = template.Template(action.root_dir) conf = config.Config(action.config) - tmpl.setConfig(conf.get()) + tmpl.set_config(conf.get()) filenames = tmpl.generate(parameters) del conf @@ -78,7 +78,7 @@ def execute(action, parameters): # list all available configd actions from processhandler import ActionHandler act_handler = ActionHandler() - actions = act_handler.listActions(['message', 'description']) + actions = act_handler.list_actions(['message', 'description']) if unicode(parameters).lower() == 'json': import json diff --git a/src/opnsense/service/modules/processhandler.py b/src/opnsense/service/modules/processhandler.py index 70661e907..3c3bb0aa1 100644 --- a/src/opnsense/service/modules/processhandler.py +++ b/src/opnsense/service/modules/processhandler.py @@ -28,8 +28,6 @@ function: unix domain socket process worker process """ -__author__ = 'Ad Schellevis' - import os import subprocess import socket @@ -45,6 +43,8 @@ import tempfile import ph_inline_actions from modules import singleton +__author__ = 'Ad Schellevis' + class Handler(object): """ Main handler class, opens unix domain socket and starts listening @@ -57,7 +57,8 @@ class Handler(object): -> execute ActionHandler command using Action objects <- send back result string """ - def __init__(self, socket_filename, config_path, config_environment={}, simulation_mode=False): + + def __init__(self, socket_filename, config_path, config_environment=None, simulation_mode=False): """ Constructor :param socket_filename: filename of unix domain socket to use @@ -65,6 +66,8 @@ class Handler(object): :param simulation_mode: emulation mode, do not start actual (script) commands :return: object """ + if config_environment is None: + config_environment = {} self.socket_filename = socket_filename self.config_path = config_path self.simulation_mode = simulation_mode @@ -77,10 +80,11 @@ class Handler(object): :return: """ while True: + # noinspection PyBroadException try: # open action handler - actHandler = ActionHandler(config_path=self.config_path, - config_environment=self.config_environment) + act_handler = ActionHandler(config_path=self.config_path, + config_environment=self.config_environment) # remove previous socket ( if exists ) try: @@ -99,7 +103,7 @@ class Handler(object): # spawn a client connection cmd_thread = HandlerClient(connection=connection, client_address=client_address, - action_handler=actHandler, + action_handler=act_handler, simulation_mode=self.simulation_mode) if self.single_threaded: # run single threaded @@ -120,7 +124,7 @@ class Handler(object): # cleanup on exit, remove socket os.remove(self.socket_filename) return - except: + except Exception: # something went wrong... send traceback to syslog, restart listener (wait for a short time) print (traceback.format_exc()) syslog.syslog(syslog.LOG_ERR, 'Handler died on %s' % traceback.format_exc()) @@ -130,6 +134,7 @@ class Handler(object): class HandlerClient(threading.Thread): """ Handle commands via specified socket connection """ + def __init__(self, connection, client_address, action_handler, simulation_mode=False): """ :param connection: socket connection object @@ -155,6 +160,7 @@ class HandlerClient(threading.Thread): exec_action = '' exec_params = '' exec_in_background = False + # noinspection PyBroadException try: # receive command, maximum data length is 4k... longer messages will be truncated data = self.connection.recv(4096) @@ -186,7 +192,7 @@ class HandlerClient(threading.Thread): # execute requested action if self.simulation_mode: - self.action_handler.showAction(exec_command, exec_action, exec_params, self.message_uuid) + self.action_handler.show_action(exec_command, exec_action, exec_params, self.message_uuid) result = 'OK' else: result = self.action_handler.execute(exec_command, exec_action, exec_params, self.message_uuid) @@ -207,23 +213,28 @@ class HandlerClient(threading.Thread): except SystemExit: # ignore system exit related errors pass - except: + except Exception: print (traceback.format_exc()) - syslog.syslog(syslog.LOG_ERR, - 'unable to sendback response [%s] for [%s][%s][%s] {%s}, message was %s' % (result, - exec_command, - exec_action, - exec_params, - self.message_uuid, - traceback.format_exc())) + syslog.syslog( + syslog.LOG_ERR, + 'unable to sendback response [%s] for [%s][%s][%s] {%s}, message was %s' % (result, + exec_command, + exec_action, + exec_params, + self.message_uuid, + traceback.format_exc() + ) + ) finally: if not exec_in_background: self.connection.close() + @singleton class ActionHandler(object): """ Start/stop services and functions using configuration data defined in conf/actions_.conf """ + def __init__(self, config_path=None, config_environment=None): """ Initialize action handler to start system functions @@ -238,6 +249,7 @@ class ActionHandler(object): # try to load data on initial start if not hasattr(self, 'action_map'): + self.action_map = {} self.load_config() def load_config(self): @@ -245,8 +257,6 @@ class ActionHandler(object): :return: None """ - - self.action_map = {} for config_filename in glob.glob('%s/actions_*.conf' % self.config_path) \ + glob.glob('%s/actions.d/actions_*.conf' % self.config_path): # this topic's name (service, filter, template, etc) @@ -260,7 +270,7 @@ class ActionHandler(object): cnf.read(config_filename) for section in cnf.sections(): # map configuration data on object - action_obj = Action(config_environment = self.config_environment) + action_obj = Action(config_environment=self.config_environment) for act_prop in cnf.items(section): setattr(action_obj, act_prop[0], act_prop[1]) @@ -274,10 +284,13 @@ class ActionHandler(object): for alias in section.split('|'): self.action_map[topic_name][alias] = action_obj - def listActions(self, attributes=[]): + def list_actions(self, attributes=None): """ list all available actions + :param attributes: :return: dict """ + if attributes is None: + attributes = [] result = {} for command in self.action_map: for action in self.action_map[command]: @@ -285,7 +298,7 @@ class ActionHandler(object): # parse second level actions # TODO: nesting actions may be better to solve recursive in here and in load_config part for subAction in self.action_map[command][action]: - cmd='%s %s %s'%(command,action,subAction) + cmd = '%s %s %s' % (command, action, subAction) result[cmd] = {} for actAttr in attributes: if hasattr(self.action_map[command][action][subAction], actAttr): @@ -293,7 +306,7 @@ class ActionHandler(object): else: result[cmd][actAttr] = '' else: - cmd='%s %s'%(command,action) + cmd = '%s %s' % (command, action) result[cmd] = {} for actAttr in attributes: if hasattr(self.action_map[command][action], actAttr): @@ -303,7 +316,7 @@ class ActionHandler(object): return result - def findAction(self, command, action, parameters): + def find_action(self, command, action, parameters): """ find action object :param command: command/topic for example interface @@ -320,7 +333,7 @@ class ActionHandler(object): # 3 level action ( "interface linkup start" for example ) if isinstance(self.action_map[command][action][parameters[0]], Action): action_obj = self.action_map[command][action][parameters[0]] - action_obj.setParameterStartPos(1) + action_obj.set_parameter_start_pos(1) elif isinstance(self.action_map[command][action], Action): action_obj = self.action_map[command][action] @@ -336,17 +349,17 @@ class ActionHandler(object): :return: OK on success, else error code """ action_params = [] - action_obj = self.findAction(command, action, parameters) + action_obj = self.find_action(command, action, parameters) if action_obj is not None: - if parameters is not None and len(parameters) > action_obj.getParameterStartPos(): - action_params = parameters[action_obj.getParameterStartPos():] + if parameters is not None and len(parameters) > action_obj.get_parameter_start_pos(): + action_params = parameters[action_obj.get_parameter_start_pos():] return '%s\n' % action_obj.execute(action_params, message_uuid) return 'Action not found\n' - def showAction(self, command, action, parameters, message_uuid): + def show_action(self, command, action, parameters, message_uuid): """ debug/simulation mode: show action information :param command: command/topic for example interface :param action: action to run ( for example linkup ) @@ -354,10 +367,10 @@ class ActionHandler(object): :param message_uuid: message unique id :return: None """ - action_obj = self.findAction(command, action, parameters) + action_obj = self.find_action(command, action, parameters) print ('---------------------------------------------------------------------') print ('execute %s.%s with parameters : %s ' % (command, action, parameters)) - print ('action object %s (%s)' % (action_obj, action_obj.command)) + print ('action object %s (%s) %s' % (action_obj, action_obj.command, message_uuid)) print ('---------------------------------------------------------------------') @@ -365,6 +378,7 @@ class Action(object): """ Action class, handles actual (system) calls. set command, parameters (template) type and log message """ + def __init__(self, config_environment): """ setup default properties :param config_environment: environment to use @@ -377,7 +391,7 @@ class Action(object): self.message = None self._parameter_start_pos = 0 - def setParameterStartPos(self, pos): + def set_parameter_start_pos(self, pos): """ :param pos: start position of parameter list @@ -385,7 +399,7 @@ class Action(object): """ self._parameter_start_pos = pos - def getParameterStartPos(self): + def get_parameter_start_pos(self): """ getter for _parameter_start_pos :return: start position of parameter list ( first argument can be part of action to start ) """ @@ -427,18 +441,18 @@ class Action(object): # use quotes on parameters to prevent code injection if script_command.count('%s') > len(parameters): # script command accepts more parameters then given, fill with empty parameters - for i in range(script_command.count('%s')-len(parameters)): + for i in range(script_command.count('%s') - len(parameters)): parameters.append("") elif len(parameters) > script_command.count('%s'): # parameters then expected, fail execution return 'Parameter mismatch' # force escape of shell exploitable characters for all user parameters - for escape_char in ['`','$','!','(',')','|']: + for escape_char in ['`', '$', '!', '(', ')', '|']: for i in range(len(parameters[0:script_command.count('%s')])): - parameters[i] = parameters[i].replace(escape_char,'\\%s'%escape_char) + parameters[i] = parameters[i].replace(escape_char, '\\%s' % escape_char) - script_command = script_command % tuple(map(lambda x: '"'+x.replace('"', '\\"')+'"', + script_command = script_command % tuple(map(lambda x: '"' + x.replace('"', '\\"') + '"', parameters[0:script_command.count('%s')])) if self.type.lower() == 'script': # execute script type command @@ -460,14 +474,15 @@ class Action(object): with tempfile.NamedTemporaryFile() as error_stream: with tempfile.NamedTemporaryFile() as output_stream: subprocess.check_call(script_command, env=self.config_environment, shell=True, - stdout=output_stream, stderr=error_stream) + stdout=output_stream, stderr=error_stream) output_stream.seek(0) error_stream.seek(0) script_output = output_stream.read() script_error_output = error_stream.read() if len(script_error_output) > 0: - syslog.syslog(syslog.LOG_ERR, '[%s] Script action stderr returned "%s"' % (message_uuid, - script_error_output.strip()[:255]) + syslog.syslog(syslog.LOG_ERR, + '[%s] Script action stderr returned "%s"' % + (message_uuid, script_error_output.strip()[:255]) ) return script_output except Exception as script_exception: diff --git a/src/opnsense/service/modules/template.py b/src/opnsense/service/modules/template.py index 86cb3c00a..9db3912e1 100644 --- a/src/opnsense/service/modules/template.py +++ b/src/opnsense/service/modules/template.py @@ -28,9 +28,6 @@ package : configd function: template handler, generate configuration files using templates """ - -__author__ = 'Ad Schellevis' - import os import os.path import syslog @@ -39,9 +36,10 @@ import copy import jinja2 import addons.template_helpers +__author__ = 'Ad Schellevis' + class Template(object): - def __init__(self, target_root_directory="/"): """ constructor :return: @@ -53,11 +51,12 @@ class Template(object): self._target_root_directory = target_root_directory # setup jinja2 environment - self._template_dir = os.path.dirname(os.path.abspath(__file__))+'/../templates/' + self._template_dir = os.path.dirname(os.path.abspath(__file__)) + '/../templates/' self._j2_env = jinja2.Environment(loader=jinja2.FileSystemLoader(self._template_dir), trim_blocks=True, - extensions=["jinja2.ext.do",]) + extensions=["jinja2.ext.do", ]) - def _read_manifest(self, filename): + @staticmethod + def _read_manifest(filename): """ :param filename: manifest filename (path/+MANIFEST) @@ -71,7 +70,8 @@ class Template(object): return result - def _read_targets(self, filename): + @staticmethod + def _read_targets(filename): """ read raw target filename masks :param filename: targets filename (path/+TARGETS) @@ -117,18 +117,19 @@ class Template(object): return result - def setConfig(self, config_data): + def set_config(self, config_data): """ set config data :param config_data: config data as dictionary/list structure :return: None """ - if type(config_data) in(dict, collections.OrderedDict): + if type(config_data) in (dict, collections.OrderedDict): self._config = config_data else: # no data given, reset self._config = {} - def __find_string_tags(self, instr): + @staticmethod + def __find_string_tags(instr): """ :param instr: string with optional tags [field.$$] :return: @@ -194,7 +195,8 @@ class Template(object): return result - def _create_directory(self, filename): + @staticmethod + def _create_directory(filename): """ create directory :param filename: create path for filename ( if not existing ) :return: None @@ -231,7 +233,7 @@ class Template(object): result_filenames[new_filename] = copy.deepcopy(result_filenames[filename]) result_filenames[new_filename][key] = target_filters[target_filter][key] - template_filename = '%s/%s'%(module_name.replace('.', '/'), src_template) + template_filename = '%s/%s' % (module_name.replace('.', '/'), src_template) # parse template, make sure issues can be traced back to their origin try: j2_page = self._j2_env.get_template(template_filename) @@ -268,8 +270,8 @@ class Template(object): # it was in the original template. # It looks like Jinja sometimes isn't consistent on placing this last end-of-line in. if len(content) > 1 and content[-1] != '\n': - src_file = '%s%s'%(self._template_dir,template_filename) - src_file_handle = open(src_file,'r') + src_file = '%s%s' % (self._template_dir, template_filename) + src_file_handle = open(src_file, 'r') src_file_handle.seek(-1, os.SEEK_END) last_bytes_template = src_file_handle.read() src_file_handle.close() @@ -298,7 +300,7 @@ class Template(object): # direct match do_generate = True elif wildcard_pos == -1 and len(module_name) < len(template_name) \ - and '%s.'%module_name == template_name[0:len(module_name)+1]: + and '%s.' % module_name == template_name[0:len(module_name) + 1]: # match child item do_generate = True