[PATCH v7 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. -- v7: 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 | 60 +++++++++++++++++++++++++++++---- dlls/kernelbase/kernelbase.spec | 2 +- 2 files changed, 54 insertions(+), 8 deletions(-) diff --git a/dlls/kernelbase/file.c b/dlls/kernelbase/file.c index e1ba92a6448..b0f30c747f6 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_impl + * + * 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. */ -BOOL WINAPI CopyFileExW( const WCHAR *source, const WCHAR *dest, LPPROGRESS_ROUTINE progress, - void *param, BOOL *cancel_ptr, DWORD flags ) -{ +static BOOL CopyFile2_impl( const WCHAR *source, const WCHAR *dest, COPYFILE2_EXTENDED_PARAMETERS *params ) { static const int buffer_size = 65536; HANDLE h1, h2; FILE_BASIC_INFORMATION info; @@ -502,6 +503,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 +587,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 +599,42 @@ done: return ret; } +/*********************************************************************** + * CopyFile2 (kernelbase.@) + */ +HRESULT WINAPI CopyFile2( PCWSTR source, 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
Kartavya Vashishtha (@kvashis) 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");
Should these be moved below to the checks for unimplemented flags? (since they are also `FIXME`) I wrote these here so they're close to the variable declarations for `cancel_ptr` and `progress`. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5020#note_60146
On Tue Feb 6 03:15:17 2024 +0000, Kartavya Vashishtha wrote:
changed this line in [version 7 of the diff](/wine/wine/-/merge_requests/5020/diffs?diff_id=97820&start_sha=c739d0dbece9e9ded9f8d61dc366a24f28175991#6c845445f68f8d3b54ba3798c9bb9e781b28b2ec_602_605) "const PCWSTR" is "const WCHAR *const", which differs from "const WCHAR *" in that the pointer variable itself can't be mutated. We don't usually bother with that in Wine, although I can't say I've seen such patches rejected either.
Unrelatedly, in general we avoid those typedefs; it's clearer just to spell out e.g. "const WCHAR *". -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5020#note_60148
On Tue Feb 6 03:06:47 2024 +0000, Kartavya Vashishtha wrote:
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? Variable ordering is something that we only tend to care about in Direct3D and areas that use the same style.
That said I would avoid having a space between two blocks. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5020#note_60149
On Tue Feb 6 02:19:55 2024 +0000, Alfred Agrell wrote:
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. It should be exported from every DLL that it's actually exported from on Windows. That includes both kernel32 and kernelbase. Microsoft documentation is unreliable.
In practice if a function is exported from kernel32 and kernelbase, kernel32 will forward it. See how it's done for other functions. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5020#note_60150
On Tue Feb 6 03:46:22 2024 +0000, Zebediah Figura wrote:
Variable ordering is something that we only tend to care about in Direct3D and areas that use the same style. That said I would avoid having a space between two blocks. That's in that page, near the bottom of section 1.1. "Sort variable declarations to a reverse Christmas tree by length."
"Note that many of these principles are applicable elsewhere." from section 1.1's header made me consider everything in both 1 and 1.1 to be Wine's coding style. Which was an oversimplification; I have been proven wrong. Just ignore me. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5020#note_60248
On Tue Feb 6 03:45:27 2024 +0000, Zebediah Figura wrote:
"const PCWSTR" is "const WCHAR *const", which differs from "const WCHAR *" in that the pointer variable itself can't be mutated. We don't usually bother with that in Wine, although I can't say I've seen such patches rejected either. Unrelatedly, in general we avoid those typedefs; it's clearer just to spell out e.g. "const WCHAR *". Oh right, good point. I never use toplevel const like that, I forgot it's a thing.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5020#note_60249
Alfred Agrell (@Alcaro) commented about dlls/kernelbase/file.c:
return ret; }
+/*********************************************************************** + * CopyFile2 (kernelbase.@) + */ +HRESULT WINAPI CopyFile2( PCWSTR source, 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);
There is a function to restore the last error, as opposed to setting a new instance of the same error. It's called, surprisingly, RestoreLastError. https://devblogs.microsoft.com/oldnewthing/20110429-00/?p=10803 But Wine currently uses RestoreLastError exactly nowhere else, so for now, better stay consistent and keep it as is. For now. I've filed !5035 to split it apart, let's see what happens. (If this one merges before 5035, updating this line belongs in 5035.) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5020#note_60250
Any hints on the test failures? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5020#note_60287
On Tue Feb 6 14:52:53 2024 +0000, Alfred Agrell wrote:
That's in that page, near the bottom of section 1.1. "Sort variable declarations to a reverse Christmas tree by length." "Note that many of these principles are applicable elsewhere." from section 1.1's header made me consider everything in both 1 and 1.1 to be Wine's coding style. Which was an oversimplification; I have been proven wrong. Just ignore me. @zfigura how strongly do you suggest that? My intention was to visually separate the parameter assignments from the variables that are used to perform the operation. I'm thinking about swapping the order, but still maintaining the space between the blocks.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5020#note_60292
On Tue Feb 6 17:10:56 2024 +0000, Kartavya Vashishtha wrote:
Any hints on the test failures? That test fails at https://gitlab.winehq.org/maljaf/wine/-/jobs/53691 and https://gitlab.winehq.org/DarkShadow44/wine/-/jobs/53536 too. It's flaky; it's random whether it passes or fails. Not your fault and not your responsibility; just ignore it.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5020#note_60293
Is @zfigura reviewing this MR or should I request a reviewer be assigned somehow? I'm asking since I don't see anything under the Reviewers tab. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5020#note_60295
Pinging on this thread again to ask for next steps. I also sent an email to the mailing list but didn't get any responses. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5020#note_60827
participants (4)
-
Alfred Agrell (＠Alcaro) -
Kartavya Vashishtha
-
Kartavya Vashishtha (＠kvashis)
-
Zebediah Figura (＠zfigura)