nes-emulator: add package - #30350
Conversation
53181b3 to
5358636
Compare
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed 1 new commit. The commit message matches the diff, and v1.0.0-r19 does exist in the upstream repository, so the source reference checks out. Formality checks are green; nothing CI-related to add.
Of the four inline notes, two look like they should be settled before merge: the postinst can fail the install transaction because preflight supplies its exit status, and the runtime directories ship root:root 0750 while the daemon is meant to run as nesd. The third — init script and UCI defaults living in the release tarball instead of multimedia/nes-emulator/files/ — is a feed-convention point, but it also means the parts of this package that actually determine its runtime behaviour are not in the diff, which is worth resolving on a new-package submission. The last one is a nit.
Generated by Claude Code
5358636 to
658d505
Compare
658d505 to
933164c
Compare
933164c to
af1380f
Compare
af1380f to
b4424df
Compare
|
Follow-up on the patch-submission checklist in
|
b4424df to
227f4cb
Compare
|
Hardware testing update for native head The later native head |
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed the rework since 5358636 — the init script and UCI defaults are now feed-local and reviewable, the postinst can no longer fail the install transaction, PKG_VERSION is a plain SemVer with an independent PKG_RELEASE, and the FCEUmm pin is a separately hash-verified secondary download with two properly-headed quilt patches. The custom variables, the OpenWrt-specific combined tarball and the build-time chmod 750 of empty data directories are all gone. Commit message matches the diff, and all three FormalityCheck runs are green on the head, so nothing CI-related to add.
I could not fetch libretro/libretro-fceumm from this session, so the Upstream-Status: Backport [...3db086e...] reference on patch 001 and the FCEUmm tarball hash are unverified here — not flagged, just not confirmed.
Nothing I found blocks a merge. The one I would settle first is the external-storage case in prepare_data_dir: the shipped config recommends moving rom_dir/save_dir onto mounted storage, and that path deliberately skips the ownership repair without checking or logging whether nesd can write there, so save states fail silently at runtime. The lockfile in world-writable /var/lock and the dead safety_migration option are worth a look; the rest is a redundant/no-op postinst and one nit.
Generated by Claude Code
227f4cb to
3ac1d59
Compare
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed the changes since 227f4cb. All four points from the last round are addressed, and the mechanical parts hold up under checking:
command exec 8>"$TOKEN_LOCK_FILE" || return 1really does return instead of terminating the shell — confirmed in dash, which BusyBox ash descends from — so the open failure is now catchable, and the lock sits inside the root-owned0750/etc/nes-emulatorinstead of world-writable/var/lock.- The new
directory_is_writable_by_nesd/directory_is_searchable_by_nesdmode globs are correct. I ran them over owner/group/other precedence, setgidsvsSand stickytvsT, and every case classified the way the kernel would, including the owner-class-denies-without-falling-through case.external_data_dir_is_usable_by_nesdwalks to/and terminates. AUTH_TOKENis gone from every path andsafety_migrationhas no remaining reference anywhere in the package.- Dropping
Package/nes-emulator/postinstis safe:default_postinstruns"$i" startfor each shipped init script on a live root (functions.sh:401-412) afteradd_group_and_user(package-pack.mk:545-546), and it does not propagate that start's exit status, so provisioning still happens and still cannot fail the install transaction.
The commit message matches the diff, and all three FormalityCheck runs are green on the head, so nothing CI-related to add.
Nothing here blocks a merge. The one I would settle is the flip side of the external-storage fix: rejecting an unusable path is the right behaviour, but a vfat/exfat USB stick — the storage the shipped config actually recommends — now produces a service that refuses to start behind a message that does not say which access is missing. The system_dir question and one nit are smaller.
Generated by Claude Code
3ac1d59 to
891dfd4
Compare
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed the changes since 3ac1d59. Both points from the last round are addressed, and the mechanical parts check out:
- The
read/writesplit is correct.directory_is_readable_by_nesduses the same owner → group → other precedence as its siblings, and its three globs (dr?[xs]??????,d???r?[xs]???,d??????r?[xt]) land on the right positions of the 10-character mode string, including the setgids/Sand stickyt/Tvariants.system_dirnow asks for read+search whilerom_dir/save_dirkeep write+search, and the requirement is threaded throughexternal_data_dir_is_usable_by_nesdwithout changing the ancestor walk. - The failure message now names the missing access, the numeric uid, the primary gid and every effective group.
log_errorjoins its arguments through"$*", so the three-argument call renders as one line. - Moving the lock off
/var/lockto/var/run/nes-emulatorcloses the world-writable-parent problem for real: base-files creates/var/runwith a plainmkdir -punder the default umask and never relaxes it (boot:26-29), unlike/var/lockright above it which is explicitlychmod 1777. So the new parent is root-owned0755,nesdcannot pre-create or squat the directory, and thedrwx------/-rw-------root-owned validation plusset -Cno-clobber creation and post-open revalidation hold.<>also avoids the truncation the old8>would have done to a lock file it did not create.
The commit message matches the diff, and all three FormalityCheck runs are green on the head, so nothing CI-related to add.
Nothing here blocks a merge, but the first inline is worth settling: the external-storage path still has a silent hard failure in front of the diagnostic this round added, and on vfat/exfat it triggers even when the stick is mounted exactly as the shipped config advises. The other is a nit about a number in the PR body.
Generated by Claude Code
891dfd4 to
6e9d006
Compare
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed the changes since 891dfd4. Two hunks, both of which hold up:
- The best-effort metadata repair is the fix the last round asked for, and I traced both external-storage outcomes through the new control flow. A stick mounted the way
nes-emulator.config:3-4advises —uid=/gid=set, defaultdmask— now getschmod 0750refused, logs the warning, and thendirectory_is_writable_by_nesdmatchesdrwxr-xr-xon the owner branch, so the service starts. A defaultuid=0,gid=0mount fails thechown, falls through to theotherclass with now, and reaches the actionable "lacks write/search access" message added last round instead of the silentreturn 1. Strict repair is unchanged for/etc/nes-emulatorand/etc/nes-emulator/*, and the ancestor walk is untouched. sleep 1 8>&-keeps the held lock descriptor out of the wait child, matching theexec 8>&-already used in theprepare_lock_directory/prepare_token_lock_filesubshells. ash saves and restores per-command redirections, so fd 8 is still open for the nextflock -n 8iteration and forrelease_token_lock. (Several short-lived children inside the locked region —uci,mktemp,mv— still inherit fd 8; no consequence, since they exit before the lock is released.)
The lock-wait accounting in the PR body now matches the code: 2+2+3 = 7 on the RPCD side, and TOKEN_LOCK_MAX_ATTEMPTS=10 yields nine sleep 1 calls here, for the stated coordinated bound of 16.
The commit message matches the diff, and all three FormalityCheck runs are green on the head, so nothing CI-related to add. I still cannot reach libretro/libretro-fceumm from this session, so the Upstream-Status: Backport reference on patch 001 and the FCEUmm tarball hash remain unverified here — not flagged, just not confirmed.
Nothing blocks a merge from my side this round; the single inline is a nit about log priority.
Generated by Claude Code
Add a router-side NES emulator service with HTTP and WebSocket streaming. Download the tagged host source and pinned FCEUmm core independently, apply the reviewed feed-local core patches, and install the procd service, UCI configuration, and an unprivileged service account. Upstream project: https://github.com/communism420/openwrt-nes-emulator Signed-off-by: Yaroslav Vereshchagin <yarik.vereshchagin1996@gmail.com>
6e9d006 to
62ec2df
Compare
openwrt-ai
left a comment
There was a problem hiding this comment.
Reviewed 1 new commit; no new issues found.
The changes since 6e9d006a are two hunks in nes-emulator.init and nothing else — git diff reports 5 insertions, 1 deletion in a single file — and both are the nit from the last round:
log_warn()mirrorslog_error()exactly apart from the priority (-p daemon.warnvs-p daemon.err), keeps the same-t "$NAME"tag and the same--guard before"$*", so the multi-argument call still renders as one line. It is defined at the top of the script alongside its sibling, well before its only call site.- That call site is the recoverable branch: the best-effort
repair_directory_metadatafailure for a newly created external directory, which the script deliberately continues from and whereexternal_data_dir_is_usable_by_nesdremains the decider. Every one of the other 24 logging calls in the file is stilllog_error, and each of those precedes a non-zero return, sodaemon.errnow consistently means "nesd will not start". The unusable-path message two lines below is untouched and still goes out aterr.
The strict-repair branches for /etc/nes-emulator and /etc/nes-emulator/*, the ancestor walk, the read/write requirement plumbing, and the lock handling are all byte-identical to the revision I checked last round, so nothing there needed re-verification.
The commit message still matches the diff, and all three FormalityCheck runs are green on the head, so nothing CI-related to add.
As in previous rounds, this session can only reach openwrt/packages, so the Upstream-Status: Backport [...3db086e...] reference on patch 001, the v1.0.0 project tag and both codeload hashes remain unverified here — not flagged, just not confirmed.
Nothing blocks a merge from my side.
Generated by Claude Code
📦 Package Details
Maintainer: @communism420
Description:
Add
nes-emulator, a router-side NES emulation service for OpenWrt.The daemon provides authenticated HTTP and WebSocket streaming. No ROM or BIOS
files are included.
The recipe downloads the canonical SemVer-tagged project source and the pinned
FCEUmm commit as two independently SHA-256-verified archives. The two required
FCEUmm changes are feed-local, reviewable patches with authorship, DCO sign-off,
and upstream-status metadata. The daemon is built as a dynamically linked PIE
with OpenWrt hardening enabled.
The generic savestate error-propagation change was merged upstream through
libretro-fceumm#653 as
commit 3db086e,
so patch 001 is marked as a backport. The package deliberately retains the
audited FCEUmm pin
76f68314: the merge tip also contains an unrelatedsavestate-format transition to 9900, while project v1.0.0 builds the embedded
core with
FCEU_VERSION_NUMERIC=9813. That coordinated transition belongs in alater project release.
The ROM-buffer change remains downstream-specific: FCEUmm advertises
need_fullpath=true, under which generic libretro frontends may leavedataand
sizeinvalid, while nesd deliberately supplies the path and the exactbuffer it hashed to preserve its hash/load invariant.
The feed owns and reviews the procd init script and UCI defaults directly. The
standard OpenWrt package lifecycle invokes the init script on a live root, and
the package-owned data tree is provisioned before the disabled-service early
return.
Existing external paths are never chowned or chmodded. For a newly created
external directory, ownership/mode repair is best-effort: a failure is logged
at
daemon.warnbecause filesystems such as vfat/exfat may rejectchown/chmodwhile mountuid,gid, anddmaskoptions already provide the required access. The finaldecision is always the effective-access check using the numeric
nesduid andall effective groups.
rom_dirandsave_dirrequire write/search access;system_dirrequires read/search access; every ancestor must be searchable.Failures report the required access and the relevant identity/mount guidance.
Metadata repair remains strict for the package-owned
/etc/nes-emulatortree.The init script and the companion
luci-app-nes-emulatorRPCD bridgecoordinate authentication-token updates through
/var/run/nes-emulator/auth.token.lock. The shared runtime directory isvalidated as root-owned mode
0700; lock files must be root-owned regularsingle-link files with mode
0600. Files are created with no-clobber semantics,opened catchably without truncation, and revalidated after opening.
The RPCD phase of token rotation permits at most seven one-second lock-wait
sleeps: two waits before the pre-dispatch token check, two inside rotation, and
three for the startup lock. A contended native restart can add at most nine
native token-lock sleeps, so the coordinated lock-wait bound is 16 rather than
seven. The companion LuCI view gives the complete non-cancellable operation a
120-second RPC budget.
This native package is the dependency of the separate LuCI submission.
🧪 Run Testing Details
aarch64_cortex-a53)62ec2dff9; LuCI038ad57ad8The complete native/LuCI pair was tested on the router using native PR head
b4424df4and LuCI PR head5028c07a. Package installation and dependencyresolution, service control, ROM upload/loading, video, audio, controller
input, save/load state, and the FPS OSD all worked on the device.
The hardware-tested emulator and FCEUmm sources remain unchanged. Current heads
add the review-driven lifecycle, external-storage, protected-lock, bounded-wait,
isolated/unbatched long-RPC, and complete file-descriptor fixes without changing
the native binary source.
The source revision exporting these heads is covered by the successful
project CI run.
That run includes:
scripts/check.shsuiteeffective-access outcomes
precedence
catchable opens, bounded waits, and rotation/start race contracts
lock-fd inheritance contracts
black-box regression suite
The recipe and unchanged native binary were also validated with current x86/64
and armsr/armv8 SDKs:
make package/feeds/packages/nes-emulator/refresh V=swas run twice; bothruns left the patch series byte-identical
git amto the pinned FCEUmm treeinspected
nesd --versionreports1.0.0BIND_NOW, stack protection, and non-executable stacks
✅ Formalities
If your PR contains a patch:
git am