Validate user configuration entries

[rany2]

Closes #207

[rany2]

@lynxthecat Thoughts on this?

[lynxthecat]

Might it be worth wrapping up the config file validation into a function?

[rany2]

It's never getting reused though, seems unnecessary

[rany2]

Might be worth changing some of the errors returned for config parsing failures, like log_msg "ERROR" "Config file error. Please check config file entries."

[lynxthecat]

I like your changes to cleanup_and_killall() in respect of the set +e and intercept_stderr. I didn't think of that issue.

[rany2]

@lynxthecat does the config_file_check variable still make sense?

[lynxthecat]

Ah that's clever - so you're checking all config value sets in the config against corresponding entries in defaults.sh to ensure any provided option is a valid option?

[rany2]

Ah that's clever - so you're checking all config value sets in the config against corresponding entries in defaults.sh to ensure any provided option is a valid option?

Pretty much

[rany2]

@lynxthecat does the config_file_check variable still make sense?

P.S. do you think it should be removed? It doesn't really make sense IMO. We have a defaults.sh that ensures that all variables the script relies on are initialized.

[rany2]

I removed it, if you think it should stay I could drop that commit from my branch before you merge.

[lynxthecat]

Ah yes - seems to make sense to remove it.

[lynxthecat]

So far so good:

root@OpenWrt-1:~/cake-autorate# ./cake-autorate.sh
ERROR; 2023-07-04-16:30:25; 1688484625.526007; The value: 'string' in config file: '/root/cake-autorate/config.primary.sh' is not a valid config entry.
ERROR; 2023-07-04-16:30:25; 1688484625.530927; The config file: '/root/cake-autorate/config.primary.sh' contains errors. Please address them. Exiting now.
DEBUG; 2023-07-04-16:30:25; 1688484625.535466; Starting: cleanup_and_killall with PID: 7278
INFO; 2023-07-04-16:30:25; 1688484625.536791; Stopping cake-autorate with PID: 7278 and config: /root/cake-autorate/config.primary.sh
INFO; 2023-07-04-16:30:25; 1688484625.538115; Killing all background processes and cleaning up temporary files.
SYSLOG; 2023-07-04-16:30:26; 1688484626.546020; Stopped cake-autorate with PID: 7278 and config: /root/cake-autorate/config.primary.sh

[lynxthecat]

@rany2 could we check that values have the same type as in the default? So e.g. where defaults is an integer, the config value must also be an integer. Where defaults is blank, config entry can be blank. Or to simplify we could just check for a non-empty value for all entries save for ping_extra_args and ping_prefix_string?

[rany2]

@lynxthecat Sure just give a few moments

[rany2]

@lynxthecat You could test in this state now

[rany2]

Things you might think are missing but are actually not:

• Verify that a number is actually octal or not (Bash does not consider a 0 being preceded in a number to be octal)
• Better float regex, technically Bash does not have the concept of a float. It's only there so that we could in future warn user that floats are not supported and provide better errors.

Things that are actually missing:

• Validation of maps/arrays. The type of the array content itself is not validated, however the fact that it is a map/array is considered regardless.
• I can't think of anything else off the top of my head right now.

Things left out on purpose:

• Numbers are not considered to be negative to simplify validation, but technically bash accepts them as numbers and could do arithmetic on them. They would be considered strings by the validator

Edit: all points addressed here except validation of maps/array content

[rany2]

ERROR; 2023-07-04-22:02:52; 1688508172.051024; The value: 'dl_if' in config file: '/root/cake-autorate/config.primary.sh' is not a valid string value. Also, negative numbers are not supported.
ERROR; 2023-07-04-22:02:52; 1688508172.156770; The value: 'min_dl_shaper_rate_kbps' in config file: '/root/cake-autorate/config.primary.sh' is not a valid number value.
ERROR; 2023-07-04-22:02:52; 1688508172.199717; The config file: '/root/cake-autorate/config.primary.sh' contains one or more errors. Exiting now.
DEBUG; 2023-07-04-22:02:52; 1688508172.203309; Starting: cleanup_and_killall with PID: 51950
INFO; 2023-07-04-22:02:52; 1688508172.203607; Stopping cake-autorate with PID: 51950 and config: /root/cake-autorate/config.primary.sh
INFO; 2023-07-04-22:02:52; 1688508172.203869; Killing all background processes and cleaning up temporary files.
SYSLOG; 2023-07-04-22:02:53; 1688508173.208124; Stopped cake-autorate with PID: 51950 and config: /root/cake-autorate/config.primary.sh
Killing all instances of cake one-by-one now.

Could we get rid of the unix time stamp from the output and reduce the debug level by default? It looks really cluttered with the default configuration. It seems difficult to read when an error occurs.

[lynxthecat]

First of all - wow. Let me catch up on this!

Could we get rid of the unix time stamp from the output and reduce the debug level by default? It looks really cluttered with the default configuration. It seems difficult to read when an error occurs.

I agree with you that the output of cake-autorate looks a little cluttered. For a while I implemented padding, but once the columns grew to a certain extent it no longer helped so much. A consideration is that we have a couple of external plotting programs (e.g. this and this super cool prometheus exporter that @bairhys wrote) that expect a certain format. I'm reluctant to remove the timestamp because so often timings are critical when checking control aspects like the bufferbloat refractory index. Reducing the default debug level has the drawback that when a user first tries cake-autorate and it does not work then they will not know why. In the end the output is only normally something you look at following problems or for offline analysis of data in a spreadsheet or one of the plotting tools.

Any thoughts on the above?

[lynxthecat]

Your typeof() is super cool:

root@OpenWrt-1:~# x=()
root@OpenWrt-1:~# printf "$(typeof x) \n"
array
root@OpenWrt-1:~# x=0
root@OpenWrt-1:~# printf "$(typeof x) \n"
array
root@OpenWrt-1:~# unset x
root@OpenWrt-1:~# x=0
root@OpenWrt-1:~# printf "$(typeof x) \n"
number
root@OpenWrt-1:~# x=0.2
root@OpenWrt-1:~# printf "$(typeof x) \n"
float
root@OpenWrt-1:~# x="string"
root@OpenWrt-1:~# printf "$(typeof x) \n"
string
root@OpenWrt-1:~# declare -A x
root@OpenWrt-1:~# printf "$(typeof x) \n"
map

[lynxthecat]

@rany2 one issue is that with some of the configuration elements can be set to either a number or a float, and that mismatch can give unnecessary errors. But perhaps the way round this is just to ensure that the defaults are floats where floats are supported.

[lynxthecat]

Looks good. I'll retest shortly. This is a fantastic addition by the way.

[lynxthecat]

All seems to be working well for me.

Should we handle entries that are not comments or assignments like the word 'exit' or is that completely unnecessary?

[lynxthecat]

What's the reason for the unset?

[rany2]

Should we handle entries that are not comments or assignments like the word 'exit' or is that completely unnecessary?

Against that because users might want to define functions that could be used to assign to the variables; and if someone defined something like exit. It would be obvious something is broken. This is mostly to prevent issues where cake-autorate silently starts behaving improperly but doesn't provide feedback that it is because the config is broken

[lynxthecat]

I think we're good to merge all of this. I can't think of anything else now. What do you think?

[rany2]

@lynxthecat Guess so, go for it. We could later add better validation for array contents; should be simplish.

[rany2]

What's the reason for the unset?

Consider this, sorry I didn't respond sooner. My email is really cluttered.

$ x=()
$ declare -p x
declare -a x=()
$ x=0
$ declare -p x
declare -a x=([0]="0")
$ unset x; x=0
$ declare -p x
declare -- x="0"

[lynxthecat]

Thanks @rany2. Could I run something past you on IRC?