Re: 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,