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

[Fabian Bläse]

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

-------------- nächster Teil --------------
Ein Dateianhang mit Binärdaten wurde abgetrennt...
Dateiname   : signature.asc
Dateityp    : application/pgp-signature
Dateigröße  : 833 bytes
Beschreibung: OpenPGP digital signature
URL         : <https://{'listname': 'franken-dev-freifunk.net', 'hostname': 'lists.freifunk.net'}/pipermail/franken-dev-freifunk.net/attachments/20190321/1da5c974/attachment.sig>