Skip to content

CDVD: Fix possible uncaught exception in CheckDiskTypeFS#4853

Merged
refractionpcsx2 merged 1 commit intoPCSX2:masterfrom
stenzek:cdvd-rootdir
Oct 1, 2021
Merged

CDVD: Fix possible uncaught exception in CheckDiskTypeFS#4853
refractionpcsx2 merged 1 commit intoPCSX2:masterfrom
stenzek:cdvd-rootdir

Conversation

@stenzek
Copy link
Copy Markdown
Contributor

@stenzek stenzek commented Oct 1, 2021

Description of Changes

I noticed this one when I had reads broken - if you manage to load an invalid ISO image, it'll throw this exception on the CPU thread.

Also replaced an explicit unique_ptr construction with make_unique.

Rationale behind Changes

This shouldn't be an exception in the first place, but at least now it doesn't kill the CPU thread/SIGABRT.

Suggested Testing Steps

None really needed, since it's an exceptional (hah) scenario. I guess you could try loading a disc image without a valid ISO structure.

@github-actions github-actions bot added the CDVD label Oct 1, 2021
@refractionpcsx2
Copy link
Copy Markdown
Member

refractionpcsx2 commented Oct 1, 2021

So on master it does this:
image

On this PR it just says this in the log then continues as if there's no disc

Open virtual disk tray
Path: [Unnamed or unknown]
ISO mounting failed: PCSX2 is unable to identify the ISO image type.

actually that was a different rubbish file, the exact same file on your PR actually does this

Open virtual disk tray
OK: Gzip quick access index read from disk: 'G:\PeopsSpu109.tar.gz.pindex.tmp'
isoFile open ok: G:\PeopsSpu109.tar.gz
Image type = Audio CD
blocks = 330
offset = 0
blocksize = 2448
blockoffset = 0
isoFile: Invalid layer0 Primary Volume Descriptor

  • CDVD Disk Open: DVD, Single layer or unknown:
    • Track 1: Data (Mode 1) (330 sectors)

Then tries to boot and gets the RSOD

@stenzek
Copy link
Copy Markdown
Contributor Author

stenzek commented Oct 1, 2021

Right, the goal was to allow it to boot to the RSOD. In my Qt branch, since I don't have any of the cross-thread exception nonsense, it just caused it to abort() before this change.

@refractionpcsx2
Copy link
Copy Markdown
Member

refractionpcsx2 commented Oct 1, 2021

If that was intended, it's fine. But from a user perspective, they have no idea that their ISO is bad now, they might just think it's region protection or something, so it would be nice to get some sort of user feedback, beyond the RSOD :P

If we can't then no worries, it's more of a nice to have.

@stenzek
Copy link
Copy Markdown
Contributor Author

stenzek commented Oct 1, 2021

Relying on the ISO directory being missing alone isn't a great indicator of that, imo.

I'd suggest displaying an error if either the directory or system.cnf can't be found, instead of just "isofilesystem file not found" (which isn't very descriptive).

Edit: This probably should be done on disc load as well, rather than EE module load. The current setup of throwing exceptions inside rec blocks is... very nasty anyway, especially when register caching is involved and you can't safely flush the state. I've got rid of them from the rest of CDVD as well.

As for user feedback, my plan was to have a function which can be called from the CPU thread to "display a modal message" on the UI thread. Which would be perfect for here. None of the UI->CPU thread calls are blocking, so there's no risk of deadlocks.

@refractionpcsx2
Copy link
Copy Markdown
Member

refractionpcsx2 commented Oct 1, 2021

Okay, well if it's something that can be revisited later to be a bit more user friendly, then that's fine :) Like if all of your try/catches fail it just says "Look mate, your ISO sucks, try another one or remake it" in a polite fashion.

If that's the plan then this LGTM for now!

Comment thread pcsx2/CDVD/CDVDaccess.cpp Outdated
@refractionpcsx2 refractionpcsx2 merged commit 97b94ac into PCSX2:master Oct 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants