| 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 | expand |
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
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
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
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 }
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(-)