[PATCH v3 0/3] 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. -- v3: create CopyFile2_impl that uses SetLastError error handling 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
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=142800 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.
participants (3)
-
Kartavya Vashishtha -
Kartavya Vashishtha (＠kvashis)
-
Marvin