Come up empty when the saved image is gone, instead of a different disc - #237
Closed
iTechMedic wants to merge 7 commits into
Closed
Come up empty when the saved image is gone, instead of a different disc#237iTechMedic wants to merge 7 commits into
iTechMedic wants to merge 7 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.
…rent disc An image that had been renamed, deleted, or left behind on another card was silently replaced by whichever file sorted first. The drive came up holding a game the user had never chosen, with nothing anywhere to say why, and the only clue was a log line nobody reads. Asked directly whether a missing image now gives an empty drive, the honest answer was no. RefreshCache now adopts a stand-in but presents the drive EMPTY, and records which image went missing so every web page can say so. The stand-in is still loaded behind the scenes because the gadget needs a geometry, and because it makes Insert instant if the user does want a disc. Three things this has to get right: - A fresh card must still auto-mount. GetCurrentImage() answers with "image.iso" whether the key is missing or genuinely set to that, so it cannot tell a remembered image from a card that has never mounted anything. Asking with an empty default can, and only a genuinely remembered image triggers the empty drive. - The stand-in must not be saved as the user's image. Recording it would lose the missing name, so the explanation would vanish on the next boot and putting the file back would no longer bring it up. - Adopting while ejected must stay ejected, and must still happen. SetDevice() clears the ejected latch unless the boot-eject is armed, so the arm is not optional here. And the fallback's !IsEjected() guard had to gain an exception for a gadget that has never come up: adopting is the only thing that initializes it, so refusing would have left the host seeing no USB device at all rather than an empty drive - reachable on the second boot after this code had itself come up empty and persisted the eject. An empty images partition still adopts nothing and still leaves the gadget uninitialized, which the QEMU boot test in tests/qemu-boot relies on: it boots a virgin card with the images partition swept, so m_FileCount is 0, the fallback never runs, and SetDevice() is never reached. CDROMService gained IsGadgetInitialized() to make that condition testable rather than inferred, and the deferred init is now commented as load-bearing rather than looking like an optimization someone could helpfully remove - naming the three symptoms validate_boot_log.py treats as FORBIDDEN_ANYWHERE, so the next person to touch it knows what depends on it.
The empty-drive behaviour for a missing image worked on a reboot and silently did nothing on a running system. Renaming the mounted image over FTP left a different game mounted and readable, with no sign the saved one had gone. SetDevice() arms the disc-swap sequence whenever one device replaces another, and Update() drove NO_MEDIUM -> UNIT_ATTENTION without ever consulting the ejected latch. So adopting a stand-in put it in the drive 100 ms later. At boot m_pDevice is null, no swap is armed, and that transition never runs - which is exactly why the boot restore looked correct and hid this for as long as the boot restore has existed. The gadget was internally inconsistent while it happened: IsEjected() kept returning true the whole time it was serving READ(10) from the disc it had supposedly ejected. Update() now refuses the transition while ejected. Insert() is unaffected and still works - the drive is empty, not disabled. Test adopting_an_image_while_ejected_stays_ejected pins it, and fails without the fix on all three of the sense key, the additional sense code, and the READ(10) status. Also: the file list no longer names the stand-in as the current image. It is what the gadget holds open, not what the user mounted, and calling it "current" while the drive reads as empty is what made a vanished image look like an ordinary disc swap. m_CurrentImagePath still points at it so the FTP and web delete guards keep protecting it.
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.
Draft, stacked on #232. Do not merge this before #232. It builds on that branch's mount error plumbing, so until #232 lands the diff below also contains its five commits. The two that belong to this PR are:
mount: come up empty when the saved image is gone, instead of a different discgadget: an ejected drive must not put a disc in by itselfOnce #232 merges the diff collapses to just those two and I will take this out of draft.
What was wrong
An image that had been renamed, deleted, or left behind on another card was silently replaced by whichever file sorted first. The drive came up holding a game the user had never chosen, with nothing anywhere to say why, and the only clue was a log line nobody reads. A user asked directly whether a missing image gives an empty drive, and the honest answer was no.
RefreshCachenow adopts a stand in but presents the drive empty, and records which image went missing so every web page can say so. The stand in is still loaded behind the scenes because the gadget needs a geometry, and because it makes Insert instant if the user does want a disc.Three things this had to get right
A fresh card must still auto mount.
GetCurrentImage()answers withimage.isowhether the key is missing or genuinely set to that, so it cannot tell a remembered image from a card that has never mounted anything. Asking with an empty default can, and only a genuinely remembered image triggers the empty drive. Without this, first boot on a new card would come up empty and look broken.The stand in must not be saved as the user's image. Recording it would lose the missing name, so the explanation would vanish on the next boot and putting the file back would no longer bring it up.
Adopting while ejected must stay ejected, and must still happen.
SetDevice()clears the ejected latch unless the boot eject is armed, so arming is not optional here. And the fallback's!IsEjected()guard needed an exception for a gadget that has never come up, because adopting is the only thing that initialises it. Refusing there would have left the host seeing no USB device at all rather than an empty drive, which is reachable on the second boot after this code has itself come up empty and persisted the eject.The second commit is a real bug, and it predates this work
The empty drive behaviour worked on a reboot and did nothing at all on a running system. Renaming the mounted image over FTP left a different game mounted and readable.
SetDevice()arms the disc swap sequence whenever one device replaces another, andUpdate()drove NO_MEDIUM to UNIT_ATTENTION without ever consulting the ejected latch, so adopting a stand in put it in the drive 100 ms later. At bootm_pDeviceis null, no swap is armed, and that transition never runs, which is why the existing boot restore looked correct and hid this for as long as the boot restore has existed. The gadget was internally inconsistent while it happened:IsEjected()kept returning true the whole time it was serving READ(10) from the disc it had supposedly ejected.adopting_an_image_while_ejected_stays_ejectedpins it, and fails without the fix on all three of the sense key, the additional sense code and the READ(10) status.Interaction with the QEMU boot test
None, and deliberately so. An empty images partition still adopts nothing, because the fallback is gated on
m_FileCount > 0, soSetDevice()is never reached and the gadget is never initialised.CDROMServicegainedIsGadgetInitialized()to make that condition a real query rather than something inferred, and the deferred init is now commented as load bearing rather than looking like an optimisation someone could helpfully remove, naming the three symptomsvalidate_boot_log.pytreats asFORBIDDEN_ANYWHERE.Hardware tested: mount an image, power off, delete or rename that file, boot again. The drive appears in the OS and is empty, and every web page carries the reason. Mounting anything deliberately clears it. Second boot without mounting anything still comes up empty, which is a distinct code path.
Host suite 149/149 on this branch.