setupapi: Implement SetupOpenLog(), SetupLogErrorA(), SetupLogErrorW(), SetupCloseLog()

[Pierre Schweitzer]

Hey,

On 07/02/2015 21:49, Nikolay Sivov wrote:
> Hi, Pierre. Some comments for your patch.
> 
>> --- a/dlls/setupapi/Makefile.in
>> +++ b/dlls/setupapi/Makefile.in
>> @@ -11,6 +11,7 @@ C_SRCS = \
>>      diskspace.c \
>>      fakedll.c \
>>      install.c \
>> +    log.c \
>>      misc.c \
>>      parser.c \
>>      query.c \
> 
> I'm not sure you need a new file for that. It depends on how much will
> be possibly added in the future, but general rule I think is to try to
> find a room in existing files.

Well, there are a lot more "log" functions in setupapi.dll. Hence my
choice to have a new file for all of these.

> 
>> +#include "wine/debug.h"
>> +#include "windef.h"
>> +#include "winbase.h"
>> +#include "winuser.h"
>> +#include "winreg.h"
>> +#include "setupapi.h"
>> +#include "winnls.h"
> 
> Looks like you don't need all of those.

IIRC, I did. I tried to reduce as much as possible.

> 
>> +HANDLE setupact = INVALID_HANDLE_VALUE;
>> +HANDLE setuperr = INVALID_HANDLE_VALUE;
> 
> Should be static.

Woops. Thanks.

> 
>> +    LPWSTR msg = (LPWSTR)MessageString;
> 
> I see what you want to do, but it's a bit ugly imho.
> 
>> +    if (MessageString)
>> +    {
>> +        len = lstrlenA(MessageString) + 1;
>> +        msg = HeapAlloc(GetProcessHeap(), 0, len * sizeof(WCHAR));
>> +        MultiByteToWideChar(CP_ACP, 0, MessageString, -1, msg, len);
>> +    }
> 
> Please check for memory allocation failure.

This was actually (and upper commented line) just a perfect copy of
SetupGetInfInformationA(). But I can do better without problem ;-).

> 
>> +    ret = WriteFile(setupact, MessageString, lstrlenW(MessageString)
>> * sizeof(WCHAR), NULL, NULL);
>> +        ret = WriteFile(setuperr, MessageString,
>> lstrlenW(MessageString) * sizeof(WCHAR), NULL, NULL);
> 
> You can probably do strlenW() once.

True.

> 
>> +    GetWindowsDirectoryW(path, MAX_PATH);
> 
> Same, you need to get this path once.

That was to save a bit of stack, but yeah, can be done.

> 
>> +    if (setupact != INVALID_HANDLE_VALUE)
>> +    {
>> +        CloseHandle(setupact);
>> +        setupact = INVALID_HANDLE_VALUE;
>> +    }
>> +
>> +    if (setuperr != INVALID_HANDLE_VALUE)
>> +    {
>> +        CloseHandle(setuperr);
>> +        setuperr = INVALID_HANDLE_VALUE;
>> +    }
> 
> You can just CloseHandle() unconditionally.
> 
> I think it also makes sense to SetupCloseLog() on PROCESS_DETACH, but
> that's easy to test. Another thing to test is to see what happens in
> multithreaded case, if it works correctly on windows you need to protect
> those handles in SetupOpenLog/SetupCloseLog, maybe somewhere else too.

First part would make sense, indeed.
For the second part, that gets a bit more tricky. I don't recall any
reference to multithreaded-safe on MSDN.
-- 
Pierre Schweitzer <pierre at reactos.org>
System & Network Administrator
Senior Kernel Developer
ReactOS Deutschland e.V.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 3940 bytes
Desc: S/MIME Cryptographic Signature
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20150207/4daa2f17/attachment.bin>