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

mail at adrianschmutzler.de mail at adrianschmutzler.de
So Jul 29 12:18:37 CEST 2018 

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.
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