[PATCH v2 0/2] MR145: kernel32: Unstub MoveFileWithProgress, add test.

[James McDonnell (＠ElementalWarrior)]

--
  v2: kernel32/tests: Implement test calling CopyFileExA with progress callback
      kernel32: Unstub MoveFileTransactedW, Use MoveFileWithProgressW
 
https://gitlab.winehq.org/wine/wine/-/merge_requests/145

[PATCH v2 1/2] kernel32: Unstub MoveFileTransactedW, Use MoveFileWithProgressW

[James McDonnell]

From: James McDonnell <topgamer7(a)gmail.com>

---
 dlls/kernel32/path.c   | 20 ++++++++++-----
 dlls/kernelbase/file.c | 58 ++++++++++++++++++++++++++++++++++++++----
 2 files changed, 67 insertions(+), 11 deletions(-)

diff --git a/dlls/kernel32/path.c b/dlls/kernel32/path.c
index 2dd3eac3c26..180edf358fa 100644
--- a/dlls/kernel32/path.c
+++ b/dlls/kernel32/path.c
@@ -143,9 +143,13 @@ BOOL WINAPI CopyFileExA(LPCSTR sourceFilename, LPCSTR destFilename,
  */
 BOOL WINAPI MoveFileTransactedA(const char *source, const char *dest, LPPROGRESS_ROUTINE progress, void *data, DWORD flags, HANDLE handle)
 {
-    FIXME("(%s, %s, %p, %p, %ld, %p)\n", debugstr_a(source), debugstr_a(dest), progress, data, flags, handle);
-    SetLastError(ERROR_CALL_NOT_IMPLEMENTED);
-    return FALSE;
+    BOOL ret;
+
+    TRACE("(%s, %s, %p, %p, %ld, %p)\n", debugstr_a(source), debugstr_a(dest), progress, data, flags, handle);
+
+    ret = MoveFileWithProgressA(source, dest, progress, data, flags);
+    FIXME("Transaction handle not finalized.");
+    return ret;
 }
 
 /**************************************************************************
@@ -153,9 +157,13 @@ BOOL WINAPI MoveFileTransactedA(const char *source, const char *dest, LPPROGRESS
  */
 BOOL WINAPI MoveFileTransactedW(const WCHAR *source, const WCHAR *dest, LPPROGRESS_ROUTINE progress, void *data, DWORD flags, HANDLE handle)
 {
-    FIXME("(%s, %s, %p, %p, %ld, %p)\n", debugstr_w(source), debugstr_w(dest), progress, data, flags, handle);
-    SetLastError(ERROR_CALL_NOT_IMPLEMENTED);
-    return FALSE;
+    BOOL ret;
+
+    TRACE("(%s, %s, %p, %p, %ld, %p)\n", debugstr_w(source), debugstr_w(dest), progress, data, flags, handle);
+
+    ret = MoveFileWithProgressW(source, dest, progress, data, flags);
+    FIXME("Transaction handle not finalized.");
+    return ret;
 }
 
 /**************************************************************************
diff --git a/dlls/kernelbase/file.c b/dlls/kernelbase/file.c
index 661bc0c2778..9615d11c68b 100644
--- a/dlls/kernelbase/file.c
+++ b/dlls/kernelbase/file.c
@@ -491,6 +491,37 @@ BOOL WINAPI DECLSPEC_HOTPATCH AreFileApisANSI(void)
 }
 
 
+/***********************************************************************
+ *           handle_progress_callback
+ * Wrapper for progress callback, if the return value is false, the copy or move operation should be cancelled
+ */
+BOOL handle_progress_callback(LPPROGRESS_ROUTINE progress, unsigned stream_count, LARGE_INTEGER total, LARGE_INTEGER total_bytes_written, unsigned int buffer_size, LARGE_INTEGER stream_bytes_transferred, BOOL *invoke_progress,
+    DWORD callback_reason, HANDLE hSourceFile, HANDLE hDestinationFile, LPVOID param)
+{
+    DWORD ret_progress;
+    LARGE_INTEGER stream_size;
+    stream_size.QuadPart = buffer_size;
+
+    if (!*invoke_progress || !progress)
+    {
+        return TRUE;
+    }
+    ret_progress = progress(total, total_bytes_written, stream_size, stream_bytes_transferred, stream_count, callback_reason, hSourceFile, hDestinationFile, param);
+
+    switch(ret_progress)
+    {
+        case PROGRESS_STOP:
+            FIXME("Handle resumable copy/move operation");
+        case PROGRESS_CANCEL:
+            return FALSE;
+        case PROGRESS_QUIET:
+            *invoke_progress = 0;
+            break;
+    }
+    return TRUE;
+}
+
+
 /***********************************************************************
  *	CopyFileExW   (kernelbase.@)
  */
@@ -501,9 +532,10 @@ BOOL WINAPI CopyFileExW( const WCHAR *source, const WCHAR *dest, LPPROGRESS_ROUT
     HANDLE h1, h2;
     FILE_BASIC_INFORMATION info;
     IO_STATUS_BLOCK io;
-    DWORD count;
-    BOOL ret = FALSE;
-    char *buffer;
+    DWORD count, res, stream_count = 0;
+    BOOL ret = FALSE, invoke_progress = 1;
+    char *buffer, *p;
+    LARGE_INTEGER total, total_written = {{0}}, stream_written = {{0}};
 
     if (!source || !dest)
     {
@@ -541,6 +573,10 @@ BOOL WINAPI CopyFileExW( const WCHAR *source, const WCHAR *dest, LPPROGRESS_ROUT
         return FALSE;
     }
 
+    if (progress) {
+        total.u.LowPart = GetFileSize(h1, (LPDWORD)&total.u.HighPart);
+    }
+
     if (!(flags & COPY_FILE_FAIL_IF_EXISTS))
     {
         BOOL same_file = FALSE;
@@ -569,15 +605,27 @@ BOOL WINAPI CopyFileExW( const WCHAR *source, const WCHAR *dest, LPPROGRESS_ROUT
         return FALSE;
     }
 
+    stream_count++;
     while (ReadFile( h1, buffer, buffer_size, &count, NULL ) && count)
     {
-        char *p = buffer;
+        p = buffer;
+        if(total_written.QuadPart == 0 && !handle_progress_callback(progress, stream_count, total, total_written,buffer_size,
+            stream_written, &invoke_progress, CALLBACK_STREAM_SWITCH, h1, h2, param))
+        {
+            break;
+        }
         while (count != 0)
         {
-            DWORD res;
             if (!WriteFile( h2, p, count, &res, NULL ) || !res) goto done;
+            total_written.QuadPart += res;
+            stream_written.QuadPart += res;
             p += res;
             count -= res;
+            if(!handle_progress_callback(progress, stream_count, total, total_written, buffer_size,
+                stream_written, &invoke_progress, CALLBACK_CHUNK_FINISHED, h1, h2, param))
+            {
+                break;
+            }
         }
     }
     ret =  TRUE;
-- 
GitLab


https://gitlab.winehq.org/wine/wine/-/merge_requests/145

[PATCH v2 2/2] kernel32/tests: Implement test calling CopyFileExA with progress callback

[James McDonnell]

From: James McDonnell <topgamer7(a)gmail.com>

---
 dlls/kernel32/tests/file.c | 139 +++++++++++++++++++++++++++++++++++++
 1 file changed, 139 insertions(+)

diff --git a/dlls/kernel32/tests/file.c b/dlls/kernel32/tests/file.c
index b29e2c3dc4e..b03827cde35 100644
--- a/dlls/kernel32/tests/file.c
+++ b/dlls/kernel32/tests/file.c
@@ -6122,6 +6122,144 @@ static void test_eof(void)
     ok(ret, "failed to delete %s, error %lu\n", debugstr_a(filename), GetLastError());
 }
 
+const char * reasons[2] = {
+  "CALLBACK_CHUNK_FINISHED",
+  "CALLBACK_STREAM_SWITCH"
+};
+
+typedef struct {
+  LARGE_INTEGER TotalFileSize;
+  LARGE_INTEGER TotalBytesTransferred;
+  LARGE_INTEGER StreamSize;
+  LARGE_INTEGER StreamBytesTransferred;
+  DWORD dwStreamNumber;
+  DWORD dwCallbackReason;
+  HANDLE hSourceFile;
+  HANDLE hDestinationFile;
+} ProgressCall;
+
+typedef struct
+{
+  ProgressCall *calls;
+  int call_count;
+  int calls_size;
+} ProgressOutput;
+
+DWORD LpprogressRoutine(LARGE_INTEGER TotalFileSize, LARGE_INTEGER TotalBytesTransferred, LARGE_INTEGER StreamSize, LARGE_INTEGER StreamBytesTransferred,
+  DWORD dwStreamNumber, DWORD dwCallbackReason, HANDLE hSourceFile, HANDLE hDestinationFile, ProgressOutput *lpData)
+{
+    ProgressCall call;
+    if (dwStreamNumber > lpData->calls_size) return 0;
+    call = (ProgressCall){
+      .TotalFileSize = TotalFileSize,
+      .TotalBytesTransferred = TotalBytesTransferred,
+      .StreamSize = StreamSize,
+      .StreamBytesTransferred = StreamBytesTransferred,
+      .dwStreamNumber = dwStreamNumber,
+      .dwCallbackReason = dwCallbackReason,
+      .hSourceFile = hSourceFile,
+      .hDestinationFile = hDestinationFile
+    };
+    lpData->calls[lpData->call_count] = call;
+    lpData->call_count += 1;
+    
+    return PROGRESS_CONTINUE;
+}
+
+static void check_progress_output(ProgressOutput progress_output, int expected_stream_count)
+{
+    int i, current_stream_number = 0, last_call = progress_output.call_count-1;
+    LARGE_INTEGER transfer_sum = {{0}}, stream_sum = {{0}};
+
+    ok(progress_output.call_count > 0,"Expected some progress calls, received %d\n", progress_output.call_count);
+
+    for (i = 0; i < progress_output.call_count; i++) {
+        ProgressCall call = progress_output.calls[i];
+
+        trace("i=%d %s TotalFileSize=%lld TotalBytesTransferred=%lld dwStreamNumber=%lu StreamBytesTransferred=%lld StreamSize=%lld\n",
+          i, reasons[call.dwCallbackReason], call.TotalFileSize.QuadPart, call.TotalBytesTransferred.QuadPart, call.dwStreamNumber,
+          call.StreamBytesTransferred.QuadPart, progress_output.calls[0].StreamSize.QuadPart);
+
+        if (call.dwCallbackReason == CALLBACK_STREAM_SWITCH) {
+            current_stream_number += 1;
+
+            ok(call.dwStreamNumber == current_stream_number,
+            "Received unexpected stream number, expected %d, actual %lu\n", current_stream_number, call.dwStreamNumber);
+
+            ok(call.StreamBytesTransferred.QuadPart == 0,
+              "Received unexpected stream bytes transferred, expected %lld, actual %lld\n", call.StreamBytesTransferred.QuadPart, transfer_sum.QuadPart);
+
+            if (i > 0) {
+                ok(stream_sum.QuadPart == call.StreamSize.QuadPart,
+                "Expected full stream was transferred\n");
+            }
+
+            stream_sum.QuadPart = 0;
+        } else if (call.dwCallbackReason == CALLBACK_CHUNK_FINISHED) {
+            transfer_sum.QuadPart = call.StreamBytesTransferred.QuadPart;
+            stream_sum.QuadPart = call.StreamBytesTransferred.QuadPart;
+        }
+        ok(transfer_sum.QuadPart == call.TotalBytesTransferred.QuadPart,
+          "Wrong total bytes transferred expected %lld, actual %lld\n", transfer_sum.QuadPart, call.TotalBytesTransferred.QuadPart);
+    }
+    ok(0 == progress_output.calls[0].TotalBytesTransferred.QuadPart,
+      "Expected no bytes transferred expected %d, actual %lld\n", 0, progress_output.calls[0].TotalBytesTransferred.QuadPart);
+
+    ok(transfer_sum.QuadPart == progress_output.calls[last_call].TotalFileSize.QuadPart,
+      "Wrong total bytes summation expected %lld, actual %lld\n", transfer_sum.QuadPart,
+      progress_output.calls[progress_output.call_count].TotalFileSize.QuadPart);
+
+    ok(expected_stream_count == progress_output.calls[last_call].dwStreamNumber,
+      "Wrong expected stream count %d, actual %lu\n", expected_stream_count, progress_output.calls[last_call].dwStreamNumber);
+}
+
+static void test_WithProgress(void)
+{
+    const char *ten = "1234567890";
+    HANDLE handle;
+    char path_src[MAX_PATH],
+      path_dest[MAX_PATH],
+      tmp_path[MAX_PATH],
+      buffer[70000];
+    ProgressCall calls[50];
+    ProgressOutput progress_output;
+    int i;
+
+    if (!GetTempPathA (sizeof (tmp_path), tmp_path))
+    {
+        return;
+    }
+
+    strcat (tmp_path, "filedir\\");
+    CreateDirectoryA(tmp_path, NULL);
+
+    strcpy(path_src, tmp_path);
+    strcpy(path_dest, tmp_path);
+    strcat (path_src, "testfile.ext.ext2");
+    strcat (path_dest, "testfile2.ext.ext2");
+
+    handle = CreateFileA(path_src, GENERIC_WRITE, 0, NULL, CREATE_ALWAYS, 0, 0);
+    for (i = 0; i < 7000; i++)
+    {
+        snprintf(buffer, 70000, "%s%s", buffer, ten);
+    }
+    for (i = 0; i < 10; i++)
+    {
+        ok(WriteFile(handle, buffer, strlen(buffer), NULL, NULL), "Could not write to file");
+    }
+    CloseHandle(handle);
+
+    progress_output = (ProgressOutput){.calls_size = 10, .call_count = 0, .calls = (ProgressCall *)&calls};
+    ok(CopyFileExA(path_src, path_dest, (LPPROGRESS_ROUTINE)LpprogressRoutine, &progress_output, NULL, 0),
+      "Failed to copy file %ld %s %s\n", GetLastError(), path_src, path_dest);
+
+    check_progress_output(progress_output, 1);
+
+    DeleteFileA(path_src);
+    DeleteFileA(path_dest);
+    RemoveDirectoryA(tmp_path);
+}
+
 START_TEST(file)
 {
     char temp_path[MAX_PATH];
@@ -6200,4 +6338,5 @@ START_TEST(file)
     test_hard_link();
     test_move_file();
     test_eof();
+    test_WithProgress();
 }
-- 
GitLab

https://gitlab.winehq.org/wine/wine/-/merge_requests/145

[PATCH v2 2/2] kernel32/tests: Implement test calling CopyFileExA with progress callback

[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=125667

Your paranoid android.


=== w7u_2qxl (32 bit report) ===

kernel32:
07e8:file: unhandled exception c0000005 at 75A875C3

=== w7u_adm (32 bit report) ===

kernel32:
0860:file: unhandled exception c0000005 at 759275C3

=== w7u_el (32 bit report) ===

kernel32:
0934:file: unhandled exception c0000005 at 750E75C3

[Zebediah Figura (＠zfigura)]

Zebediah Figura (@zfigura) commented about dlls/kernelbase/file.c:
> }
>
> +/***********************************************************************
> + *           handle_progress_callback
> + * Wrapper for progress callback, if the return value is false, the copy or move operation should be cancelled
> + */
> +BOOL handle_progress_callback(LPPROGRESS_ROUTINE progress, unsigned stream_count, LARGE_INTEGER total, LARGE_INTEGER total_bytes_written, unsigned int buffer_size, LARGE_INTEGER stream_bytes_transferred, BOOL *invoke_progress,
> This function should be static.

This line is also very long, like some others; can you please split it a bit more?

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/145#note_15567

[Zebediah Figura (＠zfigura)]

Zebediah Figura (@zfigura) commented about dlls/kernelbase/file.c:
> +    if (!*invoke_progress || !progress)
> +    {
> +        return TRUE;
> +    }
> +    ret_progress = progress(total, total_bytes_written, stream_size, stream_bytes_transferred, stream_count, callback_reason, hSourceFile, hDestinationFile, param);
> +
> +    switch(ret_progress)
> +    {
> +        case PROGRESS_STOP:
> +            FIXME("Handle resumable copy/move operation");
> +        case PROGRESS_CANCEL:
> +            return FALSE;
> +        case PROGRESS_QUIET:
> +            *invoke_progress = 0;
> +            break;
> +    }
> Should we have a FIXME for unrecognized return values?

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/145#note_15568

[Zebediah Figura (＠zfigura)]

Zebediah Figura (@zfigura) commented about dlls/kernelbase/file.c:
> +/***********************************************************************
> + *           handle_progress_callback
> + * Wrapper for progress callback, if the return value is false, the copy or move operation should be cancelled
> + */
> +BOOL handle_progress_callback(LPPROGRESS_ROUTINE progress, unsigned stream_count, LARGE_INTEGER total, LARGE_INTEGER total_bytes_written, unsigned int buffer_size, LARGE_INTEGER stream_bytes_transferred, BOOL *invoke_progress,
> +    DWORD callback_reason, HANDLE hSourceFile, HANDLE hDestinationFile, LPVOID param)
> +{
> +    DWORD ret_progress;
> +    LARGE_INTEGER stream_size;
> +    stream_size.QuadPart = buffer_size;
> +
> +    if (!*invoke_progress || !progress)
> +    {
> +        return TRUE;
> +    }

Could we simplify this by just setting the callback pointer to NULL instead?

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/145#note_15569

[Zebediah Figura (＠zfigura)]

Zebediah Figura (@zfigura) commented about dlls/kernelbase/file.c:
> return FALSE;
>      }
>
> +    if (progress) {
> +        total.u.LowPart = GetFileSize(h1, (LPDWORD)&total.u.HighPart);
> +    }
> +

There is a wine-staging patch for this which reuses the existing NtQueryInformationFile call, using FileNetworkOpenInformation to retrieve the size at the same time as other attributes. That seems simpler and more efficient.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/145#note_15570

[Zebediah Figura (＠zfigura)]

Zebediah Figura (@zfigura) commented about dlls/kernelbase/file.c:
> }
>
> +/***********************************************************************
> + *           handle_progress_callback
> + * Wrapper for progress callback, if the return value is false, the copy or move operation should be cancelled
> + */
> +BOOL handle_progress_callback(LPPROGRESS_ROUTINE progress, unsigned stream_count, LARGE_INTEGER total, LARGE_INTEGER total_bytes_written, unsigned int buffer_size, LARGE_INTEGER stream_bytes_transferred, BOOL *invoke_progress,
> +    DWORD callback_reason, HANDLE hSourceFile, HANDLE hDestinationFile, LPVOID param)
> In general, please keep your code style consistent; you're mixing snake case with camel case. (We tend to prefer the former, and avoid Hungarian notation and LP* typedefs in new code.)

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/145#note_15571

[Zebediah Figura (＠zfigura)]

Zebediah Figura (@zfigura) commented about dlls/kernelbase/file.c:
> return FALSE;
>      }
>
> +    stream_count++;

stream_count doesn't actually do anything. It's unlikely that it ever will, either, but if it does, we should handle that when it's implemented, rather than pretending to right now. I.e. just pass a fixed "1" everywhere.

Similarly total_written and stream_written are redundant and can just be combined.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/145#note_15572

[Zebediah Figura (＠zfigura)]

Zebediah Figura (@zfigura) commented about dlls/kernelbase/file.c:
> return FALSE;
>      }
>
> +    stream_count++;
>      while (ReadFile( h1, buffer, buffer_size, &count, NULL ) && count)
>      {
> -        char *p = buffer;
> +        p = buffer;
> +        if(total_written.QuadPart == 0 && !handle_progress_callback(progress, stream_count, total, total_written,buffer_size,
> +            stream_written, &invoke_progress, CALLBACK_STREAM_SWITCH, h1, h2, param))
> +        {
> +            break;
> +        }

Why is this inside of the loop?

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/145#note_15573

[Zebediah Figura (＠zfigura)]

Zebediah Figura (@zfigura) commented about dlls/kernel32/tests/file.c:
> +
> +DWORD LpprogressRoutine(LARGE_INTEGER TotalFileSize, LARGE_INTEGER TotalBytesTransferred, LARGE_INTEGER StreamSize, LARGE_INTEGER StreamBytesTransferred,
> +  DWORD dwStreamNumber, DWORD dwCallbackReason, HANDLE hSourceFile, HANDLE hDestinationFile, ProgressOutput *lpData)
> +{
> +    ProgressCall call;
> +    if (dwStreamNumber > lpData->calls_size) return 0;
> +    call = (ProgressCall){
> +      .TotalFileSize = TotalFileSize,
> +      .TotalBytesTransferred = TotalBytesTransferred,
> +      .StreamSize = StreamSize,
> +      .StreamBytesTransferred = StreamBytesTransferred,
> +      .dwStreamNumber = dwStreamNumber,
> +      .dwCallbackReason = dwCallbackReason,
> +      .hSourceFile = hSourceFile,
> +      .hDestinationFile = hDestinationFile
> +    };
> You can use ok() inside of the callback; no need to save these for later.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/145#note_15574

[Zebediah Figura (＠zfigura)]

Zebediah Figura (@zfigura) commented about dlls/kernel32/tests/file.c:
> +    LARGE_INTEGER transfer_sum = {{0}}, stream_sum = {{0}};
> +
> +    ok(progress_output.call_count > 0,"Expected some progress calls, received %d\n", progress_output.call_count);
> +
> +    for (i = 0; i < progress_output.call_count; i++) {
> +        ProgressCall call = progress_output.calls[i];
> +
> +        trace("i=%d %s TotalFileSize=%lld TotalBytesTransferred=%lld dwStreamNumber=%lu StreamBytesTransferred=%lld StreamSize=%lld\n",
> +          i, reasons[call.dwCallbackReason], call.TotalFileSize.QuadPart, call.TotalBytesTransferred.QuadPart, call.dwStreamNumber,
> +          call.StreamBytesTransferred.QuadPart, progress_output.calls[0].StreamSize.QuadPart);
> +
> +        if (call.dwCallbackReason == CALLBACK_STREAM_SWITCH) {
> +            current_stream_number += 1;
> +
> +            ok(call.dwStreamNumber == current_stream_number,
> +            "Received unexpected stream number, expected %d, actual %lu\n", current_stream_number, call.dwStreamNumber);
> Similarly, you can just always check that the stream number is 1 here.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/145#note_15575

[Zebediah Figura (＠zfigura)]

Zebediah Figura (@zfigura) commented about dlls/kernel32/tests/file.c:
> +
> +static void check_progress_output(ProgressOutput progress_output, int expected_stream_count)
> +{
> +    int i, current_stream_number = 0, last_call = progress_output.call_count-1;
> +    LARGE_INTEGER transfer_sum = {{0}}, stream_sum = {{0}};
> +
> +    ok(progress_output.call_count > 0,"Expected some progress calls, received %d\n", progress_output.call_count);
> +
> +    for (i = 0; i < progress_output.call_count; i++) {
> +        ProgressCall call = progress_output.calls[i];
> +
> +        trace("i=%d %s TotalFileSize=%lld TotalBytesTransferred=%lld dwStreamNumber=%lu StreamBytesTransferred=%lld StreamSize=%lld\n",
> +          i, reasons[call.dwCallbackReason], call.TotalFileSize.QuadPart, call.TotalBytesTransferred.QuadPart, call.dwStreamNumber,
> +          call.StreamBytesTransferred.QuadPart, progress_output.calls[0].StreamSize.QuadPart);
> +
> +        if (call.dwCallbackReason == CALLBACK_STREAM_SWITCH) {
> Shouldn't we validate that the first callback is always a CALLBACK_STREAM_SWITCH, and that there's only one of them?

In general if you find yourself writing "if" in tests, that seems a sign that your test is not restrictive enough.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/145#note_15576

[Zebediah Figura (＠zfigura)]

Zebediah Figura (@zfigura) commented about dlls/kernel32/tests/file.c:
> +    {
> +        return;
> +    }
> +
> +    strcat (tmp_path, "filedir\\");
> +    CreateDirectoryA(tmp_path, NULL);
> +
> +    strcpy(path_src, tmp_path);
> +    strcpy(path_dest, tmp_path);
> +    strcat (path_src, "testfile.ext.ext2");
> +    strcat (path_dest, "testfile2.ext.ext2");
> +
> +    handle = CreateFileA(path_src, GENERIC_WRITE, 0, NULL, CREATE_ALWAYS, 0, 0);
> +    for (i = 0; i < 7000; i++)
> +    {
> +        snprintf(buffer, 70000, "%s%s", buffer, ten);
> Why not just memcpy()?

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/145#note_15577

[Zebediah Figura (＠zfigura)]

Zebediah Figura (@zfigura) commented about dlls/kernel32/tests/file.c:
> +    }
> +    for (i = 0; i < 10; i++)
> +    {
> +        ok(WriteFile(handle, buffer, strlen(buffer), NULL, NULL), "Could not write to file");
> +    }
> +    CloseHandle(handle);
> +
> +    progress_output = (ProgressOutput){.calls_size = 10, .call_count = 0, .calls = (ProgressCall *)&calls};
> +    ok(CopyFileExA(path_src, path_dest, (LPPROGRESS_ROUTINE)LpprogressRoutine, &progress_output, NULL, 0),
> +      "Failed to copy file %ld %s %s\n", GetLastError(), path_src, path_dest);
> +
> +    check_progress_output(progress_output, 1);
> +
> +    DeleteFileA(path_src);
> +    DeleteFileA(path_dest);
> +    RemoveDirectoryA(tmp_path);
> Please check that these calls succeed.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/145#note_15578

[Zebediah Figura (＠zfigura)]

Zebediah Figura (@zfigura) commented about dlls/kernel32/tests/file.c:
> +    CreateDirectoryA(tmp_path, NULL);
> +
> +    strcpy(path_src, tmp_path);
> +    strcpy(path_dest, tmp_path);
> +    strcat (path_src, "testfile.ext.ext2");
> +    strcat (path_dest, "testfile2.ext.ext2");
> +
> +    handle = CreateFileA(path_src, GENERIC_WRITE, 0, NULL, CREATE_ALWAYS, 0, 0);
> +    for (i = 0; i < 7000; i++)
> +    {
> +        snprintf(buffer, 70000, "%s%s", buffer, ten);
> +    }
> +    for (i = 0; i < 10; i++)
> +    {
> +        ok(WriteFile(handle, buffer, strlen(buffer), NULL, NULL), "Could not write to file");
> +    }
> Why bother filling the file with specific data if you're not going to validate it? Either it's worth validating (probably not) or you can just do e.g. SetFilePointer() + SetEndOfFile().

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/145#note_15579

[Zebediah Figura (＠zfigura)]

The first patch both implements MoveFileTransacted() and adds callbacks to CopyFileEx(). Those should be separated into two different changes. Possibly the different callbacks should be split into separate patches as well.

The second patch adds tests for CopyFileExW(), but we already have tests for those, which are written in a much more consistent style with the rest of Wine. These patches don't seem to actually remove any of the relevant todos, so you should try to figure out why and fix that.

When adding more tests, please try to extend those tests rather than adding separate ones. While you're at it, there are some things that neither the existing nor your new tests check, e.g. what happens when you cancel from a CALLBACK_CHUNK_FINISHED callback.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/145#note_15580

[James McDonnell (＠ElementalWarrior)]

On Sun Nov 13 00:49:30 2022 +0000, Zebediah Figura wrote:
> Should we have a FIXME for unrecognized return values?
> The return value can only be 4 things. The last one being `continue`. I can add a FIXME, but it will likely never be called.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/145#note_16286

[James McDonnell (＠ElementalWarrior)]

On Fri Nov 18 02:48:12 2022 +0000, Zebediah Figura wrote:
> The first patch both implements MoveFileTransacted() and adds callbacks
> to CopyFileEx(). Those should be separated into two different changes.
> Possibly the different callbacks should be split into separate patches
> as well.
> The second patch adds tests for CopyFileExW(), but we already have tests
> for those, which are written in a much more consistent style with the
> rest of Wine. These patches don't seem to actually remove any of the
> relevant todos, so you should try to figure out why and fix that.
> When adding more tests, please try to extend those tests rather than
> adding separate ones. While you're at it, there are some things that
> neither the existing nor your new tests check, e.g. what happens when
> you cancel from a CALLBACK_CHUNK_FINISHED callback.
> I wasn't actually sure the caller of `MoveFileTransactedW` was doing with the progress callback. For the most part, I can probably just unstub `MoveFileTransactedW`, call the non-transacted function, and leave a FIXME for the progress callback functionality, and it would resolve the issues that Affinity Photo is having when saving.

I'll probably update the patch to just include those changes. 

Frankly I'm surprised that this hasn't been done in the 8 years since the stubs were introduced into the wine dll source.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/145#note_16287

[Zebediah Figura (＠zfigura)]

On Fri Nov 18 02:34:44 2022 +0000, James McDonnell wrote:
> The return value can only be 4 things. The last one being `continue`. I
> can add a FIXME, but it will likely never be called.
> Probably not, but Microsoft might add new enumeration elements, or applications might accidentally return bogus ones. It never hurts to be defensive.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/145#note_16408

[James McDonnell (＠ElementalWarrior)]

This was handled in !145

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/145#note_22211

[PATCH v2 0/2] MR145: kernel32: Unstub MoveFileWithProgress, add test. - closed

[James McDonnell (＠ElementalWarrior)]

This merge request was closed by James McDonnell.
-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/145