[PATCH v2 2/2] configurenetwork: Rearrangement/redesign of central commands
Tim Niemeyer
tim at tn-x.org
Sa Jan 20 16:21:11 CET 2018
Hi Adrian
Am Samstag, den 20.01.2018, 16:12 +0100 schrieb mail at adrianschmutzler.de:
> Hallo Tim,
>
> siehe unten.
>
> > -----Original Message-----
> > > > From: Tim Niemeyer [mailto:tim at tn-x.org]
> > Sent: Samstag, 20. Januar 2018 14:27
> > > > To: Adrian Schmutzler <freifunk at adrianschmutzler.de>; franken-
> > dev at freifunk.net
> > Subject: Re: [PATCH v2 2/2] configurenetwork: Rearrangement/redesign of
> > central commands
> >
> > Hi Adrian
> >
> > Ich bin sehr begeistert.
> >
> > Könntest du die beiden Sachen unten noch einarbeiten?
> >
> > Am Mittwoch, den 03.01.2018, 00:34 +0100 schrieb Adrian Schmutzler:
> > >
[..]
> > > +
> > > + echo "# Allow IPv6 RAs on WAN Port" >> /etc/sysctl.conf
> >
> > Diese Zeile muss mit zu der Stelle, wo du $wanon auswertest.
>
> Ja. Dann sollten wir aber "Allow/disallow" oder einfach "Set" nehmen,
> weil wir ja beide Fälle bearbeiten. Man könnte auch ein Switch bauen
> und die Meldung entsprechend für Ja/Nein setzen, aber das ist mir
> eigtl. zu sehr Overkill.
Von mir aus kann die Zeile auch ganz weg.
> >
> > > +
> > > > > > > > + wanon="1"
> > > > + if [ "$WANDEV" = "$SWITCHDEV" ] || [ -n "$WAN_PORTS" ];
> >
> > then
> > > > > > > > + WANDEV="${SWITCHDEV}.2"
> > > > > > > > + uci set "network.${SWITCHDEV}_2=switch_vlan"
> > > > + uci set
> >
> > "network.${SWITCHDEV}_2.device=$SWITCHHW"
> > > > > > > > + uci set "network.${SWITCHDEV}_2.vlan=2"
> > > > + uci set
> >
> > "network.${SWITCHDEV}_2.ports=$WAN_PORTS"
> > > > + fi
> > >
> > > +
> > > > > > > > + ETHMESHDEV="$SWITCHDEV.3"
> > > > > > > > + uci set "network.${SWITCHDEV}_3=switch_vlan"
> > > > > > > > + uci set "network.${SWITCHDEV}_3.device=$SWITCHHW"
> > > > > > > > + uci set "network.${SWITCHDEV}_3.vlan=3"
> > > > + uci set "network.${SWITCHDEV}_3.ports=$BATMAN_PORTS"
> > >
> > > +
> > > > > > > > + uci set network.mesh.ifname="$SWITCHDEV.1 bat0"
> > > > > > > > + uci set network.wan.ifname="$WANDEV"
> > > > + uci set network.ethmesh.ifname="$ETHMESHDEV"
> > >
> > > +
> > > > > > > > + uci -q commit network
> > > > + fi
> > >
> > > fi
> > > -
> > > -if [ "$ONE_PORT" = "YES" ] && ( ! uci -q get
> > > network.$SWITCHDEV.ifname || [ "$FORCEPARSE" = '1' ] ) ; then
> > > - uci set network.$SWITCHDEV=interface
> > > - uci set network.$SWITCHDEV.ifname=$SWITCHDEV
> > > - if [ "$ETHMODE" = "WAN" ]; then
> > > - echo "net.ipv6.conf.$WANDEV.accept_ra_defrtr = 1" >>
> > > /etc/sysctl.conf
> > > - echo "net.ipv6.conf.$WANDEV.accept_ra_pinfo = 1" >>
> > > /etc/sysctl.conf
> > > - echo "net.ipv6.conf.$WANDEV.autoconf = 1" >> /etc/sysctl.conf
> > > - echo "net.ipv6.conf.$WANDEV.accept_ra_rtr_pref = 1" >>
> > > /etc/sysctl.conf
> > > - uci set network.mesh.ifname="bat0"
> > > - uci set network.wan.ifname="$WANDEV"
> > > - uci del uci set network.ethmesh.ifname
> > > - uci del network.eth0.macaddr
> > > - elif [ "$ETHMODE" = "CLIENT" ] ; then
> > > - echo "net.ipv6.conf.$WANDEV.accept_ra_defrtr = 0" >>
> > > /etc/sysctl.conf
> > > - echo "net.ipv6.conf.$WANDEV.accept_ra_pinfo = 0" >>
> > > /etc/sysctl.conf
> > > - echo "net.ipv6.conf.$WANDEV.autoconf = 0" >> /etc/sysctl.conf
> > > - echo "net.ipv6.conf.$WANDEV.accept_ra_rtr_pref = 0" >>
> > > /etc/sysctl.conf
> > > - uci set network.mesh.ifname="bat0 $SWITCHDEV"
> > > - uci set network.wan.ifname="eth1" #eth1 because it is default
> > > in config file
> > > - uci del network.ethmesh.ifname
> > > - uci del network.eth0.macaddr
> > > - elif [ "$ETHMODE" = "BATMAN" ] ; then
> > > - echo "net.ipv6.conf.$WANDEV.accept_ra_defrtr = 0" >>
> > > /etc/sysctl.conf
> > > - echo "net.ipv6.conf.$WANDEV.accept_ra_pinfo = 0" >>
> > > /etc/sysctl.conf
> > > - echo "net.ipv6.conf.$WANDEV.autoconf = 0" >> /etc/sysctl.conf
> > > - echo "net.ipv6.conf.$WANDEV.accept_ra_rtr_pref = 0" >>
> > > /etc/sysctl.conf
> > > - uci set network.mesh.ifname="bat0"
> > > - uci set network.wan.ifname="eth1" #eth1 because it is default
> > > in config file
> > > - uci set network.ethmesh.ifname="$SWITCHDEV"
> > > - ETH0MAC="w2ap"
> > > - fi
> > > - uci commit network
> > > +if [ -n "$wanon" ]; then
> > > > > > > > + echo "net.ipv6.conf.$WANDEV.accept_ra_defrtr = $wanon" >>
> > > > +/etc/sysctl.conf
> > >
> > > > > > + echo "net.ipv6.conf.$WANDEV.accept_ra_pinfo = $wanon"
> > > +>>/etc/sysctl.conf
> > > > + echo "net.ipv6.conf.$WANDEV.autoconf = $wanon" >>
> >
> > /etc/sysctl.conf
> > > > > > > > + echo "net.ipv6.conf.$WANDEV.accept_ra_rtr_pref = $wanon" >>
> > > > +/etc/sysctl.conf
> > >
> > > fi
> >
> > Hier haben wir ein Problem. Das war zwar vorher auch schon da, sollte mMn
> > hier im Patch gefixt werden.
> > Das Problem ist, dass die Konfiguration immer weiter hinten an die
> > sysctl.conf angefügt wird.
> > Ich schlage daher vor diese Sachen nicht in die /etc/sysctl.conf zu schreiben,
> > sondern eine /etc/sysctl.d/configurenetwork.conf zu nehmen.
> > Diese kann dann nämlich jeweils komplett überschrieben werden.
>
> Das Problem ist mir bewusst und das permanente Anhängen ist Mist.
>
> Die Lösung mit der configurenetwork.conf müsstest du mir ein bisschen
> genauer erklären, das lese ich zum ersten Mal.
Es gibt im rootfs das /etc/sysctl.d/ Verzeichnis. Ich gehe davon aus,
dass man dort einzelne Files rein tun kann, die dann eben die
sysctl.conf ergänzen. Getestet habe ich das nicht. Wenn das nicht geht
muss zur Not eben ein sed helfen.
> Ich würde hier auch für einen separaten Patch votieren, denn:
> 1. Auch wenn sich viel ändert, ist der gesamte Patch im Moment im
> Wesentlichen kosmetisch. Es werden zwar Commands hin- und
> hergeschoben und anders aggregiert, aber die Funktion ändert sich
> größtenteils nicht. Da wäre, so wie ich die Änderung verstehe, dann
> schon (eher) der Fall. Das würde ich gerne trennen.
Es ist definitiv mehr als Kosmetik. Reine Kosmetik verändert den Code
selbst nicht. Hier schaltest du neue if's usw..
Von daher ganz klar: bitte hier ergänzen. Du packst das Zeug ja hier im
Patch an und es wurde im Review eine Unstimmigkeit gefunden. Dafür
machen wir das Review ja.
> 2. Ich muss mich hier mit etwas neuem auseinandersetzen, das wird die
> ganze Sache in die Länge ziehen.
Gern, kein Problem.
> 3. Der jetzige Patch ist sehr gut getestet. Dies rechtfertigt für
> mich auch tendentiell einen eigenen Commit.
Ich kann hier nicht erkennen, dass irgendjemand anderes außer dir das
getestet hat.
Bitte einfach reparieren und gut ist. Wenn wir jetzt bei jedem Finding
sagen, dass wir das in einem separaten Patch machen, dann können wir
uns das Review sparen, die Patches direkt applien und dann muss man den
Mist halt jeweils danach mit neuen Patches reparieren. Das ist eine
Entscheidung, die man treffen kann, aber im Moment wurde die so nicht
getroffen.
Tim
> Im Gegensatz zum Teil 1/2 bin hier allerdings flexibler, für mich ist
> lediglich die Tendenz anhand der Argumente, dass ein separater Patch
> besser wäre.
>
> Grüße
>
> Adrian
>
> >
> > Tim
> >
> > >
> > > /etc/init.d/network restart
> > >
> > > -if [ -n "$ETHMESHMAC" ]; then
> > > - fixMac "$ETHMESHMAC" "${SWITCHDEV}.3" "ethmesh"
> > > +if [ -n "$ETHMESHMAC" ] && [ -n "$ETHMESHDEV" ]; then
> > > + fixMac "$ETHMESHMAC" "$ETHMESHDEV" "ethmesh"
> > > fi
> > >
> > > if [ -n "$ROUTERMAC" ]; then
> > > fixMac "$ROUTERMAC" "br-mesh" "mesh"
> > > fi
> > >
> > > -if [[ -n "$ETH0MAC" ]]; then
> > > - echo "Fixing MAC on eth0"
> > > - sleep 10
> > > - NEW_MACADDR=$(cat "/sys/class/net/${ETH0MAC}/address")
> > > - uci set network.eth0.macaddr=$NEW_MACADDR
> > > - uci commit network
> > > - ifconfig eth0 down
> > > - ifconfig eth0 hw ether $NEW_MACADDR
> > > - ifconfig eth0 up
> > > - /etc/init.d/network restart
> > > -fi
> > > -
> > > if uci get network.mesh.ip6addr
> > > then
> > > echo "IPv6 for mesh is set already"
> > > --
> > > 2.7.4
> > >
>
>
-------------- nächster Teil --------------
Ein Dateianhang mit Binärdaten wurde abgetrennt...
Dateiname : signature.asc
Dateityp : application/pgp-signature
Dateigröße : 488 bytes
Beschreibung: This is a digitally signed message part
URL : <http://lists.freifunk.net/pipermail/franken-dev-freifunk.net/attachments/20180120/ce803070/attachment.sig>
Mehr Informationen über die Mailingliste franken-dev