Skip to content
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

Implement File Locking #3687

Merged
merged 6 commits into from
May 26, 2024
Merged

Implement File Locking #3687

merged 6 commits into from
May 26, 2024

Conversation

weirddan455
Copy link
Collaborator

@weirddan455 weirddan455 commented May 21, 2024

Description

I've implemented locking the entire file with sharing bits in DOS_OpenFile and DOS_CreateFile.

There is another (completely independent from sharing bits) method that lock regions of a file. This is done through INT 21 AH = 0x5c. This requires performing a check on DOS_ReadFile and DOS_WriteFile to check if the region a process is attempting to read/write to has been locked by another process.

Many of the games I've tested require Win32s (who's installer does a SHARE.EXE check). This check failed for me on both DOSBox-X and using FAKESHR.COM so we are doing better than them now 😄

Related issues

Fixes #1489

Manual testing

  • 3-D Ultra Pinball
  • Comix Zone
  • Microsoft Flight Simulator 5 - ATC Workshop
  • AFL Finals Fever
  • Mind Grid - Install errors but probably not related to this PR (also producible on main see Mind Grind install errors #3703)
  • Microsoft Word 6.1 for Windows

Checklist

Please tick the items as you have addressed them. Don't remove items; leave the ones that are not applicable unchecked.

I have:

  • followed the project's contributing guidelines and code of conduct.
  • performed a self-review of my code.
  • commented on the particularly hard-to-understand areas of my code.
  • split my work into well-defined, bisectable commits, and I named my commits well.
  • applied the appropriate labels (bug, enhancement, refactoring, documentation, etc.)
  • checked that all my commits can be built.
  • confirmed that my code does not cause performance regressions (e.g., by running the Quake benchmark).
  • added unit tests where applicable to prove the correctness of my code and to avoid future regressions.
  • made corresponding changes to the documentation or the website according to the documentation guidelines.
  • locally verified my website or documentation changes.

@weirddan455 weirddan455 added the enhancement New feature or enhancement of existing features label May 21, 2024
@weirddan455 weirddan455 self-assigned this May 21, 2024
src/dos/dos_files.cpp Outdated Show resolved Hide resolved
src/dos/dos_files.cpp Outdated Show resolved Hide resolved
src/dos/dos_files.cpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@FeralChild64 FeralChild64 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice if the code could log (LOG_WARNING) unsupported sharing modes if the application tries to use them.

@weirddan455
Copy link
Collaborator Author

Partial (region based) file locking is implemented now. This is required by Flight Simulator 5's addon Flight Shop and this now works on this branch.

It would be nice if the code could log (LOG_WARNING) unsupported sharing modes if the application tries to use them.

This supports everything now (as far as I know). Just needs more testing.

image0004

@weirddan455 weirddan455 force-pushed the wd/share branch 2 times, most recently from 27d03a5 to c027467 Compare May 25, 2024 03:55
@weirddan455
Copy link
Collaborator Author

weirddan455 commented May 25, 2024

Fixed a missing region lock check in DOS_LockFile(). You can't lock a region if some other process has already locked it.

Also, not sure if we are handling child process inheritance of file handles correctly. Documentation for the full file lock (as set by sharing bits in DOS_OpenFile()) says that child processes should inherit this sharing if the inherit bit is set.

file-lock

But documentation for region lock says it should not be inherited by child processes. File handle duplication should be handled correctly. That function simply inserts a new reference to an existing DOS_File in the caller's PSP segment. Child processes I am unclear on. Need to test and see if something breaks.

region-lock

@weirddan455 weirddan455 marked this pull request as ready for review May 25, 2024 23:56
@weirddan455
Copy link
Collaborator Author

I've done some testing and updating the top comment with games tested. Some of the games on #1489 I could not find proper images for but I tested what I could find.

It looks like we are respecting the inherit bit, at least for full file locking. Partial file locking I am still unclear on if the locking process creates a child process but I cannot find a fail case in a game so I think we can cross that bridge when we come to it.

This is done and ready for review.

@weirddan455 weirddan455 changed the title Implement File Locking (WIP) Implement File Locking May 26, 2024
src/dos/dos_files.cpp Outdated Show resolved Hide resolved
src/dos/dos_files.cpp Outdated Show resolved Hide resolved
src/dos/dos_files.cpp Outdated Show resolved Hide resolved
src/dos/dos_files.cpp Outdated Show resolved Hide resolved
src/dos/dos_files.cpp Outdated Show resolved Hide resolved
Copy link
Member

@johnnovak johnnovak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still looking good @weirddan455, just a few minor style-related comments.

@johnnovak
Copy link
Member

happens maybe 1 in 4 times you try to launch a DOS program from inside Windows 3.1.

In general, launching DOS programs in Win3.x is the last of our concerns. "Nice to the have", I guess, but should be right at the bottom of our priority list. Just use DOS 🤷🏻

@weirddan455
Copy link
Collaborator Author

In general, launching DOS programs in Win3.x is the last of our concerns. "Nice to the have", I guess, but should be right at the bottom of our priority list. Just use DOS 🤷🏻

True. Everything continues to work in DOS. It also breaks Window's MS-DOS prompt, which is the easiest way to reproduce the regression.

In any case, Word for Windows does work on this branch

image0009

@MasterO2
Copy link
Contributor

MasterO2 commented May 26, 2024

I thought Windows support was out of scope for Staging? At least that's what Staging's web site lists as a non-goal.

@Grounded0
Copy link
Collaborator

Grounded0 commented May 26, 2024

Official position seems to be DOS and Windows 3.x (including Win16 and Win32s) supported.

File locking support is also required by some DOS application software so this also benefits DOS.

@weirddan455
Copy link
Collaborator Author

I thought Windows support was out of scope for Staging? At least that's what Staging's web site lists as a non-goal.

It's Windows 9x specifically that's listed as a non-goal. We're supporting Windows 3.1 stuff.

The flags are a bitfield specifically of 8 bits.
They get set by an 8 bit register inside DOS interrupts.
Nothing ever uses or sets anything wider than 8 bits.
This sets the flags variable that was changed to uint8_t in a previous commit.
This function gets called from DOS_OpenFile which passes it a uint8_t already.
@weirddan455
Copy link
Collaborator Author

weirddan455 commented May 26, 2024

File locking is also required by some DOS application software so this also benefits DOS.

I haven't found any yet but SHARE.EXE has been a part of MS-DOS itself since at least version 4.0 so this is definitely in scope for our DOS implementation.

The big distinction between Windows 3.1 and Windows 9x is that Windows 3.1 installs and runs on top of our internal DOS implementation whereas Windows 9x wants to install its own version of DOS on top of bare metal (or bare hardware emulation).

This means things like the mount command won't work and maybe even localDrive won't work. I haven't tried it myself but I think you would have to create a FAT image of the Windows 9x install and then use the boot command to boot into it (as far as Staging is concerned boot is meant for bootable floppy images of games).

It's not so user friendly to do all that and would require a lot of work to make it so (something like DOSBox-X's GUI).

@weirddan455
Copy link
Collaborator Author

@johnnovak A Windows install would be helpful. I was running into problems with the SB16 drivers myself.

@johnnovak
Copy link
Member

I thought Windows support was out of scope for Staging? At least that's what Staging's web site lists as a non-goal.

Read the about page carefully, man 😄 Everythig you need to know is there 😄
https://www.dosbox-staging.org/about/

@Grounded0
Copy link
Collaborator

Grounded0 commented May 26, 2024

@sduensin

What was the name of DOS software that required file locking support (SHARE.EXE)?

@johnnovak
Copy link
Member

johnnovak commented May 26, 2024

True. Everything continues to work in DOS. It also breaks Window's MS-DOS prompt, which is the easiest way to reproduce the regression.

Yeah, and the guiding principle to everything we do in DOSBox should be this: does this allow to run the DOS and Win 3.x catalogue in an authentic way? DOS prompt in Win 3.x is an extreme niche; making it work brings no value in light of this goal.

Similarly, it's completely beside the point to get all the possible drivers working in Win 3.x for all the hardware we support. We just need a single supported driver combo to run all the games optimally, job done. This allows us to spend our energies in a smart way on what truly matters: to get the existing DOS/Win3.x game catalogue from 1981-2000 working optimally. Running the same game with all the 47298 different Win3.x driver combos that result in the exact same end-user experience is a non-goal and a waste of our time and energy.

Just talking in generics; I'm not claiming we're doing this type of busy-work now 😄

The big distinction between Windows 3.1 and Windows 9x is that Windows 3.1 installs and runs on top of our internal DOS implementation whereas Windows 9x wants to install its own version of DOS on top of bare metal (or bare hardware emulation).

Win 95 and 98 are a huge rabbit hole, and we have enough to do in pure DOS/Win3.x land. Hence, we really don't wanna go there, IMO. Keep the scope realistic and all.

Win 9x kinda sorta "worked" (works) in all DOSBox versions by fluke and pure random coincidence, but to make it work "properly", there would be lots of additional work to do. Then imagine dealing with all Win 9x game compatibility tickets... Just a hard no; we don't even wanna go there IMO. We are quite under-resourced just for the DOS / Win3.x stuff, we don't wanna take on something absolutely huge as Win 9x (to do it properly, not just half-assedly).

@Grounded0
Copy link
Collaborator

Grounded0 commented May 26, 2024

SHARE.EXE has been present in MS-DOS since MS-DOS 3.1 (search for SHARE.EXE):

http://www.ctyme.com/intr/int-21.htm

https://en.wikipedia.org/wiki/MS-DOS#MS-DOS_3.x

@weirddan455
Copy link
Collaborator Author

We are quite under-resourced just for the DOS / Win3.x stuff, we don't wanna take on something absolutely huge as Win 9.x (to do it properly, not just half-assedly).

I agree 100%. I'm not even sure DOSBox is the right approach at all for Win9x. Maybe DOSBox-X does a decent job, haven't tried it but all the DOS stuff we implement becomes useless and then you're also running an entirely different class of hardware. A lot of games need Pentium 2/3 class CPUs and much stronger GPUs than Voodoo.

@johnnovak
Copy link
Member

johnnovak commented May 26, 2024

I agree 100%. I'm not even sure DOSBox is the right approach at all for Win9x. Maybe DOSBox-X does a decent job, haven't tried it but all the DOS stuff we implement becomes useless and then you're also running an entirely different class of hardware. A lot of games need Pentium 2/3 class CPUs and much stronger GPUs than Voodoo.

Absolutely, even the emulated hardware requirements are significantly different, that's an excellent point. There is overlap, but I'd say Win 9x emulation should be a distinct and separate project from DOSBox; it's really its own thing, I'm in agreement with you. Huge respect to DOSBox-X for trying to pull it off, and all the best wishes in the world & good luck, but this is not for us.

A Windows install would be helpful. I was running into problems with the SB16 drivers myself.

I'll do that over the coming days, ideally sometime next week. The planned outcome is a new succinct wiki page with installation steps, and a couple of ZIP archives on archive.org if someone just wants to quickly grab it and not go through the steps.

@weirddan455
Copy link
Collaborator Author

I'll do that over the coming days, ideally sometime next week.

Sounds good. Anyway, I've addressed your comments so I'll merge soon if no objections.

@johnnovak
Copy link
Member

I'll do that over the coming days, ideally sometime next week.

Sounds good. Anyway, I've addressed your comments so I'll merge soon if no objections.

Thanks! Merge away, man 😎

@weirddan455 weirddan455 merged commit 2523021 into main May 26, 2024
32 checks passed
@interloper98
Copy link
Contributor

This use mentions that their accounting software's multi-user SQL back-end doesn't work under original dosbox, which might point to the lack of share support:

https://www.vogons.org/viewtopic.php?f=7&t=37333&p=328571&hilit=needs%20need%20share%20exe#p328571

My hunch is that it's Peachtree Accounting, because they were massive in the early 2000's -- even their 2004 release of Peachtree Complete Accounting 11 was solely DOS based!

I could only find v 8.0 on archive: https://archive.org/details/peachtree-complete-accounting-8.0-2001, but I haven't tried it w/ the latest CI builds yet.

@interloper98
Copy link
Contributor

8rn448

@weirddan455
Copy link
Collaborator Author

The DOSBox Staging team will not be held liable for any and all accounting errors.

Actually I think we've had to make a statement like that before. I vaguely remember it came out that someone was using DOSBox for some engineering software and our floating-point calculations are not always 100% accurate on all platforms compared to old x86 hardware that uses 80-bit precision.

So yeah, we won't be held liable for any collapsed bridges either.

@johnnovak
Copy link
Member

The DOSBox Staging team will not be held liable for any and all accounting errors.

Actually I think we've had to make a statement like that before. I vaguely remember it came out that someone was using DOSBox for some engineering software and our floating-point calculations are not always 100% accurate on all platforms compared to old x86 hardware that uses 80-bit precision.

So yeah, we won't be held liable for any collapsed bridges either.

I've already taken care of this.

@johnnovak
Copy link
Member

@Python-Exoproject Just pinging you to show off the rapid progress we're making on the Win 3.x emulation front 😎

@Torinde
Copy link
Contributor

Torinde commented May 26, 2024

Windows 3.1 installs and runs on top of our internal DOS implementation whereas Windows 9x wants to install its own version of DOS on top of bare metal (or bare hardware emulation).

Running Win9x from the DOSbox "DOS" would be a great project in itself - replicating what's done by DR-DOS WinBolt (which wasn't publicly released).

From what I gather it seems for Staging is relevant (barely - if a game/software needs it) only the ticked items:

@interloper98
Copy link
Contributor

interloper98 commented May 26, 2024

Peachtree Complete Accounting 9.0 runs perfectly.

The startup batch file (PEACH.BAT) first runs a tool call SHARETST.EXE and from its return code decides if it should launch the local/stand-alone PT.EXE or the network executable NETPEACH.EXE.

I suspect the SHARETST.EXE can detect if the files are sitting on a network share or are local. (The old way of running this software in a multi-user environment is that you have a Novell or NetBUI server hosting the sole copy of the software, and lots of accountants client machines running from this shared directory).

The main Staging CI build and DOSBox-X both pass the SHARETEST:

staging-82-alpha-peachtree.mp4
dosbox-x-peachtree.mp4

But older 81.1 can't run them. Not even FAKESHAR is enough to fool it:

staging-81-1-fakeshar-peachtree.mp4

Unfortunately I'm not too sure how to get Microsoft's NetBUI client working (and same w/ Novel's 2.7 client + NE2000 drivers). So I wasn't able to test a real DOS-based network share.

@Torinde - do you know how to share and mount a DOS directory across two DOSBox instances (either Ethernet or IPX, using either MS or Novel?)

@weirddan455 weirddan455 deleted the wd/share branch May 27, 2024 02:40
@Torinde
Copy link
Contributor

Torinde commented May 27, 2024

share and mount a DOS directory across two DOSBox instances

Probably better if you open a Discussion entry for that, but here is what I know:

  • DOS SMB - install MS Network Client 3.0 for SMB, TCP/IP
  • WfW 3.1x - has SMB and can install Microsoft TCP/IP-32 addon
  • VBSF from VBADOS - share the same host folder with both guest instances. Unfortunately works only with VirtualBox - the required device isn't implemented in DOSbox.

DOSbox seems more oriented towards multiplayer than shared directories, I haven't tested that:

Other forks:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or enhancement of existing features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

File locking support for better game compatibility (SHARE.EXE)
7 participants