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

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

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

Am 2024-03-18 17:27, schrieb Michael Tremer:
> Hello,
> 
>> On 16 Mar 2024, at 09:35, Jonatan Schlag <jonatan.schlag(a)ipfire.org> 
>> wrote:
>> 
>> Hi Michael,
>> 
>> Am 2024-03-12 11:32, schrieb Michael Tremer:
>>> Hello Jonatan,
>>>> On 8 Mar 2024, at 11:14, Jonatan Schlag <jonatan.schlag(a)ipfire.org> 
>>>> wrote:
>>>> Hi,
>>>> Tl;dr:
>>>> It is not sensible to duplicate code in bash and the setup command.
>>>> There should be one source for checking a network config. The 
>>>> minimal
>>>> should be done in Bash. If C is the right language to go is another
>>>> question.
>>> I believe that you will have to duplicate a couple of things quite a 
>>> bit. We use three different languages to implement the network 
>>> configuration scripts, the management utility and the web UI. That 
>>> means: Bash, C, and Perl.
>>> If we were going really bug there could be some benefit in writing a 
>>> single C library with the logic and exposing that to Perl and Bash, 
>>> but that would be a lot more work that we are willing to invest into 
>>> this project right now.
>> 
>> Would you still be OK, if I write some port for examples as loadable 
>> for bash? Or do you want everything in Bash?
> 
> Which parts would that be? I cannot think of anything that would be 
> better implemented in something else.
> 
> Let me know what you have in mind and we will discuss.
> 

Reusing the function here 
https://git.ipfire.org/?p=ipfire-2.x.git;a=blob;f=src/libsmooth/varval.c;hb=08b7500b267a54aa634fb34b67b4dfc0934ae2be#l46 
and creating a bash loadable so that I can do

readkeyvalues FILE ARRAY_NAME

and subsequently, access the data like

echo ${ARRY_NAME["GREEN_DEV"]}

So we stay bug compatible and reusing code which we use already in 
several other places.

>>>> After thought about this whole situation a little bit more, I came 
>>>> to
>>>> the conclusion that we need a better solution here. So what are the
>>>> problems with this patch and the current situation:
>>> Better solution to what? I am not entirely sure where you are going 
>>> with all of this.
>> 
>> For reading and checking our network configuration.
> 
> Reading it is not a problem as we have a script for that.

We currently do an eval on the file. I would not call this a script. Or 
am I missing something.

> 
> Checking? I think you need to be a little bit more clear.
> 

Check for correct IP addresses and so on. I would like to avoid that the 
setup utility lets you save an IP address with the bash script fails on. 
So, the best way to do this: using the same function to check the 
network config.

>>>> * We need to check for a correct network configuration before we 
>>>> start
>>>> it and when a user edits it. The editing is done via /usr/sbin/setup
>>>> which is a C program from 2001.  The startup is done via a shell
>>>> script. It is a bad idea, as we learned from ipfire-3.x, to have a
>>>> duplicated code in two languages. So it is a bad idea two write 
>>>> shell
>>>> functions to check for a valid network config and C functions. There
>>>> needs to be one way to check for a valid network config.
>>> setup is probably slightly older than 2001 :)
>>> As mentioned above, I don’t think this will hurt us too bad here 
>>> because the complexity of the network stack in IPFire 2 is 
>>> substantially smaller than IPFire 3. You will also only copy a very 
>>> small subnet of the functionality which mainly is reading and writing 
>>> the configuration. That is all. Depending on how you are splitting 
>>> it, this could be less than a hand full of functions. That sounds 
>>> very acceptable to me.
>>>> *Nearly everything can be programmed in Bash, but maybe not 
>>>> everything
>>>> should. It may be the better approach to do only the network startup
>>>> with as minimal code as possible in Bash (calling ip from something
>>>> else with something like system() is a bad idea.) Checking and 
>>>> parsing
>>>> a config file is perhaps better to be done in other languages. After 
>>>> we
>>>> parsed a config file and checked the content for validity, we could,
>>>> for example, export the necessary information as environment 
>>>> variables.
>>>> Basically, we do the same thing here, as eval "parses" the config 
>>>> and
>>>> export variables:
>>>> https://git.ipfire.org/?p=ipfire-2.x.git;a=blob;f=src/initscripts/networking/any;h=dc4796e914536dae03b70390e9447b3bb2bf127b;hb=refs/heads/next#l24
>>> Apart from copying variables back and forth this should not be the 
>>> worst bit of code we have here.
>> 
>> No, it is not. My point was, more, that I need to redefine which 
>> CONFIG_TYPE is what. I would like to avoid that.
> 
> I think there is no way to avoid that.
> 
> I believe that there would be a huge benefit from checking the entire 
> code base for this. We would be able to open a couple of extra doors 
> for the future this way.

Checking the entire code base for what?

> 
>>> Bash is a great language for these things. Can it be done better? 
>>> Yes, but you are not seriously considering to do a full rewrite of 
>>> everything in C, are you?
>> 
>> Not a full rewrite. But If I can program a function in C or in Bash 
>> and maybe reuse some Code of the installer in C, I would like to use 
>> C.
>> 
>>>> * Bash has somehow limited tooling, for example for testing. There 
>>>> are
>>>> other languages like python and pytest,  so we have to write less 
>>>> code.
>>>> It is pointless to write a testing framework for ourselves in Bash.
>>> To test a bash function, you need a bash script. There is nothing 
>>> else required.
>> 
>> Short: No. This is not the way of testing I know and thought about. I 
>> need at least covering reports. There seem to be some alternatives 
>> around here: https://github.com/bats-core/bats-core . Still C has more 
>> of these tools like https://gcc.gnu.org/onlinedocs/gcc/Gcov.html 
>> because more people use it in bigger project where these tools are 
>> needed.
>> 
>>>> *This patch duplicates code which is somehow also found here:
>>>> https://git.ipfire.org/?p=ipfire-2.x.git;a=blob;f=src/setup/networking.c;h=9dd5205e5a1cb60c4a0bb003ec2a8348848822f9;hb=refs/heads/next#l37
>>> This is not a duplicate. This is the only place where the network 
>>> card assignment happens.
>> 
>> Still, the same code is in bot my bash function and in the c function.
>> 
>>>> So what do I propose. This is a rough sketch, not a detailed plan:
>>>> There should be a program which I can call from bash, which:
>>> *script
>> 
>> Do you, insist on script?
>> 
>>>> * checks for the validity of the network config
>>>> * exports all variables which are valid.
>>>> The same config check should be used in the program which users use 
>>>> to
>>>> edit the network settings and in the web interface to change the 
>>>> zone
>>>> settings. The language for this program should not be Bash. What
>>>> language should be used largely depends on preferences. I can 
>>>> program
>>>> in C, but I try to avoid it when there is no reason, like 
>>>> performance.
>>>> A program in an interpreter language with enough tests, which are 
>>>> easy
>>>> to write when you only need to mock a config file, is equally 
>>>> stable.
>>>> But before this discussion starts, I would like to gather some 
>>>> opinions
>>>> on the general thoughts a wrote down, here.
>>> You want to write shell scripts here. Nothing else, because you will 
>>> run into new headaches:
>>> * You have a lot of shell scripts that execute the boot process, run 
>>> daemons and what not. You don’t want to touch those, so you will have 
>>> to be compatible.
>> 
>> The plan was not to rewrite everything in c. Maybe having something 
>> like 
>> https://git.savannah.gnu.org/cgit/bash.git/tree/examples/loadables/csv.c 
>> to load the file and check it for error. The rest would still be Bash.
> 
> You would have to write a parser that is entirely bug-compatible with 
> what we are using now. You don’t want to change even the smallest bit 
> of the format because you won’t have a unique way across the entire 
> distribution.

I already have a parser for this format here 
https://git.ipfire.org/?p=ipfire-2.x.git;a=blob;f=src/libsmooth/varval.c;hb=08b7500b267a54aa634fb34b67b4dfc0934ae2be#l46

> 
> If you want to write network code in C send patches for this: 
> https://git.ipfire.org/?p=network.git;a=summary

No, I am not keen on writing C. It appears to be not avoidable here, but 
I still don't like it here. But back then C was the way to go. It is not 
now any more.
> 
>>> * You will have to rewrite all sorts of callback scripts for DHCP and 
>>> PPP. You don’t want to do that.
>> No, I would rather not do that.
>> 
>>> * Shell can be written quickly.
>>> * You already have some functionality there which is working 
>>> perfectly fine. Why would you want to rewrite something that already 
>>> works so well?
>> 
>>> * I believe that the IPFire 3 code shows that you can write large 
>>> projects in shell. They work well and shell would cover everything we 
>>> need in IPFire 2.
>> 
>> I would say after my experience, it proves the opposite. Small parts 
>> can be perfectly written in shell. Dealing with config files there 
>> validity and everything else is better done in another language.
> 
> Yes, but not when 100% of the code is already there and it is written 
> in Bash. You will have to write everything again instead of improving 
> the 2% that are bothering you right now.
> 

I must say this sentence still confuses me. On the one hand, I try to 
rewrite small parts, and you answer with the whole script needs a 
rewrite. Here you seem to more at the line of rewrite as few as 
possible.

Jonatan

>>> In order to get to some sort of milestone, I believe that we need to 
>>> work with what we have and incrementally improve it. If you want to 
>>> work on a total rewrite, please look at IPFire 3 and not IPFire 2.
>> 
>> That's why I only send 5 patches.  Would not say this is a total 
>> rewrite. Even with C I would create a loadable for zone_exists and the 
>> rest would be the same. I would only use the same C Code.
>> 
>> Jonatan
>> 
>>> -Michael
>>>> Greetings
>>>> Jonatan
>>>> Am Samstag, dem 02.03.2024 um 12:09 +0100 schrieb Jonatan Schlag:
>>>>> 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" ]
>>>>> +                       ;;
>>>>> +               "orange")
>>>>> +                       [ "${CONFIG_TYPE}" = "2" ] || [
>>>>> "${CONFIG_TYPE}" = "4" ]
>>>>> +                       ;;
>>>>> +               "red")
>>>>> +                       return ${EXIT_TRUE}
>>>>> +                       ;;
>>>>> +               *)
>>>>> +                       return ${EXIT_FALSE}
>>>>> +                       ;;
>>>>> +       esac
>>>>> +}