Hello,
On 25 Mar 2024, at 07:55, Jonatan Schlag jonatan.schlag@ipfire.org wrote:
Hi,
Am 2024-03-18 17:32, schrieb Michael Tremer:
On 16 Mar 2024, at 09:43, Jonatan Schlag jonatan.schlag@ipfire.org wrote: 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 single 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 sometimes you are checking CONFIG_TYPE. I suppose you got your inspiration from here: https://git.ipfire.org/?p=ipfire-2.x.git;a=blob;f=config/cfgroot/header.pl;h...
No, that is precisely the code copied from the network script.
Should we not go and redefine CONFIG_TYPE=1 as “has a green network” to make it consistent?
According to here, there always exists a green network: https://git.ipfire.org/?p=ipfire-2.x.git;a=blob;f=src/setup/networking.c;h=9... So we could just return True.
CONFIG_TYPE is a bitmap. Almost at least: GREEN sadly didn’t 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.
So what I got here is:
00 1 01 2 10 3 11 4
as we do not use CONFIG_TYPE zero.
We do:
https://git.ipfire.org/?p=ipfire-2.x.git;a=blob;f=config/cfgroot/header.pl;h...
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 which 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 wrong.
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 systems 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 proper way for this.
I suggest, simply copying the statements from here and translate them to bash https://git.ipfire.org/?p=ipfire-2.x.git;a=blob;f=src/setup/networking.c;h=9...
Are you okay with that?
If you want to have the option to extend this I would check for the bits being 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.
I would also suggest to keep the function name short and rename it to “zone_exists”. Since the function is in “networks”, I think it is pretty clear what this function would do. There is litte chance this name would clash with anything else.
Just for the records "I would prefer to give those function a “network_” prefix so that it becomes clear where this function is from and ideally shorten “is used” to just “have”. Like so: network_have_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. “have_BLUE" I would find a little bit too generic. There is no connection from BLUE to networking. “Zone” hints heavily at networking in our context.
I will just change the name.
Jonatan
An ever moving target is hard to hit :-) Jonatan
On 2 Mar 2024, at 11:09, Jonatan Schlag jonatan.schlag@ipfire.org wrote: As our Network is quite static a case does the trick Signed-off-by: Jonatan Schlag jonatan.schlag@ipfire.org
src/initscripts/networking/functions.network | 22 ++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/src/initscripts/networking/functions.network 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="${1}"
- case "${zone}" in
- "blue")
- [ "${CONFIG_TYPE}" = "3" ] || [ "${CONFIG_TYPE}" = "4" ]
- ;;
- "green")
- [ -n "${GREEN_DEV}" ] && [ -v "GREEN_DEV" ]
What is test -v doing? I can’t even find it on the man page.
- ;;
- "orange")
- [ "${CONFIG_TYPE}" = "2" ] || [ "${CONFIG_TYPE}" = "4" ]
- ;;
- "red")
- return ${EXIT_TRUE}
- ;;
- *)
- return ${EXIT_FALSE}
- ;;
- esac
+}
2.39.2
-Michael