Re: [PATCH 3 of 3] shell32: implement SHPathPrepareForWrite

[Dmitry Timoshkov]

"Vincent Povirk" <madewokherd(a)gmail.com> wrote:
> fixes bug 9523, Knytt Stories installer fails
> +/***********************************************************************
> + *              SHPathPrepareForWriteA (SHELL32.@)
> + */
> +HRESULT WINAPI SHPathPrepareForWriteA(HWND hwnd, IUnknown *modless, LPCSTR path, DWORD flags)
> +{
> +    WCHAR wpath[MAX_PATH];
> +    MultiByteToWideChar( CP_ACP, 0, path, -1, wpath, MAX_PATH);
> +    return SHPathPrepareForWriteW(hwnd, modless, wpath, flags);

You are still using a buffer of a fixed size.
> +    /* cut off filename if necessary */
> +    if (flags & SHPPFW_IGNOREFILENAME)
> +    {
> +        last_slash = StrRChrW(path, NULL, '\\');

StrRChrW is a shlwapi export. Since shlwapi.dll is just mostly a wrapper/helper,
and it imports shell32, that would create a circular dependency. I already
suggested to use strrchrW here.
> +        temppath = HeapAlloc(GetProcessHeap(), 0, len * sizeof(WCHAR));
> +        if (!temppath)
> +            return E_OUTOFMEMORY;
> +        StrCpyNW(temppath, path, len);

Again StrCpyNW is a shlwapi export, and since you just allocated the buffer
of correct length memcpy + adding an explicit '\0' terminator look more
naturally here (if not kernel32.lstrcpynW).
> +    /* try to create the directory if asked to */
> +    if (flags & (SHPPFW_DIRCREATE|SHPPFW_ASKDIRCREATE))
> +    {
> +        if (flags & SHPPFW_ASKDIRCREATE)
> +            FIXME("treating SHPPFW_ASKDIRCREATE as SHPPFW_DIRCREATE\n");
> +
> +        SHCreateDirectoryExW(0, realpath, NULL);
> +    }

You are still not checking SHCreateDirectoryExW return value.
> +    if (temppath)
> +        HeapFree(GetProcessHeap(), 0, temppath);

There is no need to check temppath for NULL before HeapFree call.


-- 
Dmitry.

[PATCH 3 of 3] shell32: implement SHPathPrepareForWrite

[Vincent Povirk]

> You are still using a buffer of a fixed size.

Whoops, didn't think about that one.
> StrRChrW is a shlwapi export. Since shlwapi.dll is just mostly a wrapper/helper,
> and it imports shell32, that would create a circular dependency. I already
> suggested to use strrchrW here.

Um, ok. I didn't realize there were two different functions. I only
was able to find documentation on StrRChrW.

There is indeed a circular dependency (which I wouldn't have noticed),
apparently because shell32 needs to export StrRChrW.
> Again StrCpyNW is a shlwapi export, and since you just allocated the buffer
> of correct length memcpy + adding an explicit '\0' terminator look more
> naturally here (if not kernel32.lstrcpynW).

I'm allowed to use memcpy? Isn't that a native Linux function?
> You are still not checking SHCreateDirectoryExW return value.

Nope. If SHCreateDirectoryExW returns ERROR_ALREADY_EXISTS, I need to
return HRESULT_FROM_WIN32(ERROR_DIRECTORY) or S_OK, depending on
whether it is a file or a directory. And since I'd need to free
temppath if and only if I'm returning early, I think that any way I
could do that would make the code awkward/confusing.
> There is no need to check temppath for NULL before HeapFree call.

Oh. MSDN says there is. Not that I trust MSDN..

-- 
Vincent Povirk

[PATCH 3 of 3] shell32: implement SHPathPrepareForWrite

[Kuba Ober]

> > Again StrCpyNW is a shlwapi export, and since you just allocated the
> > buffer of correct length memcpy + adding an explicit '\0' terminator look
> > more naturally here (if not kernel32.lstrcpynW).
>
> I'm allowed to use memcpy? Isn't that a native Linux function?

It's a C library function. Got nothing to do with Linux.

Cheers, Kuba

[PATCH 3 of 3] shell32: implement SHPathPrepareForWrite

[Dmitry Timoshkov]

"Vincent Povirk" <madewokherd+d41d(a)gmail.com> wrote:
> > Again StrCpyNW is a shlwapi export, and since you just allocated the buffer
> > of correct length memcpy + adding an explicit '\0' terminator look more
> > naturally here (if not kernel32.lstrcpynW).
>
> I'm allowed to use memcpy? Isn't that a native Linux function?

If Wine wouldn't be able to call native Linux APIs how it would work
at all?
> > You are still not checking SHCreateDirectoryExW return value.
>
> Nope. If SHCreateDirectoryExW returns ERROR_ALREADY_EXISTS, I need to
> return HRESULT_FROM_WIN32(ERROR_DIRECTORY) or S_OK, depending on
> whether it is a file or a directory.

I already pointed out that there is no such a test to show correct behaviour
of SHCreateDirectoryExW in that case. If you claim that there is inconsistency
you need to add a test case for it.
> And since I'd need to free
> temppath if and only if I'm returning early, I think that any way I
> could do that would make the code awkward/confusing.

I don't think that handling errors makes the code confusing. If
SHCreateDirectoryExW fails you can skip a fairly slow GetFileAttributesW
call, and that's another advantage.
> > There is no need to check temppath for NULL before HeapFree call.
>
> Oh. MSDN says there is. Not that I trust MSDN..

Even there is a janitorial task in Wine to remove NULL checks before
HeapFree calls.

-- 
Dmitry.