[PATCH v2 0/2] MR5020: Draft: kernelbase/file: implement CopyFile2
copy implementation of CopyFileExW into CopyFile2, then implement CopyFileExW in terms of CopyFile2 should fix https://bugs.winehq.org/show_bug.cgi?id=55897, as Python does not use any new functionality nor the callback parameter from CopyFile2. - followed [the Wiki](https://wiki.winehq.org/Developer_Hints#Implementing_new_API_calls) for adding a new API call - run `file.c` tests and they seem to pass I have not written extensive documentation since the original function didn't seem to have any; I can do that if recommended. I originally intended to implement the callback functionality in this MR as well, but realized that would be better as a follow-up. -- v2: - replace BOOL with HRESULT https://gitlab.winehq.org/wine/wine/-/merge_requests/5020
From: Kartavya Vashishtha <sendtokartavya(a)gmail.com> implement CopyFileExW in terms of CopyFile2 --- dlls/kernelbase/file.c | 47 +++++++++++++++++++++++---------- dlls/kernelbase/kernelbase.spec | 2 +- 2 files changed, 34 insertions(+), 15 deletions(-) diff --git a/dlls/kernelbase/file.c b/dlls/kernelbase/file.c index e1ba92a6448..48e2af85005 100644 --- a/dlls/kernelbase/file.c +++ b/dlls/kernelbase/file.c @@ -487,13 +487,14 @@ BOOL WINAPI DECLSPEC_HOTPATCH AreFileApisANSI(void) return !oem_file_apis; } - /*********************************************************************** - * CopyFileExW (kernelbase.@) + * CopyFile2 (kernelbase.@) + * + * FIXME: Silently ignores: + * the callback function + * cancel_ptr */ -BOOL WINAPI CopyFileExW( const WCHAR *source, const WCHAR *dest, LPPROGRESS_ROUTINE progress, - void *param, BOOL *cancel_ptr, DWORD flags ) -{ +HRESULT WINAPI CopyFile2 ( const PCWSTR source , const PCWSTR dest, COPYFILE2_EXTENDED_PARAMETERS* params) { static const int buffer_size = 65536; HANDLE h1, h2; FILE_BASIC_INFORMATION info; @@ -502,15 +503,15 @@ BOOL WINAPI CopyFileExW( const WCHAR *source, const WCHAR *dest, LPPROGRESS_ROUT BOOL ret = FALSE; char *buffer; + DWORD flags = params? params->dwCopyFlags : 0; + if (!source || !dest) { - SetLastError( ERROR_INVALID_PARAMETER ); - return FALSE; + return HRESULT_FROM_WIN32( ERROR_INVALID_PARAMETER ); } if (!(buffer = HeapAlloc( GetProcessHeap(), 0, buffer_size ))) { - SetLastError( ERROR_NOT_ENOUGH_MEMORY ); - return FALSE; + return HRESULT_FROM_WIN32( ERROR_NOT_ENOUGH_MEMORY ); } TRACE("%s -> %s, %lx\n", debugstr_w(source), debugstr_w(dest), flags); @@ -527,7 +528,7 @@ BOOL WINAPI CopyFileExW( const WCHAR *source, const WCHAR *dest, LPPROGRESS_ROUT { WARN("Unable to open source %s\n", debugstr_w(source)); HeapFree( GetProcessHeap(), 0, buffer ); - return FALSE; + return HRESULT_FROM_WIN32( ERROR_INVALID_PARAMETER ); } if (!set_ntstatus( NtQueryInformationFile( h1, &io, &info, sizeof(info), FileBasicInformation ))) @@ -535,7 +536,7 @@ BOOL WINAPI CopyFileExW( const WCHAR *source, const WCHAR *dest, LPPROGRESS_ROUT WARN("GetFileInformationByHandle returned error for %s\n", debugstr_w(source)); HeapFree( GetProcessHeap(), 0, buffer ); CloseHandle( h1 ); - return FALSE; + return HRESULT_FROM_WIN32( GetLastError() ); } if (!(flags & COPY_FILE_FAIL_IF_EXISTS)) @@ -551,8 +552,7 @@ BOOL WINAPI CopyFileExW( const WCHAR *source, const WCHAR *dest, LPPROGRESS_ROUT { HeapFree( GetProcessHeap(), 0, buffer ); CloseHandle( h1 ); - SetLastError( ERROR_SHARING_VIOLATION ); - return FALSE; + return HRESULT_FROM_WIN32( ERROR_SHARING_VIOLATION ); } } @@ -563,7 +563,7 @@ BOOL WINAPI CopyFileExW( const WCHAR *source, const WCHAR *dest, LPPROGRESS_ROUT WARN("Unable to open dest %s\n", debugstr_w(dest)); HeapFree( GetProcessHeap(), 0, buffer ); CloseHandle( h1 ); - return FALSE; + return HRESULT_FROM_WIN32( GetLastError() ); } while (ReadFile( h1, buffer, buffer_size, &count, NULL ) && count) @@ -590,6 +590,25 @@ done: } +/*********************************************************************** + * CopyFileExW (kernelbase.@) + */ +BOOL WINAPI CopyFileExW( const WCHAR *source, const WCHAR *dest, LPPROGRESS_ROUTINE progress, + void *param, BOOL *cancel_ptr, DWORD flags ) +{ + BOOL ret; + COPYFILE2_EXTENDED_PARAMETERS params; + + params.dwSize = sizeof(params); + params.dwCopyFlags = flags; + + ret = CopyFile2( source, dest, ¶ms ); + if (ret) SetLastError( 0 ); + + return ret; +} + + /************************************************************************** * CopyFileW (kernelbase.@) */ diff --git a/dlls/kernelbase/kernelbase.spec b/dlls/kernelbase/kernelbase.spec index ffb153a46ee..bde4bc66bde 100644 --- a/dlls/kernelbase/kernelbase.spec +++ b/dlls/kernelbase/kernelbase.spec @@ -168,7 +168,7 @@ @ stdcall ConvertThreadToFiberEx(ptr long) @ stdcall ConvertToAutoInheritPrivateObjectSecurity(ptr ptr ptr ptr long ptr) @ stdcall CopyContext(ptr long ptr) -# @ stub CopyFile2 +@ stdcall CopyFile2(wstr wstr ptr) @ stdcall CopyFileExW(wstr wstr ptr ptr ptr long) @ stdcall CopyFileW(wstr wstr long) @ stdcall -arch=x86_64 CopyMemoryNonTemporal(ptr ptr long) ntdll.RtlCopyMemoryNonTemporal -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5020
From: Kartavya Vashishtha <sendtokartavya(a)gmail.com> - try using ERROR_PATH_NOT_FOUND when source or dest is NULL (due to tests/file.c:1185) - change source file open error to ERROR_FILE_NOT_FOUND, per MSDN documentation for CopyFile2 --- dlls/kernelbase/file.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/dlls/kernelbase/file.c b/dlls/kernelbase/file.c index 48e2af85005..cc21f11117a 100644 --- a/dlls/kernelbase/file.c +++ b/dlls/kernelbase/file.c @@ -503,11 +503,11 @@ HRESULT WINAPI CopyFile2 ( const PCWSTR source , const PCWSTR dest, COPYFILE2_EX BOOL ret = FALSE; char *buffer; - DWORD flags = params? params->dwCopyFlags : 0; + DWORD flags = params ? params->dwCopyFlags : 0; if (!source || !dest) { - return HRESULT_FROM_WIN32( ERROR_INVALID_PARAMETER ); + return HRESULT_FROM_WIN32( ERROR_PATH_NOT_FOUND ); } if (!(buffer = HeapAlloc( GetProcessHeap(), 0, buffer_size ))) { @@ -528,7 +528,7 @@ HRESULT WINAPI CopyFile2 ( const PCWSTR source , const PCWSTR dest, COPYFILE2_EX { WARN("Unable to open source %s\n", debugstr_w(source)); HeapFree( GetProcessHeap(), 0, buffer ); - return HRESULT_FROM_WIN32( ERROR_INVALID_PARAMETER ); + return HRESULT_FROM_WIN32( ERROR_FILE_NOT_FOUND ); } if (!set_ntstatus( NtQueryInformationFile( h1, &io, &info, sizeof(info), FileBasicInformation ))) @@ -577,7 +577,7 @@ HRESULT WINAPI CopyFile2 ( const PCWSTR source , const PCWSTR dest, COPYFILE2_EX count -= res; } } - ret = TRUE; + ret = S_OK; done: /* Maintain the timestamp of source file to destination file */ info.FileAttributes = 0; @@ -585,7 +585,6 @@ done: HeapFree( GetProcessHeap(), 0, buffer ); CloseHandle( h1 ); CloseHandle( h2 ); - if (ret) SetLastError( 0 ); return ret; } @@ -596,16 +595,17 @@ done: BOOL WINAPI CopyFileExW( const WCHAR *source, const WCHAR *dest, LPPROGRESS_ROUTINE progress, void *param, BOOL *cancel_ptr, DWORD flags ) { - BOOL ret; + HRESULT ret; COPYFILE2_EXTENDED_PARAMETERS params; params.dwSize = sizeof(params); params.dwCopyFlags = flags; ret = CopyFile2( source, dest, ¶ms ); - if (ret) SetLastError( 0 ); - - return ret; + if (ret != S_OK) { + SetLastError( ret ); + } + return SUCCEEDED(ret); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5020
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 tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=142763 Your paranoid android. === debian11b (64 bit WoW report) === kernel32: file.c:700: Test failed: copying a file to itself didn't fail (ret=0, err=-2147024864) file.c:724: Test failed: CopyFileA: unexpected error -2147024816 file.c:732: Test failed: CopyFileA: ret = 0, unexpected error -2147024894 file.c:735: Test failed: CopyFileA: ret = 0, unexpected error -2147024893 file.c:742: Test failed: copying from a read-locked file succeeded when it shouldn't have file.c:746: Test failed: copying from a file that doesn't exist failed in an unexpected way (ret=0, err=-2147024894) file.c:770: Test failed: copying to a write-locked file didn't fail (ret=0, err=-2147024864) file.c:789: Test failed: copying to a delete-locked shared file didn't fail (ret=0, err=-2147024864) file.c:815: Test failed: CopyFileA: ret = 0, unexpected error -2147024864 file.c:831: Test failed: CopyFileA with mapped dest file: expected ERROR_SHARING_VIOLATION, got -2147024864 file.c:846: Test failed: CopyFileA with mapped dest file: expected ERROR_USER_MAPPED_FILE, got -2147023672 file.c:882: Test failed: CopyFileW: unexpected error -2147024816 file.c:888: Test failed: Unexpected error 183. file.c:894: Test failed: Unexpected error 183. file.c:1185: Test failed: expected ERROR_PATH_NOT_FOUND, got -2147024893 file.c:1188: Test failed: expected ERROR_PATH_NOT_FOUND, got -2147024894 msi: install.c:3461: Test failed: Expected ERROR_SUCCESS, got 1603 install.c:3462: Test failed: Expected file to be overwritten install.c:3463: Test failed: File not installed install.c:3464: Test failed: Directory not created install.c:5375: Test failed: Expected ERROR_SUCCESS_REBOOT_REQUIRED got 1603 install.c:5312: Test failed: Expected a 'msitest' entry install.c:5330: Test failed: Expected file to match msvcp120: msvcp120.c:1445: Test failed: test_tr2_sys__Copy_file(): test 3 expect: 80, got -2147024816 msvcp120.c:1445: Test failed: test_tr2_sys__Copy_file(): test 5 expect: 5, got -2147024894 msvcp120.c:1445: Test failed: test_tr2_sys__Copy_file(): test 6 expect: 5, got -2147024894 msvcp120.c:1445: Test failed: test_tr2_sys__Copy_file(): test 9 expect: 2, got -2147024894 msvcp120.c:1445: Test failed: test_tr2_sys__Copy_file(): test 10 expect: 3, got -2147024893 msvcp120.c:1445: Test failed: test_tr2_sys__Copy_file(): test 11 expect: 5, got -2147024816 msvcp140: msvcp140.c:1609: Test failed: Got unexpected ret 2147942405. msvcp140.c:1614: Test failed: Got unexpected ret 2147942405. msvcp140.c:1626: Test failed: Got unexpected err 183. msvcp140.c:1631: Test failed: Got unexpected ret 2147942402. setupapi: install.c:1351: Test failed: Got SourcePath 'src\alpha\alpha'. install.c:1886: Test failed: Got 2 copy errors. install.c:1887: Test failed: Destination file should exist. install.c:1888: Test failed: Destination file should exist. install.c:1492: Test failed: Got unexpected error 0x80070002. shell32: shlfileop.c:1031: Test failed: CopyFileA should have fail with ERROR_ACCESS_DENIED
Alfred Agrell (@Alcaro) commented about dlls/kernelbase/file.c:
return !oem_file_apis; }
- /*********************************************************************** - * CopyFileExW (kernelbase.@) + * CopyFile2 (kernelbase.@) + * + * FIXME: Silently ignores:
Personally I'd check if they're nonnull, and if yes, print a FIXME(). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5020#note_59991
Alfred Agrell (@Alcaro) commented about dlls/kernelbase/file.c:
- if (ret) SetLastError( 0 ); return ret; }
+/*********************************************************************** + * CopyFileExW (kernelbase.@) + */ +BOOL WINAPI CopyFileExW( const WCHAR *source, const WCHAR *dest, LPPROGRESS_ROUTINE progress, + void *param, BOOL *cancel_ptr, DWORD flags ) +{ + HRESULT ret; + COPYFILE2_EXTENDED_PARAMETERS params; + + params.dwSize = sizeof(params); + params.dwCopyFlags = flags; Maybe FIXME progress/cancel here too if nonnull? (Ideally they'd be placed in the params, but PCOPYFILE2_PROGRESS_ROUTINE is quite different from LPPROGRESS_ROUTINE, and writing a converter wrapper that CopyFile2 never calls would be difficult to test, so better not.)
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5020#note_59992
Alfred Agrell (@Alcaro) commented about dlls/kernelbase/file.c:
+/*********************************************************************** + * CopyFileExW (kernelbase.@) + */ +BOOL WINAPI CopyFileExW( const WCHAR *source, const WCHAR *dest, LPPROGRESS_ROUTINE progress, + void *param, BOOL *cancel_ptr, DWORD flags ) +{ + HRESULT ret; + COPYFILE2_EXTENDED_PARAMETERS params; + + params.dwSize = sizeof(params); + params.dwCopyFlags = flags; + + ret = CopyFile2( source, dest, ¶ms ); + if (ret != S_OK) { + SetLastError( ret );
SetLastError doesn't take a HRESULT. HRESULT to SetLastError feels like a function that should exist, but I can't find it; closest I can find is HRESULT_CODE, which will do something weird if CopyFile2 returns something other than S_OK or HRESULT_FROM_WIN32(something). Maybe another reviewer can enlighten us? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5020#note_59993
Alfred Agrell (@Alcaro) commented about dlls/kernelbase/file.c:
HeapFree( GetProcessHeap(), 0, buffer ); CloseHandle( h1 ); CloseHandle( h2 ); - if (ret) SetLastError( 0 ); return ret; }
+/*********************************************************************** + * CopyFileExW (kernelbase.@) + */ +BOOL WINAPI CopyFileExW( const WCHAR *source, const WCHAR *dest, LPPROGRESS_ROUTINE progress, + void *param, BOOL *cancel_ptr, DWORD flags ) +{ + HRESULT ret; + COPYFILE2_EXTENDED_PARAMETERS params;
This struct is partially uninitialized. But CopyFile2 currently doesn't read the unwritten fields, so I don't know if that's considered a problem. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5020#note_59994
I'm just some random guy, not a maintainer, so feel free to ignore me if you feel I'm overreacting or otherwise wrong. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5020#note_59995
On Sun Feb 4 19:50:38 2024 +0000, Alfred Agrell wrote:
SetLastError doesn't take a HRESULT. HRESULT to SetLastError feels like a function that should exist, but I can't find it; closest I can find is HRESULT_CODE, which will do something weird if CopyFile2 returns something other than S_OK or HRESULT_FROM_WIN32(something). Maybe another reviewer can enlighten us? I discovered that through the failing tests as well. Turns out, [it's not straightforward (Raymond Chen's blog).](https://devblogs.microsoft.com/oldnewthing/20061103-07/?p=29133). Maybe having implementing the CopyFile2 functionality but with `SetLastError`-style error handling, and then write a wrapper over it for `CopyFile2` to convert to HRESULT?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5020#note_59996
[This structure](https://www.mail-archive.com/wine-devel(a)winehq.org/msg80571.html) will be useful for adding the callback functionality later, and leans towards implementing `CopyFile2`, while `CopyFileEx` passes a special callback that wraps the user callback to filter and transform emitted events. The only problem with that approach, as discussed above, is error handling; CopyFileEx errors can be converted to CopyFile2 errors, but the reverse is not true. The best idea that comes to mind is `CopyFile2_impl` with WIN32 errors codes and then convert into `HRESULT` for `CopyFile2`. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5020#note_60060
On Mon Feb 5 04:44:12 2024 +0000, Kartavya Vashishtha wrote:
I discovered that through the failing tests as well. Turns out, [it's not straightforward (Raymond Chen's blog)](https://devblogs.microsoft.com/oldnewthing/20061103-07/?p=29133). Maybe having CopyFile2 functionality but with `SetLastError`-style error handling, and then write a wrapper over it for `CopyFile2` to convert to HRESULT? @Alcaro `HRESULT_CODE` actually shouldn't be too bad since [`CopyFile2` _only_ returns S_OK or `HRESULT_FROM_WIN32(..)`](https://learn.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-copyf...).
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5020#note_60061
Maybe having CopyFile2 functionality but with `SetLastError`-style error handling, and then write a wrapper over it for `CopyFile2` to convert to HRESULT?
This sounds like what I'd do, at least. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5020#note_60063
On Mon Feb 5 19:23:01 2024 +0000, Zebediah Figura wrote:
Maybe having CopyFile2 functionality but with `SetLastError`-style error handling, and then write a wrapper over it for `CopyFile2` to convert to HRESULT? This sounds like what I'd do, at least. Yes, HRESULT_CODE would work for now.
But if that changes in the future, it'll return random errors like ERROR_BAD_EXE_FORMAT. If you choose that approach, I vote check HRESULT_FACILITY(), and map everything that isn't FACILITY_WIN32 to some random hardcoded error (ERROR_CANNOT_COPY, for example). Alternatively, do your proposal from yesterday. It sounds cleaner in the long run. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5020#note_60065
participants (5)
-
Alfred Agrell (＠Alcaro) -
Kartavya Vashishtha
-
Kartavya Vashishtha (＠kvashis)
-
Marvin -
Zebediah Figura (＠zfigura)