-
Notifications
You must be signed in to change notification settings - Fork 19
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 enough process handling logic to make psyq4.0 happy #46
Conversation
processes.h
Outdated
namespace processes { | ||
struct Process { | ||
pid_t pid; | ||
unsigned int exitCode; |
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.
would it be beneficial to limit this strictly to a uint32_t since wibo will mainly be for 32-bit processes?
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.
That it would. I may run with pulling in the DWORD definition from common.h to make it clear "this is a windows thing"
// YET TODO: take into account process / thread attributes, environment variables | ||
// working directory, etc. | ||
setenv("WIBO_DEBUG_INDENT", std::to_string(wibo::debugIndent + 1).c_str(), true); |
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.
By this do you mean environment variables for the process to be spawned?
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.
yup! parsing and mashing lpEnvironment into a format posix_spawn likes.
TYPE_UNUSED, | ||
TYPE_FILE, | ||
TYPE_MAPPED, | ||
TYPE_PROCESS |
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.
Did indentation get messed up here?
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.
weird! It looks fine in my editor, but is showing up on main as well (https://github.com/decompals/wibo/blob/main/handles.h)
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.
Wibo code uses tabs instead of spaces. We need to add an .editor-config or similar.
#include <sched.h> | ||
|
||
namespace processes { | ||
struct Process { |
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.
Any reason for a struct over a class?
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.
I think a struct is good, it's just data
dll/kernel32.cpp
Outdated
|
||
int status; | ||
waitpid(process->pid, &status, 0); | ||
process->exitCode = WEXITSTATUS(status); |
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.
WEXITSTATUS only makes sense if WIFEXITED -- can you add some bare-bones handling of the WIFSIGNALED case too? Mainly so we don't accidentally set the exit code to zero.
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.
good catch; adding now
for (size_t i = 1; i < strlen(lpCommandLine); i++) { | ||
if (isspace(lpCommandLine[i]) && !isspace(lpCommandLine[i - 1])) | ||
argc++; | ||
} |
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.
Should this use the same command line parsing algorithm as we use on startup?
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.
I'd definitely love to be reinventing the wheel less here. What exactly did you have in mind? Copying over the path translation and backslash escaping?
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.
Oh, hm, I misremembered the direction of that logic, we're doing the opposite here. Never mind!
lpThreadAttributes, | ||
bInheritHandles, | ||
dwCreationFlags, | ||
lpEnvironment, | ||
lpCurrentDirectory ? lpCurrentDirectory : "<none>", |
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.
I think %s doesn't handle nulls portably, so this was intentional.
#include <sched.h> | ||
|
||
namespace processes { | ||
struct Process { |
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.
I think a struct is good, it's just data
(and maybe some other things, too!)
This PR implements a bare-bones CreateProcessA, WaitForSingleObject, and GetExitCodeProcess - enough to start a process, wait for it to exit, and check it quit okay.
In order to achieve this, it introduces processes as a new type of handle. As far as I can tell, we've never tagged metadata along with handles before - more than happy for feedback on the approach taken.