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

[Pierre Schweitzer]

Hey,

On 07/02/2015 21:49, Nikolay Sivov wrote:
> Hi, Pierre. Some comments for your patch.
> > --- a/dlls/setupapi/Makefile.in
> > +++ b/dlls/setupapi/Makefile.in
> > @@ -11,6 +11,7 @@ C_SRCS = \
> >      diskspace.c \
> >      fakedll.c \
> >      install.c \
> > - log.c \
> > misc.c \
> > parser.c \
> > query.c \
>
> I'm not sure you need a new file for that. It depends on how much will
> be possibly added in the future, but general rule I think is to try to
> find a room in existing files.

Well, there are a lot more "log" functions in setupapi.dll. Hence my
choice to have a new file for all of these.
> > +#include "wine/debug.h"
> > +#include "windef.h"
> > +#include "winbase.h"
> > +#include "winuser.h"
> > +#include "winreg.h"
> > +#include "setupapi.h"
> > +#include "winnls.h"
>
> Looks like you don't need all of those.

IIRC, I did. I tried to reduce as much as possible.
> > +HANDLE setupact = INVALID_HANDLE_VALUE;
> > +HANDLE setuperr = INVALID_HANDLE_VALUE;
>
> Should be static.

Woops. Thanks.
> > - LPWSTR msg = (LPWSTR)MessageString;
>
> I see what you want to do, but it's a bit ugly imho.
> > - if (MessageString)
> > - {
> > -
> >    len = lstrlenA(MessageString) + 1;
> > -
> >    msg = HeapAlloc(GetProcessHeap(), 0, len * sizeof(WCHAR));
> > -
> >    MultiByteToWideChar(CP_ACP, 0, MessageString, -1, msg, len);
> > - }
>
> Please check for memory allocation failure.

This was actually (and upper commented line) just a perfect copy of
SetupGetInfInformationA(). But I can do better without problem ;-).
> > - ret = WriteFile(setupact, MessageString, lstrlenW(MessageString)
> > - sizeof(WCHAR), NULL, NULL);
> > -
> >    ret = WriteFile(setuperr, MessageString,
> >
> > lstrlenW(MessageString) * sizeof(WCHAR), NULL, NULL);
>
> You can probably do strlenW() once.

True.
> > - GetWindowsDirectoryW(path, MAX_PATH);
>
> Same, you need to get this path once.

That was to save a bit of stack, but yeah, can be done.
> > - if (setupact != INVALID_HANDLE_VALUE)
> > - {
> > -
> >    CloseHandle(setupact);
> > -
> >    setupact = INVALID_HANDLE_VALUE;
> > - }
> > -
> > - if (setuperr != INVALID_HANDLE_VALUE)
> > - {
> > -
> >    CloseHandle(setuperr);
> > -
> >    setuperr = INVALID_HANDLE_VALUE;
> > - }
>
> You can just CloseHandle() unconditionally.
>
> I think it also makes sense to SetupCloseLog() on PROCESS_DETACH, but
> that's easy to test. Another thing to test is to see what happens in
> multithreaded case, if it works correctly on windows you need to protect
> those handles in SetupOpenLog/SetupCloseLog, maybe somewhere else too.

First part would make sense, indeed.
For the second part, that gets a bit more tricky. I don't recall any
reference to multithreaded-safe on MSDN.