luci-mod-system: derive factory-reset LAN IP from board.json - #8912
Conversation
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed 1 new commit. The motivation is sound — CONFIG_TARGET_DEFAULT_LAN_IP_FROM_PREINIT builds really do get a wrong reconnect hint today — but as implemented the helper resolves to undefined on every current build: the RPC targets the wrong ubus object, the flash ACL group doesn't grant the method, and there is no fallback when board.json has no network.lan.ipaddr (which is the normal case). Details inline.
Generated by Claude Code
| }); | ||
|
|
||
| const callGetBoardJSON = rpc.declare({ | ||
| object: 'luci', |
There was a problem hiding this comment.
Wrong ubus object — getBoardJSON is registered on luci-rpc, not on luci. See .name = "luci-rpc" in rpcd-mod-luci/src/luci.c:2046 and the existing declaration in network.js:61-64. The luci object (/usr/share/rpcd/ucode/luci) exposes getFeatures, getLEDs, … but no getBoardJSON, so as written every call returns "Method not found", L.resolveDefault swallows it and the helper resolves to undefined.
| object: 'luci', | |
| object: 'luci-rpc', |
Generated by Claude Code
| object: 'luci', | ||
| method: 'getBoardJSON', | ||
| expect: { '': {} } | ||
| }); |
There was a problem hiding this comment.
Missing ACL grant. luci-rpc getBoardJSON is only granted by the luci-base-network-status group (luci-base.json:40), which the flash view does not depend on. The luci-mod-system-flash group's read.ubus section only lists file (luci-mod-system.json:181-183).`` This works for root (whose rpcd session is granted `*`) but silently fails with "Access denied" for any restricted user that has only the flash ACL — exactly the case this change is meant to fix. Please add `"luci-rpc": [ "getBoardJSON" ]` to `luci-mod-system-flash`'s `read.ubus`.
Generated by Claude Code
| }); | ||
|
|
||
| const factoryLanIP = () => | ||
| L.resolveDefault(callGetBoardJSON(), {}).then(bj => bj?.network?.lan?.ipaddr); |
There was a problem hiding this comment.
No fallback: this resolves to undefined on stock images, which is a regression rather than a no-op. network.lan.ipaddr is not normally present in board.json — ucidef_set_interface_lan only writes device and protocol (uci-defaults.sh:82), and the ipaddr/netmask keys are injected by the generated /etc/board.d/99-lan-ip script only when CONFIG_TARGET_DEFAULT_LAN_IP_FROM_PREINIT=y (base-files/Makefile:96-108). The 192.168.1.1 default lives in config_generate, applied at runtime when ipaddr is absent (config_generate:166).
So for the majority of builds the three call sites end up as ui.awaitReconnect(undefined, 'openwrt.lan'), and pingDevice() will probe http://undefined/…. The same happens whenever the RPC itself fails (wrong object, ACL denied, device already down), since L.resolveDefault masks the error. Mirror config_generate's default explicitly:
| L.resolveDefault(callGetBoardJSON(), {}).then(bj => bj?.network?.lan?.ipaddr); | |
| L.resolveDefault(callGetBoardJSON(), {}).then(bj => bj?.network?.lan?.ipaddr ?? '192.168.1.1'); |
nit: the commit message and PR body state "Behaviour for stock upstream builds is unchanged (board.json still contains '192.168.1.1')" — that premise does not hold; stock board.json has no network.lan.ipaddr key at all. Worth correcting the wording once the fallback is explicit.
Generated by Claude Code
| fs.exec('/sbin/firstboot', [ '-r', '-y' ]); | ||
|
|
||
| ui.awaitReconnect('192.168.1.1', 'openwrt.lan'); | ||
| factoryLanIP().then(ip => ui.awaitReconnect(ip, 'openwrt.lan')); |
There was a problem hiding this comment.
Ordering hazard: this issues a fresh RPC after /sbin/firstboot -r -y has already been fired at the line above, i.e. against a device that is in the middle of wiping its overlay and rebooting. The old code called ui.awaitReconnect() synchronously and could not miss the window; now the reconnect poller is only armed once the ubus round-trip settles, which in the common case means waiting out the request timeout ((L.env.rpctimeout ?? 20) * 1000 in rpc.js:38) — up to 20 s of dead time before polling starts, and then with the undefined address from the failed call.
Please fetch board.json up-front instead: add the L.resolveDefault(callGetBoardJSON(), {}) promise to the tasks array in load() and stash the resolved address, so all three call sites can pass a plain string synchronously. The same applies to the sysupgrade site at lines 350-351, which sits right after the equally non-returning fs.exec('/sbin/sysupgrade', args); handleRestoreConfirm (lines 175-176) is less exposed since the reboot there is a separate step, but it would benefit from the same prefetch for consistency.
Generated by Claude Code
0ea8b2f to
2509634
Compare
|
Thanks for the thorough review — all four points were valid. Force-pushed
Commit message rewritten accordingly; the incorrect "board.json still contains '192.168.1.1'" claim is gone. Verified end-to-end on a T1023 device: after "Perform reset" the reconnect poller targets |
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed 1 new commit (force-push 0ea8b2f → 2509634). All four points from the previous review are correctly addressed:
object: 'luci-rpc'now matches.name = "luci-rpc"in rpcd-mod-luci/src/luci.c:2046."luci-rpc": [ "getBoardJSON" ]added toluci-mod-system-flash'sread.ubus, mirroring luci-base.json:40; the group is the one bound to the flash page in menu.d/luci-mod-system.json:170,`` andreadis the right scope for a read-only method.- The
?? '192.168.1.1'fallback plus the module-scope initialiser restores stock behaviour. - The prefetch moved into
load(); all threeui.awaitReconnect()sites are synchronous again, and the extra sixth element in thetasksarray is harmless —render([p_fstat, hostname, procmtd, procpart, procmounts])only destructures the first five.
The commit message matches the diff and the subject scope matches the touched paths. Two minor points inline.
Generated by Claude Code
| /* Cached factory-reset LAN IP, prefetched in load() so it can be passed | ||
| * synchronously to ui.awaitReconnect() before firstboot/sysupgrade wipe | ||
| * the device and rpcd disappears. */ | ||
| let factoryLanIP = '192.168.1.1'; |
There was a problem hiding this comment.
nit: the PR description still describes the pre-force-push revision and now contradicts the commit message on two facts: it says the change introduces "a small factoryLanIP() helper backed by the existing luci.getBoardJSON rpc method" (it is a module-scoped string, and the object is luci-rpc), and it repeats "Behaviour for stock upstream builds is unchanged (board.json still contains '192.168.1.1')" — the premise the rewritten commit message correctly drops, since stock board.json has no network.lan.ipaddr key at all and the default now comes from this line. Worth syncing the body with the commit message so the merged description matches the code.
Generated by Claude Code
| ui.awaitReconnect(window.location.host); | ||
| else | ||
| ui.awaitReconnect(window.location.host, '192.168.1.1', 'openwrt.lan'); | ||
| ui.awaitReconnect(window.location.host, factoryLanIP, 'openwrt.lan'); |
There was a problem hiding this comment.
Unlike the firstboot (line 119) and restore (line 181) sites, this one runs after flashing a user-supplied image that is not necessarily the same build as the running one, so the running system's board.json is not authoritative for the post-upgrade factory IP — e.g. sysupgrading a CONFIG_TARGET_PREINIT_IP vendor build to a stock image (or vice versa) leaves the device on a different address than factoryLanIP. Before this change '192.168.1.1' was always in the probe list here; now it is replaced rather than supplemented.
Since awaitReconnect(...hosts) probes every address it is handed, keeping '192.168.1.1' as an extra candidate at this site (when it differs from factoryLanIP) would cost nothing and preserve the old coverage. Was dropping it here intentional?
Generated by Claude Code
The three flash.js call sites that invoke ui.awaitReconnect() after a
factory reset, backup restore or sysupgrade currently hardcode
'192.168.1.1' as the post-reset LAN IP. That is the built-in
config_generate default, but vendors and target images routinely ship a
different value via CONFIG_TARGET_PREINIT_IP together with
CONFIG_TARGET_DEFAULT_LAN_IP_FROM_PREINIT, which writes the target's
factory ipaddr into board.json's network.lan section. flash.js has no
way to see that value today, so users of such images are told to wait
for a device at an IP the device will never come up on.
Prefetch board.json's network.lan.ipaddr in load() via the luci-rpc
getBoardJSON method and cache it in a module-scoped factoryLanIP
string, defaulting to '192.168.1.1' both for stock builds (where
board.json has no network.lan.ipaddr key -- ucidef_set_interface_lan
only writes device and protocol; the '192.168.1.1' default is applied
at runtime by config_generate) and as a safety net if the rpc call
fails. All three ui.awaitReconnect() call sites then receive the
resolved IP synchronously.
The prefetch is important for firstboot and sysupgrade: those non-
returning fs.exec() calls fire immediately, so any ubus round-trip
issued afterwards would race against rpcd shutting down and typically
resolve only after the rpc timeout (~20s), with the wrong address. By
resolving in load() we guarantee the value is a plain string at click
time.
At the sysupgrade site the user-supplied image is not necessarily the
same build as the running one (e.g. flashing a stock image onto a
vendor build or vice versa), so the running system's board.json is not
authoritative for the post-upgrade factory IP. Keep '192.168.1.1' as
an additional awaitReconnect() candidate there to preserve the pre-
patch coverage; ui.awaitReconnect() probes every address it is handed,
so a duplicate when factoryLanIP already equals '192.168.1.1' is
harmless.
Also extend the luci-mod-system-flash ACL group's read.ubus section
with 'luci-rpc': [ 'getBoardJSON' ]; without it, restricted users with
only the flash ACL would silently fail the rpc call ('root' works only
because its rpcd session is granted '*').
Signed-off-by: Michael Pfeifroth <micpf@westermo.com>
2509634 to
208df8f
Compare
|
Force-pushed
|
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed 1 new commit (force-push 2509634 → 208df8f). The only code delta is the extra '192.168.1.1' candidate at the sysupgrade site, which addresses the previous round's question: awaitReconnect(...hosts) probes every address in parallel and pingDevice() cache-busts each request, so the duplicate on stock builds (where factoryLanIP already is 192.168.1.1) is inert, exactly as the commit message states. The commit message grew a matching paragraph and still describes the diff accurately; subject scope matches the touched paths.
One point from the previous round is still open — details inline.
Generated by Claude Code
| }); | ||
|
|
||
| const callGetBoardJSON = rpc.declare({ | ||
| object: 'luci-rpc', |
There was a problem hiding this comment.
nit: the PR description sync described in the last comment did not land — as of 208df8f the body still reads "Introduce a small factoryLanIP() helper backed by the existing luci.getBoardJSON rpc method" (it is a module-scoped string, and the object is luci-rpc per this line) and "Behaviour for stock upstream builds is unchanged (board.json still contains '192.168.1.1')", the premise the rewritten commit message correctly drops. Possibly an unsaved edit in the GitHub description box; worth re-applying so the body matches the commit message.
Generated by Claude Code
The three flash.js call sites that invoke
ui.awaitReconnect()after a factory reset, backup restore or sysupgrade currently hardcode'192.168.1.1'as the post-reset LAN IP. That is the built-inconfig_generatedefault, but vendors and target images routinely ship a different value viaCONFIG_TARGET_PREINIT_IPtogether withCONFIG_TARGET_DEFAULT_LAN_IP_FROM_PREINIT, which writes the target's factory ipaddr intoboard.json'snetwork.lansection.flash.jshas no way to see that value today, so users of such images are told to wait for a device at an IP the device will never come up on.Prefetch
board.json'snetwork.lan.ipaddrinload()via theluci-rpcgetBoardJSONmethod and cache it in a module-scopedfactoryLanIPstring, defaulting to'192.168.1.1'both for stock builds (whereboard.jsonhas nonetwork.lan.ipaddrkey —ucidef_set_interface_lanonly writesdeviceandprotocol; the'192.168.1.1'default is applied at runtime byconfig_generate) and as a safety net if the rpc call fails. All threeui.awaitReconnect()call sites then receive the resolved IP synchronously.The prefetch is important for firstboot and sysupgrade: those non-returning
fs.exec()calls fire immediately, so any ubus round-trip issued afterwards would race against rpcd shutting down and typically resolve only after the rpc timeout (~20s), with the wrong address. By resolving inload()we guarantee the value is a plain string at click time.At the sysupgrade site the user-supplied image is not necessarily the same build as the running one (e.g. flashing a stock image onto a vendor build or vice versa), so the running system's
board.jsonis not authoritative for the post-upgrade factory IP. Keep'192.168.1.1'as an additionalawaitReconnect()candidate there to preserve the pre-patch coverage;ui.awaitReconnect()probes every address it is handed, so a duplicate whenfactoryLanIPalready equals'192.168.1.1'is harmless.Also extend the
luci-mod-system-flashACL group'sread.ubussection with"luci-rpc": [ "getBoardJSON" ]; without it, restricted users with only the flash ACL would silently fail the rpc call (rootworks only because its rpcd session is granted*).