-
-
Notifications
You must be signed in to change notification settings - Fork 156
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use native file I/O in local and overlay drive #3765
Conversation
Is there a case for making these file i/o wrappers more generic so that other parts of the code could use them later. I'm talking about seek, read, write etc. Other code that could use this would be the disk image code, in which case large file support would definitely be appreciated. |
They're already fairly generic and should be fine to use elsewhere. Some of the calls (especially create and open) may need additional flags depending on the use-case but I think we can cross that bridge when we come to it. Maybe I should rename the functions to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks OK. I have no remarks.
So far tried some Windows 3.1 software, Colonization game. and MkS_Vir virus scanner, didn't find any problems/
Previously, this was left to the individual DOS_Drive classes to check. Move this to common code and set the correct error. This matches what FreeDOS does.
Also remove flushing with ftell() + fseek() Should not be needed with native I/O
Thanks for testing @FeralChild64 I pushed changes addressing the comments above. Mostly re-naming things and fixing formatting. I also changed to They should be suitable for use where-ever else in the code they might be needed. I renamed them as I said above to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is solid step in the right direction @weirddan455 🎉 Looks very good now, left one last comment about testing Blackthorne if you have the capacity. Great work!
I'll let you take a look at my last commit if you like @johnnovak addressing the Blackthrone issue. |
Added a Blackthorn never hits this case. Using unsigned integer lets it succeed. I wanted this warning to be visible to end users rather than a debug log because it seems to be rare and I want users to put this in a bug report if my assumption proves to be incorrect. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Outstanding work @weirddan455, I'm glad you've managed to remove the Blackthorne hack as well 🚀 I agree with your last note about logging a warning, that's the best we can do.
This removes a hack needed by Blackthrone See comments for in-depth explanation
Description
This PR replaces the use of C library I/O (
fopen()
and friends) with native file I/O. Windows uses Win32 API calls and everyone else uses POSIX.I was also able to remove some hacky
ftell()
+fseek()
nonsense that should not be needed since there's no library buffering. The OS itself will still buffer but that should be fine. All we care about is that a write followed by a read will return the new data.I have some more work on the file I/O planned to better handle timestamps (which will depend on native file I/O) but I thought it would be best to put that in a separate PR and get this in first.
Manual testing
Tested on Windows (both MSYS2/GCC and MSVC/Clang) and Linux. Crystal Caves and Mortal Kombat saves work which had been problematic on Windows in the past.
Other games with save state tested: Secret of Monkey Island 2, Quake, Doom, Space Quest 1
Checklist
Please tick the items as you have addressed them. Don't remove items; leave the ones that are not applicable unchecked.
I have: