v2 First patch series for the roadmap item "Get rid of CONFIG_TYPE in the network config"
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
This allows use to write test with less effort as we can reuse functions
Signed-off-by: Jonatan Schlag jonatan.schlag@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}" ]] +}
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...
Signed-off-by: Jonatan Schlag jonatan.schlag@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" + +
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@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
So we can change the style of our log messages better.
Signed-off-by: Jonatan Schlag jonatan.schlag@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 }
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@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() {
Signed-off-by: Jonatan Schlag jonatan.schlag@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... + 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
Signed-off-by: Jonatan Schlag jonatan.schlag@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}" +}
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@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
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@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}"
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@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"
Signed-off-by: Jonatan Schlag jonatan.schlag@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"
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@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"
Signed-off-by: Jonatan Schlag jonatan.schlag@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" + +
Signed-off-by: Jonatan Schlag jonatan.schlag@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'
A line without a = is clearly invalid.
Signed-off-by: Jonatan Schlag jonatan.schlag@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'
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@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
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@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 #
It already does that, so the function is not changed
Signed-off-by: Jonatan Schlag jonatan.schlag@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 ]
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
-
Jonatan Schlag