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

[Nikolay Sivov]

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.
> +#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.
> +HANDLE setupact = INVALID_HANDLE_VALUE;
> +HANDLE setuperr = INVALID_HANDLE_VALUE;

Should be static.
> +    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.
> +    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.
> +    GetWindowsDirectoryW(path, MAX_PATH);

Same, you need to get this path once.
> +    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.

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.
-- 
Pierre Schweitzer <pierre at reactos.org>
System & Network Administrator
Senior Kernel Developer
ReactOS Deutschland e.V.