luci-app-nes-emulator: add NES emulator interface - #8962
Conversation
9b5eda1 to
32686a8
Compare
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed 1 new commit; comments inline.
The headline items are two GNU-coreutils-only options in the rpcd bridge (du -b, mv -fT) that BusyBox does not implement — they would break the ROM listing, the quota accounting and every ROM import on a stock OpenWrt image, mostly silently. The stated dependency on openwrt/packages#30350 and the draft status are noted; nothing in this review depends on that package landing first.
Generated by Claude Code
32686a8 to
5028c07
Compare
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed 1 new commit; no new issues found.
The delta since 32686a85 addresses all five earlier findings, and I could not find a regression introduced by the fixes:
- The new
file_size_bytes()helper is BusyBox-safe (wc -c <file, no GNU-only flags), guards against symlinks and non-regular files, normalises BusyBoxwc's width padding withtr -d ' \t', and rejects non-numeric output — so the three formerdu -bcall sites (scan_rom_root,storage_is_available'sold_size, and the two size checks inimport_rom_locked) now fail closed rather than silently yielding an empty string. It also follows the script's existingexec 7>&- 9>&-fd-hygiene convention. - The quota scan's
find … -exec wc -c '{}' ';'emitsSIZE PATH, which the existing$1-summing awk consumes unchanged; the$1 !~ /^[0-9]+$/ { exit 2 }guard still propagates a non-zero status through theENDblock, so a malformed scan file makesstorage_is_availablereturn failure instead of a bogus total. mv -fis BusyBox-compatible and the "destination is not a regular file" guard remains in place ahead of it.- The
PKG_LICENSE_FILESline andfiles/LICENSE-MITare both gone, leaving no dangling reference;PKG_LICENSE:=MITplus the SPDX header matches the commonluci-app-*pattern. The ineffectivePKG_RELEASEis removed. - The forwarded
loadpath now documents nesd as the confinement boundary at the call site, which was what that thread asked for.
Commit message matches the diff, and all three FormalityCheck runs are green on 5028c07a.
Generated by Claude Code
|
Hardware testing update: this exact LuCI head (5028c07) was built with the official OpenWrt 25.12.5 mediatek/filogic SDK and tested successfully on an ASUS RT-AX52 Pro (aarch64_cortex-a53) together with the native package from openwrt/packages#30350. LuCI pages, service controls, settings, ROM upload/loading, streamed video and audio, controller input, save/load state, and the FCEUX-style FPS overlay all worked. The PR description now records the test evidence. This PR intentionally remains Draft until the dependent native package is merged, per the LuCI template. |
5028c07 to
db1acc3
Compare
|
Coordination update: native package review identified that both init and the LuCI RPCD bridge must lock the shared The exact amended native/LuCI exports passed the full project contracts and a clean current OpenWrt Snapshot x86/64 SDK package check/build. The LuCI APK still directly depends on |
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed the delta from 5028c07a to db1acc3c (rpcd bridge only: shared token lock path, command exec, start-lock serialisation of rotate_token).
The direction is right — command exec is genuinely required for the || return 1 to be reachable under ash, the start-lock/token-lock ordering is consistent with start_daemon so there is no deadlock, and the post-rotation re-check of a concurrently-started daemon closes the race it describes. Four things the delta leaves open, inline:
- the token lock now lives on the flash overlay and is written on every single RPC call, and it skips the hardening
prepare_start_lock_fileapplies to the start lock; - the upload lock keeps the exact
/var/lock(mode 1777) exposure the token lock was moved away from; - the
commandprefix was applied to one of threeexecredirections; the other two still silently kill the script instead of returning a JSON error; acquire_start_lock(20 s) plusacquire_token_lock(10 s) exceeds LuCI's 20 s default RPC timeout, so a contended rotation reports failure to the user after it has already succeeded.
Commit message matches the diff. All three FormalityCheck runs are green on db1acc3c. The stated dependency on openwrt/packages#30350 and the draft status are noted; the native package is not in the feed yet, so I could not verify the init script's side of the shared lock path.
Generated by Claude Code
db1acc3 to
d3041a4
Compare
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed the delta from db1acc3c to d3041a4f (rpcd bridge only: shared /var/run/nes-emulator lock directory, common prepare_lock_file helper, command exec on all three lock opens, parameterised lock budgets).
All four findings from the previous round are genuinely addressed. Moving the locks onto tmpfs removes the per-RPC flash write and gets the upload lock out of the world-writable /var/lock; /var/run is /tmp/run on stock OpenWrt (base-files/Makefile:227, created root-owned 0755 by init.d/boot), so the 0700 root-owned lock directory is not reachable by an unprivileged local account. The command exec prefix is now on all three redirections, the <> opens no longer truncate, and each is re-validated after opening. Start-lock ordering stays consistent between start_daemon and rotate_token, so there is no new deadlock. The arithmetic in the new comment at line 394-395 checks out: (3-1) + (4-1) + (3-1) = 7 sleeps for a contended rotate_token.
Three things inline, one of them substantive:
- the bounded budget was applied to
rotate_tokenonly —access,start,loadandimportstill getSTART_LOCK_MAX_ATTEMPTS=20plus the default 10-attempt pre-dispatch token wait, which reproduces the timeout symptom the earlier thread was about; - two fd-hygiene nits: the
sleepinacquire_token_lockis the only lock-wait loop that does not close the lock fds, and the fd inventory comment plus four older helpers still predate fd 8 becoming a lock.
Commit message matches the diff. All three FormalityCheck runs are green on d3041a4f. The stated dependency on openwrt/packages#30350 and the draft status are noted; the native package is still not in the feed, so I could not verify the init script's side of the shared /var/run/nes-emulator directory — in particular that it creates it with exactly 0700 root:root, which prepare_lock_directory requires on every call.
Generated by Claude Code
d3041a4 to
d34fd84
Compare
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed the delta from d3041a4f to d34fd840: the dynamic 120 s L.env.rpctimeout raise for access/start/load/import/rotate_token, the 125 s local action guard on Start/Load, and the fd-8 hygiene follow-ups in the rpcd bridge.
The save/raise/restore wrapper does capture the timeout at the right moment — rpc.call reads L.env.rpctimeout synchronously inside the new Promise executor, so the finally restore cannot race it, and the Start/Load client guard (125 s) is correctly above the RPC budget (120 s), while Import and Rotate await their real result. The rpcd side of the delta is fine: the fd inventory comment now matches reality, the token-lock retry sleep closes 7/8/9 like the other two loops, and the four cited helpers were updated.
One substantive point inline: raising a global is not equivalent to a per-call timeout, because luci-base batches RPC requests per animation frame and takes the whole batch's options — including timeout — from the first request queued. For access, which is issued from view.load() alongside luci-base's own uci.changes() on the luci-loaded event, that is likely to hand the batch a 20 s timeout again, i.e. the original symptom. nobatch: true on the long-running declarations makes it deterministic and is what luci-base does for iwinfo scan. Two nits: the remaining fd-8 sites are the ones inside the token-lock critical section, and the timeout helper is triplicated across the three views.
Commit message matches the diff, and all three FormalityCheck runs are green on d34fd840. The native package is still not in the packages feed, so the init-script side of the shared /var/run/nes-emulator lock directory remains unverified here; the draft status and the openwrt/packages#30350 dependency are noted.
Generated by Claude Code
d34fd84 to
f8ba9e6
Compare
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed the delta from d34fd840 to f8ba9e65: the new shared nes-emulator.js resource with nobatch: true on the long-running declarations, the three views switching to it, and the remaining fd 7/8/9 closures in the rpcd bridge.
The substantive finding from the previous round is properly fixed. nobatch: true reaches the transport — rpc.declare forwards options.nobatch into rpc.call, which passes it to request.post, and isQueueableRequest then refuses to queue it. So access issued from play.js load() no longer inherits the animation-frame batch's 20 s timeout from whatever luci-base queued first; each of the five calls carries its own 120 s budget. The save/raise/restore wrapper still works because rpc.call reads L.env.rpctimeout synchronously inside the new Promise executor, ahead of the wrapper's finally.
The shared-resource mechanics check out too: 'require nes-emulator as nesEmulator' matches the require regex ((\S+) accepts the hyphen, same as luci-app-olsr-viz),`` and require() returns `new _class()`, so the `baseclass.extend` prototype members resolve as `nesEmulator.getActionTimeout` / `.declareLongRunningRpc`. A top-level `resources/*.js` for an app has in-tree precedent (`lldpd.js`, `olsr-viz.js`) and `luci.mk` copies `htdocs` wholesale, so nothing extra is needed to package it. `Number(L.env.rpctimeout) || 20` is not dead code — it is what keeps `Math.max` from returning `NaN` when the env value is unset.
On the rpcd side I walked the token critical section for a remaining holder and did not find one: read_auth_token, write_auth_token, validate_token_parent, remove_legacy_token and new_token now fork every external child with 7/8/9 closed, printf is a builtin, and the two /etc/init.d/nes-emulator running probes in rotate_token run when fd 8 is already released. ROTATE_TOKEN_LOCK_MAX_ATTEMPTS is applied at the dispatcher (line 1475), so the seven-sleep arithmetic in the comment holds.
Two nits inline, one of them a small regression this delta introduced (chmod lost its 2>/dev/null).
Commit message matches the diff, and all three FormalityCheck runs are green on f8ba9e65. nes-emulator is still absent from the packages feed, so the init-script side of the shared /var/run/nes-emulator lock directory remains unverified here; the draft status and the openwrt/packages#30350 dependency are noted.
Generated by Claude Code
Add a JavaScript LuCI interface for service control, ROM import, settings, and the thin browser game client. The application directly depends on the native nes-emulator package and each runtime helper it invokes. Signed-off-by: Yaroslav Vereshchagin <yarik.vereshchagin1996@gmail.com>
f8ba9e6 to
038ad57
Compare
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed 1 new commit; no new issues found.
The delta from f8ba9e65 to 038ad57a is nine lines in the rpcd bridge and does exactly what the two open nits from the previous round asked for, with nothing else mixed in:
chmod 0640 "$temporary" 2>/dev/null 7>&- 8>&- 9>&-restores the stderr redirection that the fd closures had displaced, so all four children inwrite_auth_token's failure chain are consistent again. (mv -fon the next line still has no2>/dev/null, but it never had one and rpcd stderr only reaches syslog, so nothing to do there.)- Both
new_tokencall sites —ensure_auth_tokenand [`rotate_token`](https://github.com/openwrt/luci/blob/038ad57ad85184505509ccc9a9f19847bb3ef56c/applications/luci-app-nes-emulator/root/usr/libexec/rpcd/nes-emulator#L428-L431)`` — now use the$( exec 7>&- 8>&- 9>&-; … )form that the other ~20 sites in the file use, instead of a redirection on the function invocation.
The rewrite is behaviour-preserving in the ways that matter: closing an fd that is not open is a no-op, so exec 7>&- 9>&- is harmless in ensure_auth_token where only fd 8 is held; the substitution's exit status is still new_token's, and a failed exec inside the substitution yields an empty $token, which the existing [ "${#token}" -ne 64 ] guard catches on both paths. new_token is still resolvable from the substitution shell, and its own inner hexdump substitution keeps its closures. dash -n and sh -n parse the resulting script cleanly.
Commit message matches the diff, and all three FormalityCheck runs are green on 038ad57a. nes-emulator is still absent from the packages feed, so the init-script side of the shared /var/run/nes-emulator lock directory — in particular that it creates it as exactly 0700 root:root, which prepare_lock_directory requires on every call — remains unverified here; the draft status and the openwrt/packages#30350 dependency are noted.
Generated by Claude Code
Pull request details
Description
Add a JavaScript LuCI interface for the NES emulator service.
The application provides service status and controls, authenticated ROM upload
and loading, emulator settings, and access to the thin browser game client.
Emulation, rendering, JPEG encoding, and audio generation remain in the native
nesdservice on the router. No ROM or BIOS files are included.A shared application resource centralizes the long-running RPC policy used by
all three views. The package directly depends on its external runtime helpers;
installing it also selects and installs the native
nes-emulatorpackage.The RPCD bridge uses BusyBox-compatible file operations. Client-supplied load
paths are passed to
nesd, whose final validation canonicalizes the path,confines it to configured ROM roots, opens it with
O_NOFOLLOW, and validatesthe extension, size, and ROM header before loading.
Token creation and rotation share a lock with the native init script. Token,
upload, and startup locks live in the root-owned mode-
0700/var/run/nes-emulatortmpfs directory. Each mode-0600, root-owned,single-link lock file is safely prepared with no-clobber creation and
umask 077, opened without truncation through catchablecommand execredirection, and revalidated after opening. Descriptor 9 owns the upload lock,
8 the token lock, and 7 the startup lock. Every external child in the token
critical sections, including the outer command-substitution shells, closes all
three descriptors; startup/upload children close the held request locks too.
Rotation is serialized with startup and rechecks a daemon that may have started
concurrently. Its RPCD-side lock phase has an exact maximum of seven one-second
sleeps: two for each of the two token-lock encounters plus three for the startup
lock. A contended native restart can add at most nine native token-lock sleeps,
for a coordinated lock-wait maximum of 16.
A shared application resource declares the start-capable
access,start,load, andimportcalls, plusrotate_token, withnobatch: trueand an RPCtimeout of at least 120 seconds. Each request therefore captures its own timeout
instead of inheriting the first request in an animation-frame batch. The helper
restores the previous global value immediately after the synchronous RPC
invocation, so unrelated calls retain their configured timeout. The local
Start/Load guard is computed from the effective RPC timeout plus five seconds;
transactional Import/Rotate operations await their real result instead of
racing a false client-side timeout.
Depends on openwrt/packages#30350.
This pull request remains a draft until that native package is available to the
LuCI build, as required by the LuCI pull request template for dependent
submissions.
The English POT template was generated with LuCI's
i18n-scan.pl; translation.pofiles remain managed through Weblate.Screenshot or video of changes (if applicable)
The screenshot uses the project's freely distributable demo ROM; no commercial
game assets are included.
Maintainer (preferred)
@communism420
Tested on
OpenWrt version: OpenWrt 25.12.5
LuCI version: hardware-tested PR head
5028c07aon the OpenWrt 25.12.5 LuCI stackCurrent review heads: LuCI
038ad57ad8; native62ec2dff9Device: ASUS RT-AX52 Pro (
mediatek/filogic,aarch64_cortex-a53)Web browser(s): Chromium desktop checks; end-to-end desktop browser smoke test passed
The on-device smoke test used LuCI head
5028c07aand native headb4424df4,both built with the official OpenWrt 25.12.5
mediatek/filogicSDK. LuCI pages,service status/controls, settings, ROM upload/loading, streamed gameplay, video,
audio, controller input, save/load state, and the FCEUX-style FPS overlay all
worked on the router.
The hardware-tested emulator and FCEUmm sources remain unchanged. Current heads
add the review-driven protected lock lifecycle, external-storage diagnostics,
bounded wait accounting, isolated/unbatched long-operation RPC handling, and
complete fd 7/8/9 child-process hygiene.
The source revision exporting these heads is covered by the successful
project CI run.
It includes ShellCheck, BusyBox resource-operation contracts, dynamic unbatched
long-RPC timeout/restoration tests, shared-resource packaging/export checks,
lock-fd inheritance checks, browser-client contracts, native path-confinement
integration tests, exact export validation, a clean
native build, and the complete black-box regression suite.
The unchanged package recipes and native binary also passed a clean current
Snapshot x86/64 SDK package check/build. The application passes the current LuCI
ESLint configuration, package check, and compilation. Generated APK metadata
was inspected for direct
cgi-io,jshn,jsonfilter,luci-base,nes-emulator, andrpcddependencies. The RPCD bridge is mode0755.Checklist