From mboxrd@z Thu Jan 1 00:00:00 1970 From: Michael Tremer To: development@lists.ipfire.org Subject: Re: [Patch RFC 04/15] network initscripts: check if the zone in the current config exists Date: Mon, 21 Aug 2023 10:40:46 +0100 Message-ID: <18BEF8B3-64C7-49CD-8536-A8C6B2B7C0A5@ipfire.org> In-Reply-To: MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============3025176893804418868==" List-Id: --===============3025176893804418868== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hello, > On 18 Aug 2023, at 13:55, Jonatan Schlag wrot= e: >=20 > Hi list, >=20 > Am Mittwoch, dem 24.05.2023 um 10:00 +0100 schrieb Michael Tremer: >> Hello, >>=20 >> If you want to make this script better, why not give the zone name as >> a parameter? >>=20 >> First of all it should be renamed to something like =E2=80=9Clocal=E2=80= =9D (as in: >> not RED) and then you don=E2=80=99t have to use any symlinks and read back >> the script name like this: >>=20 >> ZONE=3D"$(basename $0)=E2=80=9D >>=20 >=20 > I did not want to make such a big change, as people might rely on these > files. But I am happy to create a script and delete the other > scripts/links. I suggest as naming: >=20 > /etc/init.d/networking/local >=20 > Calling the script could look like: >=20 > For one Zone >=20 > /etc/init.d/networking/local start ${ZONE} > /etc/init.d/networking/local stop ${ZONE} >=20 > For all Zones >=20 > /etc/init.d/networking/local start > /etc/init.d/networking/local stop >=20 > Look this fine to you? This looks good to me. >> Otherwise I agree with introducing unified functions that check >> whether we have GREEN/BLUE/ORANGE and have them in one function. We >> should then go through the entire code base and replace those calls >> first before we are introducing any new changes. >=20 > Ok, fine by me. >>=20 >> I would prefer to give those function a =E2=80=9Cnetwork_=E2=80=9D prefix = so that it >> becomes clear where this function is from and ideally shorten =E2=80=9Cis >> used=E2=80=9D to just =E2=80=9Chave=E2=80=9D. Like so: network_have_BLUE()= and >> network_have_ORANGE(). >>=20 >> Can we do that instead? >=20 >=20 > Yes, we can do that. I have only comments on the naming of the > functions. I would like to reuse the naming style of the old ipfire-3.x > networking code. So I would propose: >=20 > network_zone_exists(ZONE) >=20 > network_zones_get_all() >=20 > network_zones_get_local() >=20 > network_zones_get_nonlocal() >=20 >=20 > As we all agreed on this naming some time ago, I think it faster to use > these names instead of inventing new ones. Also okay for me. Best, -Michael >=20 > Greetings Jonatan >>=20 >> -Michael >>=20 >>> On 23 May 2023, at 18:23, Jonatan Schlag >>> wrote: >>>=20 >>> We check in /etc/init.d/network if the current Configuration >>> (RED+GREEN >>> or RED+GREEN+BLUE) contains the zone we want to start or stop. >>> We do this not in /etc/init.d/networking/green,blue,orange >>>=20 >>> As this checks make sense also there and as these scripts are >>> called >>> form /etc/init.d/network I moved the check to these scripts. >>>=20 >>> As CONFIG_TYPE =3D=3D 2 is unreadable I wrote functions to make things >>> at least a >>> litte bit prettier. >>>=20 >>> Signed-off-by: Jonatan Schlag >>> --- >>> src/initscripts/networking/any | 27 >>> +++++++++++++++++--- >>> src/initscripts/networking/functions.network | 12 +++++++++ >>> src/initscripts/system/network | 16 ++++-------- >>> 3 files changed, 41 insertions(+), 14 deletions(-) >>>=20 >>> diff --git a/src/initscripts/networking/any >>> b/src/initscripts/networking/any >>> index dc4796e91..6dba5bef9 100644 >>> --- a/src/initscripts/networking/any >>> +++ b/src/initscripts/networking/any >>> @@ -21,23 +21,44 @@ >>>=20 >>> . /etc/sysconfig/rc >>> . ${rc_functions} >>> +. /etc/init.d/networking/functions.network >>> + >>> eval $(/usr/local/bin/readhash /var/ipfire/ethernet/settings) >>>=20 >>> -if [ "$(basename $0)" =3D=3D "green" ]; then >>> +ZONE=3D"$(basename $0)" >>> + >>> + >>> +if [ "${ZONE}" =3D=3D "green" ]; then >>> + if ! is_green_used; then >>> + boot_mesg "Green zone is not configured. No action can be taken >>> on this zone." ${FAILURE} >>> + echo_failure >>> + exit 1 >>> + fi >>> + >>> DEVICE=3D"${GREEN_DEV}" >>> ADDRESS=3D"${GREEN_ADDRESS}" >>> NETADDRESS=3D"${GREEN_NETADDRESS}" >>> NETMASK=3D"${GREEN_NETMASK}" >>> DEVICE=3D"${GREEN_DEV}" >>> MTU=3D"${GREEN_MTU}" >>> -elif [ "$(basename $0)" =3D=3D "blue" ]; then >>> +elif [ "${ZONE}" =3D=3D "blue" ]; then >>> + if ! is_blue_used; then >>> + boot_mesg "Blue zone is not configured. No action can be taken on >>> this zone." ${FAILURE} >>> + echo_failure >>> + exit 1 >>> + fi >>> DEVICE=3D"${BLUE_DEV}" >>> ADDRESS=3D"${BLUE_ADDRESS}" >>> NETADDRESS=3D"${BLUE_NETADDRESS}" >>> NETMASK=3D"${BLUE_NETMASK}" >>> DEVICE=3D"${BLUE_DEV}" >>> MTU=3D"${BLUE_MTU}" >>> -elif [ "$(basename $0)" =3D=3D "orange" ]; then >>> +elif [ "${ZONE}" =3D=3D "orange" ]; then >>> + if ! is_orange_used; then >>> + boot_mesg "Orange zone is not configured. No action can be taken >>> on this zone." ${FAILURE} >>> + echo_failure >>> + exit 1 >>> + fi >>> DEVICE=3D"${ORANGE_DEV}" >>> ADDRESS=3D"${ORANGE_ADDRESS}" >>> NETADDRESS=3D"${ORANGE_NETADDRESS}" >>> diff --git a/src/initscripts/networking/functions.network >>> b/src/initscripts/networking/functions.network >>> index 4c7ad51d4..9cc4da24b 100644 >>> --- a/src/initscripts/networking/functions.network >>> +++ b/src/initscripts/networking/functions.network >>> @@ -285,3 +285,15 @@ qmi_assign_address() { >>> # Change the MAC address >>> ip link set "${intf}" address "${address}" >>> } >>> + >>> +is_blue_used() { >>> + [ "${CONFIG_TYPE}" =3D "3" ] || [ "${CONFIG_TYPE}" =3D "4" ] >>> +} >>> + >>> +is_green_used() { >>> + [ -n "${GREEN_DEV}" ] && [ -v "GREEN_DEV" ] >>> +} >>> + >>> +is_orange_used() { >>> + [ "${CONFIG_TYPE}" =3D "2" ] || [ "${CONFIG_TYPE}" =3D "4" ] >>> +} >>> diff --git a/src/initscripts/system/network >>> b/src/initscripts/system/network >>> index 0d63b4e8b..fda16919d 100644 >>> --- a/src/initscripts/system/network >>> +++ b/src/initscripts/system/network >>> @@ -54,12 +54,10 @@ case "${DO}" in >>> [ "$green" =3D=3D "1" ] && /etc/rc.d/init.d/networking/green start >>>=20 >>> # BLUE >>> - [ "$blue" =3D=3D "1" ] && [ "$CONFIG_TYPE" =3D "3" -o "$CONFIG_TYPE" =3D >>> "4" ] && \ >>> - /etc/rc.d/init.d/networking/blue start >>> + [ "$blue" =3D=3D "1" ] && /etc/rc.d/init.d/networking/blue start >>>=20 >>> # ORANGE >>> - [ "$orange" =3D=3D "1" ] && [ "$CONFIG_TYPE" =3D "2" -o "$CONFIG_TYPE" >>> =3D "4" ] && \ >>> - /etc/rc.d/init.d/networking/orange start >>> + [ "$orange" =3D=3D "1" ] && /etc/rc.d/init.d/networking/orange start >>>=20 >>> # RED >>> if [ "$red" =3D=3D "1" ]; then >>> @@ -87,18 +85,14 @@ case "${DO}" in >>> [ "$green" =3D=3D "1" ] && /etc/rc.d/init.d/networking/green stop >>>=20 >>> # BLUE >>> - [ "$blue" =3D=3D "1" ] && [ "$CONFIG_TYPE" =3D "3" -o "$CONFIG_TYPE" =3D >>> "4" ] && \ >>> - /etc/rc.d/init.d/networking/blue stop >>> + [ "$blue" =3D=3D "1" ] && /etc/rc.d/init.d/networking/blue stop >>>=20 >>> # ORANGE >>> - [ "$orange" =3D=3D "1" ] && [ "$CONFIG_TYPE" =3D "2" -o "$CONFIG_TYPE" >>> =3D "4" ] && \ >>> - /etc/rc.d/init.d/networking/orange stop >>> + [ "$orange" =3D=3D "1" ] && /etc/rc.d/init.d/networking/orange stop >>>=20 >>> # RED >>> if [ "$red" =3D=3D "1" ]; then >>> - if [ "$CONFIG_TYPE" =3D "1" -o "$CONFIG_TYPE" =3D "2" -o >>> "$CONFIG_TYPE" =3D "3" -o "$CONFIG_TYPE" =3D "4" ]; then >>> - /etc/rc.d/init.d/networking/red stop >>> - fi >>> + /etc/rc.d/init.d/networking/red stop >>> fi >>>=20 >>> exit 0 >>> --=20 >>> 2.30.2 >>>=20 >>=20 >=20 >=20 --===============3025176893804418868==--