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

[Jonatan Schlag <jonatan.schlag@ipfire.org>]

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

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. 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=9dd5205e5a1cb60c4a0bb003ec2a8348848822f9;hb=refs/heads/next#l22

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