[PATCH 1/3] zone: fix zone_config()

[PATCH 1/3] zone: fix zone_config()

[Jonatan Schlag]

[-- Attachment #1: Type: text/plain, Size: 785 bytes --]

Signed-off-by: Jonatan Schlag <jonatan.schlag(a)ipfire.org>
---
 src/functions/functions.zone | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/functions/functions.zone b/src/functions/functions.zone
index acf68f5..11a8dc2 100644
--- a/src/functions/functions.zone
+++ b/src/functions/functions.zone
@@ -565,9 +565,9 @@ zone_config() {
 			# TODO This could be also a valid hid
 			local id=${cmd}
 
-			if zone_config_id_is valid ${zone} ${id} && [[ ${1} == "edit" ]]; then
+			if zone_config_id_is_valid ${zone} ${id} && [[ ${1} == "edit" ]]; then
 				shift 1
-				zone_config_edit "${zone}" "${id}""$@"
+				zone_config_edit "${zone}" "${id}" "$@"
 			else
 				error "Unrecognized argument: ${cmd}"
 				cli_usage root-zone-config-subcommands
-- 
2.6.3

[PATCH 2/3] header-zone: refactor hook_config_edit

[Jonatan Schlag]

[-- Attachment #1: Type: text/plain, Size: 1001 bytes --]

Before a config is edit we should bring the config down and after up again.
Also we need to have 2 arguments or more.

Signed-off-by: Jonatan Schlag <jonatan.schlag(a)ipfire.org>
---
 src/header-zone | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/src/header-zone b/src/header-zone
index 2e3fa09..3b074ee 100644
--- a/src/header-zone
+++ b/src/header-zone
@@ -225,7 +225,7 @@ hook_config_destroy() {
 }
 
 hook_config_edit() {
-	assert [ $# -eq 2 ]
+	assert [ $# -ge 2 ]
 	local zone=${1}
 	# The id must be the id and not the hid.
 	local id=${2}
@@ -240,7 +240,14 @@ hook_config_edit() {
 	local hook=$(zone_config_get_hook_from_id ${zone} ${id})
 	assert isset hook
 
+	# Bring the config down
+	hook_config_cmd "down" "${zone}" "${hook}" "${hook}.${id}"
+
+	# Edit the hook
 	hook_config_cmd "edit" "${zone}" "${hook}" "${hook}.${id}" "$@"
+
+	# Bring the config up again
+	hook_config_cmd "up" "${zone}" "${hook}" "${hook}.${id}"
 }
 
 hook_config_show() {
-- 
2.6.3

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

[Jonatan Schlag]

[-- Attachment #1: Type: text/plain, Size: 1664 bytes --]

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
+
+	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
+
+	local hook="${config//.[[:digit:]]/}"
+	local id="${config//"${hook}."/}"
+
+	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}
-- 
2.6.3

Re: [PATCH 2/3] header-zone: refactor hook_config_edit

[Michael Tremer]

[-- Attachment #1: Type: text/plain, Size: 1612 bytes --]

Hi,

and no no no no. We cannot restart the zone just because some
configuration is being changed. That would disrupt connections and that
might not always be required.

The hook_up() and hook_down() functions are usually designed to take
care of this and check if a zone is already up and potentially only
change the settings that need to be changed.

That will allow to edit things on a live system without disrupting any
users which I think is very important.

-Michael

On Fri, 2017-07-14 at 21:10 +0200, Jonatan Schlag wrote:
> Before a config is edit we should bring the config down and after up
> again.
> Also we need to have 2 arguments or more.
> 
> Signed-off-by: Jonatan Schlag <jonatan.schlag(a)ipfire.org>
> ---
>  src/header-zone | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/src/header-zone b/src/header-zone
> index 2e3fa09..3b074ee 100644
> --- a/src/header-zone
> +++ b/src/header-zone
> @@ -225,7 +225,7 @@ hook_config_destroy() {
>  }
>  
>  hook_config_edit() {
> -	assert [ $# -eq 2 ]
> +	assert [ $# -ge 2 ]
>  	local zone=${1}
>  	# The id must be the id and not the hid.
>  	local id=${2}
> @@ -240,7 +240,14 @@ hook_config_edit() {
>  	local hook=$(zone_config_get_hook_from_id ${zone} ${id})
>  	assert isset hook
>  
> +	# Bring the config down
> +	hook_config_cmd "down" "${zone}" "${hook}" "${hook}.${id}"
> +
> +	# Edit the hook
>  	hook_config_cmd "edit" "${zone}" "${hook}" "${hook}.${id}"
> "$@"
> +
> +	# Bring the config up again
> +	hook_config_cmd "up" "${zone}" "${hook}" "${hook}.${id}"
>  }
>  
>  hook_config_show() {

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Michael Tremer]

[-- Attachment #1: Type: text/plain, Size: 2494 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]