[PATCH v2 2/2] configurenetwork: Rearrangement/redesign of central commands

mail at adrianschmutzler.de mail at adrianschmutzler.de
Sa Jan 20 16:12:08 CET 2018


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:
> > This patch rearranges the commands in configurenetwork. The main goal
> > is to avoid code duplications and make reading easier. Some "bugs"
> > were addressed on the way:
> > - Previously, ONEPORT devices went through both major blocks,
> >   resulting e.g. in useless eth0.X interfaces being set up (and
> >   perhaps lots of other strange things). Now, the routines for
> >   ONEPORT and normal devices are separated.
> > - Updating the /etc/sysctl.conf has been combined to a single
> >   block based on variables set earlier
> > - WANDEV is reset to WANDEV.2 were applicable to reduce number of
> >   ifs
> > - "uci del uci set" has been changed to "uci del"
> > - Spaces have been changed to Tabs were changes were done
> > - Arguments have been quoted in various places
> > - Double brackets have been replaced by single brackets
> > - ETHMODE if has been changed to use BATMAN as default (elseif
> >   changed to else)
> > - Use ETHMESHMAC for ONEPORT, introduce ETHMESHDEV
> > - Shift bit of ROUTERMAC for ONEPORT's ETHMESHMAC
> >
> > > Signed-off-by: Adrian Schmutzler <freifunk at adrianschmutzler.de>
> >
> > > Tested-by: Adrian Schmutzler <freifunk at adrianschmutzler.de>
> >
> > ---
> >
> > Tested successfully on WR841N v12 and Unifi AC Mesh
> >
> > Changes in v2:
> > - Merged with ETH0MAC replacement patch
> > ---
> >  .../fff-network/files/usr/sbin/configurenetwork    | 167
> > +++++++++------------
> >  1 file changed, 70 insertions(+), 97 deletions(-)
> >
> > diff --git
> > a/src/packages/fff/fff-network/files/usr/sbin/configurenetwork
> > b/src/packages/fff/fff-network/files/usr/sbin/configurenetwork
> > index 281f4d4..17e78db 100755
> > --- a/src/packages/fff/fff-network/files/usr/sbin/configurenetwork
> > +++ b/src/packages/fff/fff-network/files/usr/sbin/configurenetwork
> > @@ -71,114 +71,87 @@ if [ -n "$LAN1PORT" ] ; then
> >      setupPorts "$LAN1PORT" "$LAN1MODE"
> >  fi
> >
> > -if ! uci -q get network.$SWITCHDEV > /dev/null || [ "$FORCEPARSE" =
> > '1' ] ; then
> > -
> > -    SWITCHHW=$(swconfig list | awk '{ print $4 }')
> > -
> > -    uci set network.$SWITCHDEV=switch
> > -    uci set network.$SWITCHDEV.name=$SWITCHHW
> > -    uci set network.$SWITCHDEV.enable=1
> > -    uci set network.$SWITCHDEV.reset=1
> > -    uci set network.$SWITCHDEV.enable_vlan=1
> > -
> > -    uci set network.${SWITCHDEV}_1=switch_vlan
> > -    uci set network.${SWITCHDEV}_1.device=$SWITCHHW
> > -    uci set network.${SWITCHDEV}_1.vlan=1
> > -    uci set network.${SWITCHDEV}_1.ports="$CLIENT_PORTS"
> > -
> > -    echo "# Allow IPv6 RAs on WAN Port" >> /etc/sysctl.conf
> > -
> > -    if [[ "$WANDEV" = "$SWITCHDEV" ]] || ! [[ -z "$WAN_PORTS" ]];
> > then
> > -        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"
> > -
> > -        echo "net.ipv6.conf.$WANDEV.2.accept_ra_defrtr = 1" >>
> > /etc/sysctl.conf
> > -        echo "net.ipv6.conf.$WANDEV.2.accept_ra_pinfo = 1" >>
> > /etc/sysctl.conf
> > -        echo "net.ipv6.conf.$WANDEV.2.autoconf = 1" >>
> > /etc/sysctl.conf
> > -        echo "net.ipv6.conf.$WANDEV.2.accept_ra_rtr_pref = 1" >>
> > /etc/sysctl.conf
> > -    else
> > -        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
> > -    fi
> > -
> > -    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.ethmesh.ifname="$SWITCHDEV.3"
> > -
> > -    if [[ "$WANDEV" = "$SWITCHDEV" ]]; then
> > -        uci set network.wan.ifname=$WANDEV.2
> > -    else
> > -        uci set network.wan.ifname=$WANDEV
> > -    fi
> > -
> > -    uci commit network
> > +if [ "$ONE_PORT" = "YES" ] ; then
> > > +	if ! uci -q get "network.$SWITCHDEV.ifname" || [ "$FORCEPARSE" =
> '1' ] ; then
> > > +		uci set "network.$SWITCHDEV=interface"
> > > +		uci set "network.$SWITCHDEV.ifname=$SWITCHDEV"
> > > +		wanon="0"
> > > +		if [ "$ETHMODE" = "WAN" ]; then
> > > +			wanon="1"
> > > +			uci set network.mesh.ifname="bat0"
> > > +			uci set network.wan.ifname="$WANDEV"
> > > +			uci del network.ethmesh.ifname
> > > +			uci del network.eth0.macaddr
> > > +		elif [ "$ETHMODE" = "CLIENT" ] ; then
> > > +			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
> > > +		else # default=BATMAN
> > > +			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"
> > > +			ETHMESHMAC="$(macFlipLocalBit "$ROUTERMAC")"
> > > +			ETHMESHDEV="$SWITCHDEV"
> > > +		fi
> > > +		uci -q commit network
> > > +	fi
> > +else
> > > +	if ! uci -q get "network.$SWITCHDEV" > /dev/null || [ "$FORCEPARSE"
> = '1' ] ; then
> > > +		SWITCHHW="$(swconfig list | awk '{ print $4 }')"
> > +
> > > +		uci set "network.$SWITCHDEV=switch"
> > > +		uci set "network.$SWITCHDEV.name=$SWITCHHW"
> > > +		uci set "network.$SWITCHDEV.enable=1"
> > > +		uci set "network.$SWITCHDEV.reset=1"
> > > +		uci set "network.$SWITCHDEV.enable_vlan=1"
> > +
> > > +		uci set "network.${SWITCHDEV}_1=switch_vlan"
> > > +		uci set "network.${SWITCHDEV}_1.device=$SWITCHHW"
> > > +		uci set "network.${SWITCHDEV}_1.vlan=1"
> > > +		uci set "network.${SWITCHDEV}_1.ports=$CLIENT_PORTS"
> > +
> > +		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.

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

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.
2. Ich muss mich hier mit etwas neuem auseinandersetzen, das wird die ganze Sache in die Länge ziehen.
3. Der jetzige Patch ist sehr gut getestet. Dies rechtfertigt für mich auch tendentiell einen eigenen Commit.

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



Mehr Informationen über die Mailingliste franken-dev