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
