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

[Nikolay Sivov]

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.

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.
> - GetWindowsDirectoryW(win, MAX_PATH);
> - lstrcpyW(path, win);
> - lstrcatW(path, setupactlog);

This never changes, so you can init it once.
> - 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?
> - if (setupact != INVALID_HANDLE_VALUE && setuperr != INVALID_HANDLE_VALUE)
> - {
> -
>    LeaveCriticalSection(&setupapi_cs);
> -
>    SetLastError(ERROR_ALREADY_INITIALIZED);
> -
>    return TRUE;
> - }

Setting error here makes sense as tests show.
> - SetLastError(ERROR_ALREADY_EXISTS);
> - return TRUE;

But this one looks redundant.
> - ret = SetupLogErrorW((LPCWSTR)L"Test without opening\r\n", LogSevInformation);

Don't use L"" please.
> - 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?

Same for new test file - I think you can add those test in some existing 
file.