Re: setupapi: Implement SetupOpenLog(), SetupLogErrorA(), SetupLogErrorW(), SetupCloseLog() (try 3)
This is better, but still not quire right.
+CRITICAL_SECTION setuplog_critical;
Take a look at how critical section data is initialized in wine code, name needs to be adjusted to let's say 'setupapi_cs'.
+ if (MessageString) + { + len = lstrlenA(MessageString) + 1; + msg = HeapAlloc(GetProcessHeap(), 0, len * sizeof(WCHAR)); + if (msg == NULL) + { + SetLastError(ERROR_NOT_ENOUGH_MEMORY); + return FALSE; + } + MultiByteToWideChar(CP_ACP, 0, MessageString, -1, msg, len); + }
That's not how A->W conversion works, we have tons of examples for that.
OsVersionInfo.dwOSVersionInfoSize = sizeof(OSVERSIONINFOW); if (!GetVersionExW(&OsVersionInfo)) return FALSE; + InitializeCriticalSection(&setuplog_critical); SETUPAPI_hInstance = hinstDLL; break; case DLL_PROCESS_DETACH: if (lpvReserved) break; + DeleteCriticalSection(&setuplog_critical); if (CABINET_hInstance) FreeLibrary(CABINET_hInstance); break;
You won't need these calls once you init it like the rest of Wine code does. Also what about closing files on DLL_PROCESS_DETACH?
+ SetLastError(ERROR_ALREADY_EXISTS); + return TRUE;
Is it really important for some application to set this error on success? In general you only need to set it if call failed.
+ setupact = CreateFileW(path, GENERIC_WRITE, FILE_SHARE_WRITE | FILE_SHARE_READ, + NULL, OPEN_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL);
Did you check what happens if multiple processes are logging?
On 08/02/2015 13:46, Nikolay Sivov wrote:
+ if (MessageString) + { + len = lstrlenA(MessageString) + 1; + msg = HeapAlloc(GetProcessHeap(), 0, len * sizeof(WCHAR)); + if (msg == NULL) + { + SetLastError(ERROR_NOT_ENOUGH_MEMORY); + return FALSE; + } + MultiByteToWideChar(CP_ACP, 0, MessageString, -1, msg, len); + }
That's not how A->W conversion works, we have tons of examples for that.
Well.... http://source.winehq.org/git/wine.git/blob/HEAD:/dlls/setupapi/query.c#l118 Feel free to point at some other location.
OsVersionInfo.dwOSVersionInfoSize = sizeof(OSVERSIONINFOW); if (!GetVersionExW(&OsVersionInfo)) return FALSE; + InitializeCriticalSection(&setuplog_critical); SETUPAPI_hInstance = hinstDLL; break; case DLL_PROCESS_DETACH: if (lpvReserved) break; + DeleteCriticalSection(&setuplog_critical); if (CABINET_hInstance) FreeLibrary(CABINET_hInstance); break;
You won't need these calls once you init it like the rest of Wine code does. Also what about closing files on DLL_PROCESS_DETACH?
I couldn't find any evidence that Windows does this. And they'll be closed as soon the process dies.
+ SetLastError(ERROR_ALREADY_EXISTS); + return TRUE;
Is it really important for some application to set this error on success? In general you only need to set it if call failed.
Well, as my tests are showing, this is set by Windows. In case an application would rely on it, it doesn't harm to set it.
+ setupact = CreateFileW(path, GENERIC_WRITE, FILE_SHARE_WRITE | FILE_SHARE_READ, + NULL, OPEN_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL);
Did you check what happens if multiple processes are logging?
As far as my tests are correct, multiple processes can be logging. -- Pierre Schweitzer <pierre at reactos.org> System & Network Administrator Senior Kernel Developer ReactOS Deutschland e.V.
On 08.02.2015 16:17, Pierre Schweitzer wrote:
On 08/02/2015 13:46, Nikolay Sivov wrote:
+ if (MessageString) + { + len = lstrlenA(MessageString) + 1; + msg = HeapAlloc(GetProcessHeap(), 0, len * sizeof(WCHAR)); + if (msg == NULL) + { + SetLastError(ERROR_NOT_ENOUGH_MEMORY); + return FALSE; + } + MultiByteToWideChar(CP_ACP, 0, MessageString, -1, msg, len); + }
That's not how A->W conversion works, we have tons of examples for that.
Well.... http://source.winehq.org/git/wine.git/blob/HEAD:/dlls/setupapi/query.c#l118
Feel free to point at some other location.
Yes, that one should be fixed too I suppose. Clean way is to call mbtowc twice, and first call would return a length of resulting WCHAR string.
OsVersionInfo.dwOSVersionInfoSize = sizeof(OSVERSIONINFOW); if (!GetVersionExW(&OsVersionInfo)) return FALSE; + InitializeCriticalSection(&setuplog_critical); SETUPAPI_hInstance = hinstDLL; break; case DLL_PROCESS_DETACH: if (lpvReserved) break; + DeleteCriticalSection(&setuplog_critical); if (CABINET_hInstance) FreeLibrary(CABINET_hInstance); break;
You won't need these calls once you init it like the rest of Wine code does. Also what about closing files on DLL_PROCESS_DETACH?
I couldn't find any evidence that Windows does this. And they'll be closed as soon the process dies.
Yes, but it will leak every time you do LoadLibrary/FreeLibrary.
Hi, forgot to reply. Thanks for all your reviews, they have been all integrated in the latest submitted patch, which is OK (and working). And the one that was to be fixed has been fixed (and pushed upstream already :-)). Cheers, On 02/08/2015 02:28 PM, Nikolay Sivov wrote:
On 08.02.2015 16:17, Pierre Schweitzer wrote:
On 08/02/2015 13:46, Nikolay Sivov wrote:
+ if (MessageString) + { + len = lstrlenA(MessageString) + 1; + msg = HeapAlloc(GetProcessHeap(), 0, len * sizeof(WCHAR)); + if (msg == NULL) + { + SetLastError(ERROR_NOT_ENOUGH_MEMORY); + return FALSE; + } + MultiByteToWideChar(CP_ACP, 0, MessageString, -1, msg, len); + }
That's not how A->W conversion works, we have tons of examples for that.
Well.... http://source.winehq.org/git/wine.git/blob/HEAD:/dlls/setupapi/query.c#l118
Feel free to point at some other location.
Yes, that one should be fixed too I suppose. Clean way is to call mbtowc twice, and first call would return a length of resulting WCHAR string.
OsVersionInfo.dwOSVersionInfoSize = sizeof(OSVERSIONINFOW); if (!GetVersionExW(&OsVersionInfo)) return FALSE; + InitializeCriticalSection(&setuplog_critical); SETUPAPI_hInstance = hinstDLL; break; case DLL_PROCESS_DETACH: if (lpvReserved) break; + DeleteCriticalSection(&setuplog_critical); if (CABINET_hInstance) FreeLibrary(CABINET_hInstance); break;
You won't need these calls once you init it like the rest of Wine code does. Also what about closing files on DLL_PROCESS_DETACH?
I couldn't find any evidence that Windows does this. And they'll be closed as soon the process dies.
Yes, but it will leak every time you do LoadLibrary/FreeLibrary.
-- Pierre Schweitzer <pierre(a)reactos.org> System & Network Administrator Senior Kernel Developer ReactOS Deutschland e.V.
participants (2)
-
Nikolay Sivov -
Pierre Schweitzer