[PATCH v4 0/4] 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. -- v4: CopyFile2_impl: remove unused variable 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
From: Kartavya Vashishtha <sendtokartavya(a)gmail.com> - add FIXME messages for callback and cancel_ptr parameters in CopyFile2 and CopyFileEx - initialize all elements of CopyFile2 params --- dlls/kernelbase/file.c | 63 +++++++++++++++++++++++++++--------------- 1 file changed, 41 insertions(+), 22 deletions(-) diff --git a/dlls/kernelbase/file.c b/dlls/kernelbase/file.c index cc21f11117a..22fb00926fb 100644 --- a/dlls/kernelbase/file.c +++ b/dlls/kernelbase/file.c @@ -487,15 +487,9 @@ BOOL WINAPI DECLSPEC_HOTPATCH AreFileApisANSI(void) return !oem_file_apis; } -/*********************************************************************** - * CopyFile2 (kernelbase.@) - * - * FIXME: Silently ignores: - * the callback function - * cancel_ptr - */ -HRESULT WINAPI CopyFile2 ( const PCWSTR source , const PCWSTR dest, COPYFILE2_EXTENDED_PARAMETERS* params) { - static const int buffer_size = 65536; +// uses the SetLastError style of error handling +static BOOL CopyFile2_impl ( const PCWSTR source, const PCWSTR dest, COPYFILE2_EXTENDED_PARAMETERS* params) { + static const int buffer_size = 65536; HANDLE h1, h2; FILE_BASIC_INFORMATION info; IO_STATUS_BLOCK io; @@ -504,14 +498,24 @@ HRESULT WINAPI CopyFile2 ( const PCWSTR source , const PCWSTR dest, COPYFILE2_EX char *buffer; DWORD flags = params ? params->dwCopyFlags : 0; + BOOL *cancel_ptr = params ? params->pfCancel : NULL; + PCOPYFILE2_PROGRESS_ROUTINE progress = params ? params->pProgressRoutine : NULL; + void *callback_context = params ? params->pvCallbackContext : NULL; + + if (cancel_ptr) + FIXME("pfCancel is not supported\n"); + if (progress) + FIXME("PCOPYFILE2_PROGRESS_ROUTINE is not supported\n"); if (!source || !dest) { - return HRESULT_FROM_WIN32( ERROR_PATH_NOT_FOUND ); + SetLastError( ERROR_PATH_NOT_FOUND ); + return FALSE; } if (!(buffer = HeapAlloc( GetProcessHeap(), 0, buffer_size ))) { - return HRESULT_FROM_WIN32( ERROR_NOT_ENOUGH_MEMORY ); + SetLastError( ERROR_NOT_ENOUGH_MEMORY ); + return FALSE; } TRACE("%s -> %s, %lx\n", debugstr_w(source), debugstr_w(dest), flags); @@ -528,7 +532,8 @@ 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_FILE_NOT_FOUND ); + SetLastError( ERROR_FILE_NOT_FOUND ); + return FALSE; } if (!set_ntstatus( NtQueryInformationFile( h1, &io, &info, sizeof(info), FileBasicInformation ))) @@ -536,7 +541,7 @@ HRESULT WINAPI CopyFile2 ( const PCWSTR source , const PCWSTR dest, COPYFILE2_EX WARN("GetFileInformationByHandle returned error for %s\n", debugstr_w(source)); HeapFree( GetProcessHeap(), 0, buffer ); CloseHandle( h1 ); - return HRESULT_FROM_WIN32( GetLastError() ); + return FALSE; } if (!(flags & COPY_FILE_FAIL_IF_EXISTS)) @@ -552,7 +557,8 @@ HRESULT WINAPI CopyFile2 ( const PCWSTR source , const PCWSTR dest, COPYFILE2_EX { HeapFree( GetProcessHeap(), 0, buffer ); CloseHandle( h1 ); - return HRESULT_FROM_WIN32( ERROR_SHARING_VIOLATION ); + SetLastError( ERROR_SHARING_VIOLATION ); + return FALSE; } } @@ -563,7 +569,7 @@ HRESULT WINAPI CopyFile2 ( const PCWSTR source , const PCWSTR dest, COPYFILE2_EX WARN("Unable to open dest %s\n", debugstr_w(dest)); HeapFree( GetProcessHeap(), 0, buffer ); CloseHandle( h1 ); - return HRESULT_FROM_WIN32( GetLastError() ); + return FALSE; } while (ReadFile( h1, buffer, buffer_size, &count, NULL ) && count) @@ -577,7 +583,7 @@ HRESULT WINAPI CopyFile2 ( const PCWSTR source , const PCWSTR dest, COPYFILE2_EX count -= res; } } - ret = S_OK; + ret = TRUE; done: /* Maintain the timestamp of source file to destination file */ info.FileAttributes = 0; @@ -588,6 +594,16 @@ done: return ret; } +/*********************************************************************** + * CopyFile2 (kernelbase.@) + */ +HRESULT WINAPI CopyFile2 ( const PCWSTR source , const PCWSTR dest, COPYFILE2_EXTENDED_PARAMETERS* params) { + if (CopyFile2_impl(source, dest, params)) { + return S_OK; + } + return HRESULT_FROM_WIN32(GetLastError()); +} + /*********************************************************************** * CopyFileExW (kernelbase.@) @@ -595,17 +611,20 @@ done: 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; + if (progress) + FIXME("LPPROGRESS_ROUTINE is not supported\n"); + if (cancel_ptr) + FIXME("cancel_ptr is not supported\n"); + params.dwSize = sizeof(params); params.dwCopyFlags = flags; + params.pProgressRoutine = NULL; + params.pvCallbackContext = NULL; + params.pfCancel = NULL; - ret = CopyFile2( source, dest, ¶ms ); - if (ret != S_OK) { - SetLastError( ret ); - } - return SUCCEEDED(ret); + return CopyFile2_impl( source, dest, ¶ms ); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5020
From: Kartavya Vashishtha <sendtokartavya(a)gmail.com> --- dlls/kernelbase/file.c | 1 - 1 file changed, 1 deletion(-) diff --git a/dlls/kernelbase/file.c b/dlls/kernelbase/file.c index 22fb00926fb..57b9beec5ec 100644 --- a/dlls/kernelbase/file.c +++ b/dlls/kernelbase/file.c @@ -500,7 +500,6 @@ static BOOL CopyFile2_impl ( const PCWSTR source, const PCWSTR dest, COPYFILE2_E DWORD flags = params ? params->dwCopyFlags : 0; BOOL *cancel_ptr = params ? params->pfCancel : NULL; PCOPYFILE2_PROGRESS_ROUTINE progress = params ? params->pProgressRoutine : NULL; - void *callback_context = params ? params->pvCallbackContext : NULL; if (cancel_ptr) FIXME("pfCancel is not supported\n"); -- 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=142802 Your paranoid android. === debian11b (64 bit WoW report) === kernel32: file.c:732: Test failed: CopyFileA: ret = 0, unexpected error 2 file.c:742: Test failed: copying from a read-locked file succeeded when it shouldn't have file.c:888: Test failed: Unexpected error 183. file.c:894: Test failed: Unexpected error 183. file.c:1188: Test failed: expected ERROR_PATH_NOT_FOUND, got 2 msvcp120: msvcp120.c:1445: Test failed: test_tr2_sys__Copy_file(): test 5 expect: 5, got 2 msvcp120.c:1445: Test failed: test_tr2_sys__Copy_file(): test 6 expect: 5, got 2 msvcp140: msvcp140.c:1626: Test failed: Got unexpected err 183.
Alfred Agrell (@Alcaro) commented about dlls/kernelbase/file.c:
BOOL ret = FALSE; char *buffer;
+ DWORD flags = params ? params->dwCopyFlags : 0; + BOOL *cancel_ptr = params ? params->pfCancel : NULL; + PCOPYFILE2_PROGRESS_ROUTINE progress = params ? params->pProgressRoutine : NULL; + + if (cancel_ptr) + FIXME("pfCancel is not supported\n"); + if (progress) + FIXME("PCOPYFILE2_PROGRESS_ROUTINE is not supported\n"); + if (!source || !dest) { - SetLastError( ERROR_INVALID_PARAMETER ); + SetLastError( ERROR_PATH_NOT_FOUND );
Is this change intentional? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5020#note_60123
The Wine project values a clean commit history; adding then removing these HRESULT_FROM_WIN32 doesn't count as clean. You'll need to squish these commits into one, then split them to do exactly one thing each, and force push. (What counts as 'exactly one thing' is subjective, I never figured out the art of commit size.) This is also why your CI is failing. The CI compiles all commits, not just latest; pushing a fix is a good start, but you also need to force-push the previous commit into nonexistence. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5020#note_60124
Thanks for the note. I found out that `CopyFile` tests are present in `dlls/kernel32/tests` while I was trying to run the tests in `dlls/kernelbase/tests`, since the `CopyFile` code is located `dlls/kernelbase`. Hopefully, I will now be able submit patches that don't fail as often. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5020#note_60129
participants (4)
-
Alfred Agrell (＠Alcaro) -
Kartavya Vashishtha
-
Kartavya Vashishtha (＠kvashis)
-
Marvin