From mboxrd@z Thu Jan  1 00:00:00 1970
From: Jonatan Schlag <jonatan.schlag@ipfire.org>
To: development@lists.ipfire.org
Subject: Re: [PATCH 2/5] functions.network: Add network_zone_exists
Date: Sat, 16 Mar 2024 10:43:47 +0100
Message-ID: <a3be64825fecb27bebe607d25a35f6fe@ipfire.org>
In-Reply-To: <05526FFB-B7D3-4065-94C6-CBB1FDCA2C5F@ipfire.org>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="===============3120881118401495244=="
List-Id: <development.lists.ipfire.org>

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

Am 2024-03-12 10:59, schrieb Michael Tremer:
> Hello,
>=20
> We really need to go through the entire code base and come up with a=20
> single way how to check whether a zone exists or not.
>=20
> In your proposed patch, you have different ways to check whether a zone=20
> exists or not. One is to check if there is a device (e.g. GREEN_DEV),=20
> and sometimes you are checking CONFIG_TYPE.
>=20
> I suppose you got your inspiration from here:
>=20
>  =20
> https://git.ipfire.org/?p=3Dipfire-2.x.git;a=3Dblob;f=3Dconfig/cfgroot/head=
er.pl;hb=3D6d501c05583a4efa513ff4b04a48ef41d5e8170e#l147
>=20

No, that is precisely the code copied from the network script.

> Should we not go and redefine CONFIG_TYPE=3D1 as =E2=80=9Chas a green netwo=
rk=E2=80=9D to=20
> make it consistent?

According to here, there always exists a green network:=20
https://git.ipfire.org/?p=3Dipfire-2.x.git;a=3Dblob;f=3Dsrc/setup/networking.=
c;h=3D9dd5205e5a1cb60c4a0bb003ec2a8348848822f9;hb=3Drefs/heads/next#l22
So we could just return True.

>=20
> I would also suggest to keep the function name short and rename it to=20
> =E2=80=9Czone_exists=E2=80=9D. Since the function is in =E2=80=9Cnetworks=
=E2=80=9D, I think it is=20
> pretty clear what this function would do. There is litte chance this=20
> name would clash with anything else.

Just for the records

"I would prefer to give those function a =E2=80=9Cnetwork_=E2=80=9D prefix so=
 that it=20
becomes clear where this function is from and ideally shorten =E2=80=9Cis use=
d=E2=80=9D=20
to just =E2=80=9Chave=E2=80=9D. Like so: network_have_BLUE() and network_have=
_ORANGE().
" - "Re: [Patch RFC 04/15] network initscripts: check if the zone in the=20
current config exists"

An ever moving target is hard to hit :-)

Jonatan
>=20
>> On 2 Mar 2024, at 11:09, Jonatan Schlag <jonatan.schlag(a)ipfire.org>=20
>> wrote:
>>=20
>> As our Network is quite static a case does the trick
>>=20
>> Signed-off-by: Jonatan Schlag <jonatan.schlag(a)ipfire.org>
>> ---
>> src/initscripts/networking/functions.network | 22 ++++++++++++++++++++
>> 1 file changed, 22 insertions(+)
>>=20
>> diff --git a/src/initscripts/networking/functions.network=20
>> b/src/initscripts/networking/functions.network
>> index dedbb6f7f..ad0d3f36a 100644
>> --- a/src/initscripts/networking/functions.network
>> +++ b/src/initscripts/networking/functions.network
>> @@ -289,3 +289,25 @@ qmi_assign_address() {
>> # Change the MAC address
>> ip link set "${intf}" address "${address}"
>> }
>> +
>> +network_zone_exists(){
>> + local zone=3D"${1}"
>> +
>> + case "${zone}" in
>> + "blue")
>> + [ "${CONFIG_TYPE}" =3D "3" ] || [ "${CONFIG_TYPE}" =3D "4" ]
>> + ;;
>> + "green")
>> + [ -n "${GREEN_DEV}" ] && [ -v "GREEN_DEV" ]
>=20
> What is test -v doing? I can=E2=80=99t even find it on the man page.
>=20
>> + ;;
>> + "orange")
>> + [ "${CONFIG_TYPE}" =3D "2" ] || [ "${CONFIG_TYPE}" =3D "4" ]
>> + ;;
>> + "red")
>> + return ${EXIT_TRUE}
>> + ;;
>> + *)
>> + return ${EXIT_FALSE}
>> + ;;
>> + esac
>> +}
>> --
>> 2.39.2
>>=20
>=20
> -Michael

--===============3120881118401495244==--