[OpenWrt-Devel,v2,1/2] base-files: always store label MAC address in uci system config
diff mbox series

Message ID 20191104104348.63115-1-freifunk@adrianschmutzler.de
State Superseded
Delegated to: Adrian Schmutzler
Headers show
Series
  • [OpenWrt-Devel,v2,1/2] base-files: always store label MAC address in uci system config
Related show

Commit Message

Adrian Schmutzler Nov. 4, 2019, 10:43 a.m. UTC
If set, label MAC address is available from one of two sources,
device tree or board.json. So far, the function get_mac_label
was meant for retrieving the address, while an option in uci
system config was specified only for case 2 (board.json).

Since this has been perceived as counter-intuitive, this patch
changes front-end access to the label MAC address:
During first-boot, the label MAC address will be written to uci
system config file for both cases, no matter whether is was
specified in DT or in board.json (via 02_network). A user of
the label MAC address will then read the value from
system.@system[0].label_macaddr, which is easier and more intuitive
than using a function and still have an uci value set.

Since this is only changing the access to the label MAC address, it
won't interfere with the addresses stored in the code base so far.

Signed-off-by: Adrian Schmutzler <freifunk@adrianschmutzler.de>

---

This has been tested with DT address, 02_network address, and
without address.

---
 package/base-files/files/bin/config_generate   | 18 +++++++++++++-----
 .../base-files/files/lib/functions/system.sh   |  6 +++---
 2 files changed, 16 insertions(+), 8 deletions(-)

Comments

Kristian Evensen Nov. 4, 2019, 1:57 p.m. UTC | #1
Hi Adrian,

On Mon, Nov 4, 2019 at 11:44 AM Adrian Schmutzler
<freifunk@adrianschmutzler.de> wrote:
>
> If set, label MAC address is available from one of two sources,
> device tree or board.json. So far, the function get_mac_label
> was meant for retrieving the address, while an option in uci
> system config was specified only for case 2 (board.json).
>
> Since this has been perceived as counter-intuitive, this patch
> changes front-end access to the label MAC address:
> During first-boot, the label MAC address will be written to uci
> system config file for both cases, no matter whether is was
> specified in DT or in board.json (via 02_network). A user of
> the label MAC address will then read the value from
> system.@system[0].label_macaddr, which is easier and more intuitive
> than using a function and still have an uci value set.
>
> Since this is only changing the access to the label MAC address, it
> won't interfere with the addresses stored in the code base so far.
>
> Signed-off-by: Adrian Schmutzler <freifunk@adrianschmutzler.de>

I am not an authority on anything, but I don't think a "runtime" value
like the label mac should be stored in a persistent configuration
file. For example, if someone makes a mistake with the label MAC, you
would need a uci-default script for fixing up the config. You also
have the issue of devices that have already been installed, without a
uci-defaults script they will not have a label mac in UCI.

Instead, I would keep board.json as the source for label MAC and have
get_mac_label parse the JSON-file. I guess you might also have to
patch the generation of board.json to include the device tree. At
least I think that board.json is as easy to work with as uci from
scripts, Luci, etc.

BR,
Kristian
Adrian Schmutzler Nov. 4, 2019, 3:36 p.m. UTC | #2
Hi,

> -----Original Message-----
> From: Kristian Evensen [mailto:kristian.evensen@gmail.com]
> Sent: Montag, 4. November 2019 14:57
> To: Adrian Schmutzler <freifunk@adrianschmutzler.de>
> Cc: OpenWrt Development List <openwrt-devel@lists.openwrt.org>
> Subject: Re: [OpenWrt-Devel] [PATCH v2 1/2] base-files: always store label MAC
> address in uci system config
> 
> Hi Adrian,
> 
> On Mon, Nov 4, 2019 at 11:44 AM Adrian Schmutzler
> <freifunk@adrianschmutzler.de> wrote:
> >
> > If set, label MAC address is available from one of two sources,
> > device tree or board.json. So far, the function get_mac_label
> > was meant for retrieving the address, while an option in uci
> > system config was specified only for case 2 (board.json).
> >
> > Since this has been perceived as counter-intuitive, this patch
> > changes front-end access to the label MAC address:
> > During first-boot, the label MAC address will be written to uci
> > system config file for both cases, no matter whether is was
> > specified in DT or in board.json (via 02_network). A user of
> > the label MAC address will then read the value from
> > system.@system[0].label_macaddr, which is easier and more intuitive
> > than using a function and still have an uci value set.
> >
> > Since this is only changing the access to the label MAC address, it
> > won't interfere with the addresses stored in the code base so far.
> >
> > Signed-off-by: Adrian Schmutzler <freifunk@adrianschmutzler.de>
> 
> I am not an authority on anything, but I don't think a "runtime" value
> like the label mac should be stored in a persistent configuration
> file. For example, if someone makes a mistake with the label MAC, you
> would need a uci-default script for fixing up the config. You also
> have the issue of devices that have already been installed, without a
> uci-defaults script they will not have a label mac in UCI.
> 
> Instead, I would keep board.json as the source for label MAC and have
> get_mac_label parse the JSON-file. I guess you might also have to
> patch the generation of board.json to include the device tree. At
> least I think that board.json is as easy to work with as uci from
> scripts, Luci, etc.

I also thought about that.

However, I'm not aware of a case where board.json is used for anything else than setting up config files, like in a script with user-interaction etc.
So, I hesitate to start with that (as it might also be counter-intuitive for some people).

Concerning address fixes: There might also be users that don't want the address to change if it has been wrong for some time and they have implemented that particular address into their system. This might be more relevant for downstream projects than for use cases directly in OpenWrt, but this is actually a somewhat separate aspect. Despite, having the uci option will allow users to change the address (if it's wrong or for other reasons) or provide it when upstream support is not there (yet). Just having a function reading from DT/board.json would be much less user-friendly there.
However, I do admit that my arguments here are practical, while you are right that strictly the label MAC address is a device parameter that should not be altered by the user.

> You also
> have the issue of devices that have already been installed, without a
> uci-defaults script they will not have a label mac in UCI.

That's a valid argument I haven't thought about before, and it eliminates/weakens most of my arguments from the previous paragraph.

So, if I want to bring the label MAC address to users updating to 20.xx/master, I'd either have to use board.json in get_mac_label, or add label_macaddr for those not having it even though /etc/config/system already exists. One could still think about providing a label_macaddr option in uci only for _overwriting_ the provided value.

I see, I will have to think about that (again) for some time ...

Thanks for the input

Adrian


> 
> BR,
> Kristian
Kristian Evensen Nov. 4, 2019, 4:10 p.m. UTC | #3
Hi,

On Mon, Nov 4, 2019 at 4:36 PM Adrian Schmutzler
<mail@adrianschmutzler.de> wrote:
> However, I'm not aware of a case where board.json is used for anything else than setting up config files, like in a script with user-interaction etc.

While I agree that those are the most common use-cases for board.json,
board.json is used by for example luci to determine which parts of
network configuration to display.

> So, if I want to bring the label MAC address to users updating to 20.xx/master, I'd either have to use board.json in get_mac_label, or add label_macaddr for those not having it even though /etc/config/system already exists. One could still think about providing a label_macaddr option in uci only for _overwriting_ the provided value.

I am not overly familiar with them, but I think this problem can be
resolved by a uci-defaults script. As far as I have understood, those
scripts are run once and then remove. You could create a script that
checks board.json/device tree (basically a combination of the changes
in your patch) and update system-config accordingly.

BR,
Kristian

Patch
diff mbox series

diff --git a/package/base-files/files/bin/config_generate b/package/base-files/files/bin/config_generate
index 0b26afe57f..bb6117e6dc 100755
--- a/package/base-files/files/bin/config_generate
+++ b/package/base-files/files/bin/config_generate
@@ -3,6 +3,7 @@ 
 CFG=/etc/board.json
 
 . /usr/share/libubox/jshn.sh
+. /lib/functions/system.sh
 
 [ -s $CFG ] || /bin/board_detect || exit 1
 [ -s /etc/config/network -a -s /etc/config/system ] && exit 0
@@ -253,6 +254,18 @@  generate_static_system() {
 		add_list system.ntp.server='3.openwrt.pool.ntp.org'
 	EOF
 
+	local label_macaddr=$(get_mac_label_dt)
+
+	if json_is_a system object; then
+		json_select system
+			[ -n "$label_macaddr" ] || json_get_var label_macaddr label_macaddr
+		json_select ..
+	fi
+
+	if [ -n "$label_macaddr" ]; then
+		uci -q set "system.@system[-1].label_macaddr=$label_macaddr"
+	fi
+
 	if json_is_a system object; then
 		json_select system
 			local hostname
@@ -260,11 +273,6 @@  generate_static_system() {
 				uci -q set "system.@system[-1].hostname=$hostname"
 			fi
 
-			local label_macaddr
-			if json_get_var label_macaddr label_macaddr; then
-				uci -q set "system.@system[-1].label_macaddr=$label_macaddr"
-			fi
-
 			if json_is_a ntpserver array; then
 				local keys key
 				json_get_keys keys ntpserver
diff --git a/package/base-files/files/lib/functions/system.sh b/package/base-files/files/lib/functions/system.sh
index cb0508fe9c..5b4ced836c 100644
--- a/package/base-files/files/lib/functions/system.sh
+++ b/package/base-files/files/lib/functions/system.sh
@@ -12,14 +12,14 @@  get_mac_binary() {
 	hexdump -v -n 6 -s $offset -e '5/1 "%02x:" 1/1 "%02x"' $path 2>/dev/null
 }
 
-get_mac_label() {
+get_mac_label_dt() {
 	local basepath="/proc/device-tree"
 	local macdevice="$(cat "$basepath/aliases/label-mac-device" 2>/dev/null)"
 	local macaddr
 
-	[ -n "$macdevice" ] && macaddr=$(get_mac_binary "$basepath/$macdevice/mac-address" 0 2>/dev/null)
+	[ -n "$macdevice" ] || return
+	macaddr=$(get_mac_binary "$basepath/$macdevice/mac-address" 0 2>/dev/null)
 	[ -n "$macaddr" ] || macaddr=$(get_mac_binary "$basepath/$macdevice/local-mac-address" 0 2>/dev/null)
-	[ -n "$macaddr" ] || macaddr=$(uci -q get system.@system[0].label_macaddr)
 	echo $macaddr
 }