diff mbox series

[12/21] initscript functions: add readhash

Message ID	20240520090611.10406-13-jonatan.schlag@ipfire.org
State	New
Headers	From: Jonatan Schlag <jonatan.schlag@ipfire.org> To: development@lists.ipfire.org Subject: [PATCH 12/21] initscript functions: add readhash Date: Mon, 20 May 2024 11:06:02 +0200 Message-Id: <20240520090611.10406-13-jonatan.schlag@ipfire.org> In-Reply-To: <20240520090611.10406-1-jonatan.schlag@ipfire.org> References: <20240520090611.10406-1-jonatan.schlag@ipfire.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Message-ID-Hash: YZH2BSTE7MXTGNPEZLVRM2XHHKRGSIY2 Precedence: list Archived-At: <https://lists.ipfire.org/hyperkitty/list/development@lists.ipfire.org/message/YZH2BSTE7MXTGNPEZLVRM2XHHKRGSIY2/>
Series	[01/21] test: Add bash lib for colors \| [01/21] test: Add bash lib for colors [02/21] tests: Add bash lib [03/21] tests/lib.sh: Add function test_that_key_in_arry_has_value [04/21] tests/lib.sh: Add check if variable exists to test_that_key_in_arry_has_value [05/21] tests/lib.sh: Add logging functions [06/21] tests/lib.sh: adjust to pytest logging style [07/21] tests/lib.sh: Fix check if array is defined [08/21] test_that_key_in_arry_has_value: Check if the key is defined [09/21] tests/lib.sh: Use namref to access an array [10/21] test_that_key_in_arry_has_value: Check if a key in an array exists [11/21] tests: Add function to test the ouput of a bash function [12/21] initscript functions: add readhash [13/21] initscript fkt: ignore blank lines in readhash [14/21] test: Add functions test_that_array_is_defined [15/21] tests: Add functions test_that_array_doesnt_have_key [16/21] initscripts fkt: Ignore comments in readhash [17/21] initscripts fkt: ignore invalid keys in readhash [18/21] initscripts fkt: readhash should only parse lines with a = [19/21] initscripts fkt: Check for invalid values in readhash [20/21] initscripts fkt: keep readhash compatible with older implementation [21/21] initscripts fkt: Fix shebang

Commit Message

Jonatan Schlag May 20, 2024, 9:06 a.m. UTC

  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

Comments

Michael Tremer May 31, 2024, 9:53 a.m. UTC | #1

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
>

Jonatan Schlag June 2, 2024, 6:05 p.m. UTC | #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
> > 
>

diff mbox series

Patch

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"