| Message ID | 20190627174826.22653-1-fabian@blaese.de |
|---|---|
| State | Accepted |
| Headers | show |
diff --git a/src/packages/fff/fff-babeld/files/etc/gateway.d/40-babel b/src/packages/fff/fff-babeld/files/etc/gateway.d/40-babel index cc1cf5d..28a0d50 100644 --- a/src/packages/fff/fff-babeld/files/etc/gateway.d/40-babel +++ b/src/packages/fff/fff-babeld/files/etc/gateway.d/40-babel @@ -1,4 +1,5 @@ . /lib/functions.sh +. /lib/functions/fff/babel #load board specific properties BOARD="$(uci get board.model.name)" @@ -14,10 +15,9 @@ configure() { # remove interface uci -q del network.$name # remove iif-rules - uci -q del network.${name}_rule - uci -q del network.${name}_rule6 + babel_delete_iifrules "$name" # remove babel interface - uci -q del babeld.$name + babel_delete_interface "$name" fi fi } @@ -67,37 +67,16 @@ configure() { uci set network.$name.ifname=$iface # add iif-rules - uci set network.${name}_rule=rule - uci set network.${name}_rule.in="$name" - uci set network.${name}_rule.lookup='10' - uci set network.${name}_rule.priority='31' - - uci set network.${name}_rule6=rule6 - uci set network.${name}_rule6.in="$name" - uci set network.${name}_rule6.lookup='10' - uci set network.${name}_rule6.priority='31' + babel_add_iifrules "$name" || { echo "Could not add iif-rules for babelpeer $name"; exit 1; } # peer_ip - if peer_ip=$(uci -q get gateway.@gateway[0].peer_ip); then - uci set network.$name.ipaddr="$peer_ip" - elif ipaddr=$(uci -q get gateway.@client[0].ipaddr); then - # use ipaddr (without subnet) if no peer_ip set - uci set network.$name.ipaddr=$(echo $ipaddr | cut -d / -f1) - else - echo "FATAL: Neither peer_ip nor ipaddr set! No peering ipv4 set!" - exit 1 - fi - - # peer_ip6 - if peer_ip6=$(uci -q get gateway.@gateway[0].peer_ip6); then - uci set network.$name.ip6addr="$peer_ip6" - fi + uci -q delete "network.$name.ipaddr" + uci -q delete "network.$name.ip6addr" + babel_add_peeraddr "network.$name.ipaddr" + babel_add_peer6addr "network.$name.ip6addr" # add babel interface - uci set babeld.$name=interface - uci set babeld.$name.ifname=$iface - uci set babeld.$name.type=$type - uci set babeld.$name.rxcost=$rxcost + babel_add_interface "$name" "$iface" "$type" "$rxcost" || { echo "Could not add babeld interface for babelpeer $name"; exit 1; } } config_load gateway diff --git a/src/packages/fff/fff-babeld/files/lib/functions/fff/babel b/src/packages/fff/fff-babeld/files/lib/functions/fff/babel new file mode 100644 index 0000000..0d19cef --- /dev/null +++ b/src/packages/fff/fff-babeld/files/lib/functions/fff/babel @@ -0,0 +1,88 @@ +babel_add_iifrules() { + [ "$#" -ne "1" ] && return 1 + + local name="$1" + local table='10' + local prio='31' + + uci set network.${name}_rule=rule + uci set network.${name}_rule.in="$name" + uci set network.${name}_rule.lookup="$table" + uci set network.${name}_rule.priority="$prio" + + uci set network.${name}_rule6=rule6 + uci set network.${name}_rule6.in="$name" + uci set network.${name}_rule6.lookup="$table" + uci set network.${name}_rule6.priority="$prio" + + return 0 +} + +babel_delete_iifrules() { + [ "$#" -ne "1" ] && return 1 + + local name="$1" + + uci -q del network.${name}_rule + uci -q del network.${name}_rule6 + + return 0 +} + +babel_add_peeraddr() { + [ "$#" -ne "1" ] && return 1 + + local option="$1" + + if peer_ip=$(uci -q get gateway.@gateway[0].peer_ip); then + uci add_list "$option"="$peer_ip" + elif ipaddr=$(uci -q get gateway.@client[0].ipaddr); then + # use ipaddr (without subnet) if no peer_ip set + uci add_list "$option"=$(echo $ipaddr | cut -d / -f1) + else + echo "FATAL: Neither peer_ip nor ipaddr set! No peering ipv4 set!" + return 1 + fi + + return 0 +} + +babel_add_peer6addr() { + [ "$#" -ne "1" ] && return 1 + + local option="$1" + + if peer_ip6=$(uci -q get gateway.@gateway[0].peer_ip6); then + uci add_list "$option"="$peer_ip6" + else + return 1 + fi + + return 0 +} + +babel_add_interface() { + [ "$#" -ne "4" ] && return 1 + + local name="$1" + local interface="$2" + local type="$3" + local rxcost="$4" + + uci set babeld.$name=interface + uci set babeld.$name.ifname="$interface" + uci set babeld.$name.type="$type" + uci set babeld.$name.rxcost="$rxcost" + + return 0 +} + +babel_delete_interface() { + [ "$#" -ne "1" ] && return 1 + + local name="$1" + + uci -q del babeld.$name + + return 0 +}
Ich hatte noch keine Zeit, mir die beiden Patches anzusehen. Lass die vll. noch ein bisschen liegen, wenn's geht. Wenn nicht, isses aber auch nicht schlimm. Grüße Adrian > -----Original Message----- > From: franken-dev [mailto:franken-dev-bounces@freifunk.net] On Behalf Of > Fabian Bläse > Sent: Donnerstag, 27. Juni 2019 19:48 > To: franken-dev@freifunk.net > Subject: [PATCH v2 1/2] fff-babeld: Move common babeld procedures into > functions > > Various things have to be done for every interface on > which babeld shall run. > > Those procedures are moved into functions to reduce duplicate code. > > Signed-off-by: Fabian Bläse <fabian@blaese.de> > --- > Changes in v2: > - Remove exit from function calls which are allowed to fail > - Add explicit return value to functions > - Print error message when exiting due to a non-zero return value > --- > .../fff-babeld/files/etc/gateway.d/40-babel | 39 ++------ > .../fff-babeld/files/lib/functions/fff/babel | 88 +++++++++++++++++++ > 2 files changed, 97 insertions(+), 30 deletions(-) > create mode 100644 src/packages/fff/fff-babeld/files/lib/functions/fff/babel > > diff --git a/src/packages/fff/fff-babeld/files/etc/gateway.d/40-babel > b/src/packages/fff/fff-babeld/files/etc/gateway.d/40-babel > index cc1cf5d..28a0d50 100644 > --- a/src/packages/fff/fff-babeld/files/etc/gateway.d/40-babel > +++ b/src/packages/fff/fff-babeld/files/etc/gateway.d/40-babel > @@ -1,4 +1,5 @@ > . /lib/functions.sh > +. /lib/functions/fff/babel > > #load board specific properties > BOARD="$(uci get board.model.name)" > @@ -14,10 +15,9 @@ configure() { > # remove interface > uci -q del network.$name > # remove iif-rules > - uci -q del network.${name}_rule > - uci -q del network.${name}_rule6 > + babel_delete_iifrules "$name" > # remove babel interface > - uci -q del babeld.$name > + babel_delete_interface "$name" > fi > fi > } > @@ -67,37 +67,16 @@ configure() { > uci set network.$name.ifname=$iface > > # add iif-rules > - uci set network.${name}_rule=rule > - uci set network.${name}_rule.in="$name" > - uci set network.${name}_rule.lookup='10' > - uci set network.${name}_rule.priority='31' > - > - uci set network.${name}_rule6=rule6 > - uci set network.${name}_rule6.in="$name" > - uci set network.${name}_rule6.lookup='10' > - uci set network.${name}_rule6.priority='31' > + babel_add_iifrules "$name" || { echo "Could not add iif-rules for > babelpeer $name"; exit 1; } > > # peer_ip > - if peer_ip=$(uci -q get gateway.@gateway[0].peer_ip); then > - uci set network.$name.ipaddr="$peer_ip" > - elif ipaddr=$(uci -q get gateway.@client[0].ipaddr); then > - # use ipaddr (without subnet) if no peer_ip set > - uci set network.$name.ipaddr=$(echo $ipaddr | cut -d / - > f1) > - else > - echo "FATAL: Neither peer_ip nor ipaddr set! No peering > ipv4 set!" > - exit 1 > - fi > - > - # peer_ip6 > - if peer_ip6=$(uci -q get gateway.@gateway[0].peer_ip6); then > - uci set network.$name.ip6addr="$peer_ip6" > - fi > + uci -q delete "network.$name.ipaddr" > + uci -q delete "network.$name.ip6addr" > + babel_add_peeraddr "network.$name.ipaddr" > + babel_add_peer6addr "network.$name.ip6addr" > > # add babel interface > - uci set babeld.$name=interface > - uci set babeld.$name.ifname=$iface > - uci set babeld.$name.type=$type > - uci set babeld.$name.rxcost=$rxcost > + babel_add_interface "$name" "$iface" "$type" "$rxcost" || { > echo "Could not add babeld interface for babelpeer $name"; exit 1; } > } > > config_load gateway > diff --git a/src/packages/fff/fff-babeld/files/lib/functions/fff/babel > b/src/packages/fff/fff-babeld/files/lib/functions/fff/babel > new file mode 100644 > index 0000000..0d19cef > --- /dev/null > +++ b/src/packages/fff/fff-babeld/files/lib/functions/fff/babel > @@ -0,0 +1,88 @@ > +babel_add_iifrules() { > + [ "$#" -ne "1" ] && return 1 > + > + local name="$1" > + local table='10' > + local prio='31' > + > + uci set network.${name}_rule=rule > + uci set network.${name}_rule.in="$name" > + uci set network.${name}_rule.lookup="$table" > + uci set network.${name}_rule.priority="$prio" > + > + uci set network.${name}_rule6=rule6 > + uci set network.${name}_rule6.in="$name" > + uci set network.${name}_rule6.lookup="$table" > + uci set network.${name}_rule6.priority="$prio" > + > + return 0 > +} > + > +babel_delete_iifrules() { > + [ "$#" -ne "1" ] && return 1 > + > + local name="$1" > + > + uci -q del network.${name}_rule > + uci -q del network.${name}_rule6 > + > + return 0 > +} > + > +babel_add_peeraddr() { > + [ "$#" -ne "1" ] && return 1 > + > + local option="$1" > + > + if peer_ip=$(uci -q get gateway.@gateway[0].peer_ip); then > + uci add_list "$option"="$peer_ip" > + elif ipaddr=$(uci -q get gateway.@client[0].ipaddr); then > + # use ipaddr (without subnet) if no peer_ip set > + uci add_list "$option"=$(echo $ipaddr | cut -d / -f1) > + else > + echo "FATAL: Neither peer_ip nor ipaddr set! No peering ipv4 > set!" > + return 1 > + fi > + > + return 0 > +} > + > +babel_add_peer6addr() { > + [ "$#" -ne "1" ] && return 1 > + > + local option="$1" > + > + if peer_ip6=$(uci -q get gateway.@gateway[0].peer_ip6); then > + uci add_list "$option"="$peer_ip6" > + else > + return 1 > + fi > + > + return 0 > +} > + > +babel_add_interface() { > + [ "$#" -ne "4" ] && return 1 > + > + local name="$1" > + local interface="$2" > + local type="$3" > + local rxcost="$4" > + > + uci set babeld.$name=interface > + uci set babeld.$name.ifname="$interface" > + uci set babeld.$name.type="$type" > + uci set babeld.$name.rxcost="$rxcost" > + > + return 0 > +} > + > +babel_delete_interface() { > + [ "$#" -ne "1" ] && return 1 > + > + local name="$1" > + > + uci -q del babeld.$name > + > + return 0 > +} > -- > 2.22.0
Hallo Fabian, gefällt mir konzeptionell ganz gut. Für ein Review müsste ich das erstmal mit meinem Code zusammenpfriemeln, dafür habe ich gerade keine Zeit. Sehe aber kein Problem, das mit dem vorhandenen Review zu mergen. Ein Sache ist mir aber noch aufgefallen: > - > - # peer_ip6 > - if peer_ip6=$(uci -q get gateway.@gateway[0].peer_ip6); then > - uci set network.$name.ip6addr="$peer_ip6" > - fi > + uci -q delete "network.$name.ipaddr" > + uci -q delete "network.$name.ip6addr" > + babel_add_peeraddr "network.$name.ipaddr" > + babel_add_peer6addr "network.$name.ip6addr" Während der Rest deines Patches kosmetisch ist, werden hier die beiden "uci delete" hinzugefügt. Dies ist (wenn ich nichts übersehen habe) die einzige funktionale Änderung im Code. Diese sollte daher separat stattfinden. Grüße Adrian
Hallo Adrian, da die Funktion "babel_add_peeraddr" jetzt ein "uci add_list" statt einem uci set" macht, ist das uci delete vorher nötig, um das alte Verhalten beizubehalten. Man hätte das jetzt auf 2 Patches spalten können, indem zuerst von set auf add_list umgestellt wird. Mir kam das aber etwas unnötig vor. Ich weiß nicht, ob ich die nächsten Tage dazu komme, das zu applien. Wenn du das testen magst (wobei ein Review so wie ich das verstehe eigentlich keinen Test erfordert, dafür gibts ja tested-by..), können wir das gerne noch bissl liegen lassen. Gruß Fabian
Hi Fabian, erst mal ein Reviewed-by: Robert Langhammer <rlanghammer@web.de> Eine Frage ist mir noch gekommen. S. unten Am 27.06.19 um 19:48 schrieb Fabian Bläse: > Various things have to be done for every interface on > which babeld shall run. > > Those procedures are moved into functions to reduce duplicate code. > > Signed-off-by: Fabian Bläse <fabian@blaese.de> --- > diff --git a/src/packages/fff/fff-babeld/files/lib/functions/fff/babel b/src/packages/fff/fff-babeld/files/lib/functions/fff/babel > new file mode 100644 > index 0000000..0d19cef > --- /dev/null > +++ b/src/packages/fff/fff-babeld/files/lib/functions/fff/babel > @@ -0,0 +1,88 @@ > +babel_add_iifrules() { > + [ "$#" -ne "1" ] && return 1 > + > + local name="$1" > + local table='10' Wollen wir die 10 hier hard coden? Momentan ist das noch durcheinander und muss das Gleiche wie in rt_tables sein. Überall fff und nur in rt_tables die Zuordnung. Das wäre doch unabhängiger. Ich bin mir allerdings nicht sicher, ob der Alias überall funzt.
Hallo Robert, guter Einwand. Ich mache dafür einen Patch. (Oder möchtest du?) In diesem Patch würde ich die Änderungen gerne beim verschieben der bereits bestehenden Codeblöcke belassen. Aliase funktionieren, jedenfalls laut OpenWRT Dokumentation. Gruß Fabian On 04.07.19 16:29, robert wrote: >> diff --git a/src/packages/fff/fff-babeld/files/lib/functions/fff/babel b/src/packages/fff/fff-babeld/files/lib/functions/fff/babel >> new file mode 100644 >> index 0000000..0d19cef >> --- /dev/null >> +++ b/src/packages/fff/fff-babeld/files/lib/functions/fff/babel >> @@ -0,0 +1,88 @@ >> +babel_add_iifrules() { >> + [ "$#" -ne "1" ] && return 1 >> + >> + local name="$1" >> + local table='10' > Wollen wir die 10 hier hard coden? Momentan ist das noch durcheinander > und muss das Gleiche wie in rt_tables sein. Überall fff und nur in > rt_tables die Zuordnung. Das wäre doch unabhängiger. Ich bin mir > allerdings nicht sicher, ob der Alias überall funzt.
applied.
Various things have to be done for every interface on which babeld shall run. Those procedures are moved into functions to reduce duplicate code. Signed-off-by: Fabian Bläse <fabian@blaese.de> --- Changes in v2: - Remove exit from function calls which are allowed to fail - Add explicit return value to functions - Print error message when exiting due to a non-zero return value --- .../fff-babeld/files/etc/gateway.d/40-babel | 39 ++------ .../fff-babeld/files/lib/functions/fff/babel | 88 +++++++++++++++++++ 2 files changed, 97 insertions(+), 30 deletions(-) create mode 100644 src/packages/fff/fff-babeld/files/lib/functions/fff/babel