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(a)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.

> +
> +	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.

> +
> +	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.

> +
> +	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