Re: user32: Implement GetWindowModuleFileName with tests

[Dmitry Timoshkov]

"Maarten Lankhorst" <m.b.lankhorst(a)gmail.com> wrote:
> UINT WINAPI GetWindowModuleFileNameA( HWND hwnd, LPSTR lpszFileName, UINT cchFileNameMax)
>  {
> -    FIXME("GetWindowModuleFileNameA(hwnd %p, lpszFileName %p, cchFileNameMax %u) stub!\n",
> -          hwnd, lpszFileName, cchFileNameMax);
> -    return 0;
> +    LPWSTR filenameW = NULL;
> +    UINT retval;
> +
> +    if (cchFileNameMax)
> +    {
> +        filenameW = HeapAlloc(GetProcessHeap(), 0, cchFileNameMax);
> +        if (!filenameW)
> +        {
> +            SetLastError(ERROR_NOT_ENOUGH_MEMORY);
> +            return 0;
> +        }
> +    }

Probably GetWindowModuleFileNameA should simply allocate a buffer of size
MAX_PATH on the stack and fail with ERROR_FILENAME_EXCED_RANGE if unicode
counterpart returns a string longer than MAX_PATH. kernel32 APIs behave
that way.
> +    retval = GetWindowModuleFileNameW(hwnd, filenameW, cchFileNameMax);
> +    if (retval)
> +    {
> +        DWORD lasterror = GetLastError();
> +        WideCharToMultiByte(CP_ACP, 0, filenameW, -1, lpszFileName, cchFileNameMax, NULL, NULL);
> +        SetLastError(lasterror);

What's the point of saving/restoring last error value? If WideCharToMultiByte
fails you need to return an error in that case, not silently continue.

Also GetWindowModuleFileNameA also should be fixed to return correct value,
and not assume that the buffer is '\0' terminated.
> UINT WINAPI GetWindowModuleFileNameW( HWND hwnd, LPWSTR lpszFileName, UINT cchFileNameMax)
>  {
> -    FIXME("GetWindowModuleFileNameW(hwnd %p, lpszFileName %p, cchFileNameMax %u) stub!\n",
> -          hwnd, lpszFileName, cchFileNameMax);
> -    return 0;
> +    PROCESS_BASIC_INFORMATION pbi;
> +    DWORD pid, retval;
> +    HANDLE hproc;
> +    DWORD lasterror = GetLastError();
> +    UNICODE_STRING *pathname;
> +    TRACE("GetWindowModuleFileNameW(hwnd %p, lpszFileName %p, cchFileNameMax %u)\n", hwnd, lpszFileName, 
> cchFileNameMax);

TRACE/WARN/ERR/etc already print the caller's name, there is no need to
duplicate it.
> +    if (!GetWindowThreadProcessId(hwnd, &pid))
> +    {
> +        SetLastError(ERROR_INVALID_WINDOW_HANDLE);
> +        return 0;
> +    }

GetWindowThreadProcessId already sets the error to the correct value,
there is no need to guess why it failed.
> +    if (!cchFileNameMax)
> +    { /* Fail silently without setting filename */
> +        SetLastError(lasterror);
> +        return 0;
> +    }

Saving/restoring last error value doesn't look correct at all.
> +    hproc = OpenProcess(PROCESS_QUERY_INFORMATION, 0, pid);
> +    if (!hproc)
> +    {
> +        FIXME("Is this correct?");
> +        SetLastError(ERROR_INVALID_WINDOW_HANDLE);
> +        return 0;
> +    }

There is no need to guess why OpenProcess failed.
> +    retval = NtQueryInformationProcess(hproc, ProcessBasicInformation, &pbi, sizeof(pbi), NULL);
> +    pathname = &pbi.PebBaseAddress->ProcessParameters->ImagePathName;
> +    if (pathname->Length / sizeof(WCHAR) < cchFileNameMax)
> +    {
> +        memcpy(lpszFileName, pathname->Buffer, pathname->Length);
> +        lpszFileName[pathname->Length / sizeof(WCHAR)] = 0;
> +        retval = pathname->Length / sizeof(WCHAR) + 1;
> +    }
> +    else
> +    {
> +        memcpy(lpszFileName, pathname->Buffer, cchFileNameMax * sizeof(WCHAR));
> +        retval = cchFileNameMax;
> +    }

Both branches above should be merged, that will simplify the code. Just '\0'
terminate the result if there is enough space in the provided buffer.
> +    CloseHandle(hproc);
> +    SetLastError(lasterror);
> +    FIXME("--> %u %s\n", retval, debugstr_w(lpszFileName));

Left-over from debugging?


-- 
Dmitry.

user32: Implement GetWindowModuleFileName with tests

[Reece Dunn]

On 08/02/2008, Dmitry Timoshkov <dmitry(a)codeweavers.com> wrote:
> "Maarten Lankhorst" <m.b.lankhorst(a)gmail.com> wrote:
> > +    retval = GetWindowModuleFileNameW(hwnd, filenameW, cchFileNameMax);
> > +    if (retval)
> > +    {
> > +        DWORD lasterror = GetLastError();
> > +        WideCharToMultiByte(CP_ACP, 0, filenameW, -1, lpszFileName, cchFileNameMax, NULL, NULL);
> > +        SetLastError(lasterror);
>
> What's the point of saving/restoring last error value? If WideCharToMultiByte
> fails you need to return an error in that case, not silently continue.

The usual model is something like this:

    SetLastError(0xdeadbeef);
    WinApiCall();
    ok(GetLastError() == ERROR_SUCCESS, "WinApiCall failed, expected
ERROR_SUCCESS, got %d", GetLastError());

- Reece

user32: Implement GetWindowModuleFileName with tests

[Dmitry Timoshkov]

"Reece Dunn" <msclrhd(a)googlemail.com> wrote:
> > > +    retval = GetWindowModuleFileNameW(hwnd, filenameW, cchFileNameMax);
> > > +    if (retval)
> > > +    {
> > > +        DWORD lasterror = GetLastError();
> > > +        WideCharToMultiByte(CP_ACP, 0, filenameW, -1, lpszFileName, cchFileNameMax, NULL, NULL);
> > > +        SetLastError(lasterror);
> >
> > What's the point of saving/restoring last error value? If WideCharToMultiByte
> > fails you need to return an error in that case, not silently continue.
>
> The usual model is something like this:
>
> SetLastError(0xdeadbeef);
>    WinApiCall();
>    ok(GetLastError() == ERROR_SUCCESS, "WinApiCall failed, expected
> ERROR_SUCCESS, got %d", GetLastError());

In the tests, which is not the case here.

-- 
Dmitry.

user32: Implement GetWindowModuleFileName with tests

[Reece Dunn]

On 08/02/2008, Dmitry Timoshkov <dmitry(a)codeweavers.com> wrote:
> "Reece Dunn" <msclrhd(a)googlemail.com> wrote:
> > > What's the point of saving/restoring last error value? If WideCharToMultiByte
> > > fails you need to return an error in that case, not silently continue.
> >
> > The usual model is something like this:
> >
> > SetLastError(0xdeadbeef);
> >    WinApiCall();
> >    ok(GetLastError() == ERROR_SUCCESS, "WinApiCall failed, expected
> > ERROR_SUCCESS, got %d", GetLastError());
>
> In the tests, which is not the case here.

Sorry, yes. You are correct.

- Reece

user32: Implement GetWindowModuleFileName with tests

[Dmitry Timoshkov]

> +    hproc = OpenProcess(PROCESS_QUERY_INFORMATION, 0, pid);

After reading MSDN and guessing from the API name shouldn't it
simply fetch GWL_HINSTANCE and call GetModuleFileName on it?

-- 
Dmitry.

user32: Implement GetWindowModuleFileName with tests

[Dmitry Timoshkov]

"Dmitry Timoshkov" <dmitry(a)codeweavers.com> wrote:
> > +    hproc = OpenProcess(PROCESS_QUERY_INFORMATION, 0, pid);
>
> After reading MSDN and guessing from the API name shouldn't it
> simply fetch GWL_HINSTANCE and call GetModuleFileName on it?

And after looking at GetModuleFileNameW implementation, shouldn't it
treat hModule == 0 as GetModuleHandle(0) and not be 16-bit specific?
Guess this all needs a test case.

-- 
Dmitry.

user32: Implement GetWindowModuleFileName with tests

[Maarten Lankhorst]

Hi folks,

2008/2/8, Dmitry Timoshkov <dmitry(a)codeweavers.com>:
> "Dmitry Timoshkov" <dmitry(a)codeweavers.com> wrote:
> > > +    hproc = OpenProcess(PROCESS_QUERY_INFORMATION, 0, pid);
> >
> > After reading MSDN and guessing from the API name shouldn't it
> > simply fetch GWL_HINSTANCE and call GetModuleFileName on it?
>
> And after looking at GetModuleFileNameW implementation, shouldn't it
> treat hModule == 0 as GetModuleHandle(0) and not be 16-bit specific?
> Guess this all needs a test case.

I've tried to do the GetWindowLongPtr GWLP_HINSTANCE thing but it
seems to always return 0 in wine, which according to msdn means error,
but it could just be not set in wine. Perhaps GWLP_HINSTANCE needs to
be set when the window is being created?

Cheers,
Maarten.

user32: Implement GetWindowModuleFileName with tests

[Dmitry Timoshkov]

"Maarten Lankhorst" <m.b.lankhorst(a)gmail.com> wrote:
> I've tried to do the GetWindowLongPtr GWLP_HINSTANCE thing but it
> seems to always return 0 in wine, which according to msdn means error,
> but it could just be not set in wine. Perhaps GWLP_HINSTANCE needs to
> be set when the window is being created?

It works just fine in Wine, have a look at dlls/user32/tests/class.c,
test_instances().

-- 
Dmitry.