| Message ID | 1500642529-3627-2-git-send-email-freifunk@adrianschmutzler.de |
|---|---|
| State | Superseded |
| Headers | show |
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 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=- + simple-tc "$tc_device" "$tc_in" "$tc_out" +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 +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
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@adrianschmutzler.de> > > Tested-by: Adrian Schmutzler <freifunk@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. ;-) > 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, "-". > + 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". > +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. > +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
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@freifunk.net] On Behalf Of Tobias Klaus Sent: Dienstag, 1. August 2017 00:12 To: franken-dev@freifunk.net; Adrian Schmutzler <freifunk@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@adrianschmutzler.de> > > Tested-by: Adrian Schmutzler <freifunk@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@freifunk.net http://lists.freifunk.net/mailman/listinfo/franken-dev-freifunk.net
Nachtrag: Warum steht der Code im configurenetwork? => Um Race-Conditions auszuschließen, wollte ich simple-tc erst direkt aufrufen, wenn das Netzwerk "konfiguriert" ist. Da configurenetwork in der rc.local aufgerufen wird, also meines Wissens nach allem anderen was es so als boot-init-Zeugs gibt, muss ich simple-tc wieder danach callen (sonst gibt es eth0 bzw. eth1 evtl. gar nicht). Also entweder (wie im Patch) am Ende von configurenetwork einfügen oder als eigenes Skript in rc.local aufrufen (was mir nicht so gut gefallen hat). Grüße Adrian -----Original Message----- From: franken-dev [mailto:franken-dev-bounces@freifunk.net] On Behalf Of Tobias Klaus Sent: Dienstag, 1. August 2017 00:12 To: franken-dev@freifunk.net; Adrian Schmutzler <freifunk@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@adrianschmutzler.de> > > Tested-by: Adrian Schmutzler <freifunk@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. ;-) > 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, "-". > + 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". > +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. > +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@freifunk.net http://lists.freifunk.net/mailman/listinfo/franken-dev-freifunk.net
Hallo, Am Dienstag, 1. August 2017, 23:58:37 CEST schrieb mail@adrianschmutzler.de: > 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). Ja du hast recht, das hab ich falsch interpretiert. > 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)? Ich könnte mir auch vorstellen, dass der hotplug Mechanismus zu spät aktiviert wird und so das Event verloren geht. Wenn einen das interessiert, sollte man vielleicht mal auf der lede-Mailingliste oder bugtracker ansprechen. Unabhängig davon finde ich immer mehr, dass es gar nicht ausschließlich ins hotplug gehört. Mehr dazu im nächsten Block. > Den Teil mit dem procd_add_reload_trigger verstehe ich nicht ganz, da fehlt > mir der Hintergrund der Software-Konzeption. Lede bietet die Möglichkeit in initskripten ein "callback" zu hinterlegen wenn sich eine config section im uci ändert. https://wiki.openwrt.org/inbox/procd-init-scripts Und das ist ja das was wir _eigentlich_ wollen: Beim wird die simple-tc config an den kernel gegeben und wenn dich was ändert wird die neue weitergegeben. Somit müssten wir uns gar nicht mehr um explizite Aufrufen kümmern, sondern müssten nur noch die Config ändern. Da man das hotplug-Skript wahrscheinlich trotzdem weiterhin haben will (evtl ruft es dann nur noch ein reload auf dem init-Skript auf) könnte das eine race-condition erzeugen. Da die Operationen aber idempotent zu sein scheinen sollte das kein größeres Problem sein. Leider bin ich heute anders als geplant weder dazugekommen da mal umzusetzen, noch mir die weiteren Patches anzuschauen. > 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. Eigentlich hast du recht, aber ich hab das Gefühl, dass wir nicht weit von generisch weg sind und obengenanntes init-Skript könnte man evtl eben auch gleich beim paket selber einreichen. Daher würde ich das ganz gern mal probieren. > Zu den Inlines: > siehe Inline! Ebenso > Sorry für den vielen Text und beste Grüße Ebenso Tobias > -----Original Message----- > From: franken-dev [mailto:franken-dev-bounces@freifunk.net] On Behalf Of > Tobias Klaus > Sent: Dienstag, 1. August 2017 00:12 > To: franken-dev@freifunk.net; Adrian Schmutzler > <freifunk@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@adrianschmutzler.de> > > > > Tested-by: Adrian Schmutzler <freifunk@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 ... Ja du hast recht, mir ging es hauptsächlich darum den Aufruf nach unten zu ziehen. Das pflegt sich leichter. Wenn man ein bisschen rumprobiert lässt sich das bestimmt auch nur im mit dem if zweig setzen. Aber das muss nicht. > > +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. Ah, jetzt sehe ich den Unterschied. Ich würde es aber tatsächlich in kauf nehmen manchmal ein überflüssiges simple-tc aufzurufen, als an zwei Stellen duplizierten Code mitzuführen. Das sollte doch bis auf die vermutlich fast vernachlässigbare Laufzeit von simple-tc keine Nebenwirkungen haben, oder? Aber wie schon gesagt, wäre es mir am Liebsten unsere skripte ganz davon frei zu halten. > > +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@freifunk.net > http://lists.freifunk.net/mailman/listinfo/franken-dev-freifunk.net