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

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

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

Hello,

> On 25 Mar 2024, at 07:55, Jonatan Schlag <jonatan.schlag(a)ipfire.org> wrote:
> 
> Hi,
> 
> Am 2024-03-18 17:32, schrieb Michael Tremer:
>>> 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.
> 
> 
> 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;hb=08b7500b267a54aa634fb34b67b4dfc0934ae2be#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 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=9dd5205e5a1cb60c4a0bb003ec2a8348848822f9;hb=refs/heads/next#l22
> 
> 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(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