[PATCH 2/2 v4] kernel32: Support MOVEFILE_WRITE_THROUGH / CopyFileExW progress

[Tom Watson]

Signed-off-by: Tom Watson 
---
v4
Added "optional" DWORD return value for WriteFile() to stop segfaults
General tidy up and temporarily removed failing tests that require updated
kernel32.dll

[Zebediah Figura]

On 04/03/18 23:04, Tom Watson wrote:
> Signed-off-by: Tom Watson <[e-mail]
> >
>
> v4
> Added "optional" DWORD return value for WriteFile() to stop segfaults
> General tidy up and temporarily removed failing tests that require
> updated kernel32.dll

A few comments here:

+    /*trace("Chunk %d
%d\n",test_MFWPChunks,TotalBytesTransferred.QuadPart);*/
...
+    trace("Running MoveFileWithProgressW() tests...\n");
...
+        trace("Move file %s -> %s\n",source,dest);

These seem superfluous, especially when commented.

+    static const char prefix[] = {'p','f','x',0};

There's no reason not to write this inline; it's only necessary to
"spell out" WCHAR strings.

+    ok(ret != 0, "GetTempPathA error %d\n", GetLastError());
+    ok(ret < MAX_PATH, "temp path should fit into MAX_PATH\n");

I don't think you need to test these here.

+    ret = GetTempFileNameA(temp_path, prefix, 0, tmpl);
+    ok(ret != 0, "GetTempFileNameW error %d\n", GetLastError());
+    MultiByteToWideChar(CP_ACP,0,tmpl,-1,wdst,MAX_PATH);
+    ok(TRUE == DeleteFileW(wdst), "Failed to remove file %s\n",dest);

Why not just use DeleteFileA()?

+        ok(TRUE ==
MoveFileWithProgressW(wsrc,wdst,test_MoveFileWithProgressWCB,
+                    0,MOVEFILE_WRITE_THROUGH),
+                "Failed to move file %s to %s\n",source,dest);

This test doesn't actually guarantee that the callback was ever called.
I would recommend using one or more static variables to check this.

The MOVEFILE_WRITE_THROUGH flag also seems superfluous. I'm not sure
it's possible to reliably test that flag, although I could be wrong.

+/*
+ * Code temporarily disabled to pass the automated tests
+ * as the code to hanlde PROGRESS_STOP/PROGRESS_CANCEL needs
+ * to be installed on the vms
+ */

MoveFileWithProgress() is supported since Windows XP; I would be
surprised if those flags are not implemented on native Windows.

Rather, I suspect that your tests make assumptions about native
implementation of CopyFile based on Wine's implementation. It's not
necessarily true that Windows copies files in blocks of 65536 bytes. Did
you check, when testing, that your callback ever actually ran?

ἔρρωσο,
Zeb

[Tom Watson]

On Thu, Mar 8, 2018 at 10:35 PM, Zebediah Figura 
wrote:
> On 04/03/18 23:04, Tom Watson wrote:
>
> A few comments here:

Thanks for your comments, being new here I cut and pasted an earlier
example and used it as a base, I'll take them on board and see about
resubmitting.
> ...
> +/*
> -
> - Code temporarily disabled to pass the automated tests
> -
> - as the code to hanlde PROGRESS_STOP/PROGRESS_CANCEL needs
> -
> - to be installed on the vms
> - */
>
> MoveFileWithProgress() is supported since Windows XP; I would be
> surprised if those flags are not implemented on native Windows.
>
> Rather, I suspect that your tests make assumptions about native
> implementation of CopyFile based on Wine's implementation. It's not
> necessarily true that Windows copies files in blocks of 65536 bytes. Did
> you check, when testing, that your callback ever actually ran?

I did test on testbot, at the time I did not realise they VMs there are
actual systems I thought they were running the Wine dlls, you educated me
to this earlier in the week, makes more sense now. The 65536 comes from
Wine's kernel32/path.c,  https://source.winehq.org/git/wine.
git/blob/HEAD:/dlls/kernel32/path.c#l1159 so I used it to test locally,
they tests failed on testbot as it uses native win32. The test needs to be
removed/rethought as none of the VMs actually call the progress function
when copying a 128Mb file.

Thanks again for your comments.