diff mbox series

[RFC,09/15] network startup: check for correct action at start

Message ID 20230523172314.7826-10-jonatan.schlag@ipfire.org
State Changes Requested
Headers
Series [RFC,01/15] Remove ipsec interface creation from network startup |

Commit Message

Jonatan Schlag May 23, 2023, 5:23 p.m. UTC 
  If we check this at the end, we already do some calculation in the next
line. For example checking if the devices are correct. This is not
necessary as we can already stop when we get an "stoop" or something
like:

/etc/init.d/network green stop

This currently ends in an infinite loop, which gets fixed by this
change.

Signed-off-by: Jonatan Schlag <jonatan.schlag@ipfire.org>
---
 src/initscripts/system/network | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Comments

Michael Tremer May 24, 2023, 9 a.m. UTC | #1 
How can calling “exit 1” result in an infinite loop?

That rather suggests that we are never getting to that part of the code. Should we not rather fix this?

Having a catchall at the end of a script is a standard pattern and keeps the script easier to extend if new options are being added because you cannot forget to add them to the top?

-Michael

> On 23 May 2023, at 18:23, Jonatan Schlag <jonatan.schlag@ipfire.org> wrote:
> 
> If we check this at the end, we already do some calculation in the next
> line. For example checking if the devices are correct. This is not
> necessary as we can already stop when we get an "stoop" or something
> like:
> 
> /etc/init.d/network green stop
> 
> This currently ends in an infinite loop, which gets fixed by this
> change.
> 
> Signed-off-by: Jonatan Schlag <jonatan.schlag@ipfire.org>
> ---
> src/initscripts/system/network | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/src/initscripts/system/network b/src/initscripts/system/network
> index 9694165f2..06240f53c 100644
> --- a/src/initscripts/system/network
> +++ b/src/initscripts/system/network
> @@ -26,6 +26,11 @@ eval $(/usr/local/bin/readhash /var/ipfire/ethernet/settings)
> DO="${1}"
> shift
> 
> +if ! [[ "${DO}" == "start" ||  "${DO}" == "restart" || "${DO}" == "stop" ]]; then
> + echo "Usage: ${0} {start|stop|restart} [device(s)]"
> + exit 1
> +fi
> +
> if [ -n "${1}" ]; then
> ALL=0
> for i in green red blue orange; do
> @@ -100,9 +105,4 @@ case "${DO}" in
> sleep 1
> ${0} start ${ARGS}
> ;;
> -
> - *)
> - echo "Usage: ${0} {start|stop|restart} [device(s)]"
> - exit 1
> - ;;
> esac
> -- 
> 2.30.2
>
Jonatan Schlag Aug. 18, 2023, 10:23 a.m. UTC | #2 
Hi,

yes, we never get to this piece of code:

while [ ! $# = 0 ]; do
for i in green red blue orange; do
if [ "${i}" == "${1}" ]; then
eval "${i}=1"
shift
fi
done
done

This loop never ends, as ["${i}" == "${1}" ]; never evaluates to true
for $1="stop". So we would actually want to do check here if $1 is a
valid zone. So this has to wait until we have something like
"network_zone_exists()".

Greetings Jonatan


Am Mittwoch, dem 24.05.2023 um 10:00 +0100 schrieb Michael Tremer:
> How can calling “exit 1” result in an infinite loop?
> 
> That rather suggests that we are never getting to that part of the
> code. Should we not rather fix this?
> 
> Having a catchall at the end of a script is a standard pattern and
> keeps the script easier to extend if new options are being added
> because you cannot forget to add them to the top?
> 
> -Michael
> 
> > On 23 May 2023, at 18:23, Jonatan Schlag
> > <jonatan.schlag@ipfire.org> wrote:
> > 
> > If we check this at the end, we already do some calculation in the
> > next
> > line. For example checking if the devices are correct. This is not
> > necessary as we can already stop when we get an "stoop" or
> > something
> > like:
> > 
> > /etc/init.d/network green stop
> > 
> > This currently ends in an infinite loop, which gets fixed by this
> > change.
> > 
> > Signed-off-by: Jonatan Schlag <jonatan.schlag@ipfire.org>
> > ---
> > src/initscripts/system/network | 10 +++++-----
> > 1 file changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/src/initscripts/system/network
> > b/src/initscripts/system/network
> > index 9694165f2..06240f53c 100644
> > --- a/src/initscripts/system/network
> > +++ b/src/initscripts/system/network
> > @@ -26,6 +26,11 @@ eval $(/usr/local/bin/readhash
> > /var/ipfire/ethernet/settings)
> > DO="${1}"
> > shift
> > 
> > +if ! [[ "${DO}" == "start" ||  "${DO}" == "restart" || "${DO}" ==
> > "stop" ]]; then
> > + echo "Usage: ${0} {start|stop|restart} [device(s)]"
> > + exit 1
> > +fi
> > +
> > if [ -n "${1}" ]; then
> > ALL=0
> > for i in green red blue orange; do
> > @@ -100,9 +105,4 @@ case "${DO}" in
> > sleep 1
> > ${0} start ${ARGS}
> > ;;
> > -
> > - *)
> > - echo "Usage: ${0} {start|stop|restart} [device(s)]"
> > - exit 1
> > - ;;
> > esac
> > -- 
> > 2.30.2
> > 
>
diff mbox series

Patch

diff --git a/src/initscripts/system/network b/src/initscripts/system/network
index 9694165f2..06240f53c 100644
--- a/src/initscripts/system/network
+++ b/src/initscripts/system/network
@@ -26,6 +26,11 @@  eval $(/usr/local/bin/readhash /var/ipfire/ethernet/settings)
 DO="${1}"
 shift
 
+if ! [[ "${DO}" == "start" ||  "${DO}" == "restart" || "${DO}" == "stop" ]]; then
+		echo "Usage: ${0} {start|stop|restart} [device(s)]"
+		exit 1
+fi
+
 if [ -n "${1}" ]; then
 	ALL=0
 	for i in green red blue orange; do
@@ -100,9 +105,4 @@  case "${DO}" in
 		sleep 1
 		${0} start ${ARGS}
 		;;
-
-	*)
-		echo "Usage: ${0} {start|stop|restart} [device(s)]"
-		exit 1
-		;;
 esac