Mailing-List: contact stdcxx-dev-help@incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: stdcxx-dev@incubator.apache.org
Received-SPF: neutral (asf.osuosl.org: local policy)
Message-ID: <44E5EFFC.4070302@roguewave.com>
Date: Fri, 18 Aug 2006 10:51:08 -0600
From: Andrew Black <ablack@roguewave.com>
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US;
 rv:1.8.0.5) Gecko/20060720 SeaMonkey/1.0.3
MIME-Version: 1.0
To: stdcxx-dev@incubator.apache.org
Subject: Re: [patch] exec utility windows port
References: <44E5C6A1.5090002@kyiv.vdiweb.com>
In-Reply-To: <44E5C6A1.5090002@kyiv.vdiweb.com>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

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:sebor@roguewave.com]
>  > Sent: Friday, August 18, 2006 4:01 AM
>  > To: stdcxx-dev@incubator.apache.org
>  > 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.