From 3dc159a9a4ddb38ca84c2ef22c5ca3bc0ab238f2 Mon Sep 17 00:00:00 2001 From: Markus Zolliker Date: Mon, 8 Mar 2021 10:27:43 +0100 Subject: [PATCH] user friendly reporting of config errors Config errors are collected first, and raised after processing all modules. This is more user friendly. + remove redundant check for predefined accessibles in modules.py + fixed error handling for exporting parameters in params.py + fixed handling of bare attributes overwriting properties Change-Id: I894bda291ab85ccec3d771c4903393c808af0a2a --- secop/modules.py | 113 +++++++++++++++++++++----------------------- secop/params.py | 14 ++++-- secop/properties.py | 10 ++-- secop/server.py | 45 ++++++++++++++---- 4 files changed, 106 insertions(+), 76 deletions(-) diff --git a/secop/modules.py b/secop/modules.py index 32f798e..ae0a6c6 100644 --- a/secop/modules.py +++ b/secop/modules.py @@ -30,9 +30,9 @@ from secop.datatypes import ArrayOf, BoolType, EnumType, FloatRange, \ IntRange, StatusType, StringType, TextType, TupleOf, get_datatype from secop.errors import BadValueError, ConfigError, InternalError, \ ProgrammingError, SECoPError, SilentError, secop_error -from secop.lib import formatException, formatExtendedStack, mkthread +from secop.lib import formatException, mkthread from secop.lib.enum import Enum -from secop.params import PREDEFINED_ACCESSIBLES, Accessible, Command, Parameter +from secop.params import Accessible, Command, Parameter from secop.poller import BasicPoller, Poller from secop.properties import HasProperties, Property @@ -237,29 +237,27 @@ class Module(HasAccessibles): self.name = name self.valueCallbacks = {} self.errorCallbacks = {} + errors = [] # handle module properties # 1) make local copies of properties super().__init__() - # 2) check and apply properties specified in cfgdict - # specified as '. = ' - # (this is for legacy config files only) - for k, v in list(cfgdict.items()): # keep list() as dict may change during iter - if k[0] == '.': - if k[1:] in self.propertyDict: - self.setProperty(k[1:], cfgdict.pop(k)) - else: - raise ConfigError('Module %r has no property %r' % - (self.name, k[1:])) + # 2) check and apply properties specified in cfgdict as + # ' = ' + for key in self.propertyDict: + value = cfgdict.pop(key, None) + if value is None: + # legacy cfg: specified as '. = ' + value = cfgdict.pop('.' + key, None) + if value is not None: + try: + self.setProperty(key, value) + except BadValueError: + errors.append('module %s, %s: value %r does not match %r!' % + (name, key, value, self.propertyDict[key].datatype)) - # 3) check and apply properties specified in cfgdict as - # ' = ' (without '.' prefix) - for k in self.propertyDict: - if k in cfgdict: - self.setProperty(k, cfgdict.pop(k)) - - # 4) set automatic properties + # 3) set automatic properties mycls = self.__class__ myclassname = '%s.%s' % (mycls.__module__, mycls.__name__) self.implementation = myclassname @@ -293,16 +291,6 @@ class Module(HasAccessibles): if not self.export: # do not export parameters of a module not exported aobj.export = False if aobj.export: - if aobj.export is True: - predefined_obj = PREDEFINED_ACCESSIBLES.get(aname, None) - if predefined_obj: - if isinstance(aobj, predefined_obj): - aobj.export = aname - else: - raise ProgrammingError("can not use '%s' as name of a %s" % - (aname, aobj.__class__.__name__)) - else: # create custom parameter - aobj.export = '_' + aname accessiblename2attr[aobj.export] = aname accessibles[aname] = aobj # do not re-use self.accessibles as this is the same for all instances @@ -317,29 +305,31 @@ class Module(HasAccessibles): for k, v in list(cfgdict.items()): # keep list() as dict may change during iter if '.' in k[1:]: paramname, propname = k.split('.', 1) + propvalue = cfgdict.pop(k) paramobj = self.accessibles.get(paramname, None) # paramobj might also be a command (not sure if this is needed) if paramobj: if propname == 'datatype': - paramobj.setProperty('datatype', get_datatype(cfgdict.pop(k), k)) - elif propname in paramobj.getProperties(): - paramobj.setProperty(propname, cfgdict.pop(k)) - else: - raise ConfigError('Module %s: Parameter %r has no property %r!' % - (self.name, paramname, propname)) + propvalue = get_datatype(propvalue, k) + try: + paramobj.setProperty(propname, propvalue) + except KeyError: + errors.append('module %s: %s.%s does not exist' % + (self.name, paramname, propname)) + except BadValueError as e: + errors.append('module %s: %s.%s: %s' % + (self.name, paramname, propname, str(e))) else: - raise ConfigError('Module %s has no Parameter %r!' % - (self.name, paramname)) + errors.append('module %s: %s not found' % (self.name, paramname)) # 3) check config for problems: # only accept remaining config items specified in parameters - for k, v in cfgdict.items(): - if k not in self.parameters: - raise ConfigError( - 'Module %s:config Parameter %r ' - 'not understood! (use one of %s)' % - (self.name, k, ', '.join(list(self.parameters) + - list(self.propertyDict)))) + bad = [k for k in cfgdict if k not in self.parameters] + if bad: + errors.append( + 'module %s: %s does not exist (use one of %s)' % + (self.name, ', '.join(bad), ', '.join(list(self.parameters) + + list(self.propertyDict)))) # 4) complain if a Parameter entry has no default value and # is not specified in cfgdict and deal with parameters to be written. @@ -354,15 +344,15 @@ class Module(HasAccessibles): # TODO: not sure about readonly (why not a parameter which can only be written from config?) try: pobj.value = pobj.datatype(cfgdict[pname]) + self.writeDict[pname] = pobj.value except BadValueError as e: - raise ConfigError('%s.%s: %s' % (name, pname, e)) - self.writeDict[pname] = pobj.value + errors.append('module %s, parameter %s: %s' % (name, pname, e)) else: if pobj.default is None: if pobj.needscfg: - raise ConfigError('Parameter %s.%s has no default ' - 'value and was not given in config!' % - (self.name, pname)) + errors.append('module %s, parameter %s has no default ' + 'value and was not given in config!' % + (self.name, pname)) # we do not want to call the setter for this parameter for now, # this should happen on the first read pobj.readerror = ConfigError('not initialized') @@ -373,8 +363,8 @@ class Module(HasAccessibles): try: value = pobj.datatype(pobj.default) except BadValueError as e: - raise ProgrammingError('bad default for %s.%s: %s' - % (name, pname, e)) + # this should not happen, as the default is already checked in Parameter + raise ProgrammingError('bad default for %s:%s: %s' % (name, pname, e)) from None if pobj.initwrite and not pobj.readonly: # we will need to call write_ # if this is not desired, the default must not be given @@ -392,10 +382,8 @@ class Module(HasAccessibles): # note: this will NOT call write_* methods! setattr(self, k, v) except (ValueError, TypeError): - self.log.exception(formatExtendedStack()) - raise - # raise ConfigError('Module %s: config parameter %r:\n%r' % - # (self.name, k, e)) + # self.log.exception(formatExtendedStack()) + errors.append('module %s, parameter %s: %s' % (self.name, k, e)) cfgdict.pop(k) # Modify units AFTER applying the cfgdict @@ -405,9 +393,18 @@ class Module(HasAccessibles): dt.setProperty('unit', dt.unit.replace('$', self.parameters['value'].datatype.unit)) # 6) check complete configuration of * properties - self.checkProperties() - for p in self.parameters.values(): - p.checkProperties() + if not errors: + try: + self.checkProperties() + except ConfigError as e: + errors.append('module %s: %s' % (name, e)) + for pname, p in self.parameters.items(): + try: + p.checkProperties() + except ConfigError: + errors.append('module %s, parameter %s: %s' % (name, pname, e)) + if errors: + raise ConfigError('\n'.join(errors)) # helper cfg-editor def __iter__(self): diff --git a/secop/params.py b/secop/params.py index bae2011..831505e 100644 --- a/secop/params.py +++ b/secop/params.py @@ -225,10 +225,13 @@ class Parameter(Accessible): self.propertyValues.pop('default') if self.export is True: - if isinstance(self, PREDEFINED_ACCESSIBLES.get(name, type(None))): + predefined_cls = PREDEFINED_ACCESSIBLES.get(name, None) + if predefined_cls is Parameter: self.export = name - else: + elif predefined_cls is None: self.export = '_' + name + else: + raise ProgrammingError('can not use %r as name of a Parameter' % name) def copy(self): # deep copy, as datatype might be altered from config @@ -339,10 +342,13 @@ class Command(Accessible): self.datatype = CommandType(self.argument, self.result) if self.export is True: - if isinstance(self, PREDEFINED_ACCESSIBLES.get(name, type(None))): + predefined_cls = PREDEFINED_ACCESSIBLES.get(name, None) + if predefined_cls is Command: self.export = name - else: + elif predefined_cls is None: self.export = '_' + name + else: + raise ProgrammingError('can not use %r as name of a Command' % name) def __get__(self, obj, owner=None): if obj is None: diff --git a/secop/properties.py b/secop/properties.py index 36382ce..edfd5b0 100644 --- a/secop/properties.py +++ b/secop/properties.py @@ -158,7 +158,7 @@ class HasProperties(HasDescriptors): cls.propertyDict = properties # treat overriding properties with bare values for pn, po in properties.items(): - value = cls.__dict__.get(pn, po) + value = getattr(cls, pn, po) if not isinstance(value, Property): # attribute is a bare value po = Property(**po.__dict__) try: @@ -177,11 +177,11 @@ class HasProperties(HasDescriptors): """validates properties and checks for min... <= max...""" for pn, po in self.propertyDict.items(): if po.mandatory: - if pn not in self.propertyDict: + try: + self.propertyValues[pn] = po.datatype(self.propertyValues[pn]) + except (KeyError, BadValueError) as e: name = getattr(self, 'name', self.__class__.__name__) - raise ConfigError('Property %r of %s needs a value of type %r!' % (pn, name, po.datatype)) - # apply validator (which may complain further) - self.propertyValues[pn] = po.datatype(self.propertyValues[pn]) + raise ConfigError('%s.%s needs a value of type %r!' % (name, pn, po.datatype)) for pn, po in self.propertyDict.items(): if pn.startswith('min'): maxname = 'max' + pn[3:] diff --git a/secop/server.py b/secop/server.py index cde2ff8..2ccad9a 100644 --- a/secop/server.py +++ b/secop/server.py @@ -26,11 +26,12 @@ import ast import configparser import os +import sys import threading import time from collections import OrderedDict -from secop.errors import ConfigError +from secop.errors import ConfigError, SECoPError from secop.lib import formatException, get_class, getGeneralConfig from secop.modules import Attached from secop.params import PREDEFINED_ACCESSIBLES @@ -179,8 +180,8 @@ class Server: self.run() def unknown_options(self, cls, options): - raise ConfigError("%s class don't know how to handle option(s): %s" % - (cls.__name__, ', '.join(options))) + return ("%s class don't know how to handle option(s): %s" % + (cls.__name__, ', '.join(options))) def run(self): while self._restart: @@ -201,7 +202,7 @@ class Server: cls = get_class(self.INTERFACES[scheme]) with cls(scheme, self.log.getChild(scheme), opts, self) as self.interface: if opts: - self.unknown_options(cls, opts) + raise ConfigError(self.unknown_options(cls, opts)) self.log.info('startup done, handling transport messages') if systemd: systemd.daemon.notify("READY=1\nSTATUS=accepting requests") @@ -219,16 +220,26 @@ class Server: self.interface.shutdown() def _processCfg(self): + errors = [] opts = dict(self.node_cfg) cls = get_class(opts.pop('class', 'protocol.dispatcher.Dispatcher')) self.dispatcher = cls(opts.pop('name', self._cfgfiles), self.log.getChild('dispatcher'), opts, self) if opts: - self.unknown_options(cls, opts) + errors.append(self.unknown_options(cls, opts)) self.modules = OrderedDict() + badclass = None + self.lastError = None for modname, options in self.module_cfg.items(): opts = dict(options) - cls = get_class(opts.pop('class')) - modobj = cls(modname, self.log.getChild(modname), opts, self) + try: + classname = opts.pop('class') + cls = get_class(classname) + modobj = cls(modname, self.log.getChild(modname), opts, self) + except ConfigError as e: + errors.append(str(e)) + except Exception as e: + badclass = classname + errors.append('error while loading %s' % badclass) # all used args should be popped from opts! if opts: self.unknown_options(cls, opts) @@ -249,12 +260,28 @@ class Server: for modname, modobj in self.modules.items(): for propname, propobj in modobj.propertyDict.items(): if isinstance(propobj, Attached): - setattr(modobj, propobj.attrname or '_' + propname, - self.dispatcher.get_module(getattr(modobj, propname))) + try: + setattr(modobj, propobj.attrname or '_' + propname, + self.dispatcher.get_module(getattr(modobj, propname))) + except SECoPError as e: + errors.append('module %s, attached %s: %s' % (modname, propname, str(e))) + # call init on each module after registering all for modname, modobj in self.modules.items(): modobj.initModule() + if errors: + for errtxt in errors: + for line in errtxt.split('\n'): + self.log.error(line) + # print a list of config errors to stderr + sys.stderr.write('\n'.join(errors)) + sys.stderr.write('\n') + if badclass: + # force stack trace for import of last erroneous module + get_class(badclass) + sys.exit(1) + if self._testonly: return start_events = []