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: Mon, 18 Mar 2024 16:27:44 +0000 Message-ID: <124D11E4-5BDC-4D89-9CA5-5F42FE8E6369@ipfire.org> In-Reply-To: <0b56641d76c553fd60a8c92f31f3a727@ipfire.org> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="===============8033017730526179396==" List-Id: --===============8033017730526179396== Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable Hello, > On 16 Mar 2024, at 09:35, Jonatan Schlag wrot= e: >=20 > Hi Michael, >=20 > Am 2024-03-12 11:32, schrieb Michael Tremer: >> Hello Jonatan, >>> On 8 Mar 2024, at 11:14, Jonatan Schlag wro= te: >>> 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 scrip= ts, 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 singl= e 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. >=20 > Would you still be OK, if I write some port for examples as loadable for ba= sh? Or do you want everything in Bash? Which parts would that be? I cannot think of anything that would be better im= plemented in something else. Let me know what you have in mind and we will discuss. >>> 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 a= ll of this. >=20 > For reading and checking our network configuration. Reading it is not a problem as we have a script for that. Checking? I think you need to be a little bit more clear. >>> * 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 b= ecause the complexity of the network stack in IPFire 2 is substantially small= er than IPFire 3. You will also only copy a very small subnet of the function= ality which mainly is reading and writing the configuration. That is all. Dep= ending on how you are splitting it, this could be less than a hand full of fu= nctions. 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/n= etworking/any;h=3Ddc4796e914536dae03b70390e9447b3bb2bf127b;hb=3Drefs/heads/ne= xt#l24 >> Apart from copying variables back and forth this should not be the worst b= it of code we have here. >=20 > No, it is not. My point was, more, that I need to redefine which CONFIG_TYP= E 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 ba= se for this. We would be able to open a couple of extra doors for the future = this way. >> 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, a= re you? >=20 > Not a full rewrite. But If I can program a function in C or in Bash and may= be 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 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 req= uired. >=20 > 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: ht= tps://github.com/bats-core/bats-core . Still C has more of these tools like h= ttps://gcc.gnu.org/onlinedocs/gcc/Gcov.html because more people use it in big= ger 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/network= ing.c;h=3D9dd5205e5a1cb60c4a0bb003ec2a8348848822f9;hb=3Drefs/heads/next#l37 >> This is not a duplicate. This is the only place where the network card ass= ignment happens. >=20 > 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: >> *script >=20 > Do you, insist on script? >=20 >>> * 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 i= nto new headaches: >> * You have a lot of shell scripts that execute the boot process, run daemo= ns and what not. You don=E2=80=99t want to touch those, so you will have to b= e compatible. >=20 > The plan was not to rewrite everything in c. Maybe having something like ht= tps://git.savannah.gnu.org/cgit/bash.git/tree/examples/loadables/csv.c to loa= d 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=E2=80=99t want to change even the smallest bit of the= format because you won=E2=80=99t have a unique way across the entire distrib= ution. If you want to write network code in C send patches for this: https://git.ipf= ire.org/?p=3Dnetwork.git;a=3Dsummary >> * You will have to rewrite all sorts of callback scripts for DHCP and PPP.= You don=E2=80=99t want to do that. > No, I would rather not do that. >=20 >> * Shell can be written quickly. >> * You already have some functionality there which is working perfectly fin= e. Why would you want to rewrite something that already works so well? >=20 >> * 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. >=20 > 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 eve= rything 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 ar= e bothering you right now. >> 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 tota= l rewrite, please look at IPFire 3 and not IPFire 2. >=20 > That's why I only send 5 patches. Would not say this is a total rewrite. E= ven with C I would create a loadable for zone_exists and the rest would be th= e same. I would only use the same C Code. >=20 > Jonatan >=20 >> -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 >>>> --- >>>> 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=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 >>>> +} --===============8033017730526179396==--