[PATCH 1/5] zone: zone_config_check_same_setting check for config to edit

[PATCH 1/5] zone: zone_config_check_same_setting check for config to edit

[Jonatan Schlag]

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

We now check if we get the config, which we edit in this moment and when we continue.

Fixes: #11451

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

diff --git a/src/functions/functions.zone b/src/functions/functions.zone
index 1eb492f..941314d 100644
--- a/src/functions/functions.zone
+++ b/src/functions/functions.zone
@@ -1155,12 +1155,13 @@ zone_config_check_same_setting() {
 	# with the same setting is already configured for this zone.
 	# Returns True when yes and False when no.
 
-	assert [ $# -eq 4 ]
+	assert [ $# -eq 5 ]
 
 	local zone=${1}
 	local hook=${2}
-	local key=${3}
-	local value=${4}
+	local id=${3}
+	local key=${4}
+	local value=${5}
 
 	# The key should be local for this function
 	local ${key}
@@ -1171,6 +1172,12 @@ zone_config_check_same_setting() {
 		if  [[ $(zone_config_get_hook "${zone}" "${config}") != ${hook} ]]; then
 			continue
 		fi
+
+		# Check if we get the config we want to create or edit (we must ignore this config on edit)
+		if [[ "${hook}.${id}" = "${config}" ]]; then
+			continue
+		fi
+
 		# Get the value of the key for a given function
 		zone_config_settings_read "${zone}" "${config}" \
                 --ignore-superfluous-settings "${key}"
-- 
2.6.3

[PATCH 2/5] zone: zone_config_settings_write never generate an id

[Jonatan Schlag]

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

We now generate the new id inside the hook_new function.
This is required because the hook_parse_cmdline
function take now the id of the config as te first argument.

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

diff --git a/src/functions/functions.zone b/src/functions/functions.zone
index 941314d..34cc983 100644
--- a/src/functions/functions.zone
+++ b/src/functions/functions.zone
@@ -1410,16 +1410,13 @@ zone_config_settings_read() {
 }
 
 zone_config_settings_write() {
-	assert [ $# -ge 2 ]
+	assert [ $# -eq 3 ]
 
 	local zone="${1}"
 	local hook="${2}"
 	local id=${3}
 
-	if ! isset id; then
-		id=$(zone_config_get_new_id ${zone})
-		log DEBUG "ID for the config is: ${id}"
-	fi
+	assert isinteger id
 
 	local args
 	if function_exists "hook_check_config_settings"; then
-- 
2.6.3

[PATCH 3/5] header-config: pass id to hook_parse_cmdline

[Jonatan Schlag]

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

Signed-off-by: Jonatan Schlag <jonatan.schlag(a)ipfire.org>
---
 src/header-config | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/header-config b/src/header-config
index 6341a22..815d417 100644
--- a/src/header-config
+++ b/src/header-config
@@ -55,7 +55,7 @@ hook_edit() {
 		return ${EXIT_ERROR}
 	fi
 
-	if ! hook_parse_cmdline $@; then
+	if ! hook_parse_cmdline ${id} $@; then
 		# Return an error if the parsing of the cmd line fails
 		return ${EXIT_ERROR}
 	fi
-- 
2.6.3

[PATCH 4/5] config hooks: hook_parse_cmdline take id as an argument

[Jonatan Schlag]

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

All hook_parse_cmdline take now the id as the first argument. This is required because
zone_config_check_same_setting needs the id to ignore the config we edit in the moment.
All hook_parse_cmdline functions are changed to keep the generic hook_edit function working.
Another option was to write an own hook_edit function for the ipv4-static and ipv6-static hook.
But this solution is not easier then to keep things more generic and equal. Makes also maintaining easier.

Signed-off-by: Jonatan Schlag <jonatan.schlag(a)ipfire.org>
---
 src/hooks/configs/dhcp         | 4 ++++
 src/hooks/configs/ipv4-static  | 7 ++++++-
 src/hooks/configs/ipv6-auto    | 5 +++++
 src/hooks/configs/ipv6-static  | 7 ++++++-
 src/hooks/configs/pppoe-server | 5 +++++
 5 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/src/hooks/configs/dhcp b/src/hooks/configs/dhcp
index 7f6780b..f1c43f3 100644
--- a/src/hooks/configs/dhcp
+++ b/src/hooks/configs/dhcp
@@ -35,6 +35,10 @@ hook_check_config_settings() {
 }
 
 hook_parse_cmdline() {
+	local id=${1}
+	shift
+
+	assert isinteger id
 	while [ $# -gt 0 ]; do
 		case "${1}" in
 			--enable-ipv6)
diff --git a/src/hooks/configs/ipv4-static b/src/hooks/configs/ipv4-static
index ef74991..a6b010f 100644
--- a/src/hooks/configs/ipv4-static
+++ b/src/hooks/configs/ipv4-static
@@ -36,6 +36,11 @@ hook_check_config_settings() {
 }
 
 hook_parse_cmdline() {
+	local id=${1}
+	shift
+
+	assert isinteger id
+
 	local arg
 
 	while read -r arg; do
@@ -99,7 +104,7 @@ hook_parse_cmdline() {
 		exit ${EXIT_CONF_ERROR}
 	fi
 
-	if zone_config_check_same_setting "${zone}" "ipv4-static" "ADDRESS" "${ADDRESS}"; then
+	if zone_config_check_same_setting "${zone}" "ipv4-static" ${id} "ADDRESS" "${ADDRESS}"; then
 		error "An ipv4-static config with the same IPv4 address is already configured"
 		exit ${EXIT_CONF_ERROR}
 	fi
diff --git a/src/hooks/configs/ipv6-auto b/src/hooks/configs/ipv6-auto
index 375e585..e89af28 100644
--- a/src/hooks/configs/ipv6-auto
+++ b/src/hooks/configs/ipv6-auto
@@ -31,6 +31,11 @@ hook_check_config_settings() {
 }
 
 hook_parse_cmdline() {
+	local id=${1}
+	shift
+
+	assert isinteger id
+
 	local arg
 
 	while read arg; do
diff --git a/src/hooks/configs/ipv6-static b/src/hooks/configs/ipv6-static
index c41401c..bb75240 100644
--- a/src/hooks/configs/ipv6-static
+++ b/src/hooks/configs/ipv6-static
@@ -34,6 +34,11 @@ hook_check_config_settings() {
 }
 
 hook_parse_cmdline() {
+	local id=${1}
+	shift
+
+	assert isinteger id
+
 	while [ $# -gt 0 ]; do
 		case "${1}" in
 			--address=*)
@@ -49,7 +54,7 @@ hook_parse_cmdline() {
 		shift
 	done
 
-	if zone_config_check_same_setting "${zone}" "ipv6-static" "ADDRESS" "${ADDRESS}"; then
+	if zone_config_check_same_setting "${zone}" "ipv6-static" ${id} "ADDRESS" "${ADDRESS}"; then
 		error "An ipv6-static config with the same IPv6 address is already configured"
 		exit ${EXIT_CONF_ERROR}
 	fi
diff --git a/src/hooks/configs/pppoe-server b/src/hooks/configs/pppoe-server
index b4d2538..5861e70 100644
--- a/src/hooks/configs/pppoe-server
+++ b/src/hooks/configs/pppoe-server
@@ -49,6 +49,11 @@ hook_check_config_settings() {
 }
 
 hook_parse_cmdline() {
+	local id=${1}
+	shift
+
+	assert isinteger id
+
 	while [ $# -gt 0 ]; do
 		case "${1}" in
 			--dns-server=*)
-- 
2.6.3

[PATCH 5/5] config hook: generate new id inside hook_new

[Jonatan Schlag]

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

The hook_parse_cmdline function takes as first argument the id so
we need to generate the id inside hook_new

Signed-off-by: Jonatan Schlag <jonatan.schlag(a)ipfire.org>
---
 src/hooks/configs/dhcp         | 9 +++++++--
 src/hooks/configs/ipv4-static  | 9 +++++++--
 src/hooks/configs/ipv6-auto    | 9 +++++++--
 src/hooks/configs/ipv6-static  | 9 +++++++--
 src/hooks/configs/pppoe-server | 9 +++++++--
 5 files changed, 35 insertions(+), 10 deletions(-)

diff --git a/src/hooks/configs/dhcp b/src/hooks/configs/dhcp
index f1c43f3..f2a3e56 100644
--- a/src/hooks/configs/dhcp
+++ b/src/hooks/configs/dhcp
@@ -76,12 +76,17 @@ hook_new() {
 		return ${EXIT_ERROR}
 	fi
 
-	if ! hook_parse_cmdline $@; then
+	# Get a new id
+	local id=$(zone_config_get_new_id ${zone})
+	assert isinteger id
+	log DEBUG "ID for the config is: ${id}"
+
+	if ! hook_parse_cmdline ${id} $@; then
 		# Return an error if the parsing of the cmd line fails
 		return ${EXIT_ERROR}
 	fi
 
-	zone_config_settings_write "${zone}" "${HOOK}"
+	zone_config_settings_write "${zone}" "${HOOK}" ${id}
 
 	exit ${EXIT_OK}
 }
diff --git a/src/hooks/configs/ipv4-static b/src/hooks/configs/ipv4-static
index a6b010f..233fb10 100644
--- a/src/hooks/configs/ipv4-static
+++ b/src/hooks/configs/ipv4-static
@@ -120,12 +120,17 @@ hook_new() {
 
 	assert zone_exists "${zone}"
 
-	if ! hook_parse_cmdline $@; then
+	# Get a new id
+	local id=$(zone_config_get_new_id ${zone})
+	assert isinteger id
+	log DEBUG "ID for the config is: ${id}"
+
+	if ! hook_parse_cmdline ${id} $@; then
 		# Return an error if the parsing of the cmd line fails
 		return ${EXIT_ERROR}
 	fi
 
-	zone_config_settings_write "${zone}" "${HOOK}"
+	zone_config_settings_write "${zone}" "${HOOK}" ${id}
 
 	exit ${EXIT_OK}
 }
diff --git a/src/hooks/configs/ipv6-auto b/src/hooks/configs/ipv6-auto
index e89af28..7c82d45 100644
--- a/src/hooks/configs/ipv6-auto
+++ b/src/hooks/configs/ipv6-auto
@@ -62,12 +62,17 @@ hook_new() {
 		return ${EXIT_ERROR}
 	fi
 
-	if ! hook_parse_cmdline $@; then
+	# Get a new id
+	local id=$(zone_config_get_new_id ${zone})
+	assert isinteger id
+	log DEBUG "ID for the config is: ${id}"
+
+	if ! hook_parse_cmdline ${id} $@; then
 		# Return an error if the parsing of the cmd line fails
 		return ${EXIT_ERROR}
 	fi
 
-	zone_config_settings_write "${zone}" "${HOOK}"
+	zone_config_settings_write "${zone}" "${HOOK}" ${id}
 
 	exit ${EXIT_OK}
 }
diff --git a/src/hooks/configs/ipv6-static b/src/hooks/configs/ipv6-static
index bb75240..313adc0 100644
--- a/src/hooks/configs/ipv6-static
+++ b/src/hooks/configs/ipv6-static
@@ -71,12 +71,17 @@ hook_new() {
 	local zone=${1}
 	shift
 
-	if ! hook_parse_cmdline $@; then
+	# Get a new id
+	local id=$(zone_config_get_new_id ${zone})
+	assert isinteger id
+	log DEBUG "ID for the config is: ${id}"
+
+	if ! hook_parse_cmdline ${id} $@; then
 		# Return an error if the parsing of the cmd line fails
 		return ${EXIT_ERROR}
 	fi
 
-	zone_config_settings_write "${zone}" "${HOOK}"
+	zone_config_settings_write "${zone}" "${HOOK}" ${id}
 
 	exit ${EXIT_OK}
 }
diff --git a/src/hooks/configs/pppoe-server b/src/hooks/configs/pppoe-server
index 5861e70..64d700b 100644
--- a/src/hooks/configs/pppoe-server
+++ b/src/hooks/configs/pppoe-server
@@ -110,12 +110,17 @@ hook_new() {
 		return ${EXIT_ERROR}
 	fi
 
-	if ! hook_parse_cmdline $@; then
+	# Get a new id
+	local id=$(zone_config_get_new_id ${zone})
+	assert isinteger id
+	log DEBUG "ID for the config is: ${id}"
+
+	if ! hook_parse_cmdline ${id} $@; then
 		# Return an error if the parsing of the cmd line fails
 		return ${EXIT_ERROR}
 	fi
 
-	zone_config_settings_write "${zone}" "${HOOK}"
+	zone_config_settings_write "${zone}" "${HOOK}" ${id}
 
 	exit ${EXIT_OK}
 }
-- 
2.6.3

Re: [PATCH 1/5] zone: zone_config_check_same_setting check for config to edit

[Michael Tremer]

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

Hi,

this patchset looks fine, but I think the solution is a little bit too
complicated.

Could we not let hook_parse_cmdline() remain untouched and just let it parse all
inputs? We need to parse all arguments anyways before we can make a decision.
We could then check for any conflicts later in hook_edit().

That looks easier to me since:

a) the format of hook_parse_cmdline() would be the same throughout all zones,
ports and configs

b) we do not need to carry around the ID variable. It will be empty any ways
when we are creating a new configuration and then become difficult to handle.

Your thoughts?

Best,
-Michael

On Sat, 2017-08-12 at 12:22 +0200, Jonatan Schlag wrote:
> We now check if we get the config, which we edit in this moment and when we
> continue.
> 
> Fixes: #11451
> 
> Signed-off-by: Jonatan Schlag <jonatan.schlag(a)ipfire.org>
> ---
>  src/functions/functions.zone | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/src/functions/functions.zone b/src/functions/functions.zone
> index 1eb492f..941314d 100644
> --- a/src/functions/functions.zone
> +++ b/src/functions/functions.zone
> @@ -1155,12 +1155,13 @@ zone_config_check_same_setting() {
>  	# with the same setting is already configured for this zone.
>  	# Returns True when yes and False when no.
>  
> -	assert [ $# -eq 4 ]
> +	assert [ $# -eq 5 ]
>  
>  	local zone=${1}
>  	local hook=${2}
> -	local key=${3}
> -	local value=${4}
> +	local id=${3}
> +	local key=${4}
> +	local value=${5}
>  
>  	# The key should be local for this function
>  	local ${key}
> @@ -1171,6 +1172,12 @@ zone_config_check_same_setting() {
>  		if  [[ $(zone_config_get_hook "${zone}" "${config}") !=
> ${hook} ]]; then
>  			continue
>  		fi
> +
> +		# Check if we get the config we want to create or edit (we
> must ignore this config on edit)
> +		if [[ "${hook}.${id}" = "${config}" ]]; then
> +			continue
> +		fi
> +
>  		# Get the value of the key for a given function
>  		zone_config_settings_read "${zone}" "${config}" \
>                  --ignore-superfluous-settings "${key}"

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