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

Adrian Schmutzler mail at adrianschmutzler.de
Mi Mär 20 15:43:14 CET 2019


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 at freifunk.net] On Behalf Of
> Fabian Bläse
> Sent: Montag, 18. März 2019 22:54
> To: franken-dev at 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 at 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
-------------- nächster Teil --------------
Ein Dateianhang mit Binärdaten wurde abgetrennt...
Dateiname   : nicht verfügbar
Dateityp    : application/pgp-signature
Dateigröße  : 834 bytes
Beschreibung: nicht verfügbar
URL         : <https://{'listname': 'franken-dev-freifunk.net', 'hostname': 'lists.freifunk.net'}/pipermail/franken-dev-freifunk.net/attachments/20190320/d6f3904b/attachment.sig>


Mehr Informationen über die Mailingliste franken-dev