[PATCH] shell32: SHCreateDirectoryExW no CANCELLED for ALREADY_EXISTS

[John Thomson]

If SHCreateDirectoryExW is passed a non-null hWnd,
but the directory already exists,
ERROR_CANCELLED is returned, rather than ERROR_ALREADY_EXISTS

An application relying on an expected return code
may not proceed given the 0x4C7 1223 ERROR_CANCELLED,
rather than a 0xB7 183 ERROR_ALREADY_EXISTS

Warframe in-game screenshots (F6) demonstrates this behaviour

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=47023
Signed-off-by: John Thomson 
---
My demo program of SHCreateDirectoryExW with a Hwnd
returned a 183 on Windows 7, and a 1223 in wine.
I would like someone else to confirm this.

I am also unsure whether ERROR_FILE_EXISTS should also be returned
as another exception to the rule.
---
 dlls/shell32/shlfileop.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/dlls/shell32/shlfileop.c b/dlls/shell32/shlfileop.c
index c7bd54d200..1515ad3891 100644
--- a/dlls/shell32/shlfileop.c
+++ b/dlls/shell32/shlfileop.c
@@ -763,7 +763,9 @@ int WINAPI SHCreateDirectoryExW(HWND hWnd, LPCWSTR path, LPSECURITY_ATTRIBUTES s
        }
      }

-	  if (ret && hWnd && (ERROR_CANCELLED != ret))
+	  if (ret && hWnd &&
+	      ret != ERROR_CANCELLED &&
+	      ret != ERROR_ALREADY_EXISTS)
      {
        /* We failed and should show a dialog box */
        FIXME("Show system error message, creating path %s, failed with error %d\n", debugstr_w(path), ret);

[Zhiyi Zhang]

On 4/16/19 9:57 AM, John Thomson wrote:
> If SHCreateDirectoryExW is passed a non-null hWnd,
> but the directory already exists,
> ERROR_CANCELLED is returned, rather than ERROR_ALREADY_EXISTS
>
> An application relying on an expected return code
> may not proceed given the 0x4C7 1223 ERROR_CANCELLED,
> rather than a 0xB7 183 ERROR_ALREADY_EXISTS
>
> Warframe in-game screenshots (F6) demonstrates this behaviour
>
> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=47023
> Signed-off-by: John Thomson
>
> My demo program of SHCreateDirectoryExW with a Hwnd
> returned a 183 on Windows 7, and a 1223 in wine.
> I would like someone else to confirm this.
>
> I am also unsure whether ERROR_FILE_EXISTS should also be returned
> as another exception to the rule.

You can write some conformance tests and verify the behavior on TestBots(https://testbot.winehq.org/)

Thanks.
Zhiyi
> dlls/shell32/shlfileop.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/dlls/shell32/shlfileop.c b/dlls/shell32/shlfileop.c
> index c7bd54d200..1515ad3891 100644
> --- a/dlls/shell32/shlfileop.c
> +++ b/dlls/shell32/shlfileop.c
> @@ -763,7 +763,9 @@ int WINAPI SHCreateDirectoryExW(HWND hWnd, LPCWSTR path, LPSECURITY_ATTRIBUTES s
>        }
>      }
> -
>  if (ret && hWnd && (ERROR_CANCELLED != ret))
> -
>  if (ret && hWnd &&
> -
>      ret != ERROR_CANCELLED &&
> -
>      ret != ERROR_ALREADY_EXISTS)
> {
>      /* We failed and should show a dialog box */
>      FIXME("Show system error message, creating path %s, failed with error %d\n", debugstr_w(path), ret);

[John Thomson]

On Tue, 16 Apr 2019, at 12:20, Zhiyi Zhang wrote:
> You can write some conformance tests and verify the behavior on 
> TestBots(https://testbot.winehq.org/)

Thanks!

I submitted another email with just the tests, sorry for the noise.
I was not sure if a reply patch would be picked up?
It looks like it worked on Windows.
I manually downloaded the built exe, and it failed on unpatched Wine.

I wasn't able to build the Windows test executable with mingw myself.
Has make crosstest changed, or it is just me?
> "SHCreateDirectoryExW returned %d. Expected %d",

I missed a newline at the end of one of the ok strings

Should I add this newline, squash both of my commits, and submit a new version of this patch?
Any other hints or comments?

Thank you!

[Jactry Zeng]

Hi John,
On Tue, Apr 16, 2019 at 5:59 AM John Thomson <
[e-mail]> wrote:
> > "SHCreateDirectoryExW returned %d. Expected %d",
>
> I missed a newline at the end of one of the ok strings
>
> Should I add this newline, squash both of my commits, and submit a new

version of this patch?
> Any other hints or comments?

Yes, you need to send a new try.
And you will need to send your fixing and tests in one patch, or at least
in a same series.

You can check the status of your patches in
https://source.winehq.org/patches/ .

https://wiki.winehq.org/Submitting_Patches

[Zhiyi Zhang]

On 4/16/19 1:58 PM, John Thomson wrote:
> On Tue, 16 Apr 2019, at 12:20, Zhiyi Zhang wrote:
> > You can write some conformance tests and verify the behavior on 
> > TestBots(https://testbot.winehq.org/)
>
> Thanks!
>
> I submitted another email with just the tests, sorry for the noise.
> I was not sure if a reply patch would be picked up?
> It looks like it worked on Windows.
> I manually downloaded the built exe, and it failed on unpatched Wine.
>
> I wasn't able to build the Windows test executable with mingw myself.
> Has make crosstest changed, or it is just me?

crosstest is gone as part of the building PE dll changes.

make -C dlls/shell32/tests and copy the shell32_test.exe in dlls/shell32/tests to test it on windows.

You can also submit the patch directly to Testbot, which I would recommend.

Your tests failed. Please fix it.
> > "SHCreateDirectoryExW returned %d. Expected %d",
>
> I missed a newline at the end of one of the ok strings
>
> Should I add this newline, squash both of my commits, and submit a new version of this patch?
> Any other hints or comments?

You can send a new version and add a v2 to the subject for example.
> Thank you!

[PATCH v2] shell32: SHCreateDirectoryExW no CANCELLED for ALREADY_EXISTS

[John Thomson]

If SHCreateDirectoryExW is passed a non-null hWnd,
but the directory already exists,
ERROR_CANCELLED is returned in Wine,
rather than ERROR_ALREADY_EXISTS,
the Windows behaviour.

An application relying on an expected return code
may not proceed given the 0x4C7 1223 ERROR_CANCELLED,
rather than a 0xB7 183 ERROR_ALREADY_EXISTS

Warframe in-game screenshots (F6) demonstrates this behaviour

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=47023
Signed-off-by: John Thomson 
---
v2:
Add Test for conformance of SHCreateDirectoryExW with a Hwnd
when target directory already exists
---
 dlls/shell32/shlfileop.c       |  4 +++-
 dlls/shell32/tests/shlfileop.c | 16 ++++++++++++++++
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/dlls/shell32/shlfileop.c b/dlls/shell32/shlfileop.c
index c7bd54d200..1515ad3891 100644
--- a/dlls/shell32/shlfileop.c
+++ b/dlls/shell32/shlfileop.c
@@ -763,7 +763,9 @@ int WINAPI SHCreateDirectoryExW(HWND hWnd, LPCWSTR path, LPSECURITY_ATTRIBUTES s
        }
      }

-	  if (ret && hWnd && (ERROR_CANCELLED != ret))
+	  if (ret && hWnd &&
+	      ret != ERROR_CANCELLED &&
+	      ret != ERROR_ALREADY_EXISTS)
      {
        /* We failed and should show a dialog box */
        FIXME("Show system error message, creating path %s, failed with error %d\n", debugstr_w(path), ret);
diff --git a/dlls/shell32/tests/shlfileop.c b/dlls/shell32/tests/shlfileop.c
index 8fb4dcf66b..d43579329e 100644
--- a/dlls/shell32/tests/shlfileop.c
+++ b/dlls/shell32/tests/shlfileop.c
@@ -2543,6 +2543,7 @@ static void test_unicode(void)
     int ret;
     HANDLE file;
     static const WCHAR UNICODE_PATH_TO[] = {'c',':','\',0x00ae,0x00ae,'\0'};
+    HWND hwnd;

shfoW.hwnd = NULL;
     shfoW.wFunc = FO_DELETE;
@@ -2633,6 +2634,21 @@ static void test_unicode(void)
     ok(GetLastError() == ERROR_SUCCESS ||
        broken(GetLastError() == ERROR_INVALID_HANDLE), /* WinXp, win2k3 */
        "Expected ERROR_SUCCESS, got %d\n", GetLastError());
+
+    /* Check SHCreateDirectoryExW with a Hwnd
+     * returns ERROR_ALREADY_EXISTS where a directory already exists */
+    /* Get any window handle */
+    hwnd = FindWindowA(NULL, NULL);
+    ok(hwnd, "FindWindowA failed to produce a hwnd");
+    ret = SHCreateDirectoryExW(hwnd, UNICODE_PATH, NULL);
+    ok(!ret, "SHCreateDirectoryExW returned %d\n", ret);
+    /* Create already-existing directory */
+    ok(file_existsW(UNICODE_PATH), "The directory was not created\n");
+    ret = SHCreateDirectoryExW(hwnd, UNICODE_PATH, NULL);
+    ok(ret == ERROR_ALREADY_EXISTS,
+       "SHCreateDirectoryExW returned %d. Expected ALREADY EXISTS %d\n",
+       ret, ERROR_ALREADY_EXISTS);
+    RemoveDirectoryW(UNICODE_PATH);
 }

static void

[PATCH v3] shell32: SHCreateDirectoryExW no CANCELLED for ALREADY_EXISTS

[John Thomson]

If SHCreateDirectoryExW is passed a non-null hWnd,
but the directory already exists,
ERROR_CANCELLED is returned in Wine,
rather than ERROR_ALREADY_EXISTS,
the Windows behaviour.

An application relying on an expected return code
may not proceed given the 0x4C7 1223 ERROR_CANCELLED,
rather than a 0xB7 183 ERROR_ALREADY_EXISTS

Warframe in-game screenshots (F6) demonstrates this behaviour

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=47023
Signed-off-by: John Thomson 
---
v2:
Add Test for conformance of SHCreateDirectoryExW with a Hwnd
when target directory already exists

v3:
Add missing newline on ok test hwnd fail string. Thanks Gijs
---
 dlls/shell32/shlfileop.c       |  4 +++-
 dlls/shell32/tests/shlfileop.c | 16 ++++++++++++++++
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/dlls/shell32/shlfileop.c b/dlls/shell32/shlfileop.c
index c7bd54d200..1515ad3891 100644
--- a/dlls/shell32/shlfileop.c
+++ b/dlls/shell32/shlfileop.c
@@ -763,7 +763,9 @@ int WINAPI SHCreateDirectoryExW(HWND hWnd, LPCWSTR path, LPSECURITY_ATTRIBUTES s
        }
      }

-	  if (ret && hWnd && (ERROR_CANCELLED != ret))
+	  if (ret && hWnd &&
+	      ret != ERROR_CANCELLED &&
+	      ret != ERROR_ALREADY_EXISTS)
      {
        /* We failed and should show a dialog box */
        FIXME("Show system error message, creating path %s, failed with error %d\n", debugstr_w(path), ret);
diff --git a/dlls/shell32/tests/shlfileop.c b/dlls/shell32/tests/shlfileop.c
index 8fb4dcf66b..62f2cb3d6a 100644
--- a/dlls/shell32/tests/shlfileop.c
+++ b/dlls/shell32/tests/shlfileop.c
@@ -2543,6 +2543,7 @@ static void test_unicode(void)
     int ret;
     HANDLE file;
     static const WCHAR UNICODE_PATH_TO[] = {'c',':','\',0x00ae,0x00ae,'\0'};
+    HWND hwnd;

shfoW.hwnd = NULL;
     shfoW.wFunc = FO_DELETE;
@@ -2633,6 +2634,21 @@ static void test_unicode(void)
     ok(GetLastError() == ERROR_SUCCESS ||
        broken(GetLastError() == ERROR_INVALID_HANDLE), /* WinXp, win2k3 */
        "Expected ERROR_SUCCESS, got %d\n", GetLastError());
+
+    /* Check SHCreateDirectoryExW with a Hwnd
+     * returns ERROR_ALREADY_EXISTS where a directory already exists */
+    /* Get any window handle */
+    hwnd = FindWindowA(NULL, NULL);
+    ok(hwnd, "FindWindowA failed to produce a hwnd\n");
+    ret = SHCreateDirectoryExW(hwnd, UNICODE_PATH, NULL);
+    ok(!ret, "SHCreateDirectoryExW returned %d\n", ret);
+    /* Create already-existing directory */
+    ok(file_existsW(UNICODE_PATH), "The directory was not created\n");
+    ret = SHCreateDirectoryExW(hwnd, UNICODE_PATH, NULL);
+    ok(ret == ERROR_ALREADY_EXISTS,
+       "SHCreateDirectoryExW returned %d. Expected ALREADY EXISTS %d\n",
+       ret, ERROR_ALREADY_EXISTS);
+    RemoveDirectoryW(UNICODE_PATH);
 }

static void

[PATCH v4] shell32: SHCreateDirectoryExW no CANCELLED for ALREADY_EXISTS

[John Thomson]

If SHCreateDirectoryExW is passed a non-null hWnd,
but the directory already exists,
ERROR_CANCELLED is returned in Wine,
rather than ERROR_ALREADY_EXISTS,
the Windows behaviour.

An application relying on an expected return code
may not proceed given the 0x4C7 1223 ERROR_CANCELLED,
rather than a 0xB7 183 ERROR_ALREADY_EXISTS

Warframe in-game screenshots (F6) demonstrates this behaviour

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=47023
Signed-off-by: John Thomson 
---
v2:
Add Test for conformance of SHCreateDirectoryExW with a Hwnd
when target directory already exists

v3:
Add missing newline on ok test hwnd fail string. Thanks Gijs

v4:
Made SHCreateDirectoryExW with Hwnd test already existing directory
fail string consistent with existing fail strings. Thanks Gijs
---
 dlls/shell32/shlfileop.c       |  4 +++-
 dlls/shell32/tests/shlfileop.c | 15 +++++++++++++++
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/dlls/shell32/shlfileop.c b/dlls/shell32/shlfileop.c
index c7bd54d200..1515ad3891 100644
--- a/dlls/shell32/shlfileop.c
+++ b/dlls/shell32/shlfileop.c
@@ -763,7 +763,9 @@ int WINAPI SHCreateDirectoryExW(HWND hWnd, LPCWSTR path, LPSECURITY_ATTRIBUTES s
        }
      }

-	  if (ret && hWnd && (ERROR_CANCELLED != ret))
+	  if (ret && hWnd &&
+	      ret != ERROR_CANCELLED &&
+	      ret != ERROR_ALREADY_EXISTS)
      {
        /* We failed and should show a dialog box */
        FIXME("Show system error message, creating path %s, failed with error %d\n", debugstr_w(path), ret);
diff --git a/dlls/shell32/tests/shlfileop.c b/dlls/shell32/tests/shlfileop.c
index 8fb4dcf66b..474f5fd790 100644
--- a/dlls/shell32/tests/shlfileop.c
+++ b/dlls/shell32/tests/shlfileop.c
@@ -2543,6 +2543,7 @@ static void test_unicode(void)
     int ret;
     HANDLE file;
     static const WCHAR UNICODE_PATH_TO[] = {'c',':','\',0x00ae,0x00ae,'\0'};
+    HWND hwnd;

shfoW.hwnd = NULL;
     shfoW.wFunc = FO_DELETE;
@@ -2633,6 +2634,20 @@ static void test_unicode(void)
     ok(GetLastError() == ERROR_SUCCESS ||
        broken(GetLastError() == ERROR_INVALID_HANDLE), /* WinXp, win2k3 */
        "Expected ERROR_SUCCESS, got %d\n", GetLastError());
+
+    /* Check SHCreateDirectoryExW with a Hwnd
+     * returns ERROR_ALREADY_EXISTS where a directory already exists */
+    /* Get any window handle */
+    hwnd = FindWindowA(NULL, NULL);
+    ok(hwnd, "FindWindowA failed to produce a hwnd\n");
+    ret = SHCreateDirectoryExW(hwnd, UNICODE_PATH, NULL);
+    ok(!ret, "SHCreateDirectoryExW returned %d\n", ret);
+    /* Create already-existing directory */
+    ok(file_existsW(UNICODE_PATH), "The directory was not created\n");
+    ret = SHCreateDirectoryExW(hwnd, UNICODE_PATH, NULL);
+    ok(ret == ERROR_ALREADY_EXISTS,
+       "Expected ERROR_ALREADY_EXISTS, got %d\n", ret);
+    RemoveDirectoryW(UNICODE_PATH);
 }

static void