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

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

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

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/any;h=dc4796e914536dae03b70390e9447b3bb2bf127b;hb=refs/heads/next#l24

* 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=9dd5205e5a1cb60c4a0bb003ec2a8348848822f9;hb=refs/heads/next#l37

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(a)ipfire.org>
> ---
>  src/initscripts/networking/functions.network | 22
> ++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/src/initscripts/networking/functions.network
> b/src/initscripts/networking/functions.network
> index dedbb6f7f..ad0d3f36a 100644
> --- a/src/initscripts/networking/functions.network
> +++ b/src/initscripts/networking/functions.network
> @@ -289,3 +289,25 @@ qmi_assign_address() {
>         # Change the MAC address
>         ip link set "${intf}" address "${address}"
>  }
> +
> +network_zone_exists(){
> +       local zone="${1}"
> +
> +       case "${zone}" in
> +               "blue")
> +                       [ "${CONFIG_TYPE}" = "3" ] || [
> "${CONFIG_TYPE}" = "4" ]
> +                       ;;
> +               "green")
> +                       [ -n "${GREEN_DEV}" ] && [ -v "GREEN_DEV" ]
> +                       ;;
> +               "orange")
> +                       [ "${CONFIG_TYPE}" = "2" ] || [
> "${CONFIG_TYPE}" = "4" ]
> +                       ;;
> +               "red")
> +                       return ${EXIT_TRUE}
> +                       ;;
> +               *)
> +                       return ${EXIT_FALSE}
> +                       ;;
> +       esac
> +}