Hi,

Am Sa, 15. Jul, 2017 um 2:53 schrieb Michael Tremer <michael.tremer@ipfire.org>:
Hello, On Fri, 2017-07-14 at 21:10 +0200, Jonatan Schlag wrote:
Basically we just read in the config, parsing the passed command line options (so we set the new value for he passed options) and writing the config back. Signed-off-by: Jonatan Schlag <jonatan.schlag@ipfire.org> ---  src/hooks/configs/dhcp | 40 ++++++++++++++++++++++++++++++++++++++++  1 file changed, 40 insertions(+) diff --git a/src/hooks/configs/dhcp b/src/hooks/configs/dhcp index 8bb6aa9..98ef5e3 100644 --- a/src/hooks/configs/dhcp +++ b/src/hooks/configs/dhcp @@ -83,6 +83,46 @@ hook_new() {   exit ${EXIT_OK}  }   +hook_edit() { + local zone=${1} + local config=${2} + shift 2 + + if ! device_exists ${zone}; then + error "Zone '${zone}' doesn't exist." + exit ${EXIT_ERROR} + fi
I think you want to call zone_exists here. device_exists only returns true when an interface exists with that name which would result into the hook not being editable when the zone is down. That's bad. I am quite sure that this is checked before so that the hook can rely on the calling zone existing. No need to do this here again.

I would not be so sure if device_exist is called before but i will have a look at it.

+ + local ${HOOK_CONFIG_SETTINGS} + + # If reading the config fails we cannot go on + if ! zone_config_settings_read "${zone}" "${config}"; then + log ERROR "Could read the config ${config} for zone ${upl0}" + return ${EXIT_ERROR} + fi + + if ! hook_parse_cmdline $@; then + # Return an error if the parsing of the cmd line fails + return ${EXIT_ERROR} + fi + + # Check if the user disabled ipv4 and ipv6 + if ! enabled ENABLE_IPV6 && ! enabled ENABLE_IPV4; then + log ERROR "You disabled IPv6 and IPv4. At least one must be enabled" + return ${EXIT_ERROR} + fi
Should this check not be in the hook_parse_cmdline function since it is needed when the config is created, too. We don't want duplication and it would be nice if we could have a generic hook_edit() function when hook_parse_cmdline exists.

Ok
+ + local hook="${config//.[[:digit:]]/}" + local id="${config//"${hook}."/}"
Can we have functions for these two? This looks not very intuitive and we will need this in other hooks, too.
Ok

+ + if ! zone_config_settings_write "${zone}" "${hook}" ${id}; then + log ERROR "Could not write config settings file ${config} for ${zone}" + return ${EXIT_ERROR} + fi + + exit ${EXIT_OK} +} +  hook_up() {   local zone=${1}   local config=${2}
-Michael

Jonatan