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. So BLUE would have the 0x02 Bit set. Orange the 0x01 bit. Green is always there, so does red if the CONFIG_TYPE is not zero. 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?
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