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

[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

Hello Tom, thanks for contributing. I defer to others' reviews, but in
lieu thereof, I have a few comments to add:

First of all, when sending patches, can you please send them inline,
preferably using `git send-email`? This makes review easier.

Secondly, it seems you still have some C++-style comments left:

+            switch (progressExit) {
+                case PROGRESS_CONTINUE:
+                    // keep at it
+                    break;
+                case PROGRESS_QUIET:
+                    // requested not to be called again
+                    progress = NULL;
+                    break;
+                case PROGRESS_CANCEL:
+                    // cancel and clean up
+                    CloseHandle( h2 );
+                    h2 = INVALID_HANDLE_VALUE;
+                    DeleteFileW( dest );
+                    SetLastError( ERROR_REQUEST_ABORTED );
+                    goto done;
+                case PROGRESS_STOP:
+                    // do nothing, allow for a restart (??)
+                    SetLastError( ERROR_REQUEST_ABORTED );
+                    goto done;

With regard to the content of the patch itself, it would probably be
best to split this up into separate patches: first implement the
progress callback, and then MOVEFILE_WRITE_THROUGH.

It also doesn't seem clear to me that your implementation of
MOVEFILE_WRITE_THROUGH is correct. In particular: firstly, the flag
seems to guarantee that a copy-and-delete is completed *if* it is
performed, but not that setting the flag forces MoveFileWithProgress()
to perform a copy-and-delete.
Secondly, an implementation using the CopyFileEx() callback seems
excessively complicated; why not simply call FlushFileBuffers() directly?

ἔρρωσο,
Zeb