[PATCH v4 01/11] simple-tc: Fix simple-tc not being active if set

Di Aug 1 23:58:37 CEST 2017

Hallo Tobias,

zu 1.:
Wir konfigurieren simple-tc richtig.
Das hotplug.d Skript
(https://github.com/freifunk-gluon/packages/blob/90380414f10842238b7ebc21c34
dbaf986659320/net/simple-tc/files/etc/hotplug.d/net/50-simple-tc) nutzt die
config_foreach Funktion, die alle Elemente parst, die Typ 'interface' haben.
Der Name ist völlig egal und könnte auch qwertz oder sonstwie sein. Relevant
ist die option ifname (simple-tc.example.ifname), die den Bezug zum WAN
interface herstellt. Diese wird richtig gesetzt, entsprechend bin ich gegen
eine Änderung, weil diese nichts bringt und eher Probleme aufwirft.
Effektiv wird also das letzte 'interface' genutzt, das den richtigen
'ifname' eingetragen hat (z.B. falls mehrere interfaces in
/etc/config/simple-tc vorhanden sind).

zu 2.:
Das mit iface kann ich mal testen, wobei mich wundert, das andere Interfaces
hotplug.d/net ja auch triggern. Kann es sein, dass es einen
hotplug-Parameter gibt, der nicht bei allen interfaces gesetzt ist? So wie
z.B. bei SATA-Festplatten/-Ports einzeln die Hotplug-Fähigkeit im BIOS
konfiguriert werden kann (bei AHCI)?
Den Teil mit dem procd_add_reload_trigger verstehe ich nicht ganz, da fehlt
mir der Hintergrund der Software-Konzeption.
Tatsächlich habe ich die jetzige Lösung als Workaround gedacht, bis es eine
echte Lösung gibt (was meines Erachtens noch etwas dauern könnte). Da wir
ohnehin nur 'example' setzen, kann man im configurenetwork auch direkt
'example' auswerten.

Im Moment ist die Situation ja so, dass man im WebUI ein Feature einstellen
kann, dass keine Funktion hat (immerhin auf wohl allen 20170110-Geräten).
Falls eine Möglichkeit zeitnah absehbar ist, das Ganze generisch zu fixen,
würde ich diese natürlich bevorzugen (bei anderen interfaces als WAN
funktioniert das Originalskript aber wahrscheinlich sogar). Falls dies nicht
absehbar ist (und das war zum Zeitpunkt der Patcherstellung der Fall) würde
ich das lieber erstmal als Workaround reparieren und dann irgendwann später
mal ordentlich lösen, wenn jemand das Problem wirklich durchdrungen hat.

Zudem existiert ja das Problem, dass die Einstellung nicht update-sicher
ist, was ich mit einem weiteren Patch (das fff config file) beheben will.
Auch hier wird natürlich wieder _nur_ das WAN-Setting gespeichert (generisch
für alle ist das ja wenig sinnvoll), sodass wieder spezifisch 'example'
herausgegriffen wird. Im Grunde ist es also wahrscheinlich gerechtfertigt,
hier auf die nicht generische Lösung zu setzen, da die wohl als einzige von
der Mehrzahl der Nutzer genutzt wird.

Zu den Inlines:
siehe Inline!

Sorry für den vielen Text und beste Grüße

Adrian

-----Original Message-----
From: franken-dev [mailto:franken-dev-bounces at freifunk.net] On Behalf Of
Tobias Klaus
Sent: Dienstag, 1. August 2017 00:12
To: franken-dev at freifunk.net; Adrian Schmutzler
<freifunk at adrianschmutzler.de>
Subject: Re: [PATCH v4 01/11] simple-tc: Fix simple-tc not being active if
set

Hallo Adrian,

ich hab mir gerade mal das mantis-issue und den Patch angeschaut und das war
jetzt gar nicht so leicht sich hier eine Meinung zu bilden. 

Wenn ich mich nicht irre liegen dem nicht-Funktionieren von simple-tc doch 2
Probleme zugrunde:

1. Wir konfigurieren simple-tc nicht ordentlich und lassen den Standardwert
für das interface mit "example" stehen.
- ich denke hier sollten wir das "ordentlich" konfigurieren und example    
  löschen und das jeweilige "wan" konfigurieren. Dafür bietet sich
vermutlich
  ein entsprechendes uci-default script an

2. hotplug.d/net wird nicht mit unserem WAN aufgerufen. 
Nachdem ich auf die schnelle keine ordentliche Doku zu hotplug.de/net
gefunden habe, gehe ich mal davon aus, dass sich das an der falschen Stelle
befindet und eher in hotplug.d/iface landen hätte sollen. So oder so würde
ich das ganze eher über ein init-Skript lösen, das durch ein
procd_add_reload_trigger "simple-tc" immer mit dem "reload"-Parameter
aufgerufen wird, wenn sich was im /etc/config/simplce-tc ändert. Dann müssen
wir uns auch keine Gedanken machen, wann wir es manuell aufrufen müssen. Für
ein solches Skript, könnten vermutlich die meisten Teile von dem Skript
/etc/hotplug.d/net/50-simple-tc wieder verwendet werden. Die vorgeschlagene
Lösung ist mir hier etwas zu "ungenerisch", da sie fest den wert für das
interface "example" auf das wan- interface anwenden.

Zusätzlich zu diesen zwei Überlegungen, frage ich mich noch _wo_ man solche
Änderungen (vor allem die 2.) unterbringt. Eigentlich wäre hier ein Patch
gegen den gluon-Feed, der durch unser buildskript aufgespielt wird, fast das
sauberste. Diesen könnte man dann auch versuchen upstream zu kriegen. 
Allerdings können wir das auch wie vorgeschlagen in fff-network lassen(das
für uns kaputte hotplug Skript tut ja nicht weh) oder aber (das finde ich
wiederum etwas sauberer) ein fff-simpletc Paket einführen.

Wenn ich mir das so ansehe weiß ich wieder, warum ich simple-tc immer
ignoriert habe und gehofft habe, dass es einfach tut.... :-D

Ich hab inline noch ein paar Anmerkungen zum Code.

Viele Grüße
Tobias

Am Freitag, 21. Juli 2017, 15:08:39 CEST schrieb Adrian Schmutzler:
> Simple-tc is active immediately after the call and needs no restart of 
> anything
> 
> Signed-off-by: Adrian Schmutzler <freifunk at adrianschmutzler.de>
> 
> Tested-by: Adrian Schmutzler <freifunk at adrianschmutzler.de>
> ---
>  src/packages/fff/fff-network/Makefile                      |  2 +-
>  src/packages/fff/fff-network/files/usr/sbin/applysimpletc  | 14
> ++++++++++++++ .../fff/fff-network/files/usr/sbin/configurenetwork
|
> 12 ++++++++++++ .../fff/fff-web/files/www/ssl/cgi-bin/settings.html       
> |  3 +++ 4 files changed, 30 insertions(+), 1 deletion(-)
>  create mode 100755
> src/packages/fff/fff-network/files/usr/sbin/applysimpletc
> 
> diff --git a/src/packages/fff/fff-network/Makefile
> b/src/packages/fff/fff-network/Makefile index fee3f98..f7dc63c 100644
> --- a/src/packages/fff/fff-network/Makefile
> +++ b/src/packages/fff/fff-network/Makefile
> @@ -1,7 +1,7 @@
>  include $(TOPDIR)/rules.mk
> 
>  PKG_NAME:=fff-network
> -PKG_VERSION:=0.0.6
> +PKG_VERSION:=7
Das ist ein gehöriger Versionssprung. ;-)
ADRIAN: Die Versionsnummer sind inkonsistent, insofern bin ich davon
ausgegangen, von 0.0.x auf x+1 zu ändern. Beim Nodewatcher gibt es ja z.B.
auch kein 0.0.x

>  PKG_RELEASE:=1
> 
>  PKG_BUILD_DIR:=$(BUILD_DIR)/fff-network
> diff --git a/src/packages/fff/fff-network/files/usr/sbin/applysimpletc
> b/src/packages/fff/fff-network/files/usr/sbin/applysimpletc new file 
> mode
> 100755
> index 0000000..a9744b0
> --- /dev/null
> +++ b/src/packages/fff/fff-network/files/usr/sbin/applysimpletc
> @@ -0,0 +1,14 @@
> +#!/bin/sh
> +
> +tc_device=$(uci -q get "network.wan.ifname") tc_enabled=$(uci -q get 
> +"simple-tc.example.enabled") tc_in=$(uci -q get 
> +"simple-tc.example.limit_ingress")
> +tc_out=$(uci -q get "simple-tc.example.limit_egress")
> +
> +if [ "$tc_enabled" -eq 1 ] ; then
> +	test -n "$tc_in" || tc_in=-
> +	test -n "$tc_out" || tc_out=-
tc_out=${tc_out:-"-"} ist etwas eleganter und weißt tc_out den Wert von
$tc_out zu oder, falls nicht vorhanden, "-".
ADRIAN: Die Syntax kenne ich nicht ... ;-)

> +	simple-tc "$tc_device" "$tc_in" "$tc_out"
Wenn man diese Zeile ans Ende des if/else zieht kann man sich den else-Zweig
sparen und muss in Zukunft nur einen simple-tc-Aufruf "pflegen".
ADRIAN: Sicher? Denn tc_in und tc_out werden nicht auf "-" gesetzt, nur weil
tc_enabled=0 ist! Die müsste man dann ich else beide setzen ...
> +else
> +	simple-tc "$tc_device" - -
> +fi
> diff --git 
> a/src/packages/fff/fff-network/files/usr/sbin/configurenetwork
> b/src/packages/fff/fff-network/files/usr/sbin/configurenetwork index 
> 7ecfa3b..63b216a 100755
> --- a/src/packages/fff/fff-network/files/usr/sbin/configurenetwork
> +++ b/src/packages/fff/fff-network/files/usr/sbin/configurenetwork
> @@ -189,3 +189,15 @@ else
> 
>      /etc/init.d/fff-uradvd restart
>  fi
> +
> +# Apply traffic control
Wenn wir das skript "applysimpletc" doch drin lassen, sollte es hier direkt
aufgerufen werden. Auch so sparen wir uns duplizierten und vor allem
dupliziert zu pflegenden Code.
ADRIAN: War so in meiner ursprünglichen Variante. Da configurenetwork aber
beim Boot aufgerufen wird, fand ich es besser, simple-tc nur aufzurufen,
wenn es gebraucht wird. Sonst würde auf allen Geräten ohne Traffic-Control
immer "simple-tc - -" beim Start aufgerufen (was nicht sein muss, um eine
unnötige Störungsquelle zu vermeiden). In der applysimpletc wiederum braucht
man "simple-tc - -", da so beim Aufruf aus dem WebUI nach Änderung die alten
Werte gelöscht werden müssen.
Mir ist nichts eingefallen, das eleganter zu lösen.

> +tc_device=$(uci -q get "network.wan.ifname") tc_enabled=$(uci -q get 
> +"simple-tc.example.enabled") tc_in=$(uci -q get 
> +"simple-tc.example.limit_ingress")
> +tc_out=$(uci -q get "simple-tc.example.limit_egress")
> +
> +if [ "$tc_enabled" -eq 1 ] ; then
> +	test -n "$tc_in" || tc_in=-
> +	test -n "$tc_out" || tc_out=-
> +	simple-tc "$tc_device" "$tc_in" "$tc_out"
> +fi
> diff --git 
> a/src/packages/fff/fff-web/files/www/ssl/cgi-bin/settings.html
> b/src/packages/fff/fff-web/files/www/ssl/cgi-bin/settings.html index 
> a7417dc..bdbd69b 100755
> --- a/src/packages/fff/fff-web/files/www/ssl/cgi-bin/settings.html
> +++ b/src/packages/fff/fff-web/files/www/ssl/cgi-bin/settings.html
> @@ -32,6 +32,9 @@ if [ "$REQUEST_METHOD" == "POST" ] ; then
>  		uci -q set
"simple-tc.example.limit_egress=${POST_limit_egress}"
> 
>  		uci commit
> +
> +		/usr/sbin/applysimpletc
> +
>  		MSG='<span class="green">Daten gespeichert! - Bitte Router 
> neustarten.</span>' fi  fi

--
franken-dev mailing list
franken-dev at freifunk.net
http://lists.freifunk.net/mailman/listinfo/franken-dev-freifunk.net