Improve handling of longer PV names

[eddybl]

autosave defines a PV_NAME_LEN for the maximum allowed length of PVs. But there are various locations where the code uses other hard coded limits, which is sometimes 80 and sometimes 63.

• The first commit of this PR improves that and uses PV_NAME_LEN more consistently.
• The second commit then simply raises this number to 256

Background

We are using a slightly patched version of EPICS base, which increases the PV length limit from 61 to 256. While the patch itself is very small, only changing two lines of code ( https://gitlab.kit.edu/kit/ibpt/controls/epics/distribution/epics-build-tool/-/blob/master/customizations/ibpt/patches/base/01-record-name-length.patch ), autosave is not compatible to that. We therefore use this patched version of autosave since around 2019.

At least one other institute recently also started using our EPICS base patch and were encountering the same issue with autosave, so the idea came up to submit the patch upstream. The currently used number for PV_NAME_LEN seemed to be arbitrary anyway, so we could as well define a higher number ;)

[anjohnson]

The epics-base header file dbDefs.h has long defined a macro PVNAME_STRINGSZ with the size of the dbCommon::name member (currently 61). In fact I see one use of it in the verify.c file below.

The upstream macro isn't actually used in the generated *Record.h files but any change we make to the header would always be reflected in the dbCommon.dbd.pod file and vice versa. Using that upstream definition instead of defining your own would maintain backwards compatibility with other Base versions; if you need to add some margin then I would suggest keeping this macro and setting it from that.

[eddybl]

I now looked into why/when PV_NAME_LEN was changed to 80, and it happened back in 2005 in this commit: 5c4a1a4

To me it seems like there is no specific reason to change it to 80, it was probably just about making it larger than 40.

PVNAME_STRINGSZ is also the variable we patch in our epics base batch and you suggest to use that directly from the EPICS base header file? THat would probably make it even easier for us, as it would not require to patch autosave at all anymore?

Is PVNAME_STRINGSZ already available based on the current includes or would I need to add a specific new include? Should I in this case include dbDefs.h directly or is there a more generic include which will also provide the variable?

It probably would not actually make a difference in practice, but switching to PVNAME_STRINGSZ would reduce the default allowed PV length compared to the current version of autosave... not sure if that should be considered a breaking change.

[Monarda]

Inelegant, but avoiding a potentially breaking change is:

#define PV_NAME_LEN ((PVNAME_STRINGSZ>80) ? (PVNAME_STRINGSZ) : (80)) /* string containing a PV name */

This compiles (I haven't done any further tests) which implies that PVNAME_STRINGSZ is available using the current includes.

[eddybl]

I used Ivans suggestion and removed the one location where it was directly used by now also using PV_NAME_LEN

[eddybl]

Actually, now it is broken. Removing the definition of PVNAME_STRINGSZ from verfiy.c now seems to show, that PVNAME_STRINGSZ is actually not included from EPICS base itself?

[Monarda]

Apologies, yes, you're quite right. Using my version of the #define without including dbDefs.h just falls through to the #define PVNAME_STRINGSZ 61 in verify.c, as you say.

[eddybl]

I reworked the second commit, properly inlcuded dbDefs.h and it should now use the central EPICS base definition for PV length, but for backwards compability falls back to 80, if PV length is smaller than 80

[eddybl]

Actually I decided to use PVNAME_SZ instead of PVNAME_STRINGSZ as in most places + 1 is being added to PV_NAME_LEN and this is the string length without the NUL terminator.

But now I am not sure if the usage is actually consistent across all other uses (which I have not changed)... Could use some help here in judging that...

• save_restore.c uses 11 times PV_NAME_LEN - 1
• but it also defines PV_NAME_LEN + 1 in some definitions

[eddybl]

probably should add tests for that, but I don't feel confident enough to start with tests from scratch, as autosave does not seem to have tests for far...