[PATCH v2 0/1] MR6347: advpack: DelNode support the ADN_DEL_IF_EMPTY flag
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
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
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
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) 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) 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
participants (4)
-
anders -
Anders Kjersem (@anders) -
Elizabeth Figura (@zfigura) -
Marvin