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

Nikolay Sivov bunglehead at gmail.com
Sat Feb 7 14:49:17 CST 2015


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.



More information about the wine-devel mailing list