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

Cleanup some leaked file handles #3673

Merged
merged 6 commits into from May 15, 2024
Merged

Cleanup some leaked file handles #3673

merged 6 commits into from May 15, 2024

Conversation

weirddan455
Copy link
Collaborator

@weirddan455 weirddan455 commented May 13, 2024

Description

This allows Dunkle Schatten 2 to load and past the first couple of screens before it freezes. For some reason, it's very sensitive to extra files being created. It's kind of like a DOS Valgrind because it helped me clean up some leaks.

Once it gets past a certain number of files, it stomps on our device chain and then spins forever in this loop:

// start walking the device chain of real pointers
auto rp = dos_infoblock.GetDeviceChain();
while (!DOS_IsEndPointer(rp)) {
if (!is_a_driver(rp) || !DOS_DeviceHasName(rp, name)) {
rp = DOS_GetNextDevice(rp);
continue;
}
if (is_con_or_nul(rp)) {
return 0;
}
if (skip_existing_drivers && is_existing_driver(rp)) {
return 0;
}
// The device at the real pointer is a driver, has a name
// matching the given name, is neither the CON nor NUL devices,
// and (if requested) is not an existing device driver.
return rp;
}
return 0;

I haven't fixed the root problem yet but it lead me to some buggy code I could fix regardless.

Related issues

#3668

Manual testing

  • Output redirection works dir > test.txt
  • Pipe works dir | more
  • Input redirection works test.txt < more

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 DOS Issues related to DOS integration or DOS commands label May 13, 2024
@weirddan455 weirddan455 self-assigned this May 13, 2024
src/shell/shell.cpp Outdated Show resolved Hide resolved
src/shell/shell.cpp Outdated Show resolved Hide resolved
src/shell/shell.cpp Outdated Show resolved Hide resolved
src/shell/shell.cpp Outdated Show resolved Hide resolved
src/shell/shell.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.

Looks good, but I've only reviewed it superficially. I'm not familiar with this part of the codebase and I don't have time or willingness to investigate it more deeply, so I'll trust you on this one.

@weirddan455
Copy link
Collaborator Author

I've addressed your comments and tested redirection. I had a bug with pipes that I fixed.

PVS was throwing some false positive warnings so I did some #pragma pvs to disable it but now MSVC is throwing warnings about unknown pragmas. Stupid build systems man... don't worry about it MSVC that pragma is not meant for you.

@weirddan455
Copy link
Collaborator Author

CI all passes now finally. I think this is good to go but I'll give you a chance to look it over again if you want @johnnovak since I made some changes.

src/shell/shell.cpp Outdated Show resolved Hide resolved
src/shell/shell.cpp Outdated Show resolved Hide resolved
src/shell/shell.cpp Outdated Show resolved Hide resolved
src/shell/shell.cpp Outdated Show resolved Hide resolved
src/shell/shell.cpp Outdated Show resolved Hide resolved
src/shell/shell.cpp Outdated Show resolved Hide resolved
src/shell/shell.cpp Outdated Show resolved Hide resolved
src/shell/shell.cpp Outdated Show resolved Hide resolved
src/shell/shell.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 😄 A few minor comments, but good job otherwise @weirddan455 👍🏻

I found this amusing, btw:

It's kind of like a DOS Valgrind because it helped me clean up some leaks.

We now handle shell redirection without creating redundant file handles.
We set the fcb flag in DOS_OpenFile and set the PSP file handle manually.

This allows old stdin and stdout to be correctly restored.
This also prevents creating duplicate CON and NUL devices in global Files data structure.
@weirddan455 weirddan455 merged commit e9844c5 into main May 15, 2024
32 checks passed
@weirddan455 weirddan455 deleted the wd/dunkle branch May 15, 2024 22:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DOS Issues related to DOS integration or DOS commands
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants