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: <44E62CE5.6040508@roguewave.com>
Date: Fri, 18 Aug 2006 15:11:01 -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>
 <44E5EFFC.4070302@roguewave.com> <44E60252.1050801@roguewave.com>
In-Reply-To: <44E60252.1050801@roguewave.com>
Content-Type: multipart/mixed;
 boundary="------------020509080803000306020203"

--------------020509080803000306020203
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

Greetings

Attached is a patch that adds return value checking to the CloseHandle 
and WaitForSingleObject calls added in Farid's patch.

Log:
	* exec.cpp [_WIN32 || _WIN64] (exec_file): Check return value from 
calls to CloseHandle and WaitForSingleObject.

Martin Sebor wrote:
> Andrew Black wrote:
>> 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.
> 
> TCHAR is the same thing as char and LPTSTR is the same thing as char*
> as far as we're concerned. The names are Microsoft's harebrained idea
> for an internationalized API. When FormatMessage is told to allocate
> the string, the char* lpBuffer argument is expected to be a pointer
> to void*, i.e., void**, and the function does something like this:
> 
>     *(void**)lpBuffer = LocalAlloc (..., nSize);
>     sprintf (*(char**)lpBuffer, "%s", message);
> 
> There is no clean way for the caller to pass a void** to a function
> that takes a char*. Clearly a design flaw in the API but what else
> is new. We will have to live with the cast.
> 
>> 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).
> 
> Right. We should definitely check the return values of most calls
> and report errors or warnings. But trying to handle errors that
> occur while handling other errors is a catch 22, so I would skip
> this type of error checking and reporting in warn() or terminate()
> (although if FormatMessage fails we should still try to produce
> an error message without the formatted text).
> 
>>
>> 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).
> 
> I agree. Can you post a patch?
> 
> Martin

--------------020509080803000306020203
Content-Type: text/x-patch;
 name="call_check.diff"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
 filename="call_check.diff"

Index: exec.cpp
===================================================================
--- exec.cpp	(revision 432706)
+++ exec.cpp	(working copy)
@@ -952,7 +952,8 @@
     if (-1 == status.status)
         return status;
 
-    CloseHandle (child.hThread);
+    if (0 == CloseHandle (context.hThread))
+        warn_last_error ("Closing child main thread handle");
 
     /* Wait for the child process to terminate */
     wait_code = WaitForSingleObject (child.hProcess, real_timeout);
@@ -969,7 +970,8 @@
             status.error = warn_last_error ("Waiting for child process");
         }
 
-        CloseHandle (child.hProcess);
+        if (0 == CloseHandle (context.hProcess))
+            warn_last_error ("Closing child process handle");
         return status;
     }
 
@@ -1010,21 +1012,24 @@
                 status.error = warn_last_error ("Waiting for child process");
             }
 
-            CloseHandle (child.hProcess);
+            if (0 == CloseHandle (context.hProcess))
+                warn_last_error ("Closing child process handle");
             return status;
         }
     }
     /* Then hard kill the child process */
     if (0 == TerminateProcess (child.hProcess, 3))
         warn_last_error ("Terminating child process");
-    else
-        WaitForSingleObject (child.hProcess, 1000);
+    else if (WAIT_FAIL == WaitForSingleObject (child.hProcess, 1000))
+        warn_last_error ("Waiting for child process");
+
     if (0 == GetExitCodeProcess (child.hProcess, &status.status)) {
         warn_last_error ("Retrieving child process exit code");
         status.status = -1;
     }
     status.error = 3;
-    CloseHandle (child.hProcess);
+    if (0 == CloseHandle (context.hProcess))
+        warn_last_error ("Closing child process handle");
     return status;
 }
 #endif  /* _WIN{32,64} */

--------------020509080803000306020203--