[v2,1/3] fff-gateway: add package

Submitted by Fabian Blaese on March 18, 2019, 9:53 p.m.

Details

Message ID 20190318215401.4082-1-fabian@blaese.de
State Superseded
Headers show

Commit Message

Fabian Blaese March 18, 2019, 9:53 p.m.
This introduces a new script for simple gateway configuration.

The main configuregateway script is able to execute functions
for various steps like 'configure' or 'apply' from scripts in /etc/gateway.d.

This makes it easy to distribute configuration to the appropriate packages.

Signed-off-by: Fabian Bläse <fabian@blaese.de>
---

Changes in v3:
- Remove unnecessary package dependency
- Remove unnecessary shell sources
- Remove wrong sanity check left over from refactoring
---
 src/packages/fff/fff-gateway/Makefile         |  38 +++++++
 .../files/usr/sbin/configuregateway           | 101 ++++++++++++++++++
 2 files changed, 139 insertions(+)
 create mode 100644 src/packages/fff/fff-gateway/Makefile
 create mode 100755 src/packages/fff/fff-gateway/files/usr/sbin/configuregateway

Patch hide | download patch | download mbox

diff --git a/src/packages/fff/fff-gateway/Makefile b/src/packages/fff/fff-gateway/Makefile
new file mode 100644
index 0000000..4ca6889
--- /dev/null
+++ b/src/packages/fff/fff-gateway/Makefile
@@ -0,0 +1,38 @@ 
+include $(TOPDIR)/rules.mk
+
+PKG_NAME:=fff-gateway
+PKG_VERSION:=1
+PKG_RELEASE:=1
+
+PKG_BUILD_DIR:=$(BUILD_DIR)/fff-gateway
+
+include $(INCLUDE_DIR)/package.mk
+
+define Package/fff-gateway
+    SECTION:=base
+    CATEGORY:=Freifunk
+    TITLE:= Freifunk-Franken gateway configuration
+    URL:=https://www.freifunk-franken.de
+endef
+
+define Package/fff-gateway/description
+    This package configures the gateway
+endef
+
+define Build/Prepare
+	echo "all: " > $(PKG_BUILD_DIR)/Makefile
+endef
+
+define Build/Configure
+	# nothing
+endef
+
+define Build/Compile
+	# nothing
+endef
+
+define Package/fff-gateway/install
+	$(CP) ./files/* $(1)/
+endef
+
+$(eval $(call BuildPackage,fff-gateway))
diff --git a/src/packages/fff/fff-gateway/files/usr/sbin/configuregateway b/src/packages/fff/fff-gateway/files/usr/sbin/configuregateway
new file mode 100755
index 0000000..938a5c9
--- /dev/null
+++ b/src/packages/fff/fff-gateway/files/usr/sbin/configuregateway
@@ -0,0 +1,101 @@ 
+#!/bin/sh
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.	See the
+# GNU General Public License for more details.
+
+
+# IMPORTANT!!
+# DO NOT RUN THIS IN CRONJOB!
+
+execute_subshell() {
+	if [ $# -ne 1 ]; then
+		echo "Usage:" "$0" "<function>"
+	fi
+
+	for script in /etc/gateway.d/*; do
+		(
+			# unset function to prevent executing parents shell function
+			unset -f "$1"
+			. "$script"
+
+			if type "$1" | grep "shell function" > /dev/null; then
+				"$1"
+			fi
+		)
+
+		if [ $? -ne 0 ]; then
+			echo "Error when executing" "$1" "from" "$(basename "$script")"
+			exit 1
+		fi
+	done
+}
+
+configure() {
+	echo "This script might remove existing vlans, interfaces, addresses, etc."
+	read -r -p "Do you really want to continue? (y/n) " response
+	if ! [ "$response" == "y" ] || [ "$response" == "Y" ]; then
+		exit 1
+	fi
+
+	execute_subshell configure
+
+	exit 0
+}
+
+restart_services() {
+	execute_subshell reload
+	reload_config
+}
+
+apply_changes() {
+	execute_subshell apply
+	restart_services
+	exit 0
+}
+
+revert_changes() {
+	execute_subshell revert
+	exit 0
+}
+
+test_changes() {
+	restart_services
+
+	sleep 5
+	echo "services restarted. waiting up to 200s for SIGINT.."
+	sleep 200
+	echo "reverting changes.."
+
+	revert_changes
+	restart_services
+}
+
+
+usage() {
+	echo Usage: $0 [OPTION];
+	echo;
+	echo "Options:"
+	echo "	-c: configure. No commit, no restart!"
+	echo "	-t: test changes. Restarts services, waits up to 200s for SIGINT"
+	echo "	-a: apply changes"
+	echo "	-r: revert changes"
+}
+
+
+if [ $# != 1 ]; then
+	usage; exit 1
+fi
+
+case "$1" in
+	-c) configure ;;
+	-t) test_changes ;;
+	-a) apply_changes ;;
+	-r) revert_changes ;;
+	*) usage; exit 1 ;;
+esac

Comments

Robert Langhammer March 19, 2019, 12:37 a.m.
Hallo Fabian,

Reviewed-by: Robert Langhammer <rlanghammer@web.de>

Das mit den sich überschreibenden Funktionen ist mir bisher auch noch
nicht über den Weg gelaufen.  Das gefällt mir richtig gut. So simpel!

Nur der Funktionsname "execute_subshell" ist vielleicht etwas
unglücklich. Es wird ja keine Subshell für Funktionen gestartet. Aber
nicht wichtig :)

Robert

Am 18.03.19 um 22:53 schrieb Fabian Bläse:
> This introduces a new script for simple gateway configuration.
>
> The main configuregateway script is able to execute functions
> for various steps like 'configure' or 'apply' from scripts in /etc/gateway.d.
>
> This makes it easy to distribute configuration to the appropriate packages.
>
> Signed-off-by: Fabian Bläse <fabian@blaese.de>
> ---
>
> Changes in v3:
> - Remove unnecessary package dependency
> - Remove unnecessary shell sources
> - Remove wrong sanity check left over from refactoring
> ---
>  src/packages/fff/fff-gateway/Makefile         |  38 +++++++
>  .../files/usr/sbin/configuregateway           | 101 ++++++++++++++++++
>  2 files changed, 139 insertions(+)
>  create mode 100644 src/packages/fff/fff-gateway/Makefile
>  create mode 100755 src/packages/fff/fff-gateway/files/usr/sbin/configuregateway
>
> diff --git a/src/packages/fff/fff-gateway/Makefile b/src/packages/fff/fff-gateway/Makefile
> new file mode 100644
> index 0000000..4ca6889
> --- /dev/null
> +++ b/src/packages/fff/fff-gateway/Makefile
> @@ -0,0 +1,38 @@
> +include $(TOPDIR)/rules.mk
> +
> +PKG_NAME:=fff-gateway
> +PKG_VERSION:=1
> +PKG_RELEASE:=1
> +
> +PKG_BUILD_DIR:=$(BUILD_DIR)/fff-gateway
> +
> +include $(INCLUDE_DIR)/package.mk
> +
> +define Package/fff-gateway
> +    SECTION:=base
> +    CATEGORY:=Freifunk
> +    TITLE:= Freifunk-Franken gateway configuration
> +    URL:=https://www.freifunk-franken.de
> +endef
> +
> +define Package/fff-gateway/description
> +    This package configures the gateway
> +endef
> +
> +define Build/Prepare
> +	echo "all: " > $(PKG_BUILD_DIR)/Makefile
> +endef
> +
> +define Build/Configure
> +	# nothing
> +endef
> +
> +define Build/Compile
> +	# nothing
> +endef
> +
> +define Package/fff-gateway/install
> +	$(CP) ./files/* $(1)/
> +endef
> +
> +$(eval $(call BuildPackage,fff-gateway))
> diff --git a/src/packages/fff/fff-gateway/files/usr/sbin/configuregateway b/src/packages/fff/fff-gateway/files/usr/sbin/configuregateway
> new file mode 100755
> index 0000000..938a5c9
> --- /dev/null
> +++ b/src/packages/fff/fff-gateway/files/usr/sbin/configuregateway
> @@ -0,0 +1,101 @@
> +#!/bin/sh
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.	See the
> +# GNU General Public License for more details.
> +
> +
> +# IMPORTANT!!
> +# DO NOT RUN THIS IN CRONJOB!
> +
> +execute_subshell() {
> +	if [ $# -ne 1 ]; then
> +		echo "Usage:" "$0" "<function>"
> +	fi
> +
> +	for script in /etc/gateway.d/*; do
> +		(
> +			# unset function to prevent executing parents shell function
> +			unset -f "$1"
> +			. "$script"
> +
> +			if type "$1" | grep "shell function" > /dev/null; then
> +				"$1"
> +			fi
> +		)
> +
> +		if [ $? -ne 0 ]; then
> +			echo "Error when executing" "$1" "from" "$(basename "$script")"
> +			exit 1
> +		fi
> +	done
> +}
> +
> +configure() {
> +	echo "This script might remove existing vlans, interfaces, addresses, etc."
> +	read -r -p "Do you really want to continue? (y/n) " response
> +	if ! [ "$response" == "y" ] || [ "$response" == "Y" ]; then
> +		exit 1
> +	fi
> +
> +	execute_subshell configure
> +
> +	exit 0
> +}
> +
> +restart_services() {
> +	execute_subshell reload
> +	reload_config
> +}
> +
> +apply_changes() {
> +	execute_subshell apply
> +	restart_services
> +	exit 0
> +}
> +
> +revert_changes() {
> +	execute_subshell revert
> +	exit 0
> +}
> +
> +test_changes() {
> +	restart_services
> +
> +	sleep 5
> +	echo "services restarted. waiting up to 200s for SIGINT.."
> +	sleep 200
> +	echo "reverting changes.."
> +
> +	revert_changes
> +	restart_services
> +}
> +
> +
> +usage() {
> +	echo Usage: $0 [OPTION];
> +	echo;
> +	echo "Options:"
> +	echo "	-c: configure. No commit, no restart!"
> +	echo "	-t: test changes. Restarts services, waits up to 200s for SIGINT"
> +	echo "	-a: apply changes"
> +	echo "	-r: revert changes"
> +}
> +
> +
> +if [ $# != 1 ]; then
> +	usage; exit 1
> +fi
> +
> +case "$1" in
> +	-c) configure ;;
> +	-t) test_changes ;;
> +	-a) apply_changes ;;
> +	-r) revert_changes ;;
> +	*) usage; exit 1 ;;
> +esac
Fabian Blaese March 19, 2019, 7:26 a.m.
Hallo Robert,

On 19.03.19 01:37, robert wrote:
> Nur der Funktionsname "execute_subshell" ist vielleicht etwas
> unglücklich. Es wird ja keine Subshell für Funktionen gestartet. Aber
> nicht wichtig :)
Was macht dann diese Klammern "( [..] )"?

Gruß
Fabian
Adrian Schmutzler March 20, 2019, 2:43 p.m.
Hallo,

gefällt mir gut, sieht für mich nach einer sehr eleganten Lösung aus.

Einige Kleinigkeiten unten.

> -----Original Message-----
> From: franken-dev [mailto:franken-dev-bounces@freifunk.net] On Behalf Of
> Fabian Bläse
> Sent: Montag, 18. März 2019 22:54
> To: franken-dev@freifunk.net
> Subject: [PATCH v2 1/3] fff-gateway: add package
> 
> This introduces a new script for simple gateway configuration.
> 
> The main configuregateway script is able to execute functions
> for various steps like 'configure' or 'apply' from scripts in /etc/gateway.d.
> 
> This makes it easy to distribute configuration to the appropriate packages.
> 
> Signed-off-by: Fabian Bläse <fabian@blaese.de>
> ---
> 
> Changes in v3:
> - Remove unnecessary package dependency
> - Remove unnecessary shell sources
> - Remove wrong sanity check left over from refactoring
> ---
>  src/packages/fff/fff-gateway/Makefile         |  38 +++++++
>  .../files/usr/sbin/configuregateway           | 101 ++++++++++++++++++
>  2 files changed, 139 insertions(+)
>  create mode 100644 src/packages/fff/fff-gateway/Makefile
>  create mode 100755 src/packages/fff/fff-
> gateway/files/usr/sbin/configuregateway
> 
> diff --git a/src/packages/fff/fff-gateway/Makefile b/src/packages/fff/fff-
> gateway/Makefile
> new file mode 100644
> index 0000000..4ca6889
> --- /dev/null
> +++ b/src/packages/fff/fff-gateway/Makefile
> @@ -0,0 +1,38 @@
> +include $(TOPDIR)/rules.mk
> +
> +PKG_NAME:=fff-gateway
> +PKG_VERSION:=1
> +PKG_RELEASE:=1
> +
> +PKG_BUILD_DIR:=$(BUILD_DIR)/fff-gateway
> +
> +include $(INCLUDE_DIR)/package.mk
> +
> +define Package/fff-gateway
> +    SECTION:=base
> +    CATEGORY:=Freifunk
> +    TITLE:= Freifunk-Franken gateway configuration
> +    URL:=https://www.freifunk-franken.de
> +endef
> +
> +define Package/fff-gateway/description
> +    This package configures the gateway
> +endef
> +
> +define Build/Prepare
> +	echo "all: " > $(PKG_BUILD_DIR)/Makefile
> +endef
> +
> +define Build/Configure
> +	# nothing
> +endef
> +
> +define Build/Compile
> +	# nothing
> +endef
> +
> +define Package/fff-gateway/install
> +	$(CP) ./files/* $(1)/
> +endef
> +
> +$(eval $(call BuildPackage,fff-gateway))
> diff --git a/src/packages/fff/fff-gateway/files/usr/sbin/configuregateway
> b/src/packages/fff/fff-gateway/files/usr/sbin/configuregateway
> new file mode 100755
> index 0000000..938a5c9
> --- /dev/null
> +++ b/src/packages/fff/fff-gateway/files/usr/sbin/configuregateway
> @@ -0,0 +1,101 @@
> +#!/bin/sh
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.	See the
> +# GNU General Public License for more details.
> +
> +
> +# IMPORTANT!!
> +# DO NOT RUN THIS IN CRONJOB!
> +
> +execute_subshell() {

Sehe ich wie Robert. Evtl. execute_subfunction? execute_function?
Ist aber tatsächlich nicht wichtig.

> +	if [ $# -ne 1 ]; then
> +		echo "Usage:" "$0" "<function>"
> +	fi
> +
> +	for script in /etc/gateway.d/*; do
> +		(
> +			# unset function to prevent executing parents shell
> function
> +			unset -f "$1"
> +			. "$script"
> +
> +			if type "$1" | grep "shell function" > /dev/null; then

grep -q statt grep > /dev/null ?

> +				"$1"
> +			fi
> +		)
> +
> +		if [ $? -ne 0 ]; then
> +			echo "Error when executing" "$1" "from" "$(basename
> "$script")"
> +			exit 1
> +		fi
> +	done

hier ein "exit 0", wenn du oben "exit 1" machst?

> +}
> +
> +configure() {
> +	echo "This script might remove existing vlans, interfaces, addresses,
> etc."
> +	read -r -p "Do you really want to continue? (y/n) " response
> +	if ! [ "$response" == "y" ] || [ "$response" == "Y" ]; then
> +		exit 1
> +	fi
> +
> +	execute_subshell configure
> +
> +	exit 0
> +}
> +
> +restart_services() {
> +	execute_subshell reload

Entweder immer "restart" oder immer "reload"! Hier sollten wir entweder die subfunctions auf restart oder die Funktion hier auf reload ändern. Was technisch mehr Sinn macht weißt du wahrscheinlich besser.

> +	reload_config
Erst service restart und dann reload_config? Ist das so rum richtig ("ja" reicht mir, ich habs beim letzten Mal schon nicht verstanden.)


> +}
> +
> +apply_changes() {
> +	execute_subshell apply
> +	restart_services
> +	exit 0
> +}
> +
> +revert_changes() {
> +	execute_subshell revert

1. Kein reload_config/restart_services hier? Ich weiß, dass das bei test_changes mit dort steht, aber bei apply_changes hast du es mit in die Funktion gezogen. Vll. kann man das einheitlich machen (d.h. ich würde es auch bei revert_changes mit in die Funktion ziehen.
2. Was passiert, wenn zweimal das gleiche revertet wird? (z.B. zwei "subshells", die jeweils uci revert system machen. Ist das sicher?)

> +	exit 0
> +}
> +
> +test_changes() {
> +	restart_services
> +
> +	sleep 5
> +	echo "services restarted. waiting up to 200s for SIGINT.."
> +	sleep 200
> +	echo "reverting changes.."

Hier und beim Echo davor sollte man mit Großbuchstaben beginnen und entweder einen Punkt am Schluss machen oder drei.
So Zeug würde ich aber ggf. in einem separaten Patch nachreichen, das ist erstmal nicht wichtig.

> +
> +	revert_changes
> +	restart_services
> +}
> +
> +
> +usage() {
> +	echo Usage: $0 [OPTION];
> +	echo;
> +	echo "Options:"
> +	echo "	-c: configure. No commit, no restart!"
> +	echo "	-t: test changes. Restarts services, waits up to 200s for SIGINT"
> +	echo "	-a: apply changes"
> +	echo "	-r: revert changes"

Auch hier: Großbuchstaben nach dem Doppelpunkt. Auch hier: später.

Beste Grüße

Adrian

> +}
> +
> +
> +if [ $# != 1 ]; then
> +	usage; exit 1
> +fi
> +
> +case "$1" in
> +	-c) configure ;;
> +	-t) test_changes ;;
> +	-a) apply_changes ;;
> +	-r) revert_changes ;;
> +	*) usage; exit 1 ;;
> +esac
> --
> 2.21.0
Adrian Schmutzler March 20, 2019, 5 p.m.
Hallo nochmal,

wir hatten zuletzt die kompletten Makefiles auf Tabs umgestellt, auch die Package-definition und -description.
Ggf. noch mit ändern, wenn du dran denkst.

Beste Grüße

Adrian


> -----Original Message-----
> From: franken-dev [mailto:franken-dev-bounces@freifunk.net] On Behalf Of
> Fabian Bläse
> Sent: Montag, 18. März 2019 22:54
> To: franken-dev@freifunk.net
> Subject: [PATCH v2 1/3] fff-gateway: add package
> 
> This introduces a new script for simple gateway configuration.
> 
> The main configuregateway script is able to execute functions
> for various steps like 'configure' or 'apply' from scripts in /etc/gateway.d.
> 
> This makes it easy to distribute configuration to the appropriate packages.
> 
> Signed-off-by: Fabian Bläse <fabian@blaese.de>
> ---
> 
> Changes in v3:
> - Remove unnecessary package dependency
> - Remove unnecessary shell sources
> - Remove wrong sanity check left over from refactoring
> ---
>  src/packages/fff/fff-gateway/Makefile         |  38 +++++++
>  .../files/usr/sbin/configuregateway           | 101 ++++++++++++++++++
>  2 files changed, 139 insertions(+)
>  create mode 100644 src/packages/fff/fff-gateway/Makefile
>  create mode 100755 src/packages/fff/fff-
> gateway/files/usr/sbin/configuregateway
> 
> diff --git a/src/packages/fff/fff-gateway/Makefile b/src/packages/fff/fff-
> gateway/Makefile
> new file mode 100644
> index 0000000..4ca6889
> --- /dev/null
> +++ b/src/packages/fff/fff-gateway/Makefile
> @@ -0,0 +1,38 @@
> +include $(TOPDIR)/rules.mk
> +
> +PKG_NAME:=fff-gateway
> +PKG_VERSION:=1
> +PKG_RELEASE:=1
> +
> +PKG_BUILD_DIR:=$(BUILD_DIR)/fff-gateway
> +
> +include $(INCLUDE_DIR)/package.mk
> +
> +define Package/fff-gateway
> +    SECTION:=base
> +    CATEGORY:=Freifunk
> +    TITLE:= Freifunk-Franken gateway configuration
> +    URL:=https://www.freifunk-franken.de
> +endef
> +
> +define Package/fff-gateway/description
> +    This package configures the gateway
> +endef
> +
> +define Build/Prepare
> +	echo "all: " > $(PKG_BUILD_DIR)/Makefile
> +endef
> +
> +define Build/Configure
> +	# nothing
> +endef
> +
> +define Build/Compile
> +	# nothing
> +endef
> +
> +define Package/fff-gateway/install
> +	$(CP) ./files/* $(1)/
> +endef
> +
> +$(eval $(call BuildPackage,fff-gateway))
> diff --git a/src/packages/fff/fff-gateway/files/usr/sbin/configuregateway
> b/src/packages/fff/fff-gateway/files/usr/sbin/configuregateway
> new file mode 100755
> index 0000000..938a5c9
> --- /dev/null
> +++ b/src/packages/fff/fff-gateway/files/usr/sbin/configuregateway
> @@ -0,0 +1,101 @@
> +#!/bin/sh
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 3 of the License, or
> +# (at your option) any later version.
> +
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.	See the
> +# GNU General Public License for more details.
> +
> +
> +# IMPORTANT!!
> +# DO NOT RUN THIS IN CRONJOB!
> +
> +execute_subshell() {
> +	if [ $# -ne 1 ]; then
> +		echo "Usage:" "$0" "<function>"
> +	fi
> +
> +	for script in /etc/gateway.d/*; do
> +		(
> +			# unset function to prevent executing parents shell
> function
> +			unset -f "$1"
> +			. "$script"
> +
> +			if type "$1" | grep "shell function" > /dev/null; then
> +				"$1"
> +			fi
> +		)
> +
> +		if [ $? -ne 0 ]; then
> +			echo "Error when executing" "$1" "from" "$(basename
> "$script")"
> +			exit 1
> +		fi
> +	done
> +}
> +
> +configure() {
> +	echo "This script might remove existing vlans, interfaces, addresses,
> etc."
> +	read -r -p "Do you really want to continue? (y/n) " response
> +	if ! [ "$response" == "y" ] || [ "$response" == "Y" ]; then
> +		exit 1
> +	fi
> +
> +	execute_subshell configure
> +
> +	exit 0
> +}
> +
> +restart_services() {
> +	execute_subshell reload
> +	reload_config
> +}
> +
> +apply_changes() {
> +	execute_subshell apply
> +	restart_services
> +	exit 0
> +}
> +
> +revert_changes() {
> +	execute_subshell revert
> +	exit 0
> +}
> +
> +test_changes() {
> +	restart_services
> +
> +	sleep 5
> +	echo "services restarted. waiting up to 200s for SIGINT.."
> +	sleep 200
> +	echo "reverting changes.."
> +
> +	revert_changes
> +	restart_services
> +}
> +
> +
> +usage() {
> +	echo Usage: $0 [OPTION];
> +	echo;
> +	echo "Options:"
> +	echo "	-c: configure. No commit, no restart!"
> +	echo "	-t: test changes. Restarts services, waits up to 200s for SIGINT"
> +	echo "	-a: apply changes"
> +	echo "	-r: revert changes"
> +}
> +
> +
> +if [ $# != 1 ]; then
> +	usage; exit 1
> +fi
> +
> +case "$1" in
> +	-c) configure ;;
> +	-t) test_changes ;;
> +	-a) apply_changes ;;
> +	-r) revert_changes ;;
> +	*) usage; exit 1 ;;
> +esac
> --
> 2.21.0
Fabian Blaese March 21, 2019, 1:27 p.m.
Hallo,

On 20.03.19 15:43, Adrian Schmutzler wrote:
> Hallo,
> 
> gefällt mir gut, sieht für mich nach einer sehr eleganten Lösung aus.
danke :-)

>> +# IMPORTANT!!
>> +# DO NOT RUN THIS IN CRONJOB!
>> +
>> +execute_subshell() {
> 
> Sehe ich wie Robert. Evtl. execute_subfunction? execute_function?
> Ist aber tatsächlich nicht wichtig.
Nun, das Ding führt halt einen bestimmten Konfigurationsschritt in einer Subshell aus (damit die Funktionen im Fehlerfall exit-en können).
execute_with_subshell() ?

>> +	for script in /etc/gateway.d/*; do
>> +		(
>> +			# unset function to prevent executing parents shell
>> function
>> +			unset -f "$1"
>> +			. "$script"
>> +
>> +			if type "$1" | grep "shell function" > /dev/null; then
> 
> grep -q statt grep > /dev/null ?
Jo.


>> +		if [ $? -ne 0 ]; then
>> +			echo "Error when executing" "$1" "from" "$(basename
>> "$script")"
>> +			exit 1
>> +		fi
>> +	done
> 
> hier ein "exit 0", wenn du oben "exit 1" machst?
Ne. Wenn es keinen Fehler gibt, brauche ich ja auch nicht vorzeitig beenden.
Ggf. möchte man ja mehrere Konfigurationsschritte hintereinander machen oder so.

>> +restart_services() {
>> +	execute_subshell reload
> 
> Entweder immer "restart" oder immer "reload"! Hier sollten wir entweder die subfunctions auf restart oder die Funktion hier auf reload ändern. Was technisch mehr Sinn macht weißt du wahrscheinlich besser.
Stimmt. "reload" wäre wohl besser.

> 
>> +	reload_config
> Erst service restart und dann reload_config? Ist das so rum richtig ("ja" reicht mir, ich habs beim letzten Mal schon nicht verstanden.)
Nun, bei OpenWRT funktioniert das neustarten der Diensten mit veränderter Konfiguration über reload_config. Das reicht auch ein mal. (Nicht wie bei configurenetwork 400 mal)
Die Unterfunktion reload gibt Modulen nur die Möglichkeit, da noch etwas hinzuzufügen. Zum Beispiel ist das Setzen des Hostnamen im sysfs so etwas, was das Meta-Modul dann hier tut.

Gruß
Fabian

> 
> 
>> +}
>> +
>> +apply_changes() {
>> +	execute_subshell apply
>> +	restart_services
>> +	exit 0
>> +}
>> +
>> +revert_changes() {
>> +	execute_subshell revert
> 
> 1. Kein reload_config/restart_services hier? Ich weiß, dass das bei test_changes mit dort steht, aber bei apply_changes hast du es mit in die Funktion gezogen. Vll. kann man das einheitlich machen (d.h. ich würde es auch bei revert_changes mit in die Funktion ziehen.
Das revert war nur eine Möglichkeit im uci gesetzte Einstellungen, die noch nicht applied sind, zurückzusetzen. Daher ist hier auch kein reload_config nötig.
Applied werden können Dinge nur durch test und apply. test() reverted dann selbst und apply() lässt keinen revert mehr zu.

Dieses revert() verändert also (by design) nur den Konfigurationszustand im RAM.

> 2. Was passiert, wenn zweimal das gleiche revertet wird? (z.B. zwei "subshells", die jeweils uci revert system machen. Ist das sicher?)
Weiß nicht..

> Hier und beim Echo davor sollte man mit Großbuchstaben beginnen und entweder einen Punkt am Schluss machen oder drei.
> So Zeug würde ich aber ggf. in einem separaten Patch nachreichen, das ist erstmal nicht wichtig.
Feinheiten. Wenn es noch eine v3 gibt, versuche ich das anzupassen. Ansonsten $irgendwann mal.

Gruß
Fabian
Adrian Schmutzler March 21, 2019, 2:51 p.m.
Hallo Fabian,

lass das execute_subshell einfach.

Bei dem exit 0 ging es mir primär um den return status. Gibt eine Funktion da dann automatisch „0“ zurück?

>>> +revert_changes() { 
>>> +    execute_subshell revert 
>> 
>> 1. Kein reload_config/restart_services hier? Ich weiß, dass das bei test_changes mit dort steht, aber bei apply_changes hast du es mit in die Funktion gezogen. Vll. kann man das einheitlich machen (d.h. ich würde es auch bei revert_changes mit in die Funktion ziehen.
>Das revert war nur eine Möglichkeit im uci gesetzte Einstellungen, die noch nicht applied sind, zurückzusetzen. Daher ist hier auch kein reload_config nötig.
>Applied werden können Dinge nur durch test und apply. test() reverted dann selbst und apply() lässt keinen revert mehr zu.
>Dieses revert() verändert also (by design) nur den Konfigurationszustand im RAM. 

Okay, kann man so auch sehen. Lass es, wie es ist.

Bleiben von meiner Seite das grep -q und v.a. die restart/reload-Konsistenz, danach würde ich das Ding reviewen.
Kannste wegen mir auch schon mal vorab schicken (ohne 2/3 und 3/3), dann ist das schon mal weg.

Beste Grüße

Adrian