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

[Pierre Schweitzer]

On 03/11/2015 09:16 PM, Nikolay Sivov wrote:
> On 11.03.2015 22:46, Pierre Schweitzer wrote:
>> +BOOL WINAPI SetupLogErrorW(LPCWSTR MessageString, LogSeverity Severity)
>> +{
>> +    BOOL ret;
>> +    static const WCHAR null[] = {'(','n','u','l','l',')',0};
>> +    DWORD written;
>> +    DWORD len;
>> +
>> +    EnterCriticalSection(&setupapi_cs);
>> +
>> +    if (setupact == INVALID_HANDLE_VALUE || setuperr ==
>> INVALID_HANDLE_VALUE)
>> +    {
>> +        LeaveCriticalSection(&setupapi_cs);
>> +        SetLastError(ERROR_FILE_INVALID);
>> +        return FALSE;
>> +    }
>> +
>> +    if (MessageString == NULL)
>> +        MessageString = null;
>> +
>> +    len = lstrlenW(MessageString) * sizeof(WCHAR);
>> +
>> +    ret = WriteFile(setupact, MessageString, len, &written, NULL);
>> +    if (!ret)
>> +    {
>> +        LeaveCriticalSection(&setupapi_cs);
>> +        return ret;
>> +    }
>> +
>> +    if (Severity >= LogSevMaximum)
>> +    {
>> +        LeaveCriticalSection(&setupapi_cs);
>> +        return FALSE;
>> +    }
>> +
>> +    if (Severity > LogSevInformation)
>> +        ret = WriteFile(setuperr, MessageString, len, &written, NULL);
>> +
>> +    LeaveCriticalSection(&setupapi_cs);
>> +
>> +    return ret;
>> +}
> 
> It'd be probably shorter if you had return with unlock call and one
> place and just jumped there. Also it's questionable if you need to
> return when first WriteFile() failed without trying to write to error log.

Yes. I can indeed to a label + goto method. Won't harm.

Regarding the fail if first write attempt fails, I did it that way
because I suspected that if first write fails, this is likely to be out
of space (most of the time), so attempting second write won't succeed
more. Hence the bail out.

> I don't remember if I said it last time, but I don't see a reason to add
> a new file just for couple of functions.

This won't be just a couple of functions. There are more functions
regarding logging in setupapi, hence the usage for a new file. See
SetupInitializeFileLog() and the associated functions.

> 
>> +    GetWindowsDirectoryW(win, MAX_PATH);
>> +    lstrcpyW(path, win);
>> +    lstrcatW(path, setupactlog);
> 
> This never changes, so you can init it once.

Will update, thanks.

> 
>> +    setupact = CreateFileW(path, GENERIC_WRITE, FILE_SHARE_WRITE |
>> FILE_SHARE_READ,
>> +                           NULL, OPEN_ALWAYS, FILE_ATTRIBUTE_NORMAL,
>> NULL);
>> +    if (setupact == INVALID_HANDLE_VALUE)
>> +    {
>> +        LeaveCriticalSection(&setupapi_cs);
>> +        return FALSE;
>> +    }
>> +
>> +    SetFilePointer(setupact, 0, NULL, FILE_END);
> 
> Will FILE_APPEND_DATA work in this case?

GENERIC_WRITE should cover the FILE_APPEND_DATA. But you're right, I
should rather use FILE_GENERIC_WRITE, which for sure, includes
FILE_APPEND_DATA.

>> +    SetLastError(ERROR_ALREADY_EXISTS);
>> +    return TRUE;
> 
> But this one looks redundant.

Well... This error code is a bit weird, I honestly didn't expect it to
be set, but Windows appears to do so, hence my test for it. As I don't
know whether some apps would rely on it, I set it. It doesn't harm.

> 
>> +    ret = SetupLogErrorW((LPCWSTR)L"Test without opening\r\n",
>> LogSevInformation);
> 
> Don't use L"" please.

OK, just plain text, I suppose?

> 
>> +    SetLastError(0xdeadbeef);
>> +    ret = SetupLogErrorW(NULL, LogSevInformation);
>> +    error = GetLastError();
>> +    if (!ret) ok(error == ERROR_INVALID_PARAMETER, "got wrong error:
>> %d\n", error); /* Win Vista + */
>> +    else ok(ret, "SetupLogError failed\n");
> 
> This makes no sense, is it supposed to fail or not?

Well. Depends.
On Windows Vista and more, it will fail, with ERROR_INVALID_PARAMETER.
Below, it will just print (null) in the log file.
This was the solution I found to make the test work on all the available
test VMs Wine has.

As a side note, my implementation matches Win2k3 behavior: it prints
(null). I find this more compatible than the restrictive behavior in Vista+.
Of course, this is questionable.

I will submit a new patch addressing your four comments later today.

Cheers,
-- 
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: 4277 bytes
Desc: S/MIME Cryptographic Signature
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20150312/d87ce094/attachment.bin>