From mboxrd@z Thu Jan 1 00:00:00 1970 From: Michael Tremer To: development@lists.ipfire.org Subject: Re: [PATCH 2/5] functions.network: Add network_zone_exists Date: Mon, 25 Mar 2024 11:01:06 +0000 Message-ID: <21FE8B42-E273-4C17-9A78-F11CC48C3E27@ipfire.org> In-Reply-To: <13416660cfb2c3e17c8078e0f13ff9d5@ipfire.org> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============3612906513706160499==" List-Id: --===============3612906513706160499== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hello, > On 25 Mar 2024, at 07:55, Jonatan Schlag wrot= e: >=20 > Hi, >=20 > Am 2024-03-18 17:32, schrieb Michael Tremer: >>> On 16 Mar 2024, at 09:43, Jonatan Schlag wr= ote: >>> Am 2024-03-12 10:59, schrieb Michael Tremer: >>>> Hello, >>>> We really need to go through the entire code base and come up with a sin= gle way how to check whether a zone exists or not. >>>> In your proposed patch, you have different ways to check whether a zone = exists or not. One is to check if there is a device (e.g. GREEN_DEV), and som= etimes you are checking CONFIG_TYPE. >>>> I suppose you got your inspiration from here: >>>> https://git.ipfire.org/?p=3Dipfire-2.x.git;a=3Dblob;f=3Dconfig/cfgroot/h= eader.pl;hb=3D6d501c05583a4efa513ff4b04a48ef41d5e8170e#l147 >>> 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 ne= twork=E2=80=9D to make it consistent? >>> According to here, there always exists a green network: https://git.ipfir= e.org/?p=3Dipfire-2.x.git;a=3Dblob;f=3Dsrc/setup/networking.c;h=3D9dd5205e5a1= cb60c4a0bb003ec2a8348848822f9;hb=3Drefs/heads/next#l22 >>> So we could just return True. >> CONFIG_TYPE is a bitmap. Almost at least: >> GREEN sadly didn=E2=80=99t get a bit for itself. >> If RED is present (at least when it is an Ethernet connection) it has the = 0x04 bit set. >> If ORANGE exists, 0x02 is set. If BLUE exists 0x01 is set. >> So checking the bits is a bare minimum. >=20 >=20 > So what I got here is: >=20 > 00 1 > 01 2 > 10 3 > 11 4 >=20 > as we do not use CONFIG_TYPE zero. We do: https://git.ipfire.org/?p=3Dipfire-2.x.git;a=3Dblob;f=3Dconfig/cfgroot/head= er.pl;hb=3D08b7500b267a54aa634fb34b67b4dfc0934ae2be#l169 Or should I say we did? This function is not being used anywhere. It would also mean that there is a system with no network interfaces at all w= hich makes a really poor firewall. > So BLUE would have the 0x02 Bit set. Orange the 0x01 bit. I had that the other way around in my previous email. One of us got this wron= g. > Green is always there, so does red if the CONFIG_TYPE is not zero. It is not safe to assume that GREEN is always there. We have plenty of system= s in the cloud that function as a relay for VPNs and such things and so there= is a necessity for this. I believe that this has been hacked around wherever= it needed to be checked and so I am suggesting that we should create a prope= r way for this. > I suggest, simply copying the > statements from here and translate them to bash https://git.ipfire.org/?p= =3Dipfire-2.x.git;a=3Dblob;f=3Dsrc/setup/networking.c;h=3D9dd5205e5a1cb60c4a0= bb003ec2a8348848822f9;hb=3Drefs/heads/next#l22 >=20 > Are you okay with that? If you want to have the option to extend this I would check for the bits bein= g set instead of creating such a long list of integer comparisons. Note that = you will double it with every additional bit that you are adding. >=20 >>>> I would also suggest to keep the function name short and rename it to = =E2=80=9Czone_exists=E2=80=9D. Since the function is in =E2=80=9Cnetworks=E2= =80=9D, I think it is pretty clear what this function would do. There is litt= e chance this name would clash with anything else. >>> Just for the records >>> "I would prefer to give those function a =E2=80=9Cnetwork_=E2=80=9D prefi= x 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_ha= ve_BLUE() and network_have_ORANGE(). >>> " - "Re: [Patch RFC 04/15] network initscripts: check if the zone in the = current config exists" >> Well that depends on how you are looking at it. >> =E2=80=9Chave_BLUE" I would find a little bit too generic. There is no con= nection from BLUE to networking. =E2=80=9CZone=E2=80=9D hints heavily at netw= orking in our context. >=20 > I will just change the name. >=20 > Jonatan >=20 >>> An ever moving target is hard to hit :-) >>> Jonatan >>>>> On 2 Mar 2024, at 11:09, Jonatan Schlag w= rote: >>>>> As our Network is quite static a case does the trick >>>>> Signed-off-by: Jonatan Schlag >>>>> --- >>>>> src/initscripts/networking/functions.network | 22 ++++++++++++++++++++ >>>>> 1 file changed, 22 insertions(+) >>>>> diff --git a/src/initscripts/networking/functions.network b/src/initscr= ipts/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" ] >>>> What is test -v doing? I can=E2=80=99t even find it on the man page. >>>>> + ;; >>>>> + "orange") >>>>> + [ "${CONFIG_TYPE}" =3D "2" ] || [ "${CONFIG_TYPE}" =3D "4" ] >>>>> + ;; >>>>> + "red") >>>>> + return ${EXIT_TRUE} >>>>> + ;; >>>>> + *) >>>>> + return ${EXIT_FALSE} >>>>> + ;; >>>>> + esac >>>>> +} >>>>> -- >>>>> 2.39.2 >>>> -Michael --===============3612906513706160499==--