Re: setupapi: Implement SetupOpenLog(), SetupLogErrorA(), SetupLogErrorW(), SetupCloseLog() (resend)
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.
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,
On 12.03.2015 13:17, Pierre Schweitzer wrote:
- 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.
Then we need to decide which one is broken() and mark it as such, in my opinion complying with vista+ behavior would be better. These log files are write-and-forget from installer point of view, so it's unlikely that anything depends on (null) string being written.
I will submit a new patch addressing your four comments later today.
Cheers,
On 03/12/2015 11:51 AM, Nikolay Sivov wrote:
On 12.03.2015 13:17, Pierre Schweitzer wrote:
- 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.
Then we need to decide which one is broken() and mark it as such, in my opinion complying with vista+ behavior would be better. These log files are write-and-forget from installer point of view, so it's unlikely that anything depends on (null) string being written.
Well, I was more in the mood of keeping the implementation that can do more (ie, the one that write (null) instead of returning an error). But, if you prefer Vista+ behavior, then, I'll adapt.
I will submit a new patch addressing your four comments later today.
Cheers,
-
Nikolay Sivov -
Pierre Schweitzer