winapi: Implement CopyFileA #499

abaire · 2021-09-11T07:37:47Z

Implements CopyFileA.

Tests can be found at abaire/nxdk_tests

lib/winapi/winioctl.h

lib/winapi/filemanip.c

lib/winapi/winioctl.h

JayFoxRox · 2021-09-11T15:10:09Z

lib/winapi/filemanip.c

@@ -241,6 +242,130 @@ BOOL MoveFileA (LPCTSTR lpExistingFileName, LPCTSTR lpNewFileName)
    }
 }

+BOOL CopyFileA (LPCTSTR lpExistingFileName, LPCTSTR lpNewFileName, BOOL bFailIfExists)
+{
+    static const DWORD readBufferSize = 65535;


For such constants I prefer constants like 0xFFFF because it will be much more obvious where it originates (not much of an issue for 16 bit, but still; also avoids confusion with 0x10000).

Also: Why not 0x10000? Won't the unaligned value trip the allocator?

Good point, changed.

Personally, I'd prefer it like 64*1024.
As for the buffer size, it currently is only 4KiB (64KiB previously). That's a bit small imho - XDK code seem to copy in 16KiB chunks (at least that's the size Halo 2 uses to copy files), which I think is a reasonable compromise between performance and saving memory.

lib/winapi/filemanip.c

lib/xboxkrnl/xboxkrnl.h

JayFoxRox · 2021-09-11T15:16:37Z

lib/winapi/filemanip.c

+            }
+            bytesRead -= bytesWritten;
+            bufferPos += bytesWritten;
+        }


Doesn't the kernel offer some call or IOCTL to copy files?
This user-space copy loop feels a bit involved and there's probably a lot that can go wrong (stuff like not applying the proper flags to the file when creating it, not taking advantage of file-system optimizations etc.)

Totally agree; I didn't see a function that looked appropriate in xboxkrnl.h so I assumed there was no lower level method that could be utilized. Similarly I did not have luck searching the win32 docs.

Is there someplace else I could look?

Poked around a bit more, the only lower level copying I found was added as part of windows server after win2k/xp days.

Possibly something more like NtWriteFile() than using the user abstraction WriteFile() is what he's hinting at?

Ah, I assume he was looking for something more like https://docs.microsoft.com/en-us/windows/win32/api/winioctl/ni-winioctl-fsctl_duplicate_extents_to_file where the entire copy loop is handled at a lower level (but that's not available in the XBOX kernel).

I think a switch to NtWriteFile would end up duplicating much of the content of WriteFile and still operating in user-space. I don't know enough to say if the efficiency gain warrants the maintenance cost, but happy to make that change if it's preferred.

@JayFoxRox can you clarify?

lib/winapi/filemanip.c

lib/winapi/fileapi.h

lib/winapi/filemanip.c

thrimbor

This looks very close to being ready to merge now, so I should probably mention that, while this isn't explicitly mentioned in this file, the winapi part of nxdk is licensed under the MIT license. Just want to make sure you're aware and fine with that.
You don't have to add any license or copyright info to this file yourself, I'll add that when I slap SPDX headers on everything.

abaire

Thanks for the heads up on the license, MIT is totally fine with me.

lib/winapi/filemanip.c

thrimbor · 2021-10-08T16:10:11Z

I took the liberty to force push away the merge conflict.

I'll merge once the CI has run through. Thanks for the contribution!

dracc reviewed Sep 11, 2021

View reviewed changes

lib/winapi/winioctl.h Outdated Show resolved Hide resolved

lib/winapi/filemanip.c Outdated Show resolved Hide resolved