Refuse split-track cue sheets, and report what a mount actually did - #232
Closed
iTechMedic wants to merge 5 commits into
Closed
Refuse split-track cue sheets, and report what a mount actually did#232iTechMedic wants to merge 5 commits into
iTechMedic wants to merge 5 commits into
Conversation
A cue sheet that names one .bin per track cannot work here. The loader picks its data file by rewriting the cue's own extension, so it only ever opens the first one, and the parser cannot place a later file's tracks without knowing how long the earlier files are - every firmware caller uses the no-argument next_track(), which passes a previous size of zero. The arithmetic was bounded earlier so those tracks no longer land at LBAs in the billions. That stopped the nonsense but not the wrongness: the disc still mounted, with a TOC that put the wrong tracks in the wrong places. Refusing the image says what is actually true. Detection lives in cueutil.cpp and runs the real parser rather than scanning for the word FILE, so quoting, case and REM lines are read exactly as they will be when the sheet is used - a disc called "MY FILE (1996).bin" is not a split rip, and a rem'd-out FILE line is not a second file. It also lives there rather than in util.cpp so it can be tested: util.cpp is deliberately outside the host suite. Refusing is only an improvement if the user finds out, and until now they could not. Mounting is asynchronous: SetNextCDByName() queues an index and returns, so the web UI's "ok" only means the name was in the catalog. When the load failed afterwards, the old disc stayed mounted, current_cd went on pointing at it, and the sole record was one line in a log file the user has no reason to read - an unmountable image looked exactly like a mountable one. The service now keeps the reason for the last failed mount and the image-name endpoint the page already polls reports it.
…lit cues David put a split-track rip on the card and the browser offered him the individual track .bin files while the cue sheet itself was nowhere to be seen. Three separate things were wrong. The scanner listed a .cue only when a same-stem .bin sat next to it. That rule was written to keep a cue whose data file is missing out of the way, but a split-track rip is exactly the case where the stems differ - "Game.cue" against "Game (Track 1).bin" - so the disc vanished from the browser and its raw tracks were offered instead. Every cue is now listed; one that cannot be mounted says why when you try, which is more use than not being there. Nothing displayed the mount error. The previous commit gave SCSITBService a reason and put it in the /api/imagename JSON, which no page consumes - so the information existed and no user could reach it. Same mistake as reporting a bad log path only to the serial console. It is now a banner on every page, from the shared page handler. And the reason was generic. The loaders all report failure the same way, by returning nullptr, so "unsupported or damaged image" was the best the UI could say about a split-track cue, a missing .bin, a bad .mds and an unknown extension alike. They now record what actually went wrong and the mount error repeats it: a split rip says it is a split rip and what to do about it.
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.
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.
The web UI announced "Successfully mounted: Alien Trilogy (USA).cue" and then
the very next page said the image needed re-ripping. Both were generated from
the same request. Mounting is asynchronous - SetNextCDByName() hands an index
to the service task and returns as soon as it has found the name in the
catalogue - so the mount page was reporting that the file exists, in the words
of a successful mount.
SCSITBService now counts the requests its Run() loop retires, so a caller can
wait for a specific one and find out what happened to it. MountByNameAndWait()
does that and answers Success, Failed, NotFound or Timeout; the timeout is
deliberately not called a failure, because a large CHD on a slow card takes a
while and guessing wrong there would be worse than saying "still loading".
The mount page now shows the refusal and its reason and stays put instead of
bouncing to the homepage, and the mount API returns the reason rather than
{"status":"ok"}.
The HAT had the same problem in a worse form: it jumped to the homepage on any
key press, so a disc that refused to load was indistinguishable from one the
user had changed their mind about - the previously mounted image simply
reappeared as current. It now waits, stays on the image list when the mount
fails, and says so in the page title. Short, because that is all an OLED header
holds; the reason is in the web UI and the log.
The HAT cannot wait the obvious way, and this is the one thing to be careful of
here. OnButtonPress does not run in task context on every display:
PageManager::HandleButtonPress says so in a comment, and sh1106/display.cpp:299
and st7789/display.cpp:425 both call it straight from the GPIO interrupt
handler. MountByNameAndWait() sleeps on the scheduler, so calling it there puts
a task switch inside an interrupt handler and freezes the Pi hard enough to
need a power cycle - on every mount, whatever the image, success or failure.
ssd1306 escapes only because it already defers presses to ProcessPendingInput();
its own comment states the rule: the handler must do quick, lock-free work only.
So the button handler only queues the request with SetNextCDByName() and
remembers the retired-request counter, and ResolvePendingMount(), called from
Refresh() in task context, waits for that counter to move and applies the
outcome. Done on all three drivers rather than the two that would crash: on
ssd1306 the wait was safe but still blocked the display loop for up to eight
seconds, stalling the screen and the backlight timeout. MountByNameAndWait() is
documented as task-context-only, naming the two handlers that are not.
The vendor toolbox picker is deliberately left asynchronous. It runs inside a
SCSI command handler, where sleeping is not acceptable, and DOS has nowhere to
show a result anyway.
Collaborator
Author
|
Superseded by #238, which combines this with the rest of the batch into a single branch. Same commits, no changes dropped. Closing this one. |
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
1 participant
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.
Five commits, each standalone. Everything here is confirmed on hardware (Pi Zero 2 W with an ST7789 HAT) against Win98, XP and Windows 11.
What was wrong
A cue sheet with one FILE per track was accepted and mounted. Every call site passes
prev_file_size == 0, so each new FILE's tracks stack onto the previous track'sdata_start, and the loader ignores FILE names entirely: it rewrites the cue's extension and opens<cuename>.bin. The disc mounted and served wrong data.Refusing it was the easy half. Making the refusal visible took the other four commits, because a failed mount had nowhere to surface.
The commits
CueHasMultipleFiles()reuses the real CUEParser rather than hand rolling a second scanner. It lives in cueutil.cpp because util.cpp is excluded from the host test suite.m_CurrentImagePathwas written before the load was attempted, andcurrent_cdwas a raw index into a list RefreshCache rebuilds. Hiding .bin files in commit 3 shifted every index on a card with cue/bin pairs.Worth a careful look
Commit 5 contains a trap that cost me a hard lockup before I understood it.
OnButtonPressdoes not run in task context on every display: sh1106 and st7789 callPageManager::HandleButtonPressstraight from the GPIO interrupt handler, so anything that sleeps there puts a task switch inside an interrupt and freezes the Pi hard enough to need a power cycle, on every mount, whatever the image. ssd1306 escapes only because it already defers presses toProcessPendingInput. The button handler therefore only queues the request, and the outcome is collected fromRefresh()in task context. The commit message spells this out, andMountByNameAndWait()is documented as task context only.CI
Rebased onto 558bf4e, so this sits on top of the new QEMU boot test. I checked the branch against validate_boot_log.py by reading it rather than running it, since I do not have QEMU on this machine. The only line this branch adds that matches the SUSPICIOUS pattern is the split-track refusal, which fires solely when such a cue is mounted and so cannot appear on a virgin card boot. Nothing here touches the gadget init path, and the deferred init that the boot test depends on is untouched.
Host suite is 148/148 on this branch.
Follow up
A second branch is stacked on this one,
missing-image-empty-drive, which makes a saved image that has gone missing come up as an empty drive instead of silently mounting whichever file sorts first. I will open that separately once this lands rather than have two stacked PRs open at once.