Re: [PATCH 3 of 3] shell32: implement SHPathPrepareForWrite
"Vincent Povirk" <madewokherd(a)gmail.com> wrote:
HRESULT WINAPI SHPathPrepareForWriteA(HWND hwnd, IUnknown *modless, LPCSTR path, DWORD flags) { - FIXME("%p %p %s 0x%08x\n", hwnd, modless, debugstr_a(path), flags); - return S_OK; + WCHAR wpath[MAX_PATH]; + MultiByteToWideChar( CP_ACP, 0, path, -1, wpath, MAX_PATH); + return SHPathPrepareForWriteW(hwnd, modless, wpath, flags);
It's better to not use fixed buffer sizes but allocate the buffer dynamically.
+ /* make a copy of the path so we can cut off the filename */ + for (i=0; i<MAX_PATH; i++) + { + realpath[i] = path[i]; + if (path[i] == '\\') + last_slash = i; + else if (path[i] == 0) + break; + } + realpath[MAX_PATH-1] = 0; + + if (flags & SHPPFW_IGNOREFILENAME) + realpath[last_slash] = 0;
If the job must be done only when SHPPFW_IGNOREFILENAME is specified then don't do it in all cases. And using strrchrW would simplify the code a bit.
+ /* 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); + }
Why there is no check for SHCreateDirectoryExW return value?
+ + /* check if we can access the directory */ + res = GetFileAttributesW(realpath); + + if (res == INVALID_FILE_ATTRIBUTES) + { + err = GetLastError(); + if (err == ERROR_FILE_NOT_FOUND) + err = ERROR_PATH_NOT_FOUND; + return HRESULT_FROM_WIN32(err); + } + else if (res & FILE_ATTRIBUTE_DIRECTORY) + return S_OK; + else + return 0x8007010b;
0x10b == 267, therefore 0x8007010b is HRESULT_FROM_WIN32(ERROR_DIRECTORY)
--- a/dlls/shell32/tests/shpathprep.c +++ b/dlls/shell32/tests/shpathprep.c @@ -82,27 +82,27 @@ START_TEST(shpathprep) ok(res == S_OK, "res == 0x%08x\n", res);
res = SHPathPrepareForWriteA(0, 0, "c:\\testdir\\nonexistent", SHPPFW_NONE); - todo_wine ok(res == 0x80070003, "res == 0x%08x\n", res); + ok(res == 0x80070003, "res == 0x%08x\n", res);
Please use above and in other places symbolic error names instead of hex, and print expected and received codes to make life easier for those who looks at the (failing) test results without looking at the source. For some reason your mailer attaches the patch in text and html formats, it would save some time/traffic by avoiding html in wine-patches. -- Dmitry.
On 9/2/07, Dmitry Timoshkov <dmitry(a)codeweavers.com> wrote:
It's better to not use fixed buffer sizes but allocate the buffer dynamically. OK. I avoided that because I didn't want to have to worry about freeing the buffer later when it's only needed in some cases.
If the job must be done only when SHPPFW_IGNOREFILENAME is specified then don't do it in all cases. And using strrchrW would simplify the code a bit. OK.
Why there is no check for SHCreateDirectoryExW return value? The return value of SHCreateDirectoryExW doesn't matter. All that matters is whether the directory exists when the function returns. It may or may not have existed when the function was called (SHCreateDirectoryExW fails when that happens, but SHPathPrepareForWrite should not fail).
I could check the SHCreateDirectoryExW return value and probably decide on a result based on that, but I think it's better to write the result-deciding logic only once.
0x10b == 267, therefore 0x8007010b is HRESULT_FROM_WIN32(ERROR_DIRECTORY) <snip> Please use above and in other places symbolic error names instead of hex, and print expected and received codes to make life easier for those who looks at the (failing) test results without looking at the source. OK.
For some reason your mailer attaches the patch in text and html formats, it would save some time/traffic by avoiding html in wine-patches. *checks*
So it does. I should be able to avoid that now that I know to look for it. -- Vincent Povirk
"Vincent Povirk" <madewokherd+d41d(a)gmail.com> wrote:
Why there is no check for SHCreateDirectoryExW return value? The return value of SHCreateDirectoryExW doesn't matter. All that matters is whether the directory exists when the function returns. It may or may not have existed when the function was called (SHCreateDirectoryExW fails when that happens, but SHPathPrepareForWrite should not fail).
I could check the SHCreateDirectoryExW return value and probably decide on a result based on that, but I think it's better to write the result-deciding logic only once.
By checking return value of SHCreateDirectoryExW you can make a shortcut in the deciding logic, in both cases: when the function fails you exit right away (returning *correct* error to the caller and not a guessed one), in the case of success GetFileAttributes call is redundant and you exit right away again. -- Dmitry.
On 9/2/07, Dmitry Timoshkov <dmitry(a)codeweavers.com> wrote:
"Vincent Povirk" <madewokherd+d41d(a)gmail.com> wrote:
Why there is no check for SHCreateDirectoryExW return value? The return value of SHCreateDirectoryExW doesn't matter. All that matters is whether the directory exists when the function returns. It may or may not have existed when the function was called (SHCreateDirectoryExW fails when that happens, but SHPathPrepareForWrite should not fail).
I could check the SHCreateDirectoryExW return value and probably decide on a result based on that, but I think it's better to write the result-deciding logic only once.
By checking return value of SHCreateDirectoryExW you can make a shortcut in the deciding logic, in both cases: when the function fails you exit right away (returning *correct* error to the caller and not a guessed one), in the case of success GetFileAttributes call is redundant and you exit right away again.
After looking into this a bit more, yes, one of those calls will always be sufficient to do the work and determine what the result should be, at least in the cases I tested. Which one depends on flags and whether the path is relative (to my knowledge no apps actually use relative paths). -- Vincent Povirk
On 9/2/07, Vincent Povirk <madewokherd+d41d(a)gmail.com> wrote:
On 9/2/07, Dmitry Timoshkov <dmitry(a)codeweavers.com> wrote:
By checking return value of SHCreateDirectoryExW you can make a shortcut in the deciding logic, in both cases: when the function fails you exit right away (returning *correct* error to the caller and not a guessed one), in the case of success GetFileAttributes call is redundant and you exit right away again.
After looking into this a bit more, yes, one of those calls will always be sufficient to do the work and determine what the result should be, at least in the cases I tested. Which one depends on flags and whether the path is relative (to my knowledge no apps actually use relative paths).
Well, I COULD do that if SHCreateDirectoryExW worked the way the Wine API docs claim. It seems to be returning ERROR_ALREADY_EXISTS even when the thing that already exists is a file and not a directory. -- Vincent Povirk
"Vincent Povirk" <madewokherd+d41d(a)gmail.com> wrote:
Well, I COULD do that if SHCreateDirectoryExW worked the way the Wine API docs claim. It seems to be returning ERROR_ALREADY_EXISTS even when the thing that already exists is a file and not a directory.
Then it needs a separate test case and a fix. -- Dmitry.
It appears Wine's SHCreateDirectoryEx is the same as Windows XP in the way that matters to me: both return ERROR_ALREADY_EXISTS whether the thing that already exists is a file or a directory. I haven't found a situation where either version returns ERROR_FILE_EXISTS. On 9/2/07, Dmitry Timoshkov <dmitry(a)codeweavers.com> wrote:
"Vincent Povirk" <madewokherd+d41d(a)gmail.com> wrote:
Well, I COULD do that if SHCreateDirectoryExW worked the way the Wine API docs claim. It seems to be returning ERROR_ALREADY_EXISTS even when the thing that already exists is a file and not a directory.
Then it needs a separate test case and a fix.
-- Dmitry.
-- Vincent Povirk
participants (2)
-
Dmitry Timoshkov -
Vincent Povirk