epics-base / p4p

Python bindings for the PVAccess network client and server.
BSD 3-Clause "New" or "Revised" License
27 stars 38 forks source link

Warn instead of raise exc. for invalid pvlist patterns #58

Closed bhill-slac closed 3 years ago

bhill-slac commented 3 years ago

Needed to ensure typos in the frequently updated pvlist patterns do not kill the gateway. Also log the line number and invalid content so users can find and fix problems.

We have many people updating pvlist files for beamlines and while a typo or syntax error may not accomplish what the person wanted, it shouldn't bring down the gateway.

mdavidsaver commented 3 years ago

I certainly see that the error message needs improving.

I'm not sure I like the idea of treating a syntax error in a config file as a warning though.

I had a similar problem with the (xml) config files for the old channelarchiver. At the time I added a syntax check step when users were checking in changes. Maybe a better way to handle this is to add an argument to exit after parsing the config files, but before actually starting a gateway? Maybe -T (like sshd).

bhill-slac commented 3 years ago

I'm ok w/ it calling log.error instead of log.warning as long as it continues and allows the gateway to run. I also like the idea of adding a way to check syntax w/o interfering w/ a running gateway. There should also be a way to reread the access security and pvlist files w/o shutting down the gateway. CA gateway supports this which allows us to add access to needed PVs w/o interrupting active connections. I'll submit that as another issue.

bhill-slac commented 3 years ago

Tested w/ the issue I've seen before which is someone starting a pvlist pattern w/ '*' instead of '.*'. Only difference is ERROR instead of WARNING in the pva log file. ERROR:p4p.asLib.pvlist:line 8: nothing to repeat at position 0, pattern *TST:GIGE:.* Note that it would be nice to also provide the file path for the pvlist file but that's not available in pvlist.py.

mdavidsaver commented 3 years ago

After some consideration, I think that config file syntax errors need to remain hard errors (as in exit(1)). So I can't accept this change. An example of what motivates my thinking is:

SYS:.* ALLOW
SYS:SENSITIVE:.* DNY

Where omitting the negative match results in a gateway which is more permissive than intended.

I'm working on adding a new argument -T/--test-config which will work as I described above. -T will also print the names of all config files read to eg. help a deployment script copy a set of validated files. Something like:

#!/bin/sh
set -e -x

cp -v $(pvagw -T ${1}.conf | xargs echo) deploy/

systemctl restart pvagw-${1}.service
mdavidsaver commented 3 years ago

23b08e08408ad5bbade112f9913fffeb42449348 Adds the -T argument.