This ist borrowed from here:
https://git.ipfire.org/?p=network.git;a=blob_plain;f=src/functions/functions...
and can be extended later on.
Signed-off-by: Jonatan Schlag jonatan.schlag@ipfire.org --- src/initscripts/networking/functions.network | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/src/initscripts/networking/functions.network b/src/initscripts/networking/functions.network index 4c7ad51d4..dedbb6f7f 100644 --- a/src/initscripts/networking/functions.network +++ b/src/initscripts/networking/functions.network @@ -22,6 +22,10 @@ . /etc/sysconfig/rc . $rc_functions
+# Proper error codes +EXIT_TRUE=0 +EXIT_FALSE=1 + eval $(/usr/local/bin/readhash /var/ipfire/ethernet/settings) eval $(/usr/local/bin/readhash /var/ipfire/dns/settings)
As our Network is quite static a case does the trick
Signed-off-by: Jonatan Schlag jonatan.schlag@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 +}
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.
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:
* 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.
*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...
* 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.
*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=9...
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:
* 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.
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@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 +}
Hello Jonatan,
On 8 Mar 2024, at 11:14, Jonatan Schlag jonatan.schlag@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.
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’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...
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 you 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 required.
*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=9...
This is not a duplicate. This is the only place where the network card assignment 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.
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.
* You will have to rewrite all sorts of callback scripts for DHCP and PPP. You don’t 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 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.
-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@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+}
Hi Michael,
Am 2024-03-12 11:32, schrieb Michael Tremer:
Hello Jonatan,
On 8 Mar 2024, at 11:14, Jonatan Schlag jonatan.schlag@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?
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.
- 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...
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.
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=9...
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 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.
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@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+}
Hello,
On 16 Mar 2024, at 09:35, Jonatan Schlag jonatan.schlag@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@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.
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.
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’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...
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.
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=9...
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.
If you want to write network code in C send patches for this: https://git.ipfire.org/?p=network.git;a=summary
- 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.
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@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+}
Am 2024-03-18 17:27, schrieb Michael Tremer:
Hello,
On 16 Mar 2024, at 09:35, Jonatan Schlag jonatan.schlag@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@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=... 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...
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=9...
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=...
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@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+}
Hello,
On 25 Mar 2024, at 07:46, Jonatan Schlag jonatan.schlag@ipfire.org wrote:
Am 2024-03-18 17:27, schrieb Michael Tremer:
Hello,
On 16 Mar 2024, at 09:35, Jonatan Schlag jonatan.schlag@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@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=... 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.
This is what we use in C because we cannot create new variables just like that. C is not a scripting language.
In shell scripts we use “readhash”.
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.
Call it what you want. There is something that imports a KEY=VALUE configuration file into a shell script.
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...
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?
For how we interpret CONFIG_TYPE or if there are other ways used to determine whether there is a BLUE or ORANGE network.
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=9...
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=...
Reading the configuration isn’t the hard part. The harder part is interpreting the content which is what we are talking about.
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.
I want to understand what you are doing… Small changes are great, but so far this is a patch that just says “this could be written like this, too”. Many languages allow various ways to achieve the same.
That does however not bring us forward. There are some rather large problems here that we could try to fix but I don’t know whether you actually want to do that.
At the moment I only hear “this could as well be written in C”. It could. Nobody doubts that. But should it? In order to understand that, I would like to understand what your goal is. Rewriting everything in C but being 100% compatible with what we are doing now is simply a waste of time in my eyes.
-Michael
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@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+}
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;h...
Should we not go and redefine CONFIG_TYPE=1 as “has a green network” to make it consistent?
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.
On 2 Mar 2024, at 11:09, Jonatan Schlag jonatan.schlag@ipfire.org wrote:
As our Network is quite static a case does the trick
Signed-off-by: Jonatan Schlag jonatan.schlag@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
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;h...
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=9... So we could just return True.
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"
An ever moving target is hard to hit :-)
Jonatan
On 2 Mar 2024, at 11:09, Jonatan Schlag jonatan.schlag@ipfire.org wrote:
As our Network is quite static a case does the trick
Signed-off-by: Jonatan Schlag jonatan.schlag@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
On 16 Mar 2024, at 09:43, Jonatan Schlag jonatan.schlag@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;h...
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=9... 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.
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.
An ever moving target is hard to hit :-)
Jonatan
On 2 Mar 2024, at 11:09, Jonatan Schlag jonatan.schlag@ipfire.org wrote: As our Network is quite static a case does the trick Signed-off-by: Jonatan Schlag jonatan.schlag@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
Hi,
Am 2024-03-18 17:32, schrieb Michael Tremer:
On 16 Mar 2024, at 09:43, Jonatan Schlag jonatan.schlag@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;h...
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=9... 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=9...
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@ipfire.org wrote: As our Network is quite static a case does the trick Signed-off-by: Jonatan Schlag jonatan.schlag@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
Hello,
On 25 Mar 2024, at 07:55, Jonatan Schlag jonatan.schlag@ipfire.org wrote:
Hi,
Am 2024-03-18 17:32, schrieb Michael Tremer:
On 16 Mar 2024, at 09:43, Jonatan Schlag jonatan.schlag@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;h...
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=9... 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.
We do:
https://git.ipfire.org/?p=ipfire-2.x.git;a=blob;f=config/cfgroot/header.pl;h...
Or should I say we did? This function is not being used anywhere.
It would also mean that there is a system with no network interfaces at all which makes a really poor firewall.
So BLUE would have the 0x02 Bit set. Orange the 0x01 bit.
I had that the other way around in my previous email. One of us got this wrong.
Green is always there, so does red if the CONFIG_TYPE is not zero.
It is not safe to assume that GREEN is always there. We have plenty of systems in the cloud that function as a relay for VPNs and such things and so there is a necessity for this. I believe that this has been hacked around wherever it needed to be checked and so I am suggesting that we should create a proper way for this.
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=9...
Are you okay with that?
If you want to have the option to extend this I would check for the bits being set instead of creating such a long list of integer comparisons. Note that you will double it with every additional bit that you are adding.
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@ipfire.org wrote: As our Network is quite static a case does the trick Signed-off-by: Jonatan Schlag jonatan.schlag@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
This only a cosmetic change but should making the code easier changeable later on.
Signed-off-by: Jonatan Schlag jonatan.schlag@ipfire.org --- src/initscripts/system/network | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/src/initscripts/system/network b/src/initscripts/system/network index 0dcce060f..f334561de 100644 --- a/src/initscripts/system/network +++ b/src/initscripts/system/network @@ -21,6 +21,8 @@
. /etc/sysconfig/rc . ${rc_functions} +. /etc/init.d/networking/functions.network + eval $(/usr/local/bin/readhash /var/ipfire/ethernet/settings)
DO="${1}" @@ -54,11 +56,11 @@ case "${DO}" in [ "$green" == "1" ] && /etc/rc.d/init.d/networking/green start
# BLUE - [ "$blue" == "1" ] && [ "$CONFIG_TYPE" = "3" -o "$CONFIG_TYPE" = "4" ] && \ + [ "$blue" == "1" ] && network_zone_exists "blue" && \ /etc/rc.d/init.d/networking/blue start
# ORANGE - [ "$orange" == "1" ] && [ "$CONFIG_TYPE" = "2" -o "$CONFIG_TYPE" = "4" ] && \ + [ "$orange" == "1" ] && network_zone_exists "orange" && \ /etc/rc.d/init.d/networking/orange start
# RED @@ -84,16 +86,16 @@ case "${DO}" in [ "$green" == "1" ] && /etc/rc.d/init.d/networking/green stop
# BLUE - [ "$blue" == "1" ] && [ "$CONFIG_TYPE" = "3" -o "$CONFIG_TYPE" = "4" ] && \ + [ "$blue" == "1" ] && network_zone_exists "blue" && \ /etc/rc.d/init.d/networking/blue stop
# ORANGE - [ "$orange" == "1" ] && [ "$CONFIG_TYPE" = "2" -o "$CONFIG_TYPE" = "4" ] && \ + [ "$orange" == "1" ] && network_zone_exists "orange" && \ /etc/rc.d/init.d/networking/orange stop
# RED if [ "$red" == "1" ]; then - if [ "$CONFIG_TYPE" = "1" -o "$CONFIG_TYPE" = "2" -o "$CONFIG_TYPE" = "3" -o "$CONFIG_TYPE" = "4" ]; then + if network_zone_exists "red"; then /etc/rc.d/init.d/networking/red stop fi fi
Hello,
Can we no rewrite this entire script? Even after your fix, I am very unhappy with it.
-Michael
On 2 Mar 2024, at 11:09, Jonatan Schlag jonatan.schlag@ipfire.org wrote:
This only a cosmetic change but should making the code easier changeable later on.
Signed-off-by: Jonatan Schlag jonatan.schlag@ipfire.org
src/initscripts/system/network | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/src/initscripts/system/network b/src/initscripts/system/network index 0dcce060f..f334561de 100644 --- a/src/initscripts/system/network +++ b/src/initscripts/system/network @@ -21,6 +21,8 @@
. /etc/sysconfig/rc . ${rc_functions} +. /etc/init.d/networking/functions.network
eval $(/usr/local/bin/readhash /var/ipfire/ethernet/settings)
DO="${1}" @@ -54,11 +56,11 @@ case "${DO}" in [ "$green" == "1" ] && /etc/rc.d/init.d/networking/green start
# BLUE
- [ "$blue" == "1" ] && [ "$CONFIG_TYPE" = "3" -o "$CONFIG_TYPE" = "4" ] && \
- [ "$blue" == "1" ] && network_zone_exists "blue" && \
/etc/rc.d/init.d/networking/blue start
# ORANGE
- [ "$orange" == "1" ] && [ "$CONFIG_TYPE" = "2" -o "$CONFIG_TYPE" = "4" ] && \
- [ "$orange" == "1" ] && network_zone_exists "orange" && \
/etc/rc.d/init.d/networking/orange start
# RED @@ -84,16 +86,16 @@ case "${DO}" in [ "$green" == "1" ] && /etc/rc.d/init.d/networking/green stop
# BLUE
- [ "$blue" == "1" ] && [ "$CONFIG_TYPE" = "3" -o "$CONFIG_TYPE" = "4" ] && \
- [ "$blue" == "1" ] && network_zone_exists "blue" && \
/etc/rc.d/init.d/networking/blue stop
# ORANGE
- [ "$orange" == "1" ] && [ "$CONFIG_TYPE" = "2" -o "$CONFIG_TYPE" = "4" ] && \
- [ "$orange" == "1" ] && network_zone_exists "orange" && \
/etc/rc.d/init.d/networking/orange stop
# RED if [ "$red" == "1" ]; then
- if [ "$CONFIG_TYPE" = "1" -o "$CONFIG_TYPE" = "2" -o "$CONFIG_TYPE" = "3" -o "$CONFIG_TYPE" = "4" ]; then
- if network_zone_exists "red"; then
/etc/rc.d/init.d/networking/red stop fi fi -- 2.39.2
Hi,
This will happen. But you do not seem to want too many patches at once, so I started to gradually improve it, to avoid a full rewrite. I currently have some trouble getting your point, how big a patch series or the changes should be. Because both ways, full rewrite and gradually improving it seem to be wrong.
Jonatan
Am 2024-03-11 18:02, schrieb Michael Tremer:
Hello,
Can we no rewrite this entire script? Even after your fix, I am very unhappy with it.
-Michael
On 2 Mar 2024, at 11:09, Jonatan Schlag jonatan.schlag@ipfire.org wrote:
This only a cosmetic change but should making the code easier changeable later on.
Signed-off-by: Jonatan Schlag jonatan.schlag@ipfire.org
src/initscripts/system/network | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/src/initscripts/system/network b/src/initscripts/system/network index 0dcce060f..f334561de 100644 --- a/src/initscripts/system/network +++ b/src/initscripts/system/network @@ -21,6 +21,8 @@
. /etc/sysconfig/rc . ${rc_functions} +. /etc/init.d/networking/functions.network
eval $(/usr/local/bin/readhash /var/ipfire/ethernet/settings)
DO="${1}" @@ -54,11 +56,11 @@ case "${DO}" in [ "$green" == "1" ] && /etc/rc.d/init.d/networking/green start
# BLUE
- [ "$blue" == "1" ] && [ "$CONFIG_TYPE" = "3" -o "$CONFIG_TYPE" = "4"
] && \
- [ "$blue" == "1" ] && network_zone_exists "blue" && \
/etc/rc.d/init.d/networking/blue start
# ORANGE
- [ "$orange" == "1" ] && [ "$CONFIG_TYPE" = "2" -o "$CONFIG_TYPE" =
"4" ] && \
- [ "$orange" == "1" ] && network_zone_exists "orange" && \
/etc/rc.d/init.d/networking/orange start
# RED @@ -84,16 +86,16 @@ case "${DO}" in [ "$green" == "1" ] && /etc/rc.d/init.d/networking/green stop
# BLUE
- [ "$blue" == "1" ] && [ "$CONFIG_TYPE" = "3" -o "$CONFIG_TYPE" = "4"
] && \
- [ "$blue" == "1" ] && network_zone_exists "blue" && \
/etc/rc.d/init.d/networking/blue stop
# ORANGE
- [ "$orange" == "1" ] && [ "$CONFIG_TYPE" = "2" -o "$CONFIG_TYPE" =
"4" ] && \
- [ "$orange" == "1" ] && network_zone_exists "orange" && \
/etc/rc.d/init.d/networking/orange stop
# RED if [ "$red" == "1" ]; then
- if [ "$CONFIG_TYPE" = "1" -o "$CONFIG_TYPE" = "2" -o "$CONFIG_TYPE"
= "3" -o "$CONFIG_TYPE" = "4" ]; then
- if network_zone_exists "red"; then
/etc/rc.d/init.d/networking/red stop fi fi -- 2.39.2
Okay, keep them coming then :)
On 16 Mar 2024, at 09:46, Jonatan Schlag jonatan.schlag@ipfire.org wrote:
Hi,
This will happen. But you do not seem to want too many patches at once, so I started to gradually improve it, to avoid a full rewrite. I currently have some trouble getting your point, how big a patch series or the changes should be. Because both ways, full rewrite and gradually improving it seem to be wrong.
Jonatan
Am 2024-03-11 18:02, schrieb Michael Tremer:
Hello, Can we no rewrite this entire script? Even after your fix, I am very unhappy with it. -Michael
On 2 Mar 2024, at 11:09, Jonatan Schlag jonatan.schlag@ipfire.org wrote: This only a cosmetic change but should making the code easier changeable later on. Signed-off-by: Jonatan Schlag jonatan.schlag@ipfire.org
src/initscripts/system/network | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/initscripts/system/network b/src/initscripts/system/network index 0dcce060f..f334561de 100644 --- a/src/initscripts/system/network +++ b/src/initscripts/system/network @@ -21,6 +21,8 @@ . /etc/sysconfig/rc . ${rc_functions} +. /etc/init.d/networking/functions.network
eval $(/usr/local/bin/readhash /var/ipfire/ethernet/settings) DO="${1}" @@ -54,11 +56,11 @@ case "${DO}" in [ "$green" == "1" ] && /etc/rc.d/init.d/networking/green start # BLUE
- [ "$blue" == "1" ] && [ "$CONFIG_TYPE" = "3" -o "$CONFIG_TYPE" = "4" ] && \
- [ "$blue" == "1" ] && network_zone_exists "blue" && \
/etc/rc.d/init.d/networking/blue start # ORANGE
- [ "$orange" == "1" ] && [ "$CONFIG_TYPE" = "2" -o "$CONFIG_TYPE" = "4" ] && \
- [ "$orange" == "1" ] && network_zone_exists "orange" && \
/etc/rc.d/init.d/networking/orange start # RED @@ -84,16 +86,16 @@ case "${DO}" in [ "$green" == "1" ] && /etc/rc.d/init.d/networking/green stop # BLUE
- [ "$blue" == "1" ] && [ "$CONFIG_TYPE" = "3" -o "$CONFIG_TYPE" = "4" ] && \
- [ "$blue" == "1" ] && network_zone_exists "blue" && \
/etc/rc.d/init.d/networking/blue stop # ORANGE
- [ "$orange" == "1" ] && [ "$CONFIG_TYPE" = "2" -o "$CONFIG_TYPE" = "4" ] && \
- [ "$orange" == "1" ] && network_zone_exists "orange" && \
/etc/rc.d/init.d/networking/orange stop # RED if [ "$red" == "1" ]; then
- if [ "$CONFIG_TYPE" = "1" -o "$CONFIG_TYPE" = "2" -o "$CONFIG_TYPE" = "3" -o "$CONFIG_TYPE" = "4" ]; then
- if network_zone_exists "red"; then
/etc/rc.d/init.d/networking/red stop fi fi -- 2.39.2
If we only shift if $1 is red, green, blue or orange, $# never gets zero when the input is:
/etc/init.d/network green stop
When we get an invalid zone we stop the script, as this happening should not be masked.
Signed-off-by: Jonatan Schlag jonatan.schlag@ipfire.org --- src/initscripts/system/network | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/src/initscripts/system/network b/src/initscripts/system/network index f334561de..a78473110 100644 --- a/src/initscripts/system/network +++ b/src/initscripts/system/network @@ -41,12 +41,15 @@ else fi
while [ ! $# = 0 ]; do - for i in green red blue orange; do - if [ "${i}" == "${1}" ]; then - eval "${i}=1" - shift - fi - done + if network_zone_exists "${1}"; then + printf -v "${1}" '%s' '1' + else + boot_mesg "'${1}' is not a valid zone" ${FAILURE} + echo_failure + # Set ${DO} to something silly that we get an usage message + DO="error" + fi + shift done
case "${DO}" in
Signed-off-by: Jonatan Schlag jonatan.schlag@ipfire.org --- src/initscripts/system/network | 2 -- 1 file changed, 2 deletions(-)
diff --git a/src/initscripts/system/network b/src/initscripts/system/network index a78473110..ce1ddf01f 100644 --- a/src/initscripts/system/network +++ b/src/initscripts/system/network @@ -29,12 +29,10 @@ DO="${1}" shift
if [ -n "${1}" ]; then - ALL=0 for i in green red blue orange; do eval "${i}=0" done else - ALL=1 for i in green red blue orange; do eval "${i}=1" done
-
Jonatan Schlag -
Michael Tremer