[PATCH] vpn-select: Demand hood file to be provided as argument

Tim Niemeyer tim at tn-x.org
So Jul 29 12:21:18 CEST 2018 

Hi

Am 29. Juli 2018 12:18:37 MESZ schrieb mail at adrianschmutzler.de:
>Hallo nochmal,
>
>> -----Original Message-----
>> From: franken-dev [mailto:franken-dev-bounces at freifunk.net] On Behalf
>> Of Tim Niemeyer
>> Sent: Sonntag, 29. Juli 2018 12:04
>> To: franken-dev at freifunk.net
>> Subject: RE: [PATCH] vpn-select: Demand hood file to be provided as
>> argument
>> 
>> Hi Adrian
>> 
>> Am 29. Juli 2018 11:57:42 MESZ schrieb mail at adrianschmutzler.de:
>> >Hallo Tim,
>> >
>> >siehe unten.
>> >
>> >> -----Original Message-----
>> >> From: franken-dev [mailto:franken-dev-bounces at freifunk.net] On
>> Behalf
>> >> Of Tim Niemeyer
>> >> Sent: Sonntag, 29. Juli 2018 11:07
>> >> To: franken-dev at freifunk.net
>> >> Subject: Re: [PATCH] vpn-select: Demand hood file to be provided
>as
>> >> argument
>> >>
>> >> Hi
>> >>
>> >> Sieht sehr gut aus. Unten ne Kleinigkeit.
>> >>
>> >>
>> >> Am 28. Juli 2018 23:15:23 MESZ schrieb Adrian Schmutzler
>> >> <freifunk at adrianschmutzler.de>:
>> >> >By removing the reference to the hood file from vpn-select, we
>> >remove
>> >> >the entire dependency from fff-hoodutils.
>> >> >vpn-select will now work with any file provided, as long as it
>has
>> >the
>> >> >correct syntax. At the moment, the only provider is the
>> >configurehood
>> >> >script. Since the various hood file variants are handled there,
>it
>> >> >seems logical that configurehood also chooses and provides the
>> >correct
>> >> >hood file for vpn-select, instead of vpn-select which had no
>other
>> >> >contact with hood file choice.
>> >> >
>> >> >This is simple, tidy and effective.
>> >> >
>> >> >Fixes #106
>> >> >
>> >> >Signed-off-by: Adrian Schmutzler <freifunk at adrianschmutzler.de>
>> >> >
>> >> >---
>> >> >
>> >> >I raised the version for fff-vpn-select by 2, since it still was
>on
>> >1.
>> >> >The never existant version 2 would have been the initial
>> >> >KeyXchangeV2 version.
>> >> >---
>> >> > src/packages/fff/fff-hoods/Makefile                       | 2 +-
>> >> > src/packages/fff/fff-hoods/files/usr/sbin/configurehood   | 2 +-
>> >> > src/packages/fff/fff-vpn-select/Makefile                  | 2 +-
>> >> > src/packages/fff/fff-vpn-select/files/usr/sbin/vpn-select | 7
>> >++++---
>> >> > 4 files changed, 7 insertions(+), 6 deletions(-)
>> >> >
>> >> >diff --git a/src/packages/fff/fff-hoods/Makefile
>> >> >b/src/packages/fff/fff-hoods/Makefile
>> >> >index b565ac7..93fd430 100644
>> >> >--- a/src/packages/fff/fff-hoods/Makefile
>> >> >+++ b/src/packages/fff/fff-hoods/Makefile
>> >> >@@ -1,7 +1,7 @@
>> >> > include $(TOPDIR)/rules.mk
>> >> >
>> >> > PKG_NAME:=fff-hoods
>> >> >-PKG_VERSION:=2
>> >> >+PKG_VERSION:=3
>> >> > PKG_RELEASE:=1
>> >> >
>> >> > PKG_BUILD_DIR:=$(BUILD_DIR)/$(PKG_NAME)
>> >> >diff --git
>a/src/packages/fff/fff-hoods/files/usr/sbin/configurehood
>> >> >b/src/packages/fff/fff-hoods/files/usr/sbin/configurehood
>> >> >index 57c6f9f..6565c80 100755
>> >> >--- a/src/packages/fff/fff-hoods/files/usr/sbin/configurehood
>> >> >+++ b/src/packages/fff/fff-hoods/files/usr/sbin/configurehood
>> >> >@@ -195,7 +195,7 @@ if [ -s "$hoodfiletmp" ]; then
>> >> >	# and now we get to vpn-select script and load VPNs directly
>from
>> >> >/tmp/keyxchangev2data
>> >> >
>> >> > 	if hasInternet ; then
>> >> >-		sh /usr/sbin/vpn-select
>> >> >+		sh /usr/sbin/vpn-select "$hoodfiletmp"
>> >> > 	else
>> >> > 		sh /usr/sbin/vpn-stop
>> >> > 	fi
>> >> >diff --git a/src/packages/fff/fff-vpn-select/Makefile
>> >> >b/src/packages/fff/fff-vpn-select/Makefile
>> >> >index 4e2d89b..27cff09 100644
>> >> >--- a/src/packages/fff/fff-vpn-select/Makefile
>> >> >+++ b/src/packages/fff/fff-vpn-select/Makefile
>> >> >@@ -1,7 +1,7 @@
>> >> > include $(TOPDIR)/rules.mk
>> >> >
>> >> > PKG_NAME:=fff-vpn-select
>> >> >-PKG_VERSION:=1
>> >> >+PKG_VERSION:=3
>> >> > PKG_RELEASE:=1
>> >> >
>> >> > PKG_BUILD_DIR:=$(BUILD_DIR)/$(PKG_NAME)
>> >> >diff --git
>> >a/src/packages/fff/fff-vpn-select/files/usr/sbin/vpn-select
>> >> >b/src/packages/fff/fff-vpn-select/files/usr/sbin/vpn-select
>> >> >index 7c9bced..86236e8 100755
>> >> >--- a/src/packages/fff/fff-vpn-select/files/usr/sbin/vpn-select
>> >> >+++ b/src/packages/fff/fff-vpn-select/files/usr/sbin/vpn-select
>> >> >@@ -1,15 +1,16 @@
>> >> > #!/bin/sh
>> >> >
>> >> >-. /lib/functions/fff/keyxchange
>> >> > . /usr/share/libubox/jshn.sh
>> >> >
>> >> >+hoodfile="$1"
>> >> >+
>> >>
>> >> Bitte noch kurz prüfen ob der Parameter übergeben wurde und am
>> besten
>> >> auch, ob es eine lesbare Datei ist. Ansonsten mit Fehlermeldung
>> >ausbrechen.
>> >>
>> >> Tim
>> >
>> >Wir prüfen das weiter unten, nämlich ...
>> >
>> >>
>> >>
>> >> > make_config() {
>> >> > # remove old config
>> >> > >/etc/config/tunneldigger
>> >> > rm /tmp/fastd_fff_peers/*
>> >> > count=0
>> >> > Index=1
>> >> >-json_load "$(cat "$hoodfiletmp")"
>> >> >+json_load "$(cat "$hoodfile")"
>> 
>> Ich dachte weil hier dann ein leerer String geladen wird.
>
>Das steht aber innerhalb einer Funktion.

Ha. Okay. Das hab ich übersehen.

Dann passt es eigentlich so auch.

Tim

>Die Funktion make_config wird wiederum nur innerhalb der unten
>diskutierten if-Schleife aufgerufen, d.h. dass [ -s "$hoodfile" ] dann
>bereits geprüft wurde. Und dieses File wird so wohl auch nicht
>gesourced, so dass man sich über die Verwendung der Funktion an anderer
>Stelle Gedanken machen muss.
>
>Weiteres weiter unten.
>
>
>> 
>> >> > json_select vpn
>> 
>> Und hier dann vermutlich ne verwirrende Meldung kommt.
>> 
>> >> > # get fastd peers
>> >> > while json_select "$Index" > /dev/null @@ -54,7 +55,7 @@
>> >json_select
>> >> >".." # back to root  # main
>> >> >
>> >> > # Only do something when file is here and greater 0 byte -if [
>-s
>> >> >"$hoodfiletmp" ]; then
>> >> >+if [ -s "$hoodfile" ]; then
>> >
>> >... hier. Wenn $1 nicht gesetzt wurde, nicht zu einer lesbaren Datei
>> >führt oder zu einer leeren Datei führt, tut das Skript einfach
>nichts.
>> >
>> >Es gibt allerdings nicht die von dir gewünschte Fehlermeldung, das
>> >Skript tut einfach nur etwas, wenn es die richtigen Daten bekommt.
>Für
>> >mich wäre das so okay.
>> >
>> >Wenn du die Fehlermeldung möchtest, könnte man das hier als "else"
>mit
>> >einbauen, dann prüft man gleich den File, und nicht nur das
>Argument.
>> >
>> >Bitte kommentieren.
>> 
>> Im Grunde finde ich die Eingabeprüfung hier nicht sooo wichtig. Der
>> architekturuelle Umbau ist sehr gut und überwiegt.
>> 
>> Trotzdem gehört es vom Code-Style für mich einfach dazu die Eingaben
>> möglichst zeitnah zu prüfen.
>
>Ist schon klar, aber wie willst du es nun konkret haben?
>
>Ich werde jetzt mal die Variante mit else bauen, dass wird dann quasi
>direkt nach dem Start ausgeführt, es steht nur nicht ganz oben. Dafür
>muss man nicht das Gleiche zweimal machen.
>
>Grüße
>
>Adrian
>
>> 
>> Ich denke das ist nur noch ne kleine Verbesserung und du kannst
>> anschließend direkt mein reviewed by mit dran tun. Roberts wirst du
>auch
>> übernehmen können.
>> 
>> Insofern hier mein
>> Reviewed-by: Tim Niemeyer <tim at tn-x.org>
>> 
>> Wäre halt wirklich cool, wenn du die zwei Minuten noch eben rein
>stecken
>> könntest.
>> 
>> Tim
>> 
>> 
>> >Grüße
>> >
>> >Adrian
>> >
>> >
>> >> > 	# set some vars
>> >> > 	hostname=$(cat /proc/sys/kernel/hostname)
>> >> >	mac=$(awk '{ mac=toupper($1); gsub(":", "", mac); print mac }'
>> >> >/sys/class/net/br-mesh/address 2>/dev/null)

Mehr Informationen über die Mailingliste franken-dev