luci-app-nes-emulator: add NES emulator interface - #8962
Conversation
openwrt
Bot
added
add package
9b5eda1 to
32686a8
Compare
openwrt
Bot
removed
the
not following guidelines
openwrt-ai
left a comment
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
| du -b "$full" 2>/dev/null | | ||
| awk 'NR == 1 { print $1 }' |
There was a problem hiding this comment.
du -b is a GNU coreutils option. BusyBox du — the only du on a stock OpenWrt image, and coreutils-du is not in LUCI_DEPENDS — accepts only -aHLdclsxhmk and exits with a usage error here. Because the result is piped into awk, the pipeline status is awk's, so the failure is silent and $size ends up empty; the case "$size" in ''|*[!0-9]*) continue guard on the next line then drops every ROM, and the offline ROM list renders empty on any router where nesd is stopped.
wc -c also reports apparent size rather than allocated blocks, which is what the size and quota checks actually want:
| du -b "$full" 2>/dev/null | | |
| awk 'NR == 1 { print $1 }' | |
| wc -c <"$full" 2>/dev/null | tr -d ' \t' |
The same du -b is used at lines 971, 1203 and 1300, each with the identical silent-empty-string failure: line 971 makes overwriting an existing ROM always return "ROM quota exceeded", line 1203 makes every import fail with "ROM size must be between 16 bytes and 16 MiB". Line 951 uses -exec du -b '{}' ';', where the empty scan file leaves used at 0 and turns the 128 MiB ROM_QUOTA_SIZE into a no-op — -exec wc -c '{}' ';' prints SIZE PATH, which the existing $1 awk sum already handles.
For what it's worth, neither openwrt.git nor luci.git uses du -b anywhere today.
Generated by Claude Code
There was a problem hiding this comment.
Fixed in 5028c07. All du -b uses are gone. Regular non-symlink file sizes now come from a shared wc -c helper, and quota scanning uses BusyBox-compatible find -exec wc -c. The resource contract exercises both single-file and directory accounting.
| json_error "ROM quota exceeded or less than 8 MiB would remain free" | ||
| return | ||
| fi | ||
| if ! mv -fT "$tmp" "$dest" 9>&-; then |
There was a problem hiding this comment.
mv -T is GNU coreutils as well; BusyBox mv implements only -f, -i, -n (plus -v), so this fails with a usage error on a stock OpenWrt image and every import ends in "cannot store ROM in the configured directory" — and since the error path also removes $staged, the user has to re-upload just to hit the same failure.
-T is not needed here: lines 1269-1275 already reject a $dest that exists but is not a regular file, so a plain mv -f cannot silently turn into a move-into-directory.
| if ! mv -fT "$tmp" "$dest" 9>&-; then | |
| if ! mv -f "$tmp" "$dest" 9>&-; then |
Like du -b, mv -T appears nowhere in openwrt.git or luci.git today.
Generated by Claude Code
There was a problem hiding this comment.
Fixed in 5028c07. The commit now uses BusyBox-compatible mv -f; the existing destination guard still rejects anything that exists but is not a regular non-symlink file.
|
|
||
| PKG_RELEASE:=1 | ||
| PKG_LICENSE:=MIT | ||
| PKG_LICENSE_FILES:=files/LICENSE-MIT |
There was a problem hiding this comment.
PKG_LICENSE_FILES resolves relative to $(PKG_BUILD_DIR), and luci.mk's Build/Prepare copies only luasrc ucode htdocs root src into it — see the copy loop at luci.mk:181. files/ is never copied, so this declaration points at a path that does not exist in the build directory, and files/LICENSE-MIT is not installed into the package either — it is dead weight in the tree as it stands.
luci-theme-footstrap is the one package in tree that hits this and documents the workaround: keep the license text at the package root and copy it in from a Build/Prepare/<name> hook.
Simplest fix is to drop this line — the large majority of luci-app-* Makefiles declare PKG_LICENSE only, with no license file — and drop files/LICENSE-MIT with it. Otherwise move the file to the package root and add the hook.
Generated by Claude Code
There was a problem hiding this comment.
Fixed in 5028c07. PKG_LICENSE_FILES and the unreachable files/LICENSE-MIT copy have been removed from the LuCI submission; the Makefile retains the normal PKG_LICENSE:=MIT declaration.
| json_get_var path path | ||
| [ -n "$path" ] || { | ||
| json_error "path is required" | ||
| exit 0 | ||
| } | ||
| start_daemon || { | ||
| json_error "${START_ERROR:-nesd could not be started}" | ||
| exit 0 | ||
| } | ||
| json_init | ||
| json_add_string path "$path" | ||
| body="$(json_dump_without_upload_fd)" | ||
| resp="$(api_post /api/load "$body")" |
There was a problem hiding this comment.
path comes straight from the client and is forwarded to /api/load verbatim: it is not checked against $ROM_DIR or the configured extra roots, and it skips the valid_data_dir / readlink -f / extension / header checks that import_rom_locked applies to an imported ROM.
That stands out because every other client-supplied path in this script is confined — staged_path_is_valid for uploads, valid_data_dir plus resolved-equals-configured for the ROM directory — so this reads like a deliberate exception rather than an oversight. Is nesd expected to confine path to the configured roots itself? If so it would help to say so in a comment here, since the ROM list the UI offers is already restricted to those roots and any caller holding the write ACL can pass an arbitrary path.
Generated by Claude Code
There was a problem hiding this comment.
Yes, nesd is the final confinement boundary. Its /api/load path flows through resolve_rom_path() / validate_rom_path(): realpath() canonicalization, containment under a configured ROM root, extension validation, O_NOFOLLOW, maximum-size checks, and ROM header validation before loading. The RPCD code now documents that boundary. The integration suite also sends a valid ROM outside every configured root and an in-root symlink escape; both are rejected with HTTP 400. Relevant native validation: https://github.com/communism420/openwrt-nes-emulator/blob/v1.0.0-r19/package/nes-emulator/src/http.c#L2532-L2566
|
|
||
| include $(TOPDIR)/rules.mk | ||
|
|
||
| PKG_RELEASE:=1 |
There was a problem hiding this comment.
nit: PKG_RELEASE has no effect without PKG_VERSION. luci.mk sets VERSION:=$(if $(PKG_VERSION),$(if $(PKG_RELEASE),$(PKG_VERSION)-r$(PKG_RELEASE),$(PKG_VERSION)),$(PKG_SRC_VERSION)), so with no PKG_VERSION the package version falls back to the git-derived PKG_SRC_VERSION and this line is ignored. All 15 applications/*/Makefile that set PKG_RELEASE also set PKG_VERSION; the other 87 set neither.
| PKG_RELEASE:=1 |
Alternatively add a PKG_VERSION above it if you want a manually versioned package.
Generated by Claude Code
There was a problem hiding this comment.
Fixed in 5028c07. The ineffective PKG_RELEASE assignment has been removed; this application now follows the normal git-derived LuCI package version.
32686a8 to
5028c07
Compare
openwrt-ai
left a comment
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
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
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
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
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>
d34fd84 to
f8ba9e6
Compare
openwrt-ai
left a comment
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
| return 1 | ||
| if ! printf '%s\n' "$token" >"$temporary" || | ||
| ! chown root:nesd "$temporary" 2>/dev/null 7>&- 8>&- 9>&- || | ||
| ! chmod 0640 "$temporary" 7>&- 8>&- 9>&- || |
There was a problem hiding this comment.
nit: this delta dropped the 2>/dev/null that used to be on the chmod. The previous head had ! chmod 0640 "$temporary" 2>/dev/null ||; adding the fd closures replaced the stderr redirection instead of appending to it. The neighbouring chown on line 234 kept its 2>/dev/null, so a failing chmod is now the only one of the four that writes to rpcd's stderr.
| ! chmod 0640 "$temporary" 7>&- 8>&- 9>&- || | |
| ! chmod 0640 "$temporary" 2>/dev/null 7>&- 8>&- 9>&- || |
Generated by Claude Code
| if read_auth_token; then | ||
| token="$AUTH_TOKEN" | ||
| elif [ ! -e "$TOKEN_FILE" ] && [ ! -L "$TOKEN_FILE" ]; then | ||
| token="$(new_token 7>&- 8>&- 9>&-)" |
There was a problem hiding this comment.
nit: the 7>&- 8>&- 9>&- here is now redundant and is the odd one out stylistically. new_token already closes all three inside its own command substitution (lines 172-175), and it is a shell function whose only external child is that `hexdump` — so the redirection on the function invocation closes nothing that stays open. Every other site this commit touched uses the `$( exec 7>&- 8>&- 9>&-; … )` form (e.g. [`read_auth_token` lines 200-203](https://github.com/openwrt/luci/blob/f8ba9e65fa54edd55bce2dd8a8d4b29f4c050410/applications/luci-app-nes-emulator/root/usr/libexec/rpcd/nes-emulator#L200-L203)),`` which makes this one read as if the function itself needed the guard. Same applies to line 426 in rotate_token. Either drop the redirection at both call sites or, if you want it belt-and-braces, keeping it is harmless — but worth making the two consistent with each other and with the rest of the file.
Generated by Claude Code
2 participants
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
f8ba9e65fa; 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