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
+ fixed race condition in writeInitParams

Change-Id: I894bda291ab85ccec3d771c4903393c808af0a2a
Reviewed-on: https://forge.frm2.tum.de/review/c/sine2020/secop/playground/+/25128
Tested-by: Jenkins Automated Tests <pedersen+jenkins@frm2.tum.de>
Reviewed-by: Enrico Faulhaber <enrico.faulhaber@frm2.tum.de>
Reviewed-by: Markus Zolliker <markus.zolliker@psi.ch>

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
+        # 2) check and apply properties specified in cfgdict as
-        #    specified as '.<propertyname> = <propertyvalue>'
+        # '<propertyname> = <propertyvalue>'
-        #    (this is for legacy config files only)
+        for key in self.propertyDict:
-        for k, v in list(cfgdict.items()):  # keep list() as dict may change during iter
+            value = cfgdict.pop(key, None)
-            if k[0] == '.':
+            if value is None:
-                if k[1:] in self.propertyDict:
+                # legacy cfg: specified as '.<propertyname> = <propertyvalue>'
-                    self.setProperty(k[1:], cfgdict.pop(k))
+                value = cfgdict.pop('.' + key, None)
-                else:
+            if value is not None:
-                    raise ConfigError('Module %r has no property %r' %
+                try:
-                                      (self.name, k[1:]))
+                    self.setProperty(key, value)
+                except BadValueError:
+                    errors.append('%s: value %r does not match %r!' %
+                                  (key, value, self.propertyDict[key].datatype))
 
-        # 3) check and apply properties specified in cfgdict as
+        # 3) set automatic properties
-        #    '<propertyname> = <propertyvalue>' (without '.' prefix)
-        for k in self.propertyDict:
-            if k in cfgdict:
-                self.setProperty(k, cfgdict.pop(k))
-
-        # 4) 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))
+                        propvalue = get_datatype(propvalue, k)
-                    elif propname in paramobj.getProperties():
+                    try:
-                        paramobj.setProperty(propname, cfgdict.pop(k))
+                        paramobj.setProperty(propname, propvalue)
-                    else:
+                    except KeyError:
-                        raise ConfigError('Module %s: Parameter %r has no property %r!' %
+                        errors.append("'%s.%s' does not exist" %
-                                          (self.name, paramname, propname))
+                                      (paramname, propname))
+                    except BadValueError as e:
+                        errors.append('%s.%s: %s' %
+                                      (paramname, propname, str(e)))
                 else:
-                    raise ConfigError('Module %s has no Parameter %r!' %
+                    errors.append('%r not found' % paramname)
-                                      (self.name, paramname))
 
         # 3) check config for problems:
         #    only accept remaining config items specified in parameters
-        for k, v in cfgdict.items():
+        bad = [k for k in cfgdict if k not in self.parameters]
-            if k not in self.parameters:
+        if bad:
-                raise ConfigError(
+            errors.append(
-                    'Module %s:config Parameter %r '
+                '%s does not exist (use one of %s)' %
-                    'not understood! (use one of %s)' %
+                (', '.join(bad), ', '.join(list(self.parameters) +
-                    (self.name, k, ', '.join(list(self.parameters) +
+                                                      list(self.propertyDict))))
-                                             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,14 @@ 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))
+                        errors.append('%s: %s' % (pname, e))
-                    self.writeDict[pname] = pobj.value
             else:
                 if pobj.default is None:
                     if pobj.needscfg:
-                        raise ConfigError('Parameter %s.%s has no default '
+                        errors.append('%r has no default '
-                                          'value and was not given in config!' %
+                                      'value and was not given in config!' % pname)
-                                          (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 +362,8 @@ class Module(HasAccessibles):
                     try:
                         value = pobj.datatype(pobj.default)
                     except BadValueError as e:
-                        raise ProgrammingError('bad default for %s.%s: %s'
+                        # this should not happen, as the default is already checked in Parameter
-                                               % (name, pname, e))
+                        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_<pname>
                         # if this is not desired, the default must not be given
@@ -390,13 +379,12 @@ class Module(HasAccessibles):
             try:
                 # this checks also for the proper datatype
                 # note: this will NOT call write_* methods!
-                setattr(self, k, v)
+                if k in self.parameters or k in self.propertyDict:
+                    setattr(self, k, v)
+                    cfgdict.pop(k)
             except (ValueError, TypeError):
-                self.log.exception(formatExtendedStack())
+                # self.log.exception(formatExtendedStack())
-                raise
+                errors.append('module %s, parameter %s: %s' % (self.name, k, e))
-                # raise ConfigError('Module %s: config parameter %r:\n%r' %
-                #                   (self.name, k, e))
-            cfgdict.pop(k)
 
         # Modify units AFTER applying the cfgdict
         for k, v in self.parameters.items():
@@ -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()
+        if not errors:
-        for p in self.parameters.values():
+            try:
-            p.checkProperties()
+                self.checkProperties()
+            except ConfigError as e:
+                errors.append(str(e))
+            for pname, p in self.parameters.items():
+                try:
+                    p.checkProperties()
+                except ConfigError:
+                    errors.append('%s: %s' % (pname, e))
+        if errors:
+            raise ConfigError(errors)
 
     # helper cfg-editor
     def __iter__(self):
@@ -506,7 +503,7 @@ class Module(HasAccessibles):
     def pollOneParam(self, pname):
         """poll parameter <pname> with proper error handling"""
         try:
-            return getattr(self, 'read_' + pname)()
+            getattr(self, 'read_' + pname)()
         except SilentError:
             pass
         except SECoPError as e:
@@ -521,10 +518,12 @@ class Module(HasAccessibles):
         with proper error handling
         """
         for pname in list(self.writeDict):
-            if pname in self.writeDict:  # this might not be true with handlers
+            value = self.writeDict.pop(pname, Done)
+            # in the mean time, a poller or handler might already have done it
+            if value is not Done:
                 try:
                     self.log.debug('initialize parameter %s', pname)
-                    getattr(self, 'write_' + pname)(self.writeDict.pop(pname))
+                    getattr(self, 'write_' + pname)(value)
                 except SilentError:
                     pass
                 except SECoPError as e:

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:

secop/properties.py

@@ -134,7 +134,7 @@ class HasProperties(HasDescriptors):
     propertyValues = None
 
     def __init__(self):
-        super(HasProperties, self).__init__()
+        super().__init__()
         # store property values in the instance, keep descriptors on the class
         self.propertyValues = {}
         # pre-init
@@ -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:
@@ -168,20 +168,20 @@ class HasProperties(HasDescriptors):
                         if pn in getattr(base, 'propertyDict', {}):
                             if callable(value):
                                 raise ProgrammingError('method %s.%s collides with property of %s' %
-                                                       (cls.__name__, pn, base.__name__))
+                                                       (cls.__name__, pn, base.__name__)) from None
                             raise ProgrammingError('can not set property %s.%s to %r' %
-                                                   (cls.__name__, pn, value))
+                                                   (cls.__name__, pn, value)) from None
                 cls.propertyDict[pn] = po
 
     def checkProperties(self):
         """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):
                     name = getattr(self, 'name', self.__class__.__name__)
-                    raise ConfigError('Property %r of %s needs a value of type %r!' % (pn, name, po.datatype))
+                    raise ConfigError('%s.%s needs a value of type %r!' % (name, pn, po.datatype)) from None
-                # apply validator (which may complain further)
-                self.propertyValues[pn] = po.datatype(self.propertyValues[pn])
         for pn, po in self.propertyDict.items():
             if pn.startswith('min'):
                 maxname = 'max' + pn[3:]

secop/server.py

@@ -26,11 +26,13 @@
 import ast
 import configparser
 import os
+import sys
 import threading
 import time
+import traceback
 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 +181,8 @@ class Server:
             self.run()
 
     def unknown_options(self, cls, options):
-        raise ConfigError("%s class don't know how to handle option(s): %s" %
+        return ("%s class don't know how to handle option(s): %s" %
-                          (cls.__name__, ', '.join(options)))
+                (cls.__name__, ', '.join(options)))
 
     def run(self):
         while self._restart:
@@ -201,7 +203,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,20 +221,46 @@ 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()
+        failure_traceback = None  # traceback for the last error
+        failed = set()  # python modules failed to load
+        self.lastError = None
         for modname, options in self.module_cfg.items():
             opts = dict(options)
-            cls = get_class(opts.pop('class'))
+            pymodule = None
-            modobj = cls(modname, self.log.getChild(modname), opts, self)
+            try:
-            # all used args should be popped from opts!
+                classname = opts.pop('class')
-            if opts:
+                pymodule = classname.rpartition('.')[0]
-                self.unknown_options(cls, opts)
+                if pymodule in failed:
-            self.modules[modname] = modobj
+                    continue
+                cls = get_class(classname)
+            except Exception as e:
+                if str(e) == 'no such class':
+                    errors.append('%s not found' % classname)
+                else:
+                    failed.add(pymodule)
+                    failure_traceback = traceback.format_exc()
+                    errors.append('error importing %s' % classname)
+            else:
+                try:
+                    modobj = cls(modname, self.log.getChild(modname), opts, self)
+                    # all used args should be popped from opts!
+                    if opts:
+                        errors.append(self.unknown_options(cls, opts))
+                    self.modules[modname] = modobj
+                except ConfigError as e:
+                    errors.append('error creating module %s:' % modname)
+                    for errtxt in e.args[0]:
+                        errors.append('  ' + errtxt)
+                except Exception:
+                    failure_traceback = traceback.format_exc()
+                    errors.append('error creating %s' % modname)
 
         poll_table = dict()
         # all objs created, now start them up and interconnect
@@ -249,11 +277,30 @@ 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,
+                    try:
-                            self.dispatcher.get_module(getattr(modobj, propname)))
+                        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()
+            try:
+                modobj.initModule()
+            except Exception as e:
+                failure_traceback = traceback.format_exc()
+                errors.append('error initializing %s: %r' % (modname, e))
+
+        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 failure_traceback:
+                sys.stderr.write(failure_traceback)
+            sys.exit(1)
 
         if self._testonly:
             return