mbox

[3/3] dhcp: add hook_edit() function

Message ID 1500059452-3421-3-git-send-email-jonatan.schlag@ipfire.org
State Superseded
Headers

Message

Jonatan Schlag July 15, 2017, 5:10 a.m. UTC 
  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(+)

Comments

Michael Tremer July 15, 2017, 10:53 a.m. UTC | #1 
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.

> +
> +	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
Jonatan Schlag July 15, 2017, 5:10 p.m. UTC | #2 
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