Update drive_physfs.cpp to support overlay files in place of .zip files

[NebularNerd]

Add a summary of the change(s) brought by this PR here.

Permits overlay contents to be used in place of .zip contents. Replaces missing ; that causes some .zips to not work.

What issue(s) does this PR address?

Per #3535, #3355 and icculus/physfs#18 this fixes contents of overlay directory not superseding the original .zip contents, for example: changing a config file or testing a patch.

This should have uses for many people, adding/removing files from the .zip could risk breaking the game or program or corrupt the file. With this in place you can edit existing files and use the overlay version in place of the .zip original. If it breaks something you can delete the overlay and go back to the stock version. Another use could be be multiple overlays with one base .zip for testing CGA, EGA, TANDY versions without needing to duplicate the base .zip (for NTFS users you can then use folder compression so the overlays take even less space).

There is also the missing ; before -mypos on line 746 discussed in #3535 which breaks .zip functionality with some software. Restoring this fixes a lot of games that did not behave with it removed.

Does this PR introduce new feature(s)?

No

Does this PR introduce any breaking change(s)?

Not that I am aware of, @grapeli and myself have tested it in #3535 and seems to behave as expected.

Additional information

My first Pull Request 😁

[NebularNerd]

@joncampbell123 Following an update from the PhyFS author on #3535 you may wish to block the e7eca5d semicolon 'fix' on this PR for now. It appears that re-adding the ; fixes things by simply bodging something else that's incorrect into working.

The overlay commit 5410de7 should be fine as tests show no side effects.

[joncampbell123]

Just curious, what does the ';' fix?

Note that newer GCC compilers are quite good at pointing out possible problems with if() indention and whether it encloses code.

[NebularNerd]

Just curious, what does the ';' fix?

Note that newer GCC compilers are quite good at pointing out possible problems with if() indention and whether it encloses code.

@joncampbell123 I wish I knew, I'm capable of throwing a working .exe together with some changes and testing but I'm not sure what the ; fixes (or more possibly bodges) elsewhere in the code.

In #3535 it was highlighted that dc798b5 (which points to drive_local.cpp not drive_physfs.cpp, was it split off at some point?) is programatically correct in removing the ; and it's also confirmed by PhyFS author a bit later in the thread.

However doing so prevents many .zip based games from starting or running correctly, once you reinsert the ; a load of games suddenly start working. It's a longish thread now, but you'll see we've been busy testing a few cases and post reinsertion a lot more works, there are still fringe cases but they seem to handle file access in much stranger ways.

If you need specific logs or such, please let me know how you like me to provide them

[joncampbell123]

Looking at the semicolon fix, I can see why.

SEEK_END is supposed to seek relative to the end of the file. That's how it works here on Linux, in Windows, and MS-DOS. The added semicolon makes it work properly by severing the -mypos from the expression (leaving it by itself to do nothing).

lseek(fd,-4,SEEK_END) is supposed to seek to 4 bytes before the end of file (EOF-4), for example.

Perhaps the fix was needed because someone didn't fully understand what SEEK_END is supposed to do.

So the best fix for that change is to remove the -mypos entirely from the SEEK_END case.

See also: https://man7.org/linux/man-pages/man2/lseek.2.html
And also: http://www.ctyme.com/intr/rb-2799.htm

[joncampbell123]

I'll go ahead and merge the pull and remove the now useless extra.

[maron2000]

Sorry if I didn't understand correctly but shouldn't it be like below?
I don't know the rationale of once making mypos beyond file length, and correcting it in the next conditional check.

case DOS_SEEK_END:mypos += PHYSFS_fileLength(fhandle); -mypos; break; // before
case DOS_SEEK_END:mypos = PHYSFS_fileLength(fhandle); break; //after

[NebularNerd]

So the ; was fixing it by causing the -mypos to be ignored. I look forward to testing the next build.

@maron2000 Yes it looks like @joncampbell123 will remove the extraneous code to look like your example.

Update: I've been testing the commit autobuild for this and behaves as expected against my own test build.

[joncampbell123]

Sorry if I didn't understand correctly but shouldn't it be like below? I don't know the rationale of once making mypos beyond file length, and correcting it in the next conditional check.

case DOS_SEEK_END:mypos += PHYSFS_fileLength(fhandle); -mypos; break; // before
case DOS_SEEK_END:mypos = PHYSFS_fileLength(fhandle); break; //after

The file offset in lseek() is a signed integer. You can use SEEK_END with 0 or a negative number to seek relative to the end of the file. A positive offset allows seeking past the end of the file, which does not extend the file unless you then issue a write or use INT 21h to "truncate" out to that position to extend the file.

The after case should be mypos += file size for DOS_SEEK_END.

Note that the traditional offset for lseek() as a 32-bit signed integer is the reason most OSes today have an API that limit access to files larger than 2GB unless the open() call is made to enable >= 2GB, at least on 32-bit. On Linux, 32-bit processes need to use open64() or use a #define to allow access to 2GB or larger files.

Even MS-DOS has this logic. The traditional INT 21h API effectively enforces a 2GB limit for this reason, which is also the file size limit of FAT12/FAT16. FAT32 raises the file size limit to just under 4GB, but you have to use an alternate INT 21h call method to open the file to seek out that far in the DOS environment.