From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jonatan Schlag To: development@lists.ipfire.org Subject: Re: [PATCH 2/5] functions.network: Add network_zone_exists Date: Sat, 16 Mar 2024 10:35:36 +0100 Message-ID: <0b56641d76c553fd60a8c92f31f3a727@ipfire.org> In-Reply-To: MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============4948102101293455412==" List-Id: --===============4948102101293455412== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hi Michael, Am 2024-03-12 11:32, schrieb Michael Tremer: > Hello Jonatan, >=20 >> On 8 Mar 2024, at 11:14, Jonatan Schlag =20 >> 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. >=20 > I believe that you will have to duplicate a couple of things quite a=20 > bit. We use three different languages to implement the network=20 > configuration scripts, the management utility and the web UI. That=20 > means: Bash, C, and Perl. >=20 > If we were going really bug there could be some benefit in writing a=20 > single C library with the logic and exposing that to Perl and Bash, but=20 > that would be a lot more work that we are willing to invest into this=20 > project right now. Would you still be OK, if I write some port for examples as loadable for=20 bash? Or do you want everything in Bash? >=20 >> 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: >=20 > Better solution to what? I am not entirely sure where you are going=20 > with all of this. For reading and checking our network configuration. >=20 >> * 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. >=20 > setup is probably slightly older than 2001 :) >=20 > As mentioned above, I don=E2=80=99t think this will hurt us too bad here=20 > because the complexity of the network stack in IPFire 2 is=20 > substantially smaller than IPFire 3. You will also only copy a very=20 > small subnet of the functionality which mainly is reading and writing=20 > the configuration. That is all. Depending on how you are splitting it,=20 > this could be less than a hand full of functions. That sounds very=20 > acceptable to me. >=20 >> *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=20 >> we >> parsed a config file and checked the content for validity, we could, >> for example, export the necessary information as environment=20 >> 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/ne= tworking/any;h=3Ddc4796e914536dae03b70390e9447b3bb2bf127b;hb=3Drefs/heads/nex= t#l24 >=20 > Apart from copying variables back and forth this should not be the=20 > worst bit of code we have here. No, it is not. My point was, more, that I need to redefine which=20 CONFIG_TYPE is what. I would like to avoid that. >=20 > Bash is a great language for these things. Can it be done better? Yes,=20 > but you are not seriously considering to do a full rewrite of=20 > everything in C, are you? Not a full rewrite. But If I can program a function in C or in Bash and=20 maybe reuse some Code of the installer in C, I would like to use C. >=20 >> * Bash has somehow limited tooling, for example for testing. There are >> other languages like python and pytest, so we have to write less=20 >> code. >> It is pointless to write a testing framework for ourselves in Bash. >=20 > To test a bash function, you need a bash script. There is nothing else=20 > required. Short: No. This is not the way of testing I know and thought about. I=20 need at least covering reports. There seem to be some alternatives=20 around here: https://github.com/bats-core/bats-core . Still C has more=20 of these tools like https://gcc.gnu.org/onlinedocs/gcc/Gcov.html because=20 more people use it in bigger project where these tools are needed. >=20 >> *This patch duplicates code which is somehow also found here: >> https://git.ipfire.org/?p=3Dipfire-2.x.git;a=3Dblob;f=3Dsrc/setup/networki= ng.c;h=3D9dd5205e5a1cb60c4a0bb003ec2a8348848822f9;hb=3Drefs/heads/next#l37 >=20 > This is not a duplicate. This is the only place where the network card=20 > assignment happens. Still, the same code is in bot my bash function and in the c function. >=20 >> 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: >=20 > *script Do you, insist on script? >=20 >> * 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=20 >> opinions >> on the general thoughts a wrote down, here. >=20 > You want to write shell scripts here. Nothing else, because you will=20 > run into new headaches: >=20 > * You have a lot of shell scripts that execute the boot process, run=20 > daemons and what not. You don=E2=80=99t want to touch those, so you will ha= ve=20 > to be compatible. >=20 The plan was not to rewrite everything in c. Maybe having something like=20 https://git.savannah.gnu.org/cgit/bash.git/tree/examples/loadables/csv.c=20 to load the file and check it for error. The rest would still be Bash. > * You will have to rewrite all sorts of callback scripts for DHCP and=20 > PPP. You don=E2=80=99t want to do that. >=20 No, I would rather not do that. > * Shell can be written quickly. >=20 > * You already have some functionality there which is working perfectly=20 > fine. Why would you want to rewrite something that already works so=20 > well? >=20 > * I believe that the IPFire 3 code shows that you can write large=20 > projects in shell. They work well and shell would cover everything we=20 > need in IPFire 2. I would say after my experience, it proves the opposite. Small parts can=20 be perfectly written in shell. Dealing with config files there validity=20 and everything else is better done in another language. >=20 > In order to get to some sort of milestone, I believe that we need to=20 > work with what we have and incrementally improve it. If you want to=20 > 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=20 rewrite. Even with C I would create a loadable for zone_exists and the=20 rest would be the same. I would only use the same C Code. Jonatan >=20 > -Michael >=20 >>=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 --===============4948102101293455412==--