diff mbox series

[OpenWrt-Devel] bcm53xx: tidy up board.d/02_network even further

Message ID 20200403192922.44334-1-freifunk@adrianschmutzler.de
State Rejected
Headers show
Series [OpenWrt-Devel] bcm53xx: tidy up board.d/02_network even further | expand

Commit Message

Adrian Schmutzler April 3, 2020, 7:29 p.m. UTC 
This arranges the code in 02_network to resemble the structure using
lan_macaddr and wan_macaddr variables like in other targets as close
as possible (without becoming non-cosmetic).

Signed-off-by: Adrian Schmutzler <freifunk@adrianschmutzler.de>
Cc: Rafał Miłecki <rafal@milecki.pl>
---
 .../bcm53xx/base-files/etc/board.d/02_network | 27 ++++++++-----------
 1 file changed, 11 insertions(+), 16 deletions(-)

Comments

Rafał Miłecki April 3, 2020, 9:15 p.m. UTC | #1 
Hey,

On 03.04.2020 21:29, Adrian Schmutzler wrote:
> This arranges the code in 02_network to resemble the structure using
> lan_macaddr and wan_macaddr variables like in other targets as close
> as possible (without becoming non-cosmetic).

I'm sorry but I'm starring at it and I see hardly any improvement.

Maybe this makes code more similar to one from target <foo> but I don't
see why it's better to have both ucidef_set_interface_macaddr() calls
at the end of function. I'd say it's a matter of personal preference.

The old variable name "etXmacaddr" matches closely NVRAM variable names
(et0macaddr, et1macaddr and et2macaddr) so I don't see why changing it
to "etX_macaddr" would make code necessarily better. If we start doing
such changes we'll keep changing code like that over and over.
diff mbox series

Patch

diff --git a/target/linux/bcm53xx/base-files/etc/board.d/02_network b/target/linux/bcm53xx/base-files/etc/board.d/02_network
index f86f12407f..8492ec3277 100755
--- a/target/linux/bcm53xx/base-files/etc/board.d/02_network
+++ b/target/linux/bcm53xx/base-files/etc/board.d/02_network
@@ -71,37 +71,32 @@  bcm53xx_setup_interfaces()
 bcm53xx_setup_macs()
 {
 	local board="$1"
+	local lan_macaddr
+	local wan_macaddr="$(nvram get wan_hwaddr)"
+	local etX_macaddr
 
-	case "$board" in
-	dlink,dir-885l | \
-	netgear,r7900 | \
-	netgear,r8000 | \
-	netgear,r8500)
-		# As vendor doesn't use eth0 its MAC may be missing. Use one from eth2.
-		et2macaddr="$(nvram get et2macaddr)"
-		[ -n "$et2macaddr" ] && ucidef_set_interface_macaddr "lan" "$et2macaddr"
-		;;
-	esac
-
-	wan_macaddr="$(nvram get wan_hwaddr)"
 	case "$board" in
 	asus,rt-ac87u)
-		etXmacaddr=$(nvram get et1macaddr)
+		etX_macaddr=$(nvram get et1macaddr)
 		;;
 	dlink,dir-885l | \
 	netgear,r7900 | \
 	netgear,r8000 | \
 	netgear,r8500)
-		etXmacaddr=$(nvram get et2macaddr)
+		# As vendor doesn't use eth0 its MAC may be missing. Use one from eth2.
+		lan_macaddr=$(nvram get et2macaddr)
+		etX_macaddr=$(nvram get et2macaddr)
 		;;
 	*)
-		etXmacaddr=$(nvram get et0macaddr)
+		etX_macaddr=$(nvram get et0macaddr)
 		;;
 	esac
 
 	# If WAN MAC isn't explicitly set, calculate it using base MAC as reference.
-	[ -z "$wan_macaddr" -a -n "$etXmacaddr" ] && wan_macaddr=$(macaddr_add "$etXmacaddr" 1)
+	# However, best things would be to set the proper wan_macaddr directly above
+	[ -z "$wan_macaddr" -a -n "$etX_macaddr" ] && wan_macaddr=$(macaddr_add "$etX_macaddr" 1)
 
+	[ -n "$lan_macaddr" ] && ucidef_set_interface_macaddr "lan" "$lan_macaddr"
 	[ -n "$wan_macaddr" ] && ucidef_set_interface_macaddr "wan" "$wan_macaddr"
 }