[PATCH v8 0/1] MR5020: kernelbase/file: implement CopyFile2

[Kartavya Vashishtha (＠kvashis)]

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.

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.

--
  v8: implement CopyFile2
 
https://gitlab.winehq.org/wine/wine/-/merge_requests/5020

[PATCH v8 1/1] implement CopyFile2 and reimplement CopyFileEx in terms of CopyFile2

[Kartavya Vashishtha]

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..94ff3876c04 100644
--- a/dlls/kernelbase/file.c
+++ b/dlls/kernelbase/file.c
@@ -487,13 +487,18 @@ 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 ) {
+    DWORD flags = params ? params->dwCopyFlags : 0;
+    BOOL *cancel_ptr = params ? params->pfCancel : NULL;
+    PCOPYFILE2_PROGRESS_ROUTINE progress = params ? params->pProgressRoutine : NULL;
+
     static const int buffer_size = 65536;
     HANDLE h1, h2;
     FILE_BASIC_INFORMATION info;
@@ -502,6 +507,11 @@ BOOL WINAPI CopyFileExW( const WCHAR *source, const WCHAR *dest, LPPROGRESS_ROUT
     BOOL ret = FALSE;
     char *buffer;
 
+    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 d3bd59a78e3..a133380a27a 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

[PATCH v8 1/1] implement CopyFile2 and reimplement CopyFileEx in terms of CopyFile2

[Marvin]

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=143025

Your paranoid android.


=== debian11b (64 bit WoW report) ===

ddraw:
ddraw2.c:4537: Test failed: Display mode restored after good ddraw1::SetCooperativeLevel call
ddraw2.c:4550: Test failed: Display mode restored after ddraw1::SetCooperativeLevel(SETFOCUSWINDOW) call
ddraw2.c:4566: Test failed: Display mode restored after good-bad ddraw1::SetCooperativeLevel() call sequence
ddraw2.c:4585: Test failed: Display mode restored after ddraw1-ddraw2 SetCooperativeLevel() call sequence
ddraw4.c:5674: Test failed: Display mode restored after good ddraw1::SetCooperativeLevel call
ddraw4.c:5687: Test failed: Display mode restored after ddraw1::SetCooperativeLevel(SETFOCUSWINDOW) call
ddraw4.c:5703: Test failed: Display mode restored after good-bad ddraw1::SetCooperativeLevel() call sequence
ddraw4.c:5722: Test failed: Display mode restored after ddraw1-ddraw4 SetCooperativeLevel() call sequence
ddraw7.c:5434: Test failed: Display mode restored after good ddraw1::SetCooperativeLevel call
ddraw7.c:5447: Test failed: Display mode restored after ddraw1::SetCooperativeLevel(SETFOCUSWINDOW) call
ddraw7.c:5463: Test failed: Display mode restored after good-bad ddraw1::SetCooperativeLevel() call sequence
ddraw7.c:5482: Test failed: Display mode restored after ddraw1-ddraw7 SetCooperativeLevel() call sequence

[Zebediah Figura (＠zfigura)]

Zebediah Figura (@zfigura) commented about dlls/kernelbase/file.c:
> }
>
> -
> -/***********************************************************************
> - *	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 ) {
> This doesn't match the usual naming scheme for helpers. I'd just call it copy_file().

The brace placement doesn't match what's used elsewhere in this file.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/5020#note_60900

[Zebediah Figura (＠zfigura)]

Zebediah Figura (@zfigura) commented about dlls/kernelbase/file.c:
> 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.
>   */

I don't think this comment is necessary.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/5020#note_60899

[Zebediah Figura (＠zfigura)]

Zebediah Figura (@zfigura) commented about dlls/kernelbase/file.c:
> return ret;
>  }
>
> +/***********************************************************************
> + *	CopyFile2   (kernelbase.@)
> + */
> +HRESULT WINAPI CopyFile2( PCWSTR source, PCWSTR dest, COPYFILE2_EXTENDED_PARAMETERS *params ) {

Let's avoid P* in new code, please. The brace placement also doesn't match the rest of the file.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/5020#note_60901

[Zebediah Figura (＠zfigura)]

Zebediah Figura (@zfigura) 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);
> +    return res;

Perhaps better would just be to return the error directly from the helper.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/5020#note_60902

[Zebediah Figura (＠zfigura)]

Zebediah Figura (@zfigura) 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)
> The function also needs to be exported from kernel32.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/5020#note_60903

[Kartavya Vashishtha (＠kvashis)]

On Mon Feb 12 18:25:34 2024 +0000, Zebediah Figura wrote:
> Perhaps better would just be to return the error directly from the helper.
> While we could return a DWORD or ULONG from the helper function, the code _in_ the helper function still uses older functions that use SetLastError, so we still need to not clobber GetLastError.

We need to create an HRESULT in CopyFile2 since the helper functions needs to operate in WIN32 error codes (because CopyFileEx operates in WIN32 error codes) and converting to HRESULT is easier than the other way around.

So converting line 568-570:
```
        CloseHandle( h1 );
        return FALSE;
    }
```
to
```
        CloseHandle( h1 );
        return GetLastError();
    }
```

Would this look better?

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/5020#note_60919

[Zebediah Figura (＠zfigura)]

On Mon Feb 12 21:19:05 2024 +0000, Kartavya Vashishtha wrote:
> While we could return a DWORD or ULONG from the helper function, the
> code _in_ the helper function still uses older functions that use
> SetLastError, so we still need to not clobber GetLastError.
> We need to create an HRESULT in CopyFile2 since the helper functions
> needs to operate in WIN32 error codes (because CopyFileEx operates in
> WIN32 error codes) and converting to HRESULT is easier than the other
> way around.
> So converting line 568-570:
> ```
>         CloseHandle( h1 );
>         return FALSE;
>     }
> ```
> to
> ```
>         CloseHandle( h1 );
>         return GetLastError();
>     }
> ```
> Would this look better?
> Eh, I missed that there's other last-error functions being used inside the helper. So never mind then.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/5020#note_60927