v2 First patch series for the roadmap item "Get rid of CONFIG_TYPE in the network config"

v2 First patch series for the roadmap item "Get rid of CONFIG_TYPE in the network config"

[Jonatan Schlag]

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

Hi list,

This is the second version of the first patch series for the roadmap item: https://www.ipfire.org/docs/roadmap/get-rid-of-configtype-in-network-config . This patch series introduces a bash testing library and a function readhash, which we will use in every bash script which needs to read our config files.
As this patch set is already massive, I thought it might be better to discuss the function first before I introduce it in every script.
Otherwise, we would talk about 50 patches and not 18. This amount would be somehow not reviewable.

I kept the [[ over the single [, as from my point of view these are far
more intutive. No pathname expansion or word splitting takes place in [[
which is the saver option. See also https://google.github.io/styleguide/shellguide.html#test----and--- 

Patches in this series are:

  # The first 9 patches introduce a bash testing library
  [PATCH v2 01/18] tests: Add bash lib
  [PATCH v2 02/18] tests/lib.sh: Add function test_value_in_array
  [PATCH v2 03/18] tests/lib.sh: Add check if variable exists to
  [PATCH v2 04/18] tests/lib.sh: Add logging functions
  [PATCH v2 05/18] tests/lib.sh: adjust to pytest logging style
  [PATCH v2 06/18] test_value_in_array: Check if the key is defined
  [PATCH v2 07/18] tests: Add function to test the ouput of a bash
  [PATCH v2 08/18] test: Add functions test_that_array_is_defined
  [PATCH v2 09/18] tests: Add functions test_that_array_doesnt_have_key
  # The next patch add the readhash function
  [PATCH v2 10/18] initscript functions: add readhash
  # The next 5 Patches are concerned with error handling
  [PATCH v2 11/18] initscript fkt: ignore blank lines in readhash
  [PATCH v2 12/18] initscripts fkt: Ignore comments in readhash
  [PATCH v2 13/18] initscripts fkt: ignore invalid keys in readhash
  [PATCH v2 14/18] initscripts fkt: Check for invalid values in readhash
  [PATCH v2 15/18] initscripts fkt: readhash should only parse lines with a =
  # The title says it all:
  [PATCH v2 16/18] initscripts fkt: keep readhash compatible with older
  # Keep my linter happy
  [PATCH v2 17/18] initscripts fkt: Fix shebang
  # Check that we fail on a missing file
  [PATCH v2 18/18] initscripts fkt: Check that readhash returns 1 on a

[PATCH v2 01/18] tests: Add bash lib

[Jonatan Schlag]

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

This allows use to write test with less effort as we can reuse functions

Signed-off-by: Jonatan Schlag <jonatan.schlag(a)ipfire.org>
---
 tests/lib.sh | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)
 create mode 100644 tests/lib.sh

diff --git a/tests/lib.sh b/tests/lib.sh
new file mode 100644
index 000000000..dd5e3f535
--- /dev/null
+++ b/tests/lib.sh
@@ -0,0 +1,27 @@
+#!/usr/bin/bash
+
+# Get the path of this file.
+# This ist rather complex as we do not want the calling script file
+# That why we use BASH_SOURCE[0]
+LIB_DIR="$(readlink -f "${BASH_SOURCE[0]}")"
+# In LIB_DIR is currently saved the path to this file you are currently reading
+# but we need the directory where it is located so:
+LIB_DIR="$(dirname "${LIB_DIR}")"
+
+
+. ${LIB_DIR}/lib_color.sh
+
+test_command() {
+
+	if ! "$@" ; then
+		echo -e "${CLR_RED_BG} Test failed: ${*} ${CLR_RESET}"
+		return 1
+	else
+		echo -e "${CLR_GREEN_BG} Test succeded: ${*} ${CLR_RESET}"
+		return 0
+	fi
+}
+
+var_has_value() {
+	[[ "${!1}" == "${2}" ]]
+}
-- 
2.39.2

[PATCH v2 02/18] tests/lib.sh: Add function test_value_in_array

[Jonatan Schlag]

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

Test if a given array has the specified value stored under key.

! does not work here to access the array by variable name. So the solution here is:
https://unix.stackexchange.com/questions/60584/how-to-use-a-variable-as-part-of-an-array-name/60585#60585

Signed-off-by: Jonatan Schlag <jonatan.schlag(a)ipfire.org>
---
 tests/lib.sh                                  | 14 +++++++++++++
 .../src/initscripts/system/functions/test2.sh | 21 +++++++++++++++++++
 2 files changed, 35 insertions(+)
 create mode 100755 tests/src/initscripts/system/functions/test2.sh

diff --git a/tests/lib.sh b/tests/lib.sh
index dd5e3f535..716922024 100644
--- a/tests/lib.sh
+++ b/tests/lib.sh
@@ -25,3 +25,17 @@ test_command() {
 var_has_value() {
 	[[ "${!1}" == "${2}" ]]
 }
+
+test_value_in_array() {
+	local -n array="${1}"
+	local key="${2}"
+	local value="${3}"
+
+	if [[ "${array[${key}]}" == "${value}" ]] ; then
+		echo -e "${CLR_GREEN_BG}Test succeded: The array '${1}' contains the value '${value}' under the key '${key}' ${CLR_RESET}"
+		return 0
+	else
+		echo -e "${CLR_RED_BG}Test failed: The array '${1}' contains the value '${array[${key}]}' under the key '${key} and not '${value}' ${CLR_RESET}"
+		return 1
+	fi
+}
diff --git a/tests/src/initscripts/system/functions/test2.sh b/tests/src/initscripts/system/functions/test2.sh
new file mode 100755
index 000000000..a568ed2a4
--- /dev/null
+++ b/tests/src/initscripts/system/functions/test2.sh
@@ -0,0 +1,21 @@
+#!/usr/bin/bash
+
+SCRIPT_PATH="$(dirname "$(readlink -f "$0")")"
+
+ROOT="$(readlink -f "${SCRIPT_PATH}/../../../../..")"
+
+. ${ROOT}/tests/lib.sh
+
+. ${ROOT}/src/initscripts/system/functions
+
+# read the date in
+readhash "CONFIG" "${SCRIPT_PATH}/data/1"
+
+# test if we read the correct data
+test_that_key_in_arry_has_value "CONFIG" "RED_DHCP_HOSTNAME" "ipfire"
+test_that_key_in_arry_has_value "CONFIG" "BLUE_MACADDR" "bc:30:7d:58:6b:e3"
+
+test_that_output_is "${SCRIPT_PATH}/data/1_output_stdout" "1" readhash "CONFIG" "${SCRIPT_PATH}/data/1"
+test_that_output_is "${SCRIPT_PATH}/data/1_output_stderr" "2" readhash "CONFIG" "${SCRIPT_PATH}/data/1"
+ 
+
-- 
2.39.2

[PATCH v2 03/18] tests/lib.sh: Add check if variable exists to test_value_in_array

[Jonatan Schlag]

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

We cannot use [ -v ] here as this does not work. We need to check if the
array is correctly declared.

Signed-off-by: Jonatan Schlag <jonatan.schlag(a)ipfire.org>
---
 tests/lib.sh | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/tests/lib.sh b/tests/lib.sh
index 716922024..4fce151f8 100644
--- a/tests/lib.sh
+++ b/tests/lib.sh
@@ -28,9 +28,18 @@ var_has_value() {
 
 test_value_in_array() {
 	local -n array="${1}"
+	local arrayname="${1}"
 	local key="${2}"
 	local value="${3}"
 
+	# `declare -p` print out how the variable with the name $arrayname was declared
+	# If the array was not declared as indexed or associative array we fail. We cannot check for a value in an array if 
+	# we were not given array.
+	if [[ ! "$(declare  -p "${arrayname}")" =~ "declare -a" && ! "$(declare  -p "${arrayname}")" =~ "declare -A" ]]; then
+		echo -e "${CLR_RED_BG}Test failed: The array '${1}' does not exists. The variable is not set.${CLR_RESET}'"
+		return 1
+	fi
+
 	if [[ "${array[${key}]}" == "${value}" ]] ; then
 		echo -e "${CLR_GREEN_BG}Test succeded: The array '${1}' contains the value '${value}' under the key '${key}' ${CLR_RESET}"
 		return 0
-- 
2.39.2

[PATCH v2 04/18] tests/lib.sh: Add logging functions

[Jonatan Schlag]

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

So we can change the style of our log messages better.

Signed-off-by: Jonatan Schlag <jonatan.schlag(a)ipfire.org>
---
 tests/lib.sh | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/tests/lib.sh b/tests/lib.sh
index 4fce151f8..e462f4add 100644
--- a/tests/lib.sh
+++ b/tests/lib.sh
@@ -11,13 +11,21 @@ LIB_DIR="$(dirname "${LIB_DIR}")"
 
 . ${LIB_DIR}/lib_color.sh
 
+log_test_failed(){
+	echo -e "${CLR_RED_BG}Test failed: ${*}${CLR_RESET}'"
+}
+
+log_test_succeded(){
+	echo -e "${CLR_GREEN_BG}Test succeded: ${*}${CLR_RESET}"
+}
+
 test_command() {
 
 	if ! "$@" ; then
-		echo -e "${CLR_RED_BG} Test failed: ${*} ${CLR_RESET}"
+		log_test_failed "${*}"
 		return 1
 	else
-		echo -e "${CLR_GREEN_BG} Test succeded: ${*} ${CLR_RESET}"
+		log_test_succeded "${*}"
 		return 0
 	fi
 }
@@ -36,15 +44,15 @@ test_value_in_array() {
 	# If the array was not declared as indexed or associative array we fail. We cannot check for a value in an array if 
 	# we were not given array.
 	if [[ ! "$(declare  -p "${arrayname}")" =~ "declare -a" && ! "$(declare  -p "${arrayname}")" =~ "declare -A" ]]; then
-		echo -e "${CLR_RED_BG}Test failed: The array '${1}' does not exists. The variable is not set.${CLR_RESET}'"
+		log_test_failed "The array '${1}' does not exists. The variable is not set."
 		return 1
 	fi
 
 	if [[ "${array[${key}]}" == "${value}" ]] ; then
-		echo -e "${CLR_GREEN_BG}Test succeded: The array '${1}' contains the value '${value}' under the key '${key}' ${CLR_RESET}"
+		log_test_succeded "The array '${1}' contains the value '${value}' under the key '${key}'"
 		return 0
 	else
-		echo -e "${CLR_RED_BG}Test failed: The array '${1}' contains the value '${array[${key}]}' under the key '${key} and not '${value}' ${CLR_RESET}"
+		log_test_failed "The array '${1}' contains the value '${array[${key}]}' under the key '${key} and not '${value}'"
 		return 1
 	fi
 }
-- 
2.39.2

[PATCH v2 05/18] tests/lib.sh: adjust to pytest logging style

[Jonatan Schlag]

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

Black on white is still the best to read. So we only style FAILED or
PASSED in green or red. This is also tested with different background
colors. As we only style PASSED or FAILED it works without any problems

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

diff --git a/tests/lib.sh b/tests/lib.sh
index e462f4add..f49a94748 100644
--- a/tests/lib.sh
+++ b/tests/lib.sh
@@ -12,11 +12,11 @@ LIB_DIR="$(dirname "${LIB_DIR}")"
 . ${LIB_DIR}/lib_color.sh
 
 log_test_failed(){
-	echo -e "${CLR_RED_BG}Test failed: ${*}${CLR_RESET}'"
+	echo -e "${CLR_RED_R}FAILED:${CLR_RESET} ${*}"
 }
 
 log_test_succeded(){
-	echo -e "${CLR_GREEN_BG}Test succeded: ${*}${CLR_RESET}"
+	echo -e "${CLR_GREEN_R}PASSED:${CLR_RESET} ${*}"
 }
 
 test_command() {
-- 
2.39.2

[PATCH v2 06/18] test_value_in_array: Check if the key is defined

[Jonatan Schlag]

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

Signed-off-by: Jonatan Schlag <jonatan.schlag(a)ipfire.org>
---
 tests/lib.sh | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/tests/lib.sh b/tests/lib.sh
index f49a94748..af8c632cf 100644
--- a/tests/lib.sh
+++ b/tests/lib.sh
@@ -48,6 +48,14 @@ test_value_in_array() {
 		return 1
 	fi
 
+	# If key is not defined we return _
+	# If the key is defined we return nothing
+	# See also https://www.gnu.org/software/bash/manual/html_node/Shell-Parameter-Expansion.html
+	if [[ "${array["${key}"]+_}" == ""  ]]; then
+		log_test_failed "The array does not contain the key '${key}', valid keys are: ${!array[*]}"
+		return 1
+	fi
+
 	if [[ "${array[${key}]}" == "${value}" ]] ; then
 		log_test_succeded "The array '${1}' contains the value '${value}' under the key '${key}'"
 		return 0
-- 
2.39.2

[PATCH v2 07/18] tests: Add function to test the ouput of a bash function

[Jonatan Schlag]

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

Signed-off-by: Jonatan Schlag <jonatan.schlag(a)ipfire.org>
---
 tests/lib.sh | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/tests/lib.sh b/tests/lib.sh
index af8c632cf..4110ed2d8 100644
--- a/tests/lib.sh
+++ b/tests/lib.sh
@@ -64,3 +64,32 @@ test_value_in_array() {
 		return 1
 	fi
 }
+
+test_that_output_is(){
+	local reference_output_file="${1}"
+	local file_descriptor="${2}"
+	shift
+	shift
+
+	local command="$@"
+
+	local temp="$(mktemp)"
+
+	case "${file_descriptor}" in
+		"stdout"|"1")
+			$command 1> "${temp}" 2>/dev/null
+			;;
+		"stderr"|"2")
+			$command 2> "${temp}" 1>/dev/null
+			;;
+	esac
+
+	if diff -u "${temp}" "${reference_output_file}" &> /dev/null; then
+		log_test_succeded "The output of command '${command}' on file descriptor '${file_descriptor}' is equal to the reference output."
+	else
+		log_test_failed "The output of command '${command}' on file descriptor '${file_descriptor}' is unequal to the reference output."
+		diff -u --color "${reference_output_file}" "${temp}"
+	fi
+
+	rm "${temp}"
+}
-- 
2.39.2

[PATCH v2 08/18] test: Add functions test_that_array_is_defined

[Jonatan Schlag]

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

we need this check in multiple places so it makes sense to move this to
a separate function.

Signed-off-by: Jonatan Schlag <jonatan.schlag(a)ipfire.org>
---
 tests/lib.sh | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/tests/lib.sh b/tests/lib.sh
index 4110ed2d8..32481b6a5 100644
--- a/tests/lib.sh
+++ b/tests/lib.sh
@@ -34,11 +34,8 @@ var_has_value() {
 	[[ "${!1}" == "${2}" ]]
 }
 
-test_value_in_array() {
-	local -n array="${1}"
+test_that_array_is_defined() {
 	local arrayname="${1}"
-	local key="${2}"
-	local value="${3}"
 
 	# `declare -p` print out how the variable with the name $arrayname was declared
 	# If the array was not declared as indexed or associative array we fail. We cannot check for a value in an array if 
@@ -46,7 +43,19 @@ test_value_in_array() {
 	if [[ ! "$(declare  -p "${arrayname}")" =~ "declare -a" && ! "$(declare  -p "${arrayname}")" =~ "declare -A" ]]; then
 		log_test_failed "The array '${1}' does not exists. The variable is not set."
 		return 1
+	else
+		log_test_succeded "The array '${1}' is defined."
+		return 0
 	fi
+}
+
+test_value_in_array() {
+	local -n array="${1}"
+	local arrayname="${1}"
+	local key="${2}"
+	local value="${3}"
+
+	test_that_array_is_defined "${arrayname}"  || return 1
 
 	# If key is not defined we return _
 	# If the key is defined we return nothing
-- 
2.39.2

[PATCH v2 09/18] tests: Add functions test_that_array_doesnt_have_key

[Jonatan Schlag]

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

Apparently we can set way more keys then I expected. So we need a
function to check that we do not set certain key. Some keys need to be
skipped.

Signed-off-by: Jonatan Schlag <jonatan.schlag(a)ipfire.org>
---
 tests/lib.sh | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/tests/lib.sh b/tests/lib.sh
index 32481b6a5..bb06e11c2 100644
--- a/tests/lib.sh
+++ b/tests/lib.sh
@@ -74,6 +74,24 @@ test_value_in_array() {
 	fi
 }
 
+test_that_array_doesnt_have_key() {
+	local -n array="${1}"
+	local arrayname="${1}"
+	local key="${2}"
+
+	test_that_array_is_defined "${arrayname}"  || return 1
+
+	if [[ "${array["${key}"]+_}" == "_"  ]]; then
+		log_test_failed "The array '${arrayname}' does contain the key '${key}'."
+		return 1
+	else
+		log_test_succeded "The array '${arrayname}' does not contain the key '${key}'"
+		return 0
+	fi
+
+}
+
+
 test_that_output_is(){
 	local reference_output_file="${1}"
 	local file_descriptor="${2}"
-- 
2.39.2

[PATCH v2 10/18] initscript functions: add readhash

[Jonatan Schlag]

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

To avoid the usage of eval and to store the config in an key value
array, we introduce an new function. The tests only check if we
read the correct value to the correct variable.

One comment on the implementation as this has created some headache:

>From https://www.gnu.org/software/bash/manual/bash.html#Bourne-Shell-Builtins

	"When used in a function, declare makes each name local, as with the local command, unless the -g option is used."

So we need to use -g here

Signed-off-by: Jonatan Schlag <jonatan.schlag(a)ipfire.org>
---
 src/initscripts/system/functions               | 15 +++++++++++++++
 tests/src/initscripts/system/functions/data/1  | 17 +++++++++++++++++
 tests/src/initscripts/system/functions/test.sh | 16 ++++++++++++++++
 3 files changed, 48 insertions(+)
 create mode 100644 tests/src/initscripts/system/functions/data/1
 create mode 100755 tests/src/initscripts/system/functions/test.sh

diff --git a/src/initscripts/system/functions b/src/initscripts/system/functions
index b610143ab..44ce999d3 100644
--- a/src/initscripts/system/functions
+++ b/src/initscripts/system/functions
@@ -891,3 +891,18 @@ volume_fs_type() {
 
 	stat -f --format="%T" ${1}
 }
+
+readhash() {
+	local array="${1}"
+	local file="${2}"
+
+	declare -A -g "${array}"
+
+	local line
+	while read -r line; do
+		local key="${line%=*}"
+		local val="${line#*=}"
+
+		printf -v "${array}[${key}]" "%s" "${val}"
+	done < "${file}"
+}
diff --git a/tests/src/initscripts/system/functions/data/1 b/tests/src/initscripts/system/functions/data/1
new file mode 100644
index 000000000..8aca9422b
--- /dev/null
+++ b/tests/src/initscripts/system/functions/data/1
@@ -0,0 +1,17 @@
+CONFIG_TYPE=3
+GREEN_DEV=green0
+GREEN_MACADDR=00:c0:08:8a:a0:47
+GREEN_DRIVER=r8175
+RED_DEV=red0
+RED_MACADDR=00:c0:08:8a:a0:56
+RED_DRIVER=r8283
+BLUE_DEV='blue0 net0'
+BLUE_MACADDR=bc:30:7d:58:6b:e3
+BLUE_DRIVER=rt2800
+RED_DHCP_HOSTNAME=ipfire
+RED_DHCP_FORCE_MTU=
+RED_ADDRESS=0.0.0.0
+RED_NETMASK=0.0.0.0
+RED_TYPE=PPPOE
+RED_NETADDRESS=0.0.0.0
+
diff --git a/tests/src/initscripts/system/functions/test.sh b/tests/src/initscripts/system/functions/test.sh
new file mode 100755
index 000000000..dbb2d8a62
--- /dev/null
+++ b/tests/src/initscripts/system/functions/test.sh
@@ -0,0 +1,16 @@
+#!/usr/bin/bash
+
+SCRIPT_PATH="$(dirname "$(readlink -f "$0")")"
+
+ROOT="$(readlink -f "${SCRIPT_PATH}/../../../../..")"
+
+. ${ROOT}/tests/lib.sh
+
+. ${ROOT}/src/initscripts/system/functions
+
+# read the date in
+readhash "CONFIG" "${SCRIPT_PATH}/data/1"
+
+# test if we read the correct data
+test_value_in_array "CONFIG" "RED_DHCP_HOSTNAME" "ipfire"
+test_value_in_array "CONFIG" "BLUE_MACADDR" "bc:30:7d:58:6b:e3"
-- 
2.39.2

[PATCH v2 11/18] initscript fkt: ignore blank lines in readhash

[Jonatan Schlag]

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

Signed-off-by: Jonatan Schlag <jonatan.schlag(a)ipfire.org>
---
 src/initscripts/system/functions                            | 6 ++++++
 tests/src/initscripts/system/functions/data/1_output_stderr | 0
 tests/src/initscripts/system/functions/data/1_output_stdout | 0
 tests/src/initscripts/system/functions/test.sh              | 3 +++
 4 files changed, 9 insertions(+)
 create mode 100644 tests/src/initscripts/system/functions/data/1_output_stderr
 create mode 100644 tests/src/initscripts/system/functions/data/1_output_stdout

diff --git a/src/initscripts/system/functions b/src/initscripts/system/functions
index 44ce999d3..3f01be9e0 100644
--- a/src/initscripts/system/functions
+++ b/src/initscripts/system/functions
@@ -900,6 +900,12 @@ readhash() {
 
 	local line
 	while read -r line; do
+
+		# Skip Blank Lines
+		if [[ ${line} =~ ^[[:space:]]*$ ]]; then
+			continue
+		fi
+
 		local key="${line%=*}"
 		local val="${line#*=}"
 
diff --git a/tests/src/initscripts/system/functions/data/1_output_stderr b/tests/src/initscripts/system/functions/data/1_output_stderr
new file mode 100644
index 000000000..e69de29bb
diff --git a/tests/src/initscripts/system/functions/data/1_output_stdout b/tests/src/initscripts/system/functions/data/1_output_stdout
new file mode 100644
index 000000000..e69de29bb
diff --git a/tests/src/initscripts/system/functions/test.sh b/tests/src/initscripts/system/functions/test.sh
index dbb2d8a62..751be6884 100755
--- a/tests/src/initscripts/system/functions/test.sh
+++ b/tests/src/initscripts/system/functions/test.sh
@@ -14,3 +14,6 @@ readhash "CONFIG" "${SCRIPT_PATH}/data/1"
 # test if we read the correct data
 test_value_in_array "CONFIG" "RED_DHCP_HOSTNAME" "ipfire"
 test_value_in_array "CONFIG" "BLUE_MACADDR" "bc:30:7d:58:6b:e3"
+
+test_that_output_is "${SCRIPT_PATH}/data/1_output_stdout" "1" readhash "CONFIG" "${SCRIPT_PATH}/data/1"
+test_that_output_is "${SCRIPT_PATH}/data/1_output_stderr" "2" readhash "CONFIG" "${SCRIPT_PATH}/data/1"
-- 
2.39.2

[PATCH v2 12/18] initscripts fkt: Ignore comments in readhash

[Jonatan Schlag]

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

As '#Another Comment' is a valid key we test this change by checking if
the comments do not end up as keys in our array.

Signed-off-by: Jonatan Schlag <jonatan.schlag(a)ipfire.org>
---
 src/initscripts/system/functions               | 5 +++++
 tests/src/initscripts/system/functions/data/1  | 3 +++
 tests/src/initscripts/system/functions/test.sh | 6 ++++++
 3 files changed, 14 insertions(+)

diff --git a/src/initscripts/system/functions b/src/initscripts/system/functions
index 3f01be9e0..6107463fc 100644
--- a/src/initscripts/system/functions
+++ b/src/initscripts/system/functions
@@ -906,6 +906,11 @@ readhash() {
 			continue
 		fi
 
+		# Skip Comments
+		if [[ ${line} =~ ^#.*$ ]]; then
+			continue
+		fi
+
 		local key="${line%=*}"
 		local val="${line#*=}"
 
diff --git a/tests/src/initscripts/system/functions/data/1 b/tests/src/initscripts/system/functions/data/1
index 8aca9422b..c75620b6b 100644
--- a/tests/src/initscripts/system/functions/data/1
+++ b/tests/src/initscripts/system/functions/data/1
@@ -5,6 +5,7 @@ GREEN_DRIVER=r8175
 RED_DEV=red0
 RED_MACADDR=00:c0:08:8a:a0:56
 RED_DRIVER=r8283
+# Another Comment
 BLUE_DEV='blue0 net0'
 BLUE_MACADDR=bc:30:7d:58:6b:e3
 BLUE_DRIVER=rt2800
@@ -15,3 +16,5 @@ RED_NETMASK=0.0.0.0
 RED_TYPE=PPPOE
 RED_NETADDRESS=0.0.0.0
 
+# Comment for testing  
+    # Comment for testing Comments with spaces before
diff --git a/tests/src/initscripts/system/functions/test.sh b/tests/src/initscripts/system/functions/test.sh
index 751be6884..915f098a0 100755
--- a/tests/src/initscripts/system/functions/test.sh
+++ b/tests/src/initscripts/system/functions/test.sh
@@ -15,5 +15,11 @@ readhash "CONFIG" "${SCRIPT_PATH}/data/1"
 test_value_in_array "CONFIG" "RED_DHCP_HOSTNAME" "ipfire"
 test_value_in_array "CONFIG" "BLUE_MACADDR" "bc:30:7d:58:6b:e3"
 
+# Test that comments are skipped
+# apparently the way we read the file strips the whitespace, so the key does not contain any whitespace either
+test_that_array_doesnt_have_key "CONFIG" "# Another Comment"
+test_that_array_doesnt_have_key "CONFIG" "# Comment for testing"
+test_that_array_doesnt_have_key "CONFIG" "# Comment for testing Comments with spaces before"
+
 test_that_output_is "${SCRIPT_PATH}/data/1_output_stdout" "1" readhash "CONFIG" "${SCRIPT_PATH}/data/1"
 test_that_output_is "${SCRIPT_PATH}/data/1_output_stderr" "2" readhash "CONFIG" "${SCRIPT_PATH}/data/1"
-- 
2.39.2

[PATCH v2 13/18] initscripts fkt: ignore invalid keys in readhash

[Jonatan Schlag]

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

Signed-off-by: Jonatan Schlag <jonatan.schlag(a)ipfire.org>
---
 src/initscripts/system/functions              |  6 ++++++
 tests/src/initscripts/system/functions/data/2 | 20 +++++++++++++++++++
 .../system/functions/data/2_output_stderr     |  4 ++++
 .../system/functions/data/2_output_stdout     |  0
 .../src/initscripts/system/functions/test.sh  | 16 +++++++++++++++
 5 files changed, 46 insertions(+)
 create mode 100644 tests/src/initscripts/system/functions/data/2
 create mode 100644 tests/src/initscripts/system/functions/data/2_output_stderr
 create mode 100644 tests/src/initscripts/system/functions/data/2_output_stdout

diff --git a/src/initscripts/system/functions b/src/initscripts/system/functions
index 6107463fc..bbcfab95d 100644
--- a/src/initscripts/system/functions
+++ b/src/initscripts/system/functions
@@ -914,6 +914,12 @@ readhash() {
 		local key="${line%=*}"
 		local val="${line#*=}"
 
+		# Skip lines with an invalid key
+		if ! [[ ${key} =~ ^[A-Za-z_][A-Za-z0-9_]*$ ]]; then
+			echo "Invalid key '${key}'" >&2
+			continue
+		fi
+
 		printf -v "${array}[${key}]" "%s" "${val}"
 	done < "${file}"
 }
diff --git a/tests/src/initscripts/system/functions/data/2 b/tests/src/initscripts/system/functions/data/2
new file mode 100644
index 000000000..3e1a7028b
--- /dev/null
+++ b/tests/src/initscripts/system/functions/data/2
@@ -0,0 +1,20 @@
+CONFIG_TYPE=3
+GREEN_DEV=green0
+GREEN_MACADDR=00:c0:08:8a:a0:47
+GREEN_DRIVER=r8175
+-RED_DEV=red0
+RE??D_MACADDR=00:c0:08:8a:a0:56
+RED&&_DRIVER=r8283
+# Another Comment
+0BLUE_DEV='blue0 net0'
+BLUE_MACADDR=bc:30:7d:58:6b:e3
+BLUE_DRIVER=rt2800
+RED_DHCP_HOSTNAME=ipfire
+RED_DHCP_FORCE_MTU=
+RED_ADDRESS=0.0.0.0
+RED_NETMASK=0.0.0.0
+RED_TYPE=PPPOE
+RED_NETADDRESS=0.0.0.0
+
+# Comment for testing  
+    # Comment for testing Comments with spaces before
diff --git a/tests/src/initscripts/system/functions/data/2_output_stderr b/tests/src/initscripts/system/functions/data/2_output_stderr
new file mode 100644
index 000000000..dfcf2154b
--- /dev/null
+++ b/tests/src/initscripts/system/functions/data/2_output_stderr
@@ -0,0 +1,4 @@
+Invalid key '-RED_DEV'
+Invalid key 'RE??D_MACADDR'
+Invalid key 'RED&&_DRIVER'
+Invalid key '0BLUE_DEV'
diff --git a/tests/src/initscripts/system/functions/data/2_output_stdout b/tests/src/initscripts/system/functions/data/2_output_stdout
new file mode 100644
index 000000000..e69de29bb
diff --git a/tests/src/initscripts/system/functions/test.sh b/tests/src/initscripts/system/functions/test.sh
index 915f098a0..a2d6535a5 100755
--- a/tests/src/initscripts/system/functions/test.sh
+++ b/tests/src/initscripts/system/functions/test.sh
@@ -23,3 +23,19 @@ test_that_array_doesnt_have_key "CONFIG" "# Comment for testing Comments with sp
 
 test_that_output_is "${SCRIPT_PATH}/data/1_output_stdout" "1" readhash "CONFIG" "${SCRIPT_PATH}/data/1"
 test_that_output_is "${SCRIPT_PATH}/data/1_output_stderr" "2" readhash "CONFIG" "${SCRIPT_PATH}/data/1"
+
+# Check with invalid Lines (values and keys)
+readhash "CONFIG2" "${SCRIPT_PATH}/data/2" &> /dev/null
+
+# test if we read the correct data
+test_value_in_array "CONFIG2" "RED_DHCP_HOSTNAME" "ipfire"
+test_value_in_array "CONFIG2" "BLUE_MACADDR" "bc:30:7d:58:6b:e3"
+
+# We could do some complex checking if we would create functions to check for correct values and keys.
+# We would be then able to mock these function and check if they are correctly called and if the data
+# does not end up in our array.
+# I think the more simpler way of checking the logged errors is the fastes way here.
+test_that_output_is "${SCRIPT_PATH}/data/2_output_stdout" "1" readhash "CONFIG2" "${SCRIPT_PATH}/data/2"
+test_that_output_is "${SCRIPT_PATH}/data/2_output_stderr" "2" readhash "CONFIG2" "${SCRIPT_PATH}/data/2"
+
+
-- 
2.39.2

[PATCH v2 14/18] initscripts fkt: Check for invalid values in readhash

[Jonatan Schlag]

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

Signed-off-by: Jonatan Schlag <jonatan.schlag(a)ipfire.org>
---
 src/initscripts/system/functions                          | 6 ++++++
 tests/src/initscripts/system/functions/data/2             | 8 ++++----
 .../src/initscripts/system/functions/data/2_output_stderr | 4 ++++
 3 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/src/initscripts/system/functions b/src/initscripts/system/functions
index bbcfab95d..ce9d2a5ad 100644
--- a/src/initscripts/system/functions
+++ b/src/initscripts/system/functions
@@ -920,6 +920,12 @@ readhash() {
 			continue
 		fi
 
+		# Skip lines with invalid values
+		if ! [[ ${val} =~ ^[\'][\ A-Za-z0-9=/,.:%_@#+-]*[\']$ ]] && ! [[ ${val} =~ ^[A-Za-z0-9=/,.:%_@#+-]*$ ]]; then
+			echo "Invalid value '${val}' for key '${key}'" >&2
+			continue
+		fi
+
 		printf -v "${array}[${key}]" "%s" "${val}"
 	done < "${file}"
 }
diff --git a/tests/src/initscripts/system/functions/data/2 b/tests/src/initscripts/system/functions/data/2
index 3e1a7028b..3060ad880 100644
--- a/tests/src/initscripts/system/functions/data/2
+++ b/tests/src/initscripts/system/functions/data/2
@@ -1,7 +1,7 @@
-CONFIG_TYPE=3
-GREEN_DEV=green0
-GREEN_MACADDR=00:c0:08:8a:a0:47
-GREEN_DRIVER=r8175
+CONFIG_TYPE=?3
+GREEN_DEV=gree!n0
+GREEN_MACADDR=00:c0:08:8a  :a0:47
+GREEN_DRIVER="r8175"
 -RED_DEV=red0
 RE??D_MACADDR=00:c0:08:8a:a0:56
 RED&&_DRIVER=r8283
diff --git a/tests/src/initscripts/system/functions/data/2_output_stderr b/tests/src/initscripts/system/functions/data/2_output_stderr
index dfcf2154b..f29e94b19 100644
--- a/tests/src/initscripts/system/functions/data/2_output_stderr
+++ b/tests/src/initscripts/system/functions/data/2_output_stderr
@@ -1,3 +1,7 @@
+Invalid value '?3' for key 'CONFIG_TYPE'
+Invalid value 'gree!n0' for key 'GREEN_DEV'
+Invalid value '00:c0:08:8a  :a0:47' for key 'GREEN_MACADDR'
+Invalid value '"r8175"' for key 'GREEN_DRIVER'
 Invalid key '-RED_DEV'
 Invalid key 'RE??D_MACADDR'
 Invalid key 'RED&&_DRIVER'
-- 
2.39.2

[PATCH v2 15/18] initscripts fkt: readhash should only parse lines with a =

[Jonatan Schlag]

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

A line without a = is clearly invalid.

Signed-off-by: Jonatan Schlag <jonatan.schlag(a)ipfire.org>
---
 src/initscripts/system/functions                            | 6 ++++++
 tests/src/initscripts/system/functions/data/2               | 1 +
 tests/src/initscripts/system/functions/data/2_output_stderr | 1 +
 3 files changed, 8 insertions(+)

diff --git a/src/initscripts/system/functions b/src/initscripts/system/functions
index ce9d2a5ad..c9c4c0675 100644
--- a/src/initscripts/system/functions
+++ b/src/initscripts/system/functions
@@ -911,6 +911,12 @@ readhash() {
 			continue
 		fi
 
+		# Skip lines without a =
+		if ! [[ ${line} =~ [^=]*=[^=]*$ ]]; then
+			echo "Invalid line '${line}'" >&2
+			continue
+		fi
+
 		local key="${line%=*}"
 		local val="${line#*=}"
 
diff --git a/tests/src/initscripts/system/functions/data/2 b/tests/src/initscripts/system/functions/data/2
index 3060ad880..760294c30 100644
--- a/tests/src/initscripts/system/functions/data/2
+++ b/tests/src/initscripts/system/functions/data/2
@@ -15,6 +15,7 @@ RED_ADDRESS=0.0.0.0
 RED_NETMASK=0.0.0.0
 RED_TYPE=PPPOE
 RED_NETADDRESS=0.0.0.0
+Line_without_a_equal_sign_is_also_invalid
 
 # Comment for testing  
     # Comment for testing Comments with spaces before
diff --git a/tests/src/initscripts/system/functions/data/2_output_stderr b/tests/src/initscripts/system/functions/data/2_output_stderr
index f29e94b19..82f035e26 100644
--- a/tests/src/initscripts/system/functions/data/2_output_stderr
+++ b/tests/src/initscripts/system/functions/data/2_output_stderr
@@ -6,3 +6,4 @@ Invalid key '-RED_DEV'
 Invalid key 'RE??D_MACADDR'
 Invalid key 'RED&&_DRIVER'
 Invalid key '0BLUE_DEV'
+Invalid line 'Line_without_a_equal_sign_is_also_invalid'
-- 
2.39.2

[PATCH v2 16/18] initscripts fkt: keep readhash compatible with older implementation

[Jonatan Schlag]

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

With the use of eval BLUE_DEV='blue0 net0' stored "blue0 net0" in the
variable BLUE_DEV not "'blue0 net0'"

Signed-off-by: Jonatan Schlag <jonatan.schlag(a)ipfire.org>
---
 src/initscripts/system/functions               | 4 ++++
 tests/src/initscripts/system/functions/test.sh | 1 +
 2 files changed, 5 insertions(+)

diff --git a/src/initscripts/system/functions b/src/initscripts/system/functions
index c9c4c0675..6d72e4119 100644
--- a/src/initscripts/system/functions
+++ b/src/initscripts/system/functions
@@ -932,6 +932,10 @@ readhash() {
 			continue
 		fi
 
+		# strip leading and trailing single quotes
+		val="${val#\'}"
+		val="${val%\'}"
+
 		printf -v "${array}[${key}]" "%s" "${val}"
 	done < "${file}"
 }
diff --git a/tests/src/initscripts/system/functions/test.sh b/tests/src/initscripts/system/functions/test.sh
index a2d6535a5..e7f695f55 100755
--- a/tests/src/initscripts/system/functions/test.sh
+++ b/tests/src/initscripts/system/functions/test.sh
@@ -14,6 +14,7 @@ readhash "CONFIG" "${SCRIPT_PATH}/data/1"
 # test if we read the correct data
 test_value_in_array "CONFIG" "RED_DHCP_HOSTNAME" "ipfire"
 test_value_in_array "CONFIG" "BLUE_MACADDR" "bc:30:7d:58:6b:e3"
+test_value_in_array "CONFIG" "BLUE_DEV" "blue0 net0"
 
 # Test that comments are skipped
 # apparently the way we read the file strips the whitespace, so the key does not contain any whitespace either
-- 
2.39.2

[PATCH v2 17/18] initscripts fkt: Fix shebang

[Jonatan Schlag]

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

We use features only available in bash. So we should state correctly
that the script should be executed in bash. As sh is a symlink to bash
this makes not differences on a ipfire system. But my linter is less
chatty with this change.

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

diff --git a/src/initscripts/system/functions b/src/initscripts/system/functions
index 6d72e4119..0775d74a1 100644
--- a/src/initscripts/system/functions
+++ b/src/initscripts/system/functions
@@ -1,4 +1,4 @@
-#!/bin/sh
+#!/bin/bash
 ###############################################################################
 #                                                                             #
 # IPFire.org - A linux based firewall                                         #
-- 
2.39.2

[PATCH v2 18/18] initscripts fkt: Check that readhash returns 1 on a missing file

[Jonatan Schlag]

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

It already does that, so the function is not changed

Signed-off-by: Jonatan Schlag <jonatan.schlag(a)ipfire.org>
---
 tests/src/initscripts/system/functions/test.sh | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/tests/src/initscripts/system/functions/test.sh b/tests/src/initscripts/system/functions/test.sh
index e7f695f55..dbcbd45ef 100755
--- a/tests/src/initscripts/system/functions/test.sh
+++ b/tests/src/initscripts/system/functions/test.sh
@@ -10,6 +10,7 @@ ROOT="$(readlink -f "${SCRIPT_PATH}/../../../../..")"
 
 # read the date in
 readhash "CONFIG" "${SCRIPT_PATH}/data/1"
+test_command [ $? == 0 ]
 
 # test if we read the correct data
 test_value_in_array "CONFIG" "RED_DHCP_HOSTNAME" "ipfire"
@@ -27,6 +28,8 @@ test_that_output_is "${SCRIPT_PATH}/data/1_output_stderr" "2" readhash "CONFIG"
 
 # Check with invalid Lines (values and keys)
 readhash "CONFIG2" "${SCRIPT_PATH}/data/2" &> /dev/null
+test_command [ $? == 0 ]
+
 
 # test if we read the correct data
 test_value_in_array "CONFIG2" "RED_DHCP_HOSTNAME" "ipfire"
@@ -39,4 +42,6 @@ test_value_in_array "CONFIG2" "BLUE_MACADDR" "bc:30:7d:58:6b:e3"
 test_that_output_is "${SCRIPT_PATH}/data/2_output_stdout" "1" readhash "CONFIG2" "${SCRIPT_PATH}/data/2"
 test_that_output_is "${SCRIPT_PATH}/data/2_output_stderr" "2" readhash "CONFIG2" "${SCRIPT_PATH}/data/2"
 
-
+# Check non existent file
+readhash "CONFIG3" "${SCRIPT_PATH}/data/-1" &> /dev/null
+test_command [ $? == 1 ]
-- 
2.39.2

Re: v2 First patch series for the roadmap item "Get rid of CONFIG_TYPE in the network config"

[Jonatan Schlag]

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

Hi,

Is there anything which needs to be done from my side to get this patch 
set merged? I doubt that it makes sense to work further on this roadmap 
item without the basics being done.

Jonatan

Am 2024-06-16 18:02, schrieb Jonatan Schlag:
> Hi list,
> 
> This is the second version of the first patch series for the roadmap 
> item: 
> https://www.ipfire.org/docs/roadmap/get-rid-of-configtype-in-network-config 
> . This patch series introduces a bash testing library and a function 
> readhash, which we will use in every bash script which needs to read 
> our config files.
> As this patch set is already massive, I thought it might be better to 
> discuss the function first before I introduce it in every script.
> Otherwise, we would talk about 50 patches and not 18. This amount would 
> be somehow not reviewable.
> 
> I kept the [[ over the single [, as from my point of view these are far
> more intutive. No pathname expansion or word splitting takes place in 
> [[
> which is the saver option. See also 
> https://google.github.io/styleguide/shellguide.html#test----and---
> 
> Patches in this series are:
> 
>   # The first 9 patches introduce a bash testing library
>   [PATCH v2 01/18] tests: Add bash lib
>   [PATCH v2 02/18] tests/lib.sh: Add function test_value_in_array
>   [PATCH v2 03/18] tests/lib.sh: Add check if variable exists to
>   [PATCH v2 04/18] tests/lib.sh: Add logging functions
>   [PATCH v2 05/18] tests/lib.sh: adjust to pytest logging style
>   [PATCH v2 06/18] test_value_in_array: Check if the key is defined
>   [PATCH v2 07/18] tests: Add function to test the ouput of a bash
>   [PATCH v2 08/18] test: Add functions test_that_array_is_defined
>   [PATCH v2 09/18] tests: Add functions test_that_array_doesnt_have_key
>   # The next patch add the readhash function
>   [PATCH v2 10/18] initscript functions: add readhash
>   # The next 5 Patches are concerned with error handling
>   [PATCH v2 11/18] initscript fkt: ignore blank lines in readhash
>   [PATCH v2 12/18] initscripts fkt: Ignore comments in readhash
>   [PATCH v2 13/18] initscripts fkt: ignore invalid keys in readhash
>   [PATCH v2 14/18] initscripts fkt: Check for invalid values in 
> readhash
>   [PATCH v2 15/18] initscripts fkt: readhash should only parse lines 
> with a =
>   # The title says it all:
>   [PATCH v2 16/18] initscripts fkt: keep readhash compatible with older
>   # Keep my linter happy
>   [PATCH v2 17/18] initscripts fkt: Fix shebang
>   # Check that we fail on a missing file
>   [PATCH v2 18/18] initscripts fkt: Check that readhash returns 1 on a