I have a couple thoughts about this proposed patch.
The first observation is related to the warn_last_error change. It
appears to me that the type of error_text should be changed from LPTSTR
to TCHAR, eliminating the need for the typecast in the call to
FormatMessage. Also, the return value from LocalFree probably should be
checked (There isn't much we can do if it fails, but we probably should
warn the user that it failed, and provide him/her with the error number,
but not a string translation to avoid entering a potentially infinite loop).
The second observation is related to the final call to
WaitForSingleObject and the new calls to CloseHandle. For consistency,
we should check the return value from each of these calls, and call
warn_last_error if they failed (failure being defined as 0 for
CloseHandle and and WAIT_FAILED for WaitForSingleObject).
--Andrew Black
Farid Zaripov wrote:
> -----Original Message-----
> From: Martin Sebor [mailto:[EMAIL PROTECTED]
> Sent: Friday, August 18, 2006 4:01 AM
> To: [email protected]
> Subject: Re: [patch] exec utility windows port
>
> Andrew Black wrote:
> > And, take 2 on the patch. I believe I've resolved the
> issue with soft
> > killing the child processes, but I am not completely happy with it.
> > Part of this reason for this unhappiness is that the MSDN
> > documentation on process creation flags seems to indicate that the
> > Control-C signal handling is disabled on the new process group.
> > Unfortunately, I don't think there's any way around this. Still, I
> > don't see error messages when a child process is killed, so
> I think it will work.
>
> Looks good! Committed thus:
> http://svn.apache.org/viewvc?rev=432452&view=rev
>
> Let's see what Farid says -- I assume this will be integrated
> into the next version of the Windows build infrastructure, right?
I have looked to the path today and found some problems
(the patch, fixes that problems is attached).
ChangeLog for patch:
* exec.cpp [_WIN32 || _WIN64] (warn_last_error):
(exec_file): Added wait after sending Ctrl+Break signal;
added wait after TerminateProcess(); Closed handles of
child process.
exec.cpp, function warn_last_error(): if FORMAT_MESSAGE_ALLOCATE_BUFFER
is used then lpBuffer points to the place where pointer to the allocated
buffer is stored.
----------------------------
LPTSTR error_text = 0;
if (FormatMessage (FORMAT_MESSAGE_ALLOCATE_BUFFER |
FORMAT_MESSAGE_FROM_SYSTEM, NULL, error,
MAKELANGID (LANG_NEUTRAL, SUBLANG_DEFAULT),
(LPTSTR)&error_text, 0, NULL)) {
warn ("%s failed with error %d: %s\n", action, error,
error_text);
LocalFree (error_text);
}
else {
warn ("%s failed with error %d. Additionally, FormatMessage "
"failed with error %d.\n", action, error, GetLastError
());
}
-------------------------------
exec.cpp, function exec_file(): missed WaitForSingleObject() call after
sending CTRL_BREAK_EVENT signal. At the line GenerateConsoleCtrlEvent
(CTRL_BREAK_EVENT,...) wait_code != WAIT_TIMEOUT and TerminateProcess()
will be called immidiately; the handles child.hProcess and child.hThread
should be closed.
-------------------------------
if (VER_PLATFORM_WIN32_NT == OSVer.dwPlatformId) {
if (0 == GenerateConsoleCtrlEvent (CTRL_C_EVENT,
child.dwProcessId))
warn_last_error ("Sending child process Control-C");
wait_code = WaitForSingleObject (child.hProcess, 1000);
if (WAIT_TIMEOUT != wait_code) {
if (WAIT_OBJECT_0 == wait_code) {
if (0 == GetExitCodeProcess (child.hProcess,
&status.status)) {
warn_last_error ("Retrieving child process exit code");
status.status = -1;
}
status.error = 1;
}
else {
status.status = -1;
status.error = warn_last_error ("Waiting for child
process");
}
return status;
}
if (0 == GenerateConsoleCtrlEvent (CTRL_BREAK_EVENT,
child.dwProcessId))
warn_last_error ("Sending child process Control-Break");
if (WAIT_TIMEOUT != wait_code) {
if (WAIT_OBJECT_0 == wait_code) {
if (0 == GetExitCodeProcess (child.hProcess,
&status.status)) {
warn_last_error ("Retrieving child process exit code");
status.status = -1;
}
status.error = 2;
}
else {
status.status = -1;
status.error = warn_last_error ("Waiting for child
process");
}
return status;
}
}
/* Then hard kill the child process */
if (0 == TerminateProcess (child.hProcess, 3))
warn_last_error ("Terminating child process");
if (0 == GetExitCodeProcess (child.hProcess, &status.status)) {
warn_last_error ("Retrieving child process exit code");
status.status = -1;
}
And I suggest to add WaitForSingleObject() after a TerminateProcess()
because the process may not be terminated yet at the moment
TerminateProcess() returns (MSDN says: "TerminateProcess initiates
termination and returns immediately.").
Farid.
