From mboxrd@z Thu Jan  1 00:00:00 1970
From: Michael Tremer <michael.tremer@ipfire.org>
To: development@lists.ipfire.org
Subject:
 Re: [Patch RFC 10/15] network startup: Refactor how cmd args are processed
Date: Wed, 24 May 2023 10:00:33 +0100
Message-ID: <428C99C7-53A0-4C1C-B4AB-53C77EEA7CC2@ipfire.org>
In-Reply-To: <20230523172314.7826-11-jonatan.schlag@ipfire.org>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="===============0743763090551446060=="
List-Id: <development.lists.ipfire.org>

--===============0743763090551446060==
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable

Phew, this has already been a rubbish script.

One option could have been using =E2=80=9Cprintf -v=E2=80=9D to get rid of th=
e eval call.

Otherwise, I fail to see the improvement here? It is just a different way to =
write this?!

Since you have introduced new functions to check whether a certain zone is av=
ailable, why no just call those functions like this:

  if network_has_BLUE; then
    /etc/init.d/networking/local BLUE start
  fi

-Michael

> On 23 May 2023, at 18:23, Jonatan Schlag <jonatan.schlag(a)ipfire.org> wrot=
e:
>=20
> This avoids eval and all other sorts of things. We also now exit when we
> get an invalid zone name.
>=20
> Signed-off-by: Jonatan Schlag <jonatan.schlag(a)ipfire.org>
> ---
> src/initscripts/system/network | 52 ++++++++++++++++++++--------------
> 1 file changed, 31 insertions(+), 21 deletions(-)
>=20
> diff --git a/src/initscripts/system/network b/src/initscripts/system/network
> index 06240f53c..008fbbe2b 100644
> --- a/src/initscripts/system/network
> +++ b/src/initscripts/system/network
> @@ -31,41 +31,51 @@ if ! [[ "${DO}" =3D=3D "start" ||  "${DO}" =3D=3D "rest=
art" || "${DO}" =3D=3D "stop" ]]; the
> exit 1
> fi
>=20
> -if [ -n "${1}" ]; then
> - ALL=3D0
> - for i in green red blue orange; do
> - eval "${i}=3D0"
> - done
> -else
> - ALL=3D1
> - for i in green red blue orange; do
> - eval "${i}=3D1"
> - done
> +declare -A ZONE_ACTION
> +
> +ZONE_ACTION[blue]=3Dfalse
> +ZONE_ACTION[green]=3Dfalse
> +ZONE_ACTION[orange]=3Dfalse
> +ZONE_ACTION[red]=3Dfalse
> +
> +if [ $# -eq 0 ]; then
> + ZONE_ACTION[blue]=3Dtrue
> + ZONE_ACTION[green]=3Dtrue
> + ZONE_ACTION[orange]=3Dtrue
> + ZONE_ACTION[red]=3Dtrue
> fi
>=20
> -while [ ! $# =3D 0 ]; do
> +while [ $# -ne 0 ]; do
> + ZONE_VALID=3Dfalse
> for i in green red blue orange; do
> if [ "${i}" =3D=3D "${1}" ]; then
> - eval "${i}=3D1"
> + ZONE_ACTION[${i}]=3Dtrue
> + ZONE_VALID=3Dtrue
> shift
> + break
> fi
> done
> +
> + if ! ${ZONE_VALID}; then
> + echo "'${1}' is not a valid zone. Cannot go on."
> + exit 1
> + fi
> done
>=20
> case "${DO}" in
> start)
> # Starting interfaces...
> # GREEN
> - [ "$green" =3D=3D "1" ] && /etc/rc.d/init.d/networking/green start
> + ${ZONE_ACTION[green]} && /etc/rc.d/init.d/networking/green start
>=20
> # BLUE
> - [ "$blue" =3D=3D "1" ] && /etc/rc.d/init.d/networking/blue start
> + ${ZONE_ACTION[blue]} && /etc/rc.d/init.d/networking/blue start
>=20
> # ORANGE
> - [ "$orange" =3D=3D "1" ] && /etc/rc.d/init.d/networking/orange start
> + ${ZONE_ACTION[orange]} && /etc/rc.d/init.d/networking/orange start
>=20
> # RED
> - [ "$red" =3D=3D "1" ] && /etc/rc.d/init.d/networking/red start
> + ${ZONE_ACTION[red]} && /etc/rc.d/init.d/networking/red start
>=20
> boot_mesg "Mounting network file systems..."
> mount -a -O _netdev
> @@ -79,16 +89,16 @@ case "${DO}" in
>=20
> # Stopping interfaces...
> # GREEN
> - [ "$green" =3D=3D "1" ] && /etc/rc.d/init.d/networking/green stop
> + ${ZONE_ACTION[green]} && /etc/rc.d/init.d/networking/green stop
>=20
> # BLUE
> - [ "$blue" =3D=3D "1" ] && /etc/rc.d/init.d/networking/blue stop
> + ${ZONE_ACTION[blue]} && /etc/rc.d/init.d/networking/blue stop
>=20
> # ORANGE
> - [ "$orange" =3D=3D "1" ] && /etc/rc.d/init.d/networking/orange stop
> + ${ZONE_ACTION[orange]} && /etc/rc.d/init.d/networking/orange stop
>=20
> # RED
> - if [ "$red" =3D=3D "1" ]; then
> + if ${ZONE_ACTION[red]}; then
> /etc/rc.d/init.d/networking/red stop
> fi
>=20
> @@ -97,7 +107,7 @@ case "${DO}" in
>=20
> restart)
> for i in green red blue orange; do
> - if [ "${!i}" =3D=3D "1" ]; then
> + if {ZONE_ACTION[${i}]}; then
> ARGS+=3D" ${i}"
> fi
> done
> --=20
> 2.30.2
>=20


--===============0743763090551446060==--