luci-app-olsr: fix blank olsr pages - #8715
Conversation
25c8465 to
b87e2f7
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
b87e2f7 to
daaa103
Compare
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed 3 new commits. The common/common_js.js → olsr/common_js.js path fix and the olsr.etx_color/olsr.snr_colors qualifier fix both look correct (the module is exported via 'require olsr.common_js as olsr' and lives at resources/olsr/common_js.js). One inline question on the manual JSON escaping in the rpcd backend.
Generated by Claude Code
| json_dump | ||
| { | ||
| printf '{"jsonreq4":"' | ||
| echo "/${otable}" | nc 127.0.0.1 "${v4_port}" | sed -n '/^[}{ ]/p' | sed 's/\\/\\\\/g; s/"/\\"/g' |
There was a problem hiding this comment.
This sed escapes only \ and ", but the piped olsrd output is multi-line: sed -n '/^[}{ ]/p' prints multiple lines, so literal newline (0x0A) characters get embedded into the jsonreq4 string value. The old json_add_string/json_dump path escaped those (newline → \n); this manual replacement drops that.
The value is ultimately consumed in the browser by JSON.parse(json.jsonreq4),`` and a literal newline inside a JSON string is invalid per ECMA-262 — JSON.parse rejects it (reproduced: Node's `JSON.parse` throws `Bad control character in string literal`). The outer rpcd parse uses json-c (`json_tokener_new()`, non-strict) which may tolerate and re-normalize the newline on output, which could be why it works in your screenshots — but that masking is version-dependent and doesn't cover other control chars (tab, etc.).
Is this relying on json-c normalizing the embedded newlines? If so it's fragile; the escaping should also cover newlines and other control characters (< 0x20). The same applies to the v6 line below (line 63).
Generated by Claude Code
There was a problem hiding this comment.
personal investigation suggests olsrd always responds with a single-line json, but your suggestion is more robust. please recheck with the latest commit.
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed 1 new commit (9effc8a8 "handle multi-line jsoninfo response"), which addresses the prior question about embedded newlines. The added s/\n/\\n/g does not actually escape them — sed works line-by-line, so the multi-line case still emits invalid JSON. One inline comment with a tested replacement.
Generated by Claude Code
| json_dump | ||
| { | ||
| printf '{"jsonreq4":"' | ||
| echo "/${otable}" | nc 127.0.0.1 "${v4_port}" | sed -n '/^[}{ ]/p' | sed 's/\\/\\\\/g; s/"/\\"/g; s/\n/\\n/g' |
There was a problem hiding this comment.
The new s/\n/\\n/g does not escape the embedded newlines it's meant to handle: sed processes input one line at a time and strips the trailing newline before the substitution runs, so the pattern space never contains a \n for the multi-line case. The inter-line newlines sed -n '/^[}{ ]/p' emits still land verbatim between the printf quotes, producing a literal control character inside the JSON string value — which is invalid per ECMA-262 and rejected by JSON.parse(json.jsonreq4) in the browser.
Reproduced (multi-line input still yields a raw 0x0A in the value):
$ { printf '{"jsonreq4":"'; printf '{"a":1}\n{"b":2}\n' | sed -n '/^[}{ ]/p' | sed 's/\\/\\\\/g; s/"/\\"/g; s/\n/\\n/g'; printf '"}'; } | cat -A
{"jsonreq4":"{\"a\":1}$
{\"b\":2}$
"}
To actually escape across the whole multi-line stream (and avoid a stray trailing newline), the lines need to be joined explicitly. A single-pass awk does this cleanly:
| echo "/${otable}" | nc 127.0.0.1 "${v4_port}" | sed -n '/^[}{ ]/p' | sed 's/\\/\\\\/g; s/"/\\"/g; s/\n/\\n/g' | |
| echo "/${otable}" | nc 127.0.0.1 "${v4_port}" | sed -n '/^[}{ ]/p' | awk '{ gsub(/\\/,"\\\\"); gsub(/"/,"\\\""); printf "%s%s", (NR>1?"\\n":""), $0 }' |
Same fix applies to the v6 line at line 63 below.
Generated by Claude Code
There was a problem hiding this comment.
you're right, the newest commit should fix it
9effc8a to
a3d69b9
Compare
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed 1 new commit (a3d69b9 "handle multi-line jsoninfo response"); no new issues found. The new json_stringescape() awk helper correctly addresses the prior multi-line escaping concern: it escapes \ and " (matching ["\\] first, so order is safe), \t and \r, and joins multiple lines with a literal \n, producing valid JSON for the embedded jsonreq4/jsonreq6 string values (verified with a JSON parser against multi-line input). The same helper is applied to both the v4 and v6 lines.
Generated by Claude Code
b5bdf84 to
be4f003
Compare
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed 1 new commit (be4f0039 "re-add missing interface creation"); no new issues found. Re-adding uci.add('olsrd'/'olsrd6', 'Interface') before uci.save() is correct: the following .then reads the pending add entry back out of uci.changes() to derive the new section id for navigation, so without the uci.add() the section id stayed null and the edit page loaded blank. The fix is applied symmetrically to both the v4 (olsrd.js) and v6 (olsrd6.js) views, and the commit message matches the diff.
Generated by Claude Code
be4f003 to
5c77225
Compare
the functions `etx_color` and `snr_colors` were previously moved to common_js, but their callsites were not updated to match. Signed-off-by: Victoria Grain <victoria+openwrt@gratux.xyz>
due to ash's argument list limit, large jsons blow up the rpc call. as a workaround, the jsoninfo is escaped manually. Signed-off-by: Victoria Grain <victoria+openwrt@gratux.xyz>
common_js was previously moved from common/ to olsr/ but not all references had been updated to match. Signed-off-by: Victoria Grain <victoria+openwrt@gratux.xyz>
olsrd seems to always respond with a single line json but this is more robust. Signed-off-by: Victoria Grain <victoria+openwrt@gratux.xyz>
commit c567bfc removed the uci.add() call to create a new interface, seemingly without considering its side-effects. this lead to trying to configure a non-existant interface, resulting in an empty settings page. Signed-off-by: Victoria Grain <victoria+openwrt@gratux.xyz>
5c77225 to
10b08da
Compare
|
rebased onto master because ESLint complained about missing files |
openwrt-ai
left a comment
There was a problem hiding this comment.
The branch was rebased onto master (head 10b08da); the resulting diff is unchanged in content from the state reviewed previously — the common_js path fix, the olsr.etx_color/olsr.snr_colors qualifiers, the json_stringescape awk helper, and the symmetric uci.add() interface-creation re-add all match what was already reviewed. Re-verified the awk escaper and the printf-based JSON assembly independently; no new issues found. All three FormalityCheck runs are green on the current head.
Generated by Claude Code
|
Merged, thanks! |
|
Thank you for the changes. When will we see the updated package in 25.12? |
Pull request details
Description
On OpenWrt 25.12.4 some OLSR Status pages (Neighbours, Routes, Topology), as well as the settings page when adding a new interface, are completely blank.
This PR introduces three fixes to restore functionality:
etx_colorandsnr_colorsfromolsr/common_js(these functions were previously moved, but their call sites had not been updated to match)additionally, some code still referenced
common/common_js. this is now updated toolsr/common_js.Screenshot or video of changes (if applicable)
Maintainer (preferred)
unsure who I should ping here.
last person to touch the Makefile was @systemcrash.
Tested on
OpenWrt version: OpenWrt 25.12.4 (r32933-4ccb782af7)
LuCI version: LuCI openwrt-25.12 branch (26.167.08537~9ccd99b)
Web browser(s): LibreWolf 152.0-1
Checklist
Signed-off-by: <my@email.address>row (viagit commit --signoff).<package name>: titlefirst line subject for packages.PKG_VERSIONin the Makefile.