[PATCH v6 0/1] 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. -- v6: create CopyFile2_impl and CopyFile2 https://gitlab.winehq.org/wine/wine/-/merge_requests/5020
From: Kartavya Vashishtha <sendtokartavya(a)gmail.com> --- dlls/kernelbase/file.c | 57 +++++++++++++++++++++++++++++---- dlls/kernelbase/kernelbase.spec | 2 +- 2 files changed, 51 insertions(+), 8 deletions(-) diff --git a/dlls/kernelbase/file.c b/dlls/kernelbase/file.c index e1ba92a6448..5b14016cf1f 100644 --- a/dlls/kernelbase/file.c +++ b/dlls/kernelbase/file.c @@ -487,13 +487,11 @@ BOOL WINAPI DECLSPEC_HOTPATCH AreFileApisANSI(void) return !oem_file_apis; } +// uses the SetLastError style of error handling since CopyFileEx returns +// WIN32 error codes and HRESULT to WIN32 conversion is easier +// than the other way around. -/*********************************************************************** - * CopyFileExW (kernelbase.@) - */ -BOOL WINAPI CopyFileExW( const WCHAR *source, const WCHAR *dest, LPPROGRESS_ROUTINE progress, - void *param, BOOL *cancel_ptr, DWORD flags ) -{ +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; @@ -502,6 +500,15 @@ BOOL WINAPI CopyFileExW( const WCHAR *source, const WCHAR *dest, LPPROGRESS_ROUT 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 ); @@ -577,7 +584,7 @@ BOOL WINAPI CopyFileExW( const WCHAR *source, const WCHAR *dest, LPPROGRESS_ROUT count -= res; } } - ret = TRUE; + ret = TRUE; done: /* Maintain the timestamp of source file to destination file */ info.FileAttributes = 0; @@ -589,6 +596,42 @@ done: return ret; } +/*********************************************************************** + * CopyFile2 (kernelbase.@) + */ +HRESULT WINAPI CopyFile2 ( const PCWSTR source , const PCWSTR dest, COPYFILE2_EXTENDED_PARAMETERS* params) { + DWORD prev_err = GetLastError(); + HRESULT res; + + res = CopyFile2_impl(source, dest, params) ? S_OK : HRESULT_FROM_WIN32(GetLastError()); + + SetLastError(prev_err); + return res; +} + + +/*********************************************************************** + * CopyFileExW (kernelbase.@) + */ +BOOL WINAPI CopyFileExW( const WCHAR *source, const WCHAR *dest, LPPROGRESS_ROUTINE progress, + void *param, BOOL *cancel_ptr, DWORD flags ) +{ + 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; + + return CopyFile2_impl( source, dest, ¶ms ); +} + /************************************************************************** * 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
Alfred Agrell (@Alcaro) commented about dlls/kernelbase/file.c:
return ret; }
+/*********************************************************************** + * CopyFile2 (kernelbase.@) + */ +HRESULT WINAPI CopyFile2 ( const PCWSTR source , const PCWSTR dest, COPYFILE2_EXTENDED_PARAMETERS* params) {
PCWSTR is const already (Pointer to Const Wide STRing), twice the const doesn't really do anything. That space after source doesn't do much either. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5020#note_60136
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;
Those variables are in wrong order per Wine's coding style ...but so are the existing ones right above, so I'm not sure what to do there. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5020#note_60137
Alfred Agrell (@Alcaro) commented about dlls/kernelbase/kernelbase.spec:
@ stdcall ConvertThreadToFiberEx(ptr long) @ stdcall ConvertToAutoInheritPrivateObjectSecurity(ptr ptr ptr ptr long ptr) @ stdcall CopyContext(ptr long ptr) -# @ stub CopyFile2 +@ stdcall CopyFile2(wstr wstr ptr) https://learn.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-copyf... says this function is in kernel32, not kernelbase. Probably just MS docs being wrong, but feels like a good candidate for double checking.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5020#note_60138
Alfred Agrell (@Alcaro) commented about dlls/kernelbase/file.c:
return !oem_file_apis; }
+// uses the SetLastError style of error handling since CopyFileEx returns +// WIN32 error codes and HRESULT to WIN32 conversion is easier +// than the other way around.
-/*********************************************************************** - * CopyFileExW (kernelbase.@) - */ -BOOL WINAPI CopyFileExW( const WCHAR *source, const WCHAR *dest, LPPROGRESS_ROUTINE progress, - void *param, BOOL *cancel_ptr, DWORD flags ) -{ +static BOOL CopyFile2_impl ( const PCWSTR source, const PCWSTR dest, COPYFILE2_EXTENDED_PARAMETERS* params) {
Should the asterisk be attached to the type or the name? You're doing one thing on params, and another on cancel_ptr ten lines down. I have an opinion on that question, but I believe the Wine guideline is just 'stay consistent', so my opinion is irrelevant. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5020#note_60139
Yep, this looks much better. Still a few nitpicks, but I think that's everything I'll find. (But I can't promise how much the maintainers will have to say, other than 'less than before'.) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5020#note_60140
On Tue Feb 6 01:52:49 2024 +0000, Kartavya Vashishtha wrote:
changed this line in [version 5 of the diff](/wine/wine/-/merge_requests/5020/diffs?diff_id=97811&start_sha=4a1eb43b7d48cbd502fea7f6e8423373538643ea#6c845445f68f8d3b54ba3798c9bb9e781b28b2ec_511_514) Thanks for catching that.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5020#note_60141
On Tue Feb 6 02:19:54 2024 +0000, Alfred Agrell wrote:
Those variables are in wrong order per Wine's coding style ...but so are the existing ones right above, so I'm not sure what to do there. The [coding practices section of the Wiki](https://wiki.winehq.org/Wine_Developer%27s_Guide/Coding_Practice#Some_notes_...) doesn't seem to comment on variable ordering. Can you point me somewhere where I can read more about it?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5020#note_60142
participants (3)
-
Alfred Agrell (＠Alcaro) -
Kartavya Vashishtha
-
Kartavya Vashishtha (＠kvashis)