Hi list,
This is 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 21. This amount would be somehow not reviewable. If this patch series get merged, fine. If you want to wait for all the patches also fine, but as told, this will get a huge patch set.
Patches in this series are:
[PATCH 01/21] test: Add bash lib for colors [PATCH 02/21] tests: Add bash lib [PATCH 03/21] tests/lib.sh: Add function [PATCH 04/21] tests/lib.sh: Add check if variable exists to [PATCH 05/21] tests/lib.sh: Add logging functions [PATCH 06/21] tests/lib.sh: adjust to pytest logging style [PATCH 07/21] tests/lib.sh: Fix check if array is defined [PATCH 08/21] test_that_key_in_arry_has_value: Check if the key is [PATCH 09/21] tests/lib.sh: Use namref to access an array [PATCH 10/21] test_that_key_in_arry_has_value: Check if a key in an [PATCH 11/21] tests: Add function to test the ouput of a bash [PATCH 12/21] initscript functions: add readhash [PATCH 13/21] initscript fkt: ignore blank lines in readhash [PATCH 14/21] test: Add functions test_that_array_is_defined [PATCH 15/21] tests: Add functions test_that_array_doesnt_have_key [PATCH 16/21] initscripts fkt: Ignore comments in readhash [PATCH 17/21] initscripts fkt: ignore invalid keys in readhash [PATCH 18/21] initscripts fkt: readhash should only parse lines with [PATCH 19/21] initscripts fkt: Check for invalid values in readhash [PATCH 20/21] initscripts fkt: keep readhash compatible with older [PATCH 21/21] initscripts fkt: Fix shebang
Greetings Jonatan
This is borrowed from here: https://git.ipfire.org/?p=network.git;a=blob;f=src/functions/functions.color...
Signed-off-by: Jonatan Schlag jonatan.schlag@ipfire.org --- tests/lib_color.sh | 43 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) create mode 100644 tests/lib_color.sh
diff --git a/tests/lib_color.sh b/tests/lib_color.sh new file mode 100644 index 000000000..6ca1c9864 --- /dev/null +++ b/tests/lib_color.sh @@ -0,0 +1,43 @@ +#!/usr/bin/bash + +# Regular colours +CLR_BLACK_R='\e[0;30m' +CLR_RED_R='\e[0;31m' +CLR_GREEN_R='\e[0;32m' +CLR_YELLOW_R='\e[0;33m' +CLR_BLUE_R='\e[0;34m' +CLR_PURPLE_R='\e[0;35m' +CLR_CYAN_R='\e[0;36m' +CLR_WHITE_R='\e[0;37m' + +# Bold colours +CLR_BLACK_B='\e[1;30m' +CLR_RED_B='\e[1;31m' +CLR_GREEN_B='\e[1;32m' +CLR_YELLOW_B='\e[1;33m' +CLR_BLUE_B='\e[1;34m' +CLR_PURPLE_B='\e[1;35m' +CLR_CYAN_B='\e[1;36m' +CLR_WHITE_B='\e[1;37m' + +# Background colors +CLR_BLACK_BG='\e[40m' +CLR_RED_BG='\e[41m' +CLR_GREEN_BG='\e[42m' +CLR_YELLOW_BG='\e[43m' +CLR_BLUE_BG='\e[44m' +CLR_PURPLE_BG='\e[45m' +CLR_CYAN_BG='\e[46m' +CLR_WHITE_BG='\e[47m' + +# Font decoration +FONT_RESET="\e[0m" +FONT_BOLD="\e[1m" +FONT_UNDERLINED="\e[4m" +FONT_BLINKING="\e[5m" +FONT_INVERTED="\e[7m" + +# Reset everything +CLR_RESET="${FONT_RESET}" + +
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 | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) create mode 100644 tests/lib.sh
diff --git a/tests/lib.sh b/tests/lib.sh new file mode 100644 index 000000000..7749d5158 --- /dev/null +++ b/tests/lib.sh @@ -0,0 +1,20 @@ +#!/usr/bin/bash + +LIB_DIR="$(dirname "$(readlink -f "${BASH_SOURCE[0]}")")" + +. ${LIB_DIR}/lib_color.sh + +test_that() { + + 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}" ]] +}
Hello,
On 20 May 2024, at 10:05, Jonatan Schlag jonatan.schlag@ipfire.org wrote:
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 | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) create mode 100644 tests/lib.sh
diff --git a/tests/lib.sh b/tests/lib.sh new file mode 100644 index 000000000..7749d5158 --- /dev/null +++ b/tests/lib.sh @@ -0,0 +1,20 @@ +#!/usr/bin/bash
+LIB_DIR="$(dirname "$(readlink -f "${BASH_SOURCE[0]}")")"
???
I don’t think this is very intuitive.
+. ${LIB_DIR}/lib_color.sh
+test_that() {
- 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
+}
We have had so many discussions about how to name functions. “That” is quite generic in my opinion.
Why not “test_command” or something like that?
+var_has_value() {
- [[ "${!1}" == "${2}" ]]
+}
2.39.2
Hi,
Am Freitag, dem 31.05.2024 um 10:47 +0100 schrieb Michael Tremer:
Hello,
On 20 May 2024, at 10:05, Jonatan Schlag jonatan.schlag@ipfire.org wrote:
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 | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) create mode 100644 tests/lib.sh
diff --git a/tests/lib.sh b/tests/lib.sh new file mode 100644 index 000000000..7749d5158 --- /dev/null +++ b/tests/lib.sh @@ -0,0 +1,20 @@ +#!/usr/bin/bash
+LIB_DIR="$(dirname "$(readlink -f "${BASH_SOURCE[0]}")")"
???
I don’t think this is very intuitive.
Ok, what am I supposed to do about it? I need the path where this file is located, and I cannot hard-code it. Should I add a comment?
Jonatan
+. ${LIB_DIR}/lib_color.sh
+test_that() {
- 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
+}
We have had so many discussions about how to name functions. “That” is quite generic in my opinion.
Why not “test_command” or something like that?
+var_has_value() {
- [[ "${!1}" == "${2}" ]]
+}
2.39.2
Hello,
On 2 Jun 2024, at 18:49, Jonatan Schlag jonatan.schlag@ipfire.org wrote:
Hi,
Am Freitag, dem 31.05.2024 um 10:47 +0100 schrieb Michael Tremer:
Hello,
On 20 May 2024, at 10:05, Jonatan Schlag jonatan.schlag@ipfire.org wrote:
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 | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) create mode 100644 tests/lib.sh
diff --git a/tests/lib.sh b/tests/lib.sh new file mode 100644 index 000000000..7749d5158 --- /dev/null +++ b/tests/lib.sh @@ -0,0 +1,20 @@ +#!/usr/bin/bash
+LIB_DIR="$(dirname "$(readlink -f "${BASH_SOURCE[0]}")")"
???
I don’t think this is very intuitive.
Ok, what am I supposed to do about it? I need the path where this file is located, and I cannot hard-code it. Should I add a comment?
Generally speaking, I like to split things into one line per task. So that it is very easy to see what is happening when reading the code. This approach also has some more advantages. For example, you can easily put a print statement between lines to see if you are getting the output that you are expecting. That makes debugging nice and easy.
When I say generally, there are of course exceptions like very trivial things that can often be put into the same line.
Commenting is also a lot easier when you only have to comment one thing at once. You need to add way more comments, please.
-Michael
Jonatan
+. ${LIB_DIR}/lib_color.sh
+test_that() {
- 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
+}
We have had so many discussions about how to name functions. “That” is quite generic in my opinion.
Why not “test_command” or something like that?
+var_has_value() {
- [[ "${!1}" == "${2}" ]]
+}
2.39.2
I think the name says it all.
Signed-off-by: Jonatan Schlag jonatan.schlag@ipfire.org --- tests/lib.sh | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
diff --git a/tests/lib.sh b/tests/lib.sh index 7749d5158..c51257e79 100644 --- a/tests/lib.sh +++ b/tests/lib.sh @@ -18,3 +18,17 @@ test_that() { var_has_value() { [[ "${!1}" == "${2}" ]] } + +test_that_key_in_arry_has_value() { + local 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 +}
On 20 May 2024, at 10:05, Jonatan Schlag jonatan.schlag@ipfire.org wrote:
I think the name says it all.
Signed-off-by: Jonatan Schlag jonatan.schlag@ipfire.org
tests/lib.sh | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
diff --git a/tests/lib.sh b/tests/lib.sh index 7749d5158..c51257e79 100644 --- a/tests/lib.sh +++ b/tests/lib.sh @@ -18,3 +18,17 @@ test_that() { var_has_value() { [[ "${!1}" == "${2}" ]] }
+test_that_key_in_arry_has_value() {
This is again very long and has a typo.
- local 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
+}
2.39.2
Signed-off-by: Jonatan Schlag jonatan.schlag@ipfire.org --- tests/lib.sh | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/tests/lib.sh b/tests/lib.sh index c51257e79..373b7c3a0 100644 --- a/tests/lib.sh +++ b/tests/lib.sh @@ -24,6 +24,11 @@ test_that_key_in_arry_has_value() { local key="${2}" local value="${3}"
+ if [[ ! -v "${array}" ]]; 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
On 20 May 2024, at 10:05, Jonatan Schlag jonatan.schlag@ipfire.org wrote:
Signed-off-by: Jonatan Schlag jonatan.schlag@ipfire.org
tests/lib.sh | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/tests/lib.sh b/tests/lib.sh index c51257e79..373b7c3a0 100644 --- a/tests/lib.sh +++ b/tests/lib.sh @@ -24,6 +24,11 @@ test_that_key_in_arry_has_value() { local key="${2}" local value="${3}"
- if [[ ! -v "${array}" ]]; then
- echo -e "${CLR_RED_BG}Test failed: The array '${1}' does not exists. The variable is not set.${CLR_RESET}'"
Typo.
You never want to use double brackets unless you have something that [ does not understand. As far as I know this is only regular expressions.
- 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
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 373b7c3a0..6483c41c3 100644 --- a/tests/lib.sh +++ b/tests/lib.sh @@ -4,13 +4,21 @@ LIB_DIR="$(dirname "$(readlink -f "${BASH_SOURCE[0]}")")"
. ${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_that() {
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 } @@ -25,15 +33,15 @@ test_that_key_in_arry_has_value() { local value="${3}"
if [[ ! -v "${array}" ]]; 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.
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 6483c41c3..079755410 100644 --- a/tests/lib.sh +++ b/tests/lib.sh @@ -5,11 +5,11 @@ LIB_DIR="$(dirname "$(readlink -f "${BASH_SOURCE[0]}")")" . ${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_that() {
Hello,
You are making an assumption that people have a certain background color of their terminal...
On 20 May 2024, at 10:05, Jonatan Schlag jonatan.schlag@ipfire.org wrote:
Black on white is still the best to read. So we only style FAILED or PASSED in green or red.
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 6483c41c3..079755410 100644 --- a/tests/lib.sh +++ b/tests/lib.sh @@ -5,11 +5,11 @@ LIB_DIR="$(dirname "$(readlink -f "${BASH_SOURCE[0]}")")" . ${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_that() {
2.39.2
-v does a bad job here. We need to use declare here.
Signed-off-by: Jonatan Schlag jonatan.schlag@ipfire.org --- tests/lib.sh | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/tests/lib.sh b/tests/lib.sh index 079755410..0f4de8e43 100644 --- a/tests/lib.sh +++ b/tests/lib.sh @@ -29,10 +29,11 @@ var_has_value() {
test_that_key_in_arry_has_value() { local array="${!1}" + local arrayname="${1}" local key="${2}" local value="${3}"
- if [[ ! -v "${array}" ]]; then + 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 fi
On 20 May 2024, at 10:05, Jonatan Schlag jonatan.schlag@ipfire.org wrote:
-v does a bad job here. We need to use declare here.
Signed-off-by: Jonatan Schlag jonatan.schlag@ipfire.org
tests/lib.sh | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/tests/lib.sh b/tests/lib.sh index 079755410..0f4de8e43 100644 --- a/tests/lib.sh +++ b/tests/lib.sh @@ -29,10 +29,11 @@ var_has_value() {
test_that_key_in_arry_has_value() { local array="${!1}"
- local arrayname="${1}"
local key="${2}" local value="${3}"
- if [[ ! -v "${array}" ]]; then
- 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
Please comment code like this. It is hard to read and not obvious to understand.
fi
2.39.2
Signed-off-by: Jonatan Schlag jonatan.schlag@ipfire.org --- tests/lib.sh | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/tests/lib.sh b/tests/lib.sh index 0f4de8e43..006862da6 100644 --- a/tests/lib.sh +++ b/tests/lib.sh @@ -38,6 +38,11 @@ test_that_key_in_arry_has_value() { return 1 fi
+ if [[ ! -v "${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
! does not work here. 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 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/lib.sh b/tests/lib.sh index 006862da6..ad7c12cb0 100644 --- a/tests/lib.sh +++ b/tests/lib.sh @@ -28,7 +28,7 @@ var_has_value() { }
test_that_key_in_arry_has_value() { - local array="${!1}" + local -n array="${1}" local arrayname="${1}" local key="${2}" local value="${3}"
Signed-off-by: Jonatan Schlag jonatan.schlag@ipfire.org --- tests/lib.sh | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/tests/lib.sh b/tests/lib.sh index ad7c12cb0..81bc1439c 100644 --- a/tests/lib.sh +++ b/tests/lib.sh @@ -38,7 +38,8 @@ test_that_key_in_arry_has_value() { return 1 fi
- if [[ ! -v "${array[${key}]}" ]]; then + + if [[ "${array["${key}"]+_}" == "" ]]; then log_test_failed "The array does not contain the key '${key}', valid keys are: ${!array[*]}" return 1 fi
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 81bc1439c..9ce0201b1 100644 --- a/tests/lib.sh +++ b/tests/lib.sh @@ -52,3 +52,32 @@ test_that_key_in_arry_has_value() { 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}" +}
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..ec502e199 --- /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_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"
Hello,
On 20 May 2024, at 10:06, Jonatan Schlag jonatan.schlag@ipfire.org wrote:
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}"
+}
Okay, so here we are getting to the main bit.
I think this is nice and simply. The function creates the array regardless and would empty any content which is what we want.
You should however check if the file exists and can be read as the function would now not return any error. It would write some error message, but you cannot programmatically check that.
Should we have a matching writehash function?
-Michael
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..ec502e199 --- /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_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"
2.39.2
Hi,
Am Freitag, dem 31.05.2024 um 10:53 +0100 schrieb Michael Tremer:
Hello,
On 20 May 2024, at 10:06, Jonatan Schlag jonatan.schlag@ipfire.org wrote:
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}"
+}
Okay, so here we are getting to the main bit.
I think this is nice and simply. The function creates the array regardless and would empty any content which is what we want.
Thank you.
You should however check if the file exists and can be read as the function would now not return any error. It would write some error message, but you cannot programmatically check that.
I will add code for that.
Should we have a matching writehash function?
I would add this function when we need it. Less dead code :-).
Jonatan
-Michael
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..ec502e199 --- /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_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" -- 2.39.2
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 ec502e199..8d644b8cd 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_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"
On 20 May 2024, at 10:06, Jonatan Schlag jonatan.schlag@ipfire.org wrote:
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#*=}"
I don’t think this is quite sufficient.
You might have other lines that are invalid. For example “=VALUE”. I think we should check that key isn’t empty, and we should limit what characters can be used in keys and values as it used to be before in the script.
https://git.ipfire.org/?p=ipfire-2.x.git;a=blob;f=src/scripts/readhash;h=bff...
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 ec502e199..8d644b8cd 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_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
Hi,
Am Freitag, dem 31.05.2024 um 10:55 +0100 schrieb Michael Tremer:
On 20 May 2024, at 10:06, Jonatan Schlag jonatan.schlag@ipfire.org wrote:
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#*=}"
I don’t think this is quite sufficient.
You might have other lines that are invalid. For example “=VALUE”. I think we should check that key isn’t empty, and we should limit what characters can be used in keys and values as it used to be before in the script.
https://git.ipfire.org/?p=ipfire-2.x.git;a=blob;f=src/scripts/readhash;h=bff...
Hi,
I will merge all commits for checks in the next series. There are more checks to come. In a later commit, I check for correct values and keys.
Jonatan
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 ec502e199..8d644b8cd 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_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
Your tests I didn’t understand at all. You need to structure those in a more simple way.
On 2 Jun 2024, at 19:07, Jonatan Schlag jonatan.schlag@ipfire.org wrote:
Hi,
Am Freitag, dem 31.05.2024 um 10:55 +0100 schrieb Michael Tremer:
On 20 May 2024, at 10:06, Jonatan Schlag jonatan.schlag@ipfire.org wrote:
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#*=}"
I don’t think this is quite sufficient.
You might have other lines that are invalid. For example “=VALUE”. I think we should check that key isn’t empty, and we should limit what characters can be used in keys and values as it used to be before in the script.
https://git.ipfire.org/?p=ipfire-2.x.git;a=blob;f=src/scripts/readhash;h=bff...
Hi,
I will merge all commits for checks in the next series. There are more checks to come. In a later commit, I check for correct values and keys.
Jonatan
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 ec502e199..8d644b8cd 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_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
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 | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/tests/lib.sh b/tests/lib.sh index 9ce0201b1..29f4e3b71 100644 --- a/tests/lib.sh +++ b/tests/lib.sh @@ -27,17 +27,25 @@ var_has_value() { [[ "${!1}" == "${2}" ]] }
-test_that_key_in_arry_has_value() { - local -n array="${1}" +test_that_array_is_defined() { local arrayname="${1}" - local key="${2}" - local value="${3}"
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_that_key_in_arry_has_value() { + local -n array="${1}" + local arrayname="${1}" + local key="${2}" + local value="${3}"
+ test_that_array_is_defined "${arrayname}" || return 1
if [[ "${array["${key}"]+_}" == "" ]]; then log_test_failed "The array does not contain the key '${key}', valid keys are: ${!array[*]}"
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 29f4e3b71..1eab92cf7 100644 --- a/tests/lib.sh +++ b/tests/lib.sh @@ -61,6 +61,24 @@ test_that_key_in_arry_has_value() { 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}"
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 8d644b8cd..f1348a9e8 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_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 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 | 11 ++++++++++ 5 files changed, 41 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 f1348a9e8..9981cc732 100755 --- a/tests/src/initscripts/system/functions/test.sh +++ b/tests/src/initscripts/system/functions/test.sh @@ -23,3 +23,14 @@ 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_that_key_in_arry_has_value "CONFIG2" "RED_DHCP_HOSTNAME" "ipfire" +test_that_key_in_arry_has_value "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"
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 bbcfab95d..38a1316a8 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 3e1a7028b..d1c4d327d 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 dfcf2154b..cccd19af6 100644 --- a/tests/src/initscripts/system/functions/data/2_output_stderr +++ b/tests/src/initscripts/system/functions/data/2_output_stderr @@ -2,3 +2,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'
On 20 May 2024, at 10:06, Jonatan Schlag jonatan.schlag@ipfire.org wrote:
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 bbcfab95d..38a1316a8 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#*=}"
You can simply check if key and value have content. That should be a lot easier than whatever this is.
diff --git a/tests/src/initscripts/system/functions/data/2 b/tests/src/initscripts/system/functions/data/2 index 3e1a7028b..d1c4d327d 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 dfcf2154b..cccd19af6 100644 --- a/tests/src/initscripts/system/functions/data/2_output_stderr +++ b/tests/src/initscripts/system/functions/data/2_output_stderr @@ -2,3 +2,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
Hi,
Am Freitag, dem 31.05.2024 um 10:56 +0100 schrieb Michael Tremer:
On 20 May 2024, at 10:06, Jonatan Schlag jonatan.schlag@ipfire.org wrote:
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 bbcfab95d..38a1316a8 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#*=}"
You can simply check if key and value have content. That should be a lot easier than whatever this is.
Hi,
Unfortunately not. A line like
Line_without_a_equal_sign_is_also_invalid
result in an entry like this:
Key = Line_without_a_equal_sign_is_also_invalid Value = Line_without_a_equal_sign_is_also_invalid
We simply do not split without an equal sign, so key and value are equal. And maybe are also valid keys and values.
Jonatan
diff --git a/tests/src/initscripts/system/functions/data/2 b/tests/src/initscripts/system/functions/data/2 index 3e1a7028b..d1c4d327d 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 dfcf2154b..cccd19af6 100644 --- a/tests/src/initscripts/system/functions/data/2_output_stderr +++ b/tests/src/initscripts/system/functions/data/2_output_stderr @@ -2,3 +2,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
Hello,
On 2 Jun 2024, at 19:09, Jonatan Schlag jonatan.schlag@ipfire.org wrote:
Hi,
Am Freitag, dem 31.05.2024 um 10:56 +0100 schrieb Michael Tremer:
On 20 May 2024, at 10:06, Jonatan Schlag jonatan.schlag@ipfire.org wrote:
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 bbcfab95d..38a1316a8 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#*=}"
You can simply check if key and value have content. That should be a lot easier than whatever this is.
Hi,
Unfortunately not. A line like
Line_without_a_equal_sign_is_also_invalid
Okay, let’s say this is indeed an empty line (and not just a key without a value).
result in an entry like this:
Key = Line_without_a_equal_sign_is_also_invalid Value = Line_without_a_equal_sign_is_also_invalid
Yes, you can’t use the parameter expansions then and need to check first if you have an = character at all (you can simply do that with a case statement instead of a complicated regular expression.
We simply do not split without an equal sign, so key and value are equal. And maybe are also valid keys and values.
That isn’t a very good check. It could theoretically happen.
-Michael
Jonatan
diff --git a/tests/src/initscripts/system/functions/data/2 b/tests/src/initscripts/system/functions/data/2 index 3e1a7028b..d1c4d327d 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 dfcf2154b..cccd19af6 100644 --- a/tests/src/initscripts/system/functions/data/2_output_stderr +++ b/tests/src/initscripts/system/functions/data/2_output_stderr @@ -2,3 +2,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
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 38a1316a8..c9c4c0675 100644 --- a/src/initscripts/system/functions +++ b/src/initscripts/system/functions @@ -926,6 +926,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 d1c4d327d..760294c30 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 cccd19af6..82f035e26 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'
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 9981cc732..1a715d7d9 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_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_key_in_arry_has_value "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 #
Hello Jonatan,
On 20 May 2024, at 10:05, Jonatan Schlag jonatan.schlag@ipfire.org wrote:
Hi list,
This is 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 21. This amount would be somehow not reviewable. If this patch series get merged, fine. If you want to wait for all the patches also fine, but as told, this will get a huge patch set.
Yes, I am happy to have smaller patch sets so that we get those changes shipped. They have so many advantages. We must however not forget to continue working on those projects after the first steps have been merged, otherwise we have tons of dead code in the distribution.
I will reply to the individual patches.
Generally, this is too many emails. I think you might want to condense things slightly more using “git rebase -i” to squash fixups together with the original commit.
-Michael
Patches in this series are:
[PATCH 01/21] test: Add bash lib for colors [PATCH 02/21] tests: Add bash lib [PATCH 03/21] tests/lib.sh: Add function [PATCH 04/21] tests/lib.sh: Add check if variable exists to [PATCH 05/21] tests/lib.sh: Add logging functions [PATCH 06/21] tests/lib.sh: adjust to pytest logging style [PATCH 07/21] tests/lib.sh: Fix check if array is defined [PATCH 08/21] test_that_key_in_arry_has_value: Check if the key is [PATCH 09/21] tests/lib.sh: Use namref to access an array [PATCH 10/21] test_that_key_in_arry_has_value: Check if a key in an [PATCH 11/21] tests: Add function to test the ouput of a bash [PATCH 12/21] initscript functions: add readhash [PATCH 13/21] initscript fkt: ignore blank lines in readhash [PATCH 14/21] test: Add functions test_that_array_is_defined [PATCH 15/21] tests: Add functions test_that_array_doesnt_have_key [PATCH 16/21] initscripts fkt: Ignore comments in readhash [PATCH 17/21] initscripts fkt: ignore invalid keys in readhash [PATCH 18/21] initscripts fkt: readhash should only parse lines with [PATCH 19/21] initscripts fkt: Check for invalid values in readhash [PATCH 20/21] initscripts fkt: keep readhash compatible with older [PATCH 21/21] initscripts fkt: Fix shebang
Greetings Jonatan
-
Jonatan Schlag -
Michael Tremer