[PATCH v2 0/1] MR6347: advpack: DelNode support the ADN_DEL_IF_EMPTY flag

[Anders Kjersem (＠anders)]

Fixes a bug where `rundll32.exe advpack.dll,DelNodeRunDLL32 "c:\test",1` is only supposed to delete "test" if the directory is empty.

Also causes the [DelDirs Inf section](https://www.mdgx.com/INF_web/deldirs.htm) to work correctly.

Notes:
- I changed the code so it ignores the result of `SetFileAttributesW` because the user might not have FILE_WRITE_ATTRIBUTES even if they have DELETE rights. This seems to match Windows (XP NTFS).

--
  v2: advpack: DelNode support ADN_DEL_IF_EMPTY flag
 
https://gitlab.winehq.org/wine/wine/-/merge_requests/6347

[PATCH v2 1/1] advpack: DelNode support ADN_DEL_IF_EMPTY flag

[anders]

From: anders <andersdev(a)proton.me>

---
 dlls/advpack/files.c         | 10 ++++++++--
 dlls/advpack/tests/advpack.c |  9 ++++++++-
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/dlls/advpack/files.c b/dlls/advpack/files.c
index 2bf2481cca4..fa247cb3f07 100644
--- a/dlls/advpack/files.c
+++ b/dlls/advpack/files.c
@@ -320,6 +320,9 @@ static HRESULT DELNODE_recurse_dirtree(LPWSTR fname, DWORD flags)
         BOOL done = TRUE;
         int fname_len = lstrlenW(fname);
 
+        if (flags & ADN_DEL_IF_EMPTY)
+            goto onlyemptydir;
+
         /* Generate a path with wildcard suitable for iterating */
         if (fname_len && fname[fname_len-1] != '\\') fname[fname_len++] = '\\';
         lstrcpyW(fname + fname_len, L"*");
@@ -347,8 +350,10 @@ static HRESULT DELNODE_recurse_dirtree(LPWSTR fname, DWORD flags)
 
         if (done)
         {
+onlyemptydir:
             TRACE("%s: directory\n", debugstr_w(fname));
-            if (SetFileAttributesW(fname, FILE_ATTRIBUTE_NORMAL) && RemoveDirectoryW(fname))
+            SetFileAttributesW(fname, FILE_ATTRIBUTE_NORMAL);
+            if (RemoveDirectoryW(fname))
             {
                 ret = S_OK;
             }
@@ -357,7 +362,8 @@ static HRESULT DELNODE_recurse_dirtree(LPWSTR fname, DWORD flags)
     else
     {
         TRACE("%s: file\n", debugstr_w(fname));
-        if (SetFileAttributesW(fname, FILE_ATTRIBUTE_NORMAL) && DeleteFileW(fname))
+        SetFileAttributesW(fname, FILE_ATTRIBUTE_NORMAL);
+        if (DeleteFileW(fname))
         {
             ret = S_OK;
         }
diff --git a/dlls/advpack/tests/advpack.c b/dlls/advpack/tests/advpack.c
index 33d5f4bc52b..7c6eaabea9f 100644
--- a/dlls/advpack/tests/advpack.c
+++ b/dlls/advpack/tests/advpack.c
@@ -160,6 +160,10 @@ static void delnode_test(void)
     hr = pDelNode(lstrcatA(currDir, "\\DelNodeTestDir"), 0);
     ok (hr == S_OK, "DelNode failed deleting an empty directory\n");
     currDir[currDirLen] = '\0';
+    CreateDirectoryA("DelNodeTestDir", NULL);
+    hr = pDelNode(lstrcatA(currDir, "\\DelNodeTestDir"), ADN_DEL_IF_EMPTY);
+    ok (hr == S_OK, "DelNode ADN_DEL_IF_EMPTY failed deleting an empty directory\n");
+    currDir[currDirLen] = '\0';
 
     /* Test deletion of a directory containing one file. */
     CreateDirectoryA("DelNodeTestDir", NULL);
@@ -167,7 +171,10 @@ static void delnode_test(void)
         CREATE_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL);
     assert(hn != INVALID_HANDLE_VALUE);
     CloseHandle(hn);
-    hr = pDelNode(lstrcatA(currDir, "\\DelNodeTestDir"), 0);
+    lstrcatA(currDir, "\\DelNodeTestDir")
+    hr = pDelNode(currDir, ADN_DEL_IF_EMPTY);
+    ok (FAILED(hr), "DelNode ADN_DEL_IF_EMPTY should not delete non-empty\n");
+    hr = pDelNode(currDir, 0);
     ok (hr == S_OK, "DelNode failed deleting a directory containing one file\n");
     currDir[currDirLen] = '\0';
 
-- 
GitLab

https://gitlab.winehq.org/wine/wine/-/merge_requests/6347

[PATCH v2 1/1] advpack: DelNode support ADN_DEL_IF_EMPTY flag

[Marvin]

Hi,

It looks like your patch introduced the new failures shown below.
Please investigate and fix them before resubmitting your patch.
If they are not new, fixing them anyway would help a lot. Otherwise
please ask for the known failures list to be updated.

The full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=147960

Your paranoid android.


=== build (build log) ===

../wine/dlls/advpack/tests/advpack.c:174:42: error: expected ‘;’ before ‘hr’
Task: The exe32 Wine build failed

=== debian11 (build log) ===

../wine/dlls/advpack/tests/advpack.c:174:42: error: expected ‘;’ before ‘hr’
Task: The win32 Wine build failed

=== debian11b (build log) ===

../wine/dlls/advpack/tests/advpack.c:174:42: error: expected ‘;’ before ‘hr’
Task: The wow64 Wine build failed

[Elizabeth Figura (＠zfigura)]

That's not a very meaningful test; the point of the flag is to prevent non-empty trees from being deleted, but you're not testing that case.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/6347#note_79798

[Elizabeth Figura (＠zfigura)]

Elizabeth Figura (@zfigura) commented about dlls/advpack/files.c:
> BOOL done = TRUE;
>          int fname_len = lstrlenW(fname);
>
> +        if (flags & ADN_DEL_IF_EMPTY)
> +            goto onlyemptydir;

Surely this can be done without a goto.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/6347#note_79799

[Elizabeth Figura (＠zfigura)]

Elizabeth Figura (@zfigura) commented about dlls/advpack/files.c:
> if (done)
>          {
> +onlyemptydir:
>              TRACE("%s: directory\n", debugstr_w(fname));
> -            if (SetFileAttributesW(fname, FILE_ATTRIBUTE_NORMAL) && RemoveDirectoryW(fname))
> +            SetFileAttributesW(fname, FILE_ATTRIBUTE_NORMAL);

That's a separate and unrelated change; it should be in its own commit and supported by tests.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/6347#note_79802