From mboxrd@z Thu Jan 1 00:00:00 1970 From: Michael Tremer To: development@lists.ipfire.org Subject: Re: [PATCH 2/5] functions.network: Add network_zone_exists Date: Tue, 12 Mar 2024 10:32:00 +0000 Message-ID: In-Reply-To: <3eb4906520ec8096f27686b4d10b59a565ee8a09.camel@ipfire.org> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============3677768434128949246==" List-Id: --===============3677768434128949246== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hello Jonatan, > On 8 Mar 2024, at 11:14, Jonatan Schlag wrote: >=20 > Hi, >=20 > Tl;dr: >=20 > 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. > 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. > * 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=E2=80=99t think this will hurt us too bad here beca= use 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 functionali= ty which mainly is reading and writing the configuration. That is all. Depend= ing on how you are splitting it, this could be less than a hand full of funct= ions. 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=3Dipfire-2.x.git;a=3Dblob;f=3Dsrc/initscripts/net= working/any;h=3Ddc4796e914536dae03b70390e9447b3bb2bf127b;hb=3Drefs/heads/next= #l24 Apart from copying variables back and forth this should not be the worst bit = of code we have here. Bash is a great language for these things. Can it be done better? Yes, but yo= u are not seriously considering to do a full rewrite of everything in C, are = you? > * 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 requir= ed. > *This patch duplicates code which is somehow also found here: > https://git.ipfire.org/?p=3Dipfire-2.x.git;a=3Dblob;f=3Dsrc/setup/networkin= g.c;h=3D9dd5205e5a1cb60c4a0bb003ec2a8348848822f9;hb=3Drefs/heads/next#l37 This is not a duplicate. This is the only place where the network card assign= ment happens. > 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 > * checks for the validity of the network config > * exports all variables which are valid. >=20 > 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=E2=80=99t want to touch those, so you will have to be c= ompatible. * You will have to rewrite all sorts of callback scripts for DHCP and PPP. Yo= u don=E2=80=99t want to 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. In order to get to some sort of milestone, I believe that we need to work wit= h what we have and incrementally improve it. If you want to work on a total r= ewrite, please look at IPFire 3 and not IPFire 2. -Michael >=20 > Greetings > Jonatan >=20 >=20 >=20 > Am Samstag, dem 02.03.2024 um 12:09 +0100 schrieb Jonatan Schlag: >> As our Network is quite static a case does the trick >>=20 >> Signed-off-by: Jonatan Schlag >> --- >> src/initscripts/networking/functions.network | 22 >> ++++++++++++++++++++ >> 1 file changed, 22 insertions(+) >>=20 >> 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=3D"${1}" >> + >> + case "${zone}" in >> + "blue") >> + [ "${CONFIG_TYPE}" =3D "3" ] || [ >> "${CONFIG_TYPE}" =3D "4" ] >> + ;; >> + "green") >> + [ -n "${GREEN_DEV}" ] && [ -v "GREEN_DEV" ] >> + ;; >> + "orange") >> + [ "${CONFIG_TYPE}" =3D "2" ] || [ >> "${CONFIG_TYPE}" =3D "4" ] >> + ;; >> + "red") >> + return ${EXIT_TRUE} >> + ;; >> + *) >> + return ${EXIT_FALSE} >> + ;; >> + esac >> +} >=20 --===============3677768434128949246==--