Re: [PATCH 2/5] functions.network: Add network_zone_exists

[Michael Tremer <michael.tremer@ipfire.org>]

[-- Attachment #1: Type: text/plain, Size: 3529 bytes --]



> On 16 Mar 2024, at 09:43, Jonatan Schlag <jonatan.schlag(a)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;hb=6d501c05583a4efa513ff4b04a48ef41d5e8170e#l147
> 
> 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=9dd5205e5a1cb60c4a0bb003ec2a8348848822f9;hb=refs/heads/next#l22
> 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.

> 
>> 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.

> 
> An ever moving target is hard to hit :-)
> 
> Jonatan
>>> On 2 Mar 2024, at 11:09, Jonatan Schlag <jonatan.schlag(a)ipfire.org> wrote:
>>> As our Network is quite static a case does the trick
>>> Signed-off-by: Jonatan Schlag <jonatan.schlag(a)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