Re: [Patch RFC 04/15] network initscripts: check if the zone in the current config exists

[Michael Tremer <michael.tremer@ipfire.org>]

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

Hello,

> On 18 Aug 2023, at 13:55, Jonatan Schlag <jonatan.schlag(a)ipfire.org> wrote:
> 
> Hi list,
> 
> Am Mittwoch, dem 24.05.2023 um 10:00 +0100 schrieb Michael Tremer:
>> Hello,
>> 
>> If you want to make this script better, why not give the zone name as
>> a parameter?
>> 
>> First of all it should be renamed to something like “local” (as in:
>> not RED) and then you don’t have to use any symlinks and read back
>> the script name like this:
>> 
>>   ZONE="$(basename $0)”
>> 
> 
> I did not want to make such a big change, as people might rely on these
> files. But I am happy to create a script and delete the other
> scripts/links. I suggest as naming:
> 
> /etc/init.d/networking/local
> 
> Calling the script could look like:
> 
> For one Zone
> 
> /etc/init.d/networking/local start ${ZONE}
> /etc/init.d/networking/local stop ${ZONE}
> 
> For all Zones
> 
> /etc/init.d/networking/local start
> /etc/init.d/networking/local stop
> 
> Look this fine to you?

This looks good to me.

>> Otherwise I agree with introducing unified functions that check
>> whether we have GREEN/BLUE/ORANGE and have them in one function. We
>> should then go through the entire code base and replace those calls
>> first before we are introducing any new changes.
> 
> Ok, fine by me.
>> 
>> 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().
>> 
>> Can we do that instead?
> 
> 
> Yes, we can do that. I have only comments on the naming of the
> functions. I would like to reuse the naming style of the old ipfire-3.x
> networking code. So I would propose:
> 
> network_zone_exists(ZONE)
> 
> network_zones_get_all()
> 
> network_zones_get_local()
> 
> network_zones_get_nonlocal()
> 
> 
> As we all agreed on this naming some time ago, I think it faster to use
> these names instead of inventing new ones.

Also okay for me.

Best,
-Michael

> 
> Greetings Jonatan
>> 
>> -Michael
>> 
>>> On 23 May 2023, at 18:23, Jonatan Schlag
>>> <jonatan.schlag(a)ipfire.org> wrote:
>>> 
>>> We check in /etc/init.d/network if the current Configuration
>>> (RED+GREEN
>>> or RED+GREEN+BLUE) contains the zone we want to start or stop.
>>> We do this not in /etc/init.d/networking/green,blue,orange
>>> 
>>> As this checks make sense also there and as these scripts are
>>> called
>>> form /etc/init.d/network I moved the check to these scripts.
>>> 
>>> As CONFIG_TYPE == 2 is unreadable I wrote functions to make things
>>> at least a
>>> litte bit prettier.
>>> 
>>> Signed-off-by: Jonatan Schlag <jonatan.schlag(a)ipfire.org>
>>> ---
>>> src/initscripts/networking/any               | 27
>>> +++++++++++++++++---
>>> src/initscripts/networking/functions.network | 12 +++++++++
>>> src/initscripts/system/network               | 16 ++++--------
>>> 3 files changed, 41 insertions(+), 14 deletions(-)
>>> 
>>> diff --git a/src/initscripts/networking/any
>>> b/src/initscripts/networking/any
>>> index dc4796e91..6dba5bef9 100644
>>> --- a/src/initscripts/networking/any
>>> +++ b/src/initscripts/networking/any
>>> @@ -21,23 +21,44 @@
>>> 
>>> . /etc/sysconfig/rc
>>> . ${rc_functions}
>>> +. /etc/init.d/networking/functions.network
>>> +
>>> eval $(/usr/local/bin/readhash /var/ipfire/ethernet/settings)
>>> 
>>> -if [ "$(basename $0)" == "green" ]; then
>>> +ZONE="$(basename $0)"
>>> +
>>> +
>>> +if [ "${ZONE}" == "green" ]; then
>>> + if ! is_green_used; then
>>> + boot_mesg "Green zone is not configured. No action can be taken
>>> on this zone." ${FAILURE}
>>> + echo_failure
>>> + exit 1
>>> + fi
>>> +
>>> DEVICE="${GREEN_DEV}"
>>> ADDRESS="${GREEN_ADDRESS}"
>>> NETADDRESS="${GREEN_NETADDRESS}"
>>> NETMASK="${GREEN_NETMASK}"
>>> DEVICE="${GREEN_DEV}"
>>> MTU="${GREEN_MTU}"
>>> -elif [ "$(basename $0)" == "blue" ]; then
>>> +elif [ "${ZONE}" == "blue" ]; then
>>> + if ! is_blue_used; then
>>> + boot_mesg "Blue zone is not configured. No action can be taken on
>>> this zone." ${FAILURE}
>>> + echo_failure
>>> + exit 1
>>> + fi
>>> DEVICE="${BLUE_DEV}"
>>> ADDRESS="${BLUE_ADDRESS}"
>>> NETADDRESS="${BLUE_NETADDRESS}"
>>> NETMASK="${BLUE_NETMASK}"
>>> DEVICE="${BLUE_DEV}"
>>> MTU="${BLUE_MTU}"
>>> -elif [ "$(basename $0)" == "orange" ]; then
>>> +elif [ "${ZONE}" == "orange" ]; then
>>> + if ! is_orange_used; then
>>> + boot_mesg "Orange zone is not configured. No action can be taken
>>> on this zone." ${FAILURE}
>>> + echo_failure
>>> + exit 1
>>> + fi
>>> DEVICE="${ORANGE_DEV}"
>>> ADDRESS="${ORANGE_ADDRESS}"
>>> NETADDRESS="${ORANGE_NETADDRESS}"
>>> diff --git a/src/initscripts/networking/functions.network
>>> b/src/initscripts/networking/functions.network
>>> index 4c7ad51d4..9cc4da24b 100644
>>> --- a/src/initscripts/networking/functions.network
>>> +++ b/src/initscripts/networking/functions.network
>>> @@ -285,3 +285,15 @@ qmi_assign_address() {
>>> # Change the MAC address
>>> ip link set "${intf}" address "${address}"
>>> }
>>> +
>>> +is_blue_used() {
>>> + [ "${CONFIG_TYPE}" = "3" ] || [ "${CONFIG_TYPE}" = "4" ]
>>> +}
>>> +
>>> +is_green_used() {
>>> + [ -n "${GREEN_DEV}" ] && [ -v "GREEN_DEV" ]
>>> +}
>>> +
>>> +is_orange_used() {
>>> + [ "${CONFIG_TYPE}" = "2" ] || [ "${CONFIG_TYPE}" = "4" ]
>>> +}
>>> diff --git a/src/initscripts/system/network
>>> b/src/initscripts/system/network
>>> index 0d63b4e8b..fda16919d 100644
>>> --- a/src/initscripts/system/network
>>> +++ b/src/initscripts/system/network
>>> @@ -54,12 +54,10 @@ case "${DO}" in
>>> [ "$green" == "1" ] && /etc/rc.d/init.d/networking/green start
>>> 
>>> # BLUE
>>> - [ "$blue" == "1" ] && [ "$CONFIG_TYPE" = "3" -o "$CONFIG_TYPE" =
>>> "4" ] && \
>>> - /etc/rc.d/init.d/networking/blue start
>>> + [ "$blue" == "1" ] && /etc/rc.d/init.d/networking/blue start
>>> 
>>> # ORANGE
>>> - [ "$orange" == "1" ] && [ "$CONFIG_TYPE" = "2" -o "$CONFIG_TYPE"
>>> = "4" ] && \
>>> - /etc/rc.d/init.d/networking/orange start
>>> + [ "$orange" == "1" ] && /etc/rc.d/init.d/networking/orange start
>>> 
>>> # RED
>>> if [ "$red" == "1" ]; then
>>> @@ -87,18 +85,14 @@ case "${DO}" in
>>> [ "$green" == "1" ] && /etc/rc.d/init.d/networking/green stop
>>> 
>>> # BLUE
>>> - [ "$blue" == "1" ] && [ "$CONFIG_TYPE" = "3" -o "$CONFIG_TYPE" =
>>> "4" ] && \
>>> - /etc/rc.d/init.d/networking/blue stop
>>> + [ "$blue" == "1" ] && /etc/rc.d/init.d/networking/blue stop
>>> 
>>> # ORANGE
>>> - [ "$orange" == "1" ] && [ "$CONFIG_TYPE" = "2" -o "$CONFIG_TYPE"
>>> = "4" ] && \
>>> - /etc/rc.d/init.d/networking/orange stop
>>> + [ "$orange" == "1" ] && /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
>>> - /etc/rc.d/init.d/networking/red stop
>>> - fi
>>> + /etc/rc.d/init.d/networking/red stop
>>> fi
>>> 
>>> exit 0
>>> -- 
>>> 2.30.2
>>> 
>> 
> 
>