Mount reliability, logging, FTP and MDS fixes - #239
Merged
Conversation
iTechMedic
force-pushed
the
trimmed-fixes
branch
6 times, most recently
from
July 30, 2026 02:39
a9d888e to
e914b6b
Compare
A mount is queued and finishes long after the web request was answered "ok", so a failure had no way back to the user: the page said nothing and the reason only reached the log. The service now records why the last attempt failed and the image-name endpoint the page already polls carries it back.
Every loader returns nullptr on failure, so the reason it gave never left the log. The loaders now record it and the web UI shows it, which turns "it did not mount" into something a user can act on. Cue sheets are listed whether or not a same-stem .bin sits beside them: hiding them made a rip that needs attention look like it was never there.
The rule was "list a .bin unless a same-stem .cue exists", which is exactly backwards. Mounting a .bin rewrites its extension and reads the .cue first, so a .bin whose cue is missing cannot be mounted at all - and that is precisely the set the browser was offering. The ones that could be mounted, through their cue, were the ones it hid. Nobody noticed while stems matched, because the cue was listed alongside and the hidden .bin was the right thing to hide. A split-track rip broke the symmetry: "Game.cue" against "Game (Track 1).bin" share no stem, so the browser filled up with track files, every one of them a dead end. A .bin is now never listed. Either its cue is there, and that cue represents the disc, or it is not, and there is nothing to mount. Drops siblingWithExtExists(), which has no callers left. (cherry picked from commit da3e0a2)
David mounted a split-track cue, got the refusal banner, and the page went on to say "Current File Loaded: Alien Trilogy (USA).cue" with that same file marked (Current) in the list. The host had never been given it - SetDevice() is not reached on a failed load - so the UI was reporting a disc that does not exist. Three causes, all of which the new error banner made visible rather than created. m_CurrentImagePath was filled in before the load was attempted and never put back when it failed, so a refused image immediately became the "current" path and its folder the current folder. current_cd is an index into the file list, and RefreshCache rebuilds that list without revisiting it. Any rescan that changes the ordering silently repoints it at a different file - and hiding .bin files, one commit ago, shifted every index on a card with cue/bin pairs. The mounted disc is now remembered by path and the index re-derived from it after each scan, which also means the UI reports nothing as current when the mounted file has genuinely gone away. The pick-something-to-mount fallback chose the first image in the list with no memory of what had just failed, so on a card whose only image is unmountable it retried the same one on every upload, delete and FTP change, re-raising the banner each time. Also widens the error buffer: the split-track message was being cut off mid-word at 160 characters. Re-deriving the index has a consequence that has to be handled in the same breath. current_cd is an int that is legitimately -1 when nothing is mounted; GetCurrentCD() returns it as size_t, so -1 becomes SIZE_MAX and GetName() answers an out-of-range index with nullptr. Both callers used that pointer where null is undefined - pagehandlerbase constructs a std::string from it on every page the web server serves, and the image-name API hands it to nlohmann's JSON. That was survivable only while current_cd was set once at the first successful mount and never cleared, so outside a narrow window at boot it was always valid. Re-deriving it makes "nothing is mounted" a state the UI can actually reach: an ejected drive that has not mounted anything this session, or a mounted file that is no longer in the list. GetCurrentCDName() now returns "" and both callers are guarded. Without this the device froze hard enough to need a power cycle after swapping to and from an image that refuses to mount. (cherry picked from commit 7d0934d)
A user reported a Pi that had gone slow and had no log file. His log path was "0:/SD:/usbode-logs.txt" - which the web form built for him out of "SD:/usbode-logs.txt", by pasting "0:/" on the front of anything that did not already start with it. That path cannot be opened, and everything that follows from a failed open was wrong. The cost. Run() backed off 20 ms whenever a message failed to reach the file, which is right for a write that might succeed next time and useless when there is no file at all: every message failed, so the log queue retired 50 events a second, on a scheduler task, and dragged the rest of the system along with it. The daemon now distinguishes a transient write failure from having no file to write to, and only pays for the first. The hazard. m_bFileInitialized had no initializer and was not in the constructor's init list, so on the failing path it held whatever was in that memory. Non-zero meant LogMessage() wrote to an unopened FIL and the destructor closed one. The silence. Initialize()'s result was discarded by the constructor, which was itself discarded by the caller, and the message it did log named neither the path nor the reason. Boot now says which file it could not open, with the FatFs error, and where to go and fix it. The path itself was borrowed rather than copied, from the config store, which is free to replace it while the daemon is still running - and does, when the log path is edited from the web UI. Validation moves to where the bad value came from. The form now strips whatever volume the user typed, refuses one that is not the boot partition (only 0: is mounted when the daemon starts), refuses a directory or a leftover colon, and checks that the parent directory exists - FatFs will not create one, so a path under a missing directory is accepted and then fails at every boot with nothing to show for it. An empty value now means "off", which the page already displayed but the write path ignored, so the setting could not be cleared. The daemon is now compiled into the host test suite, which needed a real event queue on the logger stub, a sleep counter on the scheduler stub, and write support in the FatFs shim. Four of the five fixes are pinned by a test that goes red when the fix is reverted; the uninitialised flag is a bool, so reading a poisoned one is undefined and no assertion can pin it - the sanitizer build names all three read sites instead, and the README now says so. Also drops an unused LOG_FILE define in kernel.cpp that named a different file from the actual default. (cherry picked from commit e54ecce)
The help text still promised the old behaviour - that whatever you type gets 0:/ pasted on the front - which is exactly how the reported bad value was produced. It now says what is actually accepted. (cherry picked from commit a846639)
Found by David testing the reported bad path on hardware: he set logfile=0:/SD:/usbode-logs.txt, rebooted, and "didn't see any warning". The warning was emitted - to the serial console, which is the only target a SCREEN_HEADLESS build has. So the config page went on presenting a log path that was doing nothing, which is the situation this whole fix exists to end. The config page now shows the daemon's live status next to the path, in red when the file could not be opened, naming the path and the FatFs error. The log viewer had two problems of its own. It opened a hardcoded "/usbode-logs.txt" regardless of what was configured, so anyone who set a different path got a blank page and no hint they were looking in the wrong place; it now reads the configured file. And a missing log file rendered exactly like an empty one - nothing at all - so it now says why there is nothing to show. Both take the text from the daemon rather than working it out for themselves, so the two pages cannot drift apart or from what the daemon actually did. (cherry picked from commit 20d12cb)
…inter
Seek() decided it was already in position by comparing Tell() against the
offset it was asked for. Those are not the same kind of number: Tell() is a
byte offset into the MDF, the argument is an address on the disc. On an image
with 2448-byte sectors they run at different rates, and on one with an
unstored pregap the disc address moves where the file offset does not.
When they did coincide the function returned early, and the early return
skipped recording the LBA. Read() takes both its gap detection and its
subchannel stride from that LBA, so it went on serving whichever frame the
reader was last on - and reported success doing it.
Two ways in, both ordinary:
* a contiguous 2352-byte image makes the two offsets equal at every frame,
so reading a track to its end and then reading on lands in the pregap
with a stale LBA, the hole goes undetected, and the next track's bytes
come back where zeros belong. This is the Video CD case: on the real
SVIDEOCD image, LBA 526 returned track 2's volume descriptor.
* a 2448-byte image makes them equal every 49th frame, since 49 * 2448 is
51 * 2352. Ending a read at frame 48 and then reading frame 51 served
frame 49.
Compare against the file offset actually computed, and set the LBA before any
early exit can skip it. The plain read path now advances the position like the
other two already did, so the class keeps one definition of where it is.
Two smaller holes closed alongside, both reachable through the same gap:
a read shorter than one frame skipped the gap check on its size alone, and
ReadSubchannel() failed outright on a frame Seek() and Read() were both
willing to answer with zeros.
Gap detection was also O(frames x tracks) on every read - a full track-table
walk per frame - and most images have no hole at all. Init() now settles that
once by comparing the summed track lengths against the disc length, which
makes the common path cheaper than it was before this change.
Four tests, one per fix, each verified by putting the bug back and watching
that test go red. Checked against real Alcohol images too (DESCENT_II,
SVIDEOCD, NFSSEBBC): the sparse ones take the walk path, the single-track one
does not, and the pre-fix reader fails the pregap read on SVIDEOCD itself.
(cherry picked from commit 16abff1)
The audio control commands have no data phase, but several left m_nnumber_blocks set: PLAY AUDIO (10)/(12) parked the track length there, and SEEK, PAUSE/RESUME, STOP and PLAY AUDIO MSF left whatever a read in flight had put there. onXferCmplt reads that counter to decide whether more data is owed or the CSW should go out, which is why every other data-returning handler zeroes it. Left set, the next command that returns data streams raw sectors where its status belongs: a MECHANISM STATUS asking for 8 bytes came back with 1024008, the endpoint stalled, and a host waiting on that transfer hung until the drive was unplugged. Skipping tracks quickly is what makes it likely. The test table-drives all six opcodes with a read deliberately left pending. (cherry picked from commit 7d01853)
Some .mds files carry no track lengths and the frame count falls back to the MDF size. That can only happen when every length is zero, which also means no frame counts as stored and FindTrackForLBA never matches, so the sparse check declared the whole disc a hole and served zeros instead of the image. Seek had the same blind spot: with no track containing the LBA it took the pregap path and left the file pointer alone, so only a sequential read worked. These images now map LBA straight onto the MDF. Coverage is measured with the track ranges merged rather than summed. An overlap was counted twice, so an overlap the size of a real hole added up to a full disc and turned gap-aware reads back off. Both found by CodeRabbit. (cherry picked from commit 9232280)
f_write reports a full volume as FR_OK with a short byte count, and f_sync can fail after it, so checking only the FRESULT called a lost entry written. That alone would have reintroduced the collapse this daemon exists to avoid: a card that stays full fails every write, and backing off 20 ms per event is the same starvation an unopenable path used to cause. A run of failures now gives up on the file and says so in the web UI. Short write and failed sync found by CodeRabbit. (cherry picked from commit 5fd1fab)
A data file that would not open was always called missing, sending the user after a file that was sitting right there. Only FR_NO_FILE and FR_NO_PATH mean missing now; anything else says it would not open and carries the FatFs code. The config page also dropped a requested reboot silently when the log path was rejected. It now says the reboot was cancelled, which keeps the error on screen long enough to read. Both found by CodeRabbit.
FA_CREATE_NEW opened an existing file through "r+b" instead of returning FR_EXIST, and f_write never maintained obj.objsize, leaving f_size stale after an append. The seam now injects the two ways a full card presents, FR_OK with a short count and a failing f_sync, which is what the log daemon's write checks are tested against. Both shim defects found by CodeRabbit.
iTechMedic
force-pushed
the
trimmed-fixes
branch
from
July 30, 2026 14:52
e914b6b to
632e6cf
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This collects the work from yesterday into a single branch instead of six. It is 13 commits on top of main (558bf4e), rebased clean with no merge commits. Tested on hardware: ran on a Pi yesterday, stable, no lockups.
Mounting and cue handling
A cue sheet that splits its tracks across several files used to mount anyway, and the drive would come up with the wrong disc in it. It is now refused outright. Along with that, the mount path stopped reporting success when the image had actually failed to load, and it now says what the mount did rather than just that the filename was found. When an image cannot be mounted, the reason is shown to the user instead of being swallowed. Saved images that have gone missing now leave the drive empty rather than quietly substituting a different disc, and an ejected drive no longer puts a disc back in by itself. The file browser also stopped listing .bin files, which were never mountable on their own.
Logging
A bad log file path could take the whole system down with it. It cannot now. The web UI shows whether file logging is actually working, and the config page describes the path rule that the form enforces.
FTP
Deleting, renaming or overwriting the currently mounted image is now refused.
MDS
The reader tracks its read position directly instead of inferring it from the file pointer.
Hot path
Removed the log line that fired on every SCSI read.
Tests
The host regression suite passes at 162 tests, 0 failures, including two new groups added here: FTP path matching, and file log daemon.