BACKENDS: FS: Use stat() fallback for all unexpected dirent d_type values#6505
Merged
sev- merged 2 commits intoscummvm:masterfrom Mar 26, 2025
Merged
Conversation
…lumes getChildren() may have succeeded but found no children at all, for some reason. Properly print a warning in this case as well.
bluegr
reviewed
Mar 24, 2025
Contributor
Author
|
by the way, it looks like we do this in current scummvm/backends/fs/posix/posix-fs.cpp Lines 70 to 75 in 38af188 i.e. a file is marked as but when we rely on the scummvm/backends/fs/posix/posix-fs.cpp Lines 195 to 209 in 38af188 I can't say whether this discrepancy is intended. It looks like it's been this way for years. |
dwatteau
force-pushed
the
fix/backends-fs-posix-d_type-stat-fallback
branch
from
2bc31fc to
4937f17
Compare
…lues On systems where dirent provides a `d_type` field, we currently fall back to stat() only for DT_UNKNOWN values. Do so for all unexpected `d_type` values instead (that is, anything different from DT_DIR, DT_REG and DT_LNK). This is because there's no guarantee that `d_type` will be meaningful for all OSes and filesystems. One such example is macOS Tiger, where `d_type` will hold bogus values for the .aiff files of cddafs mount points (as triggered by MacOSXAudioCDManager). (This bug appears to have been fixed in cddafs-242.0.1, around the Snow Leopard area.) When using stat() over the same files, the proper file type is returned, though. Hence the need for the stat() fallback to be triggered in more cases than DT_UNKNOWN. This fixes Indy3 FM-TOWNS music being silent on macOS Tiger, when playing from the original CD.
dwatteau
force-pushed
the
fix/backends-fs-posix-d_type-stat-fallback
branch
from
4937f17 to
49d5e88
Compare
Member
|
Thank you, makes sense. |
dwatteau
added a commit
that referenced
this pull request
Apr 29, 2025
…< 10.6.2 This fixes Indy3 FM-TOWNS music being silent on macOS Tiger, when playing from the original CD. A more comprehensive fix was added in GH-6505 PR for the 2.10.0 release, but for bugfix 2.9.1 I've reworked it so that a more minimal fix is used, only targeting the impacted old macOS releases.
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
3 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.
Problem
On OSX Tiger, playing Indy3 FM-TOWNS from its original CD (which has both an ISO-9660 layer and a Redbook layer) produced no music.
Explanation
It looks like that's because we rely on dirent's
d_type(on systems where it's available). Thing is, this field is not guaranteed to be meaningful for all filesystems (even on Linux; as seen in itsreaddir(3)manual page).In OSX Tiger in particular, dirent returns completely bogus
d_typevalues for the files being part of the CDDAFS volumes (used by MacOSXAudioCDManager). I'd getDT_CHRfor the AIFF files, and3(which has noDT_*value name!) for the TOC file. (It looks like this is because theircddafsmodule, up until cddafs-240.0.9 in 2008, uses improper constants ford_type. This was fixed in cddafs-242.0.1, apparently part of macOS 10.6.2.).The
posix-fs.cppcode would then flag all the.aifffiles as invalid, and then MacOSXAudioCDManager had nothing left to play.If you
stat()those files, though, you do get proper file type values. But the currentposix-fs.cppcode only falls back tostat()forDT_UNKNOWNvalues.Suggested fix
Fall back to
stat()for all unexpected values (that is, anything different from:DT_DIR,DT_REG,DT_LNK), and not justDT_UNKNOWN.This makes our
d_typeusage a bit more robust on more limited implementations. It also effectively unbreaks Indy3 TOWNS on OSX Tiger.Still, this code is shared by many things, so we may need a careful review of the possible side effects of this change.
EDIT: FreeBSD also documents this
d_typebehavior ([1],[2]) for some filesystems.