Hi Neels,
On Wed, Oct 04, 2017 at 12:48:48AM +0200, Neels Hofmeyr wrote:
On Tue, Oct 03, 2017 at 08:49:04AM +0800, Harald Welte wrote:
- CTRL uses '.' to separate individual elements/nodes of the command
- a lot of rate_ctr groups we use so far use '.' in their group names (e.g. 'bssgp.bss_ctx') and counter names (e.g. 'tx.bytes')
- This makes the counters un-exportable via CTRL
I'm thinking, we could make the current rate_ctr_group_alloc() replace '.' with ':' automatically to fix all older and current builds' CTRL for rate_ctr.
Has been implemented in https://gerrit.osmocom.org/#/c/4131
At the same time we can deprecate rarate_ctr_group_alloc() and provide rate_ctr_group_alloc2(), which then aborts the program when rate_ctr names don't adhere to strict identifier rules. Thus we get a compile-time warning for applications that haven't reviewed correctness of rate_ctr naming ("rate_ctr_group_alloc() is deprecated...") and once an application has moved on we make sure no new non-compliant names are introduced. However, newly introduced names will get checked only during run time.
I think this is too much effort. We can easily grep through all our applications and replace the names without missing something. External (non-Osmocom) users have never approached us with any feedback, so I doubt they even exist.
Another addition could be a script like verify_value_string_arrays_are_terminated.py but for struct rate_ctr_group_desc arrays' identifier adherence, to verify even before compilation.
Also probably overkill? I mean, an unterminated value_string_array will end up in a bug. But a counter name with '.' will only result in slight increased memory use (once, at startup, when doing the . -> : conversion).
Should we further restrict the CTRL interface strings (and those of systems exporting to CTRL) to standard US 7-bit ASCII with a limited set of special characters such as ":-_@" but prevent any non-printable chars
For identifier validation, we should probably include that identifiers must not start with a number.
Why is that?
I think we could also include ",~+^"
In the permitted characters? I'm not sure, I think permitting too many special characters just constrains what we can do in terms of core syntax of interfaces like CTRL later on...
valid = isalpha(str[0]) && each(str[1..len] as c){ isalnum(c) || c in ":-_@,~+^" }
My current proposal is in https://gerrit.osmocom.org/#/c/4128
Losely related is the OsmoHLR idea for CTRL interface to use names like
SET subscriber.by-imsi.1234567898765.ps-enabled 1
where a CTRL identifier name is used for inputting an IMSI string. If we do the same identifier checking during incoming CTRL interface commands, we would forbid the IMSI name because it starts with a number.
I don't think there's something wrong with that. Why no number at first digit?