Re: setupapi: Implement SetupOpenLog(), SetupLogErrorA(), SetupLogErrorW(), SetupCloseLog() (try 3)

[Nikolay Sivov]

This is better, but still not quire right.
> +CRITICAL_SECTION setuplog_critical;

Take a look at how critical section data is initialized in wine code, 
name needs to be adjusted to let's say 'setupapi_cs'.
> - if (MessageString)
> - {
> -
>    len = lstrlenA(MessageString) + 1;
> -
>    msg = HeapAlloc(GetProcessHeap(), 0, len * sizeof(WCHAR));
> -
>    if (msg == NULL)
> -
>    {
> -
>        SetLastError(ERROR_NOT_ENOUGH_MEMORY);
> -
>        return FALSE;
> -
>    }
> -
>    MultiByteToWideChar(CP_ACP, 0, MessageString, -1, msg, len);
> - }

That's not how A->W conversion works, we have tons of examples for that.
>      OsVersionInfo.dwOSVersionInfoSize = sizeof(OSVERSIONINFOW);
>      if (!GetVersionExW(&OsVersionInfo))
>          return FALSE;
> -
>    InitializeCriticalSection(&setuplog_critical);
>    SETUPAPI_hInstance = hinstDLL;
>    break;
> case DLL_PROCESS_DETACH:
>        if (lpvReserved) break;
> -
>    DeleteCriticalSection(&setuplog_critical);
>    if (CABINET_hInstance) FreeLibrary(CABINET_hInstance);
>    break;

You won't need these calls once you init it like the rest of Wine code 
does. Also what about closing files on DLL_PROCESS_DETACH?
> - SetLastError(ERROR_ALREADY_EXISTS);
> - return TRUE;

Is it really important for some application to set this error on 
success? In general you only need to set it if call failed.
> - setupact = CreateFileW(path, GENERIC_WRITE, FILE_SHARE_WRITE | FILE_SHARE_READ,
> -
>                       NULL, OPEN_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL);

Did you check what happens if multiple processes are logging?

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

[Pierre Schweitzer]

On 08/02/2015 13:46, Nikolay Sivov wrote:
> > - if (MessageString)
> > - {
> > -
> >    len = lstrlenA(MessageString) + 1;
> > -
> >    msg = HeapAlloc(GetProcessHeap(), 0, len * sizeof(WCHAR));
> > -
> >    if (msg == NULL)
> > -
> >    {
> > -
> >        SetLastError(ERROR_NOT_ENOUGH_MEMORY);
> > -
> >        return FALSE;
> > -
> >    }
> > -
> >    MultiByteToWideChar(CP_ACP, 0, MessageString, -1, msg, len);
> > - }
>
> That's not how A->W conversion works, we have tons of examples for that.

Well....
http://source.winehq.org/git/wine.git/blob/HEAD:/dlls/setupapi/query.c#l118

Feel free to point at some other location.
> >      OsVersionInfo.dwOSVersionInfoSize = sizeof(OSVERSIONINFOW);
> >      if (!GetVersionExW(&OsVersionInfo))
> >          return FALSE;
> > -
> >    InitializeCriticalSection(&setuplog_critical);
> >    SETUPAPI_hInstance = hinstDLL;
> >    break;
> > case DLL_PROCESS_DETACH:
> >        if (lpvReserved) break;
> > -
> >    DeleteCriticalSection(&setuplog_critical);
> >    if (CABINET_hInstance) FreeLibrary(CABINET_hInstance);
> >    break;
>
> You won't need these calls once you init it like the rest of Wine code
> does. Also what about closing files on DLL_PROCESS_DETACH?

I couldn't find any evidence that Windows does this.
And they'll be closed as soon the process dies.
> > - SetLastError(ERROR_ALREADY_EXISTS);
> > - return TRUE;
>
> Is it really important for some application to set this error on
> success? In general you only need to set it if call failed.

Well, as my tests are showing, this is set by Windows. In case an
application would rely on it, it doesn't harm to set it.
> > - setupact = CreateFileW(path, GENERIC_WRITE, FILE_SHARE_WRITE |
> >
> > FILE_SHARE_READ,
> > -
> >                       NULL, OPEN_ALWAYS, FILE_ATTRIBUTE_NORMAL,
> >
> > NULL);
>
> Did you check what happens if multiple processes are logging?

As far as my tests are correct, multiple processes can be logging.

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

[Nikolay Sivov]

On 08.02.2015 16:17, Pierre Schweitzer wrote:
> On 08/02/2015 13:46, Nikolay Sivov wrote:
> > > - if (MessageString)
> > > - {
> > > -
> > >    len = lstrlenA(MessageString) + 1;
> > > -
> > >    msg = HeapAlloc(GetProcessHeap(), 0, len * sizeof(WCHAR));
> > > -
> > >    if (msg == NULL)
> > > -
> > >    {
> > > -
> > >        SetLastError(ERROR_NOT_ENOUGH_MEMORY);
> > > -
> > >        return FALSE;
> > > -
> > >    }
> > > -
> > >    MultiByteToWideChar(CP_ACP, 0, MessageString, -1, msg, len);
> > > - }
> >
> > That's not how A->W conversion works, we have tons of examples for that.
>
> Well....
> http://source.winehq.org/git/wine.git/blob/HEAD:/dlls/setupapi/query.c#l118
>
> Feel free to point at some other location.

Yes, that one should be fixed too I suppose. Clean way is to call mbtowc 
twice, and first call would return a length of resulting WCHAR string.
> > >       OsVersionInfo.dwOSVersionInfoSize = sizeof(OSVERSIONINFOW);
> > >       if (!GetVersionExW(&OsVersionInfo))
> > >           return FALSE;
> > > -
> > >    InitializeCriticalSection(&setuplog_critical);
> > >     SETUPAPI_hInstance = hinstDLL;
> > >     break;
> > > case DLL_PROCESS_DETACH:
> > >     if (lpvReserved) break;
> > > -
> > >    DeleteCriticalSection(&setuplog_critical);
> > >     if (CABINET_hInstance) FreeLibrary(CABINET_hInstance);
> > >     break;
> >
> > You won't need these calls once you init it like the rest of Wine code
> > does. Also what about closing files on DLL_PROCESS_DETACH?
>
> I couldn't find any evidence that Windows does this.
> And they'll be closed as soon the process dies.

Yes, but it will leak every time you do LoadLibrary/FreeLibrary.

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

[Pierre Schweitzer]

Hi,

forgot to reply. Thanks for all your reviews, they have been all
integrated in the latest submitted patch, which is OK (and working).

And the one that was to be fixed has been fixed (and pushed upstream
already :-)).

Cheers,

On 02/08/2015 02:28 PM, Nikolay Sivov wrote:
> On 08.02.2015 16:17, Pierre Schweitzer wrote:
> > On 08/02/2015 13:46, Nikolay Sivov wrote:
> > > > - if (MessageString)
> > > > - {
> > > > -
> > > >    len = lstrlenA(MessageString) + 1;
> > > > -
> > > >    msg = HeapAlloc(GetProcessHeap(), 0, len * sizeof(WCHAR));
> > > > -
> > > >    if (msg == NULL)
> > > > -
> > > >    {
> > > > -
> > > >        SetLastError(ERROR_NOT_ENOUGH_MEMORY);
> > > > -
> > > >        return FALSE;
> > > > -
> > > >    }
> > > > -
> > > >    MultiByteToWideChar(CP_ACP, 0, MessageString, -1, msg, len);
> > > > - }
> > >
> > > That's not how A->W conversion works, we have tons of examples for that.
> >
> > Well....
> > http://source.winehq.org/git/wine.git/blob/HEAD:/dlls/setupapi/query.c#l118
> >
> > Feel free to point at some other location.
>
> Yes, that one should be fixed too I suppose. Clean way is to call mbtowc
> twice, and first call would return a length of resulting WCHAR string.
> > > >       OsVersionInfo.dwOSVersionInfoSize = sizeof(OSVERSIONINFOW);
> > > >       if (!GetVersionExW(&OsVersionInfo))
> > > >           return FALSE;
> > > > -
> > > >    InitializeCriticalSection(&setuplog_critical);
> > > >     SETUPAPI_hInstance = hinstDLL;
> > > >     break;
> > > > case DLL_PROCESS_DETACH:
> > > >     if (lpvReserved) break;
> > > > -
> > > >    DeleteCriticalSection(&setuplog_critical);
> > > >     if (CABINET_hInstance) FreeLibrary(CABINET_hInstance);
> > > >     break;
> > >
> > > You won't need these calls once you init it like the rest of Wine code
> > > does. Also what about closing files on DLL_PROCESS_DETACH?
> >
> > I couldn't find any evidence that Windows does this.
> > And they'll be closed as soon the process dies.
>
> Yes, but it will leak every time you do LoadLibrary/FreeLibrary.