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

pass extended startup info directly #1863

Closed
wants to merge 1 commit into from

Conversation

drizzd
Copy link

@drizzd drizzd commented Oct 3, 2018

The extended startup info is not passed to CreateProcess. It is passed
indirectly as &si.StartupInfo. Since StartupInfo is the first member of
the extended startup info struct, the address of si.StartupInfo
coincides with the address of si.

Although it makes no difference functionally, it seems cleaner to pass
extended startup info directly, as suggested also by the
UpdateProcThreadAttribute example:

https://docs.microsoft.com/en-us/windows/desktop/api/processthreadsapi/nf-processthreadsapi-updateprocthreadattribute

Signed-off-by: Clemens Buchacher drizzd@gmx.net

The extended startup info is not passed to CreateProcess. It is passed
indirectly as &si.StartupInfo. Since StartupInfo is the first member of
the extended startup info struct, the address of si.StartupInfo
coincides with the address of si.

Although it makes no difference functionally, it seems cleaner to pass
extended startup info directly, as suggested also by the
UpdateProcThreadAttribute example:

https://docs.microsoft.com/en-us/windows/desktop/api/processthreadsapi/nf-processthreadsapi-updateprocthreadattribute

Signed-off-by: Clemens Buchacher <drizzd@gmx.net>
@dscho
Copy link
Member

dscho commented Oct 4, 2018

No worries about the failing test: this is on Linux, using Clang, and the failure is in a git daemon test (so most likely it is a flakey test that upstream Git still has not managed to stabilize).

@dscho
Copy link
Member

dscho commented Oct 11, 2018

The extended startup info is not passed to CreateProcess. It is passed
indirectly as &si.StartupInfo. Since StartupInfo is the first member of
the extended startup info struct, the address of si.StartupInfo
coincides with the address of si.

Hmm.

My reading on the situation is that it has to be the first field so that CreateProcessW() works correctly, even when you pass an extended startup info. (And the new version agrees with the example in the documentation.)

So functionally, both versions work, and will always be guaranteed to work.

Practically, however, I like the additional reassurance I get from the compile-time safety where I do not cast a pointer to a different struct type.

So I tend to think that it would be better to keep the existing version. Would you agree?

@drizzd
Copy link
Author

drizzd commented Oct 14, 2018

I agree in practice it will likely always work. However, the API does not guarantee that. Since CreateProcessW knows that it receives extended startup info, StartupInfo does not have to be the first field for CreateProcessW to do all the right things. It is an implementation detail, and this is not obvious to the reader of the code in Git. And this is what bothers me more than the theoretical incorrectness: While reading the source code, it did not make sense to me that we are creating and modifying the extended startup struct, but we never use it anywhere. I really thought this was a bug until I tracked down the definition of the extended startup struct and realized how it works. I would like to spare the next reader the confusion and time spent to figure it out.

Having said that, please feel free to close the pull request if you still disagree.

@dscho
Copy link
Member

dscho commented Oct 16, 2018

I agree in practice it will likely always work. However, the API does not guarantee that.

Actually, it does. As can be seen from the current code, the parameter really has the type of the non-extended startup info. If the first field was not said non-extended startup info, CreateProcessW() would fail to find the correct information, and the program would most likely crash.

@dscho
Copy link
Member

dscho commented Nov 16, 2018

Okay, I made up my mind and I really prefer the compile-time safety of the current version.

@dscho dscho closed this Nov 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants