Switch to using winapi for file I/O

[thrimbor]

Due to requiring properly mounted devices, this will be a breaking change for some users, so I recommend to merge this after #14.

This changes the file I/O functions in the xbox backend to use the winapi functions instead of the old X*-functions. I used PDCLib's shepherd-branch for flag reference, the _PDCLIB_w32errno() function for translating winapi errors to errno values also comes from there.

I gave this some testing, and it seemed to work fine, but I do recommend everyone to test their code with this to make sure this doesn't break in some specific cases.

[JayFoxRox]

I'm not sure if I did something wrong, but the test results were bad. I'm not expecting all of these to work / be changed, but the results are unexpected:

• assert(fopen("c:\\filec.test", "wb") != NULL);
• assert(fopen("d:\\filed.test", "wb") != NULL);

This works, as expected.

• //assert(fopen("C:\\fileC.test", "wb") != NULL); // Broken
• //assert(fopen("D:\\fileD.test", "wb") != NULL); // Broken

This does not work, and that really surprised me. If anything, I'd expect this to work, but not the lowercase variant. I mounted my filesystem with nxMountDrive('C', "\\Device\\Harddisk0\\Partition2\\"); (uppercase C)

Also see this MSDN doc:

Volume designators (drive letters) are similarly case-insensitive. For example, "D:\" and "d:\" refer to the same volume.

(I did not check if this might have to do with the lowercase variants already existing)

• //assert(fopen("c:/filecf.test", "wb") != NULL); // Broken
• //assert(fopen("C:/fileCf.test", "wb") != NULL); // Broken
• //assert(fopen("d:/filedf.test", "wb") != NULL); // Broken
• //assert(fopen("D:/fileDf.test", "wb") != NULL); // Broken

These were unexpected, because the MSDN even says:

The name of the file or device to be created or opened. You may use either forward slashes (/) or backslashes () in this name.

I did not check if this is also an issue with CreateFile and other winapi functions, but I'd assume so (hence the quote from MSDN).

If the kernel doesn't handle this, then we'll probably need a translation function in our winapi.

• //assert(fopen(".\\filedot.test", "wb") != NULL); // Broken
• //assert(fopen("./filedotf.test", "wb") != NULL); // Broken
• //assert(fopen("file.test", "wb") != NULL); // Broken

These do not work. This was somewhat expected (I think). However, this is also severly limiting. A lot of the software I've seen uses such filepaths. I don't have a Windows machine to test these, but I'd assume their CreateFile to handle this, or at the very least I'd assume that their libc handles this for portability.

(I'm aware that D:\\ was advocated for Xbox, but I'm not sure if we should really expect people to keep using that; I'm also not sure how the XDK handled relative paths)

• Things I did not try

I did not try parent-paths such as .. or path like \\foo to be in the root of the current drive (I'd assume the MS libc to allow these things).

[thrimbor]

I'm not sure if I did something wrong, but the test results were bad. I'm not expecting all of these to work / be changed, but the results are unexpected:

* **`assert(fopen("c:\\filec.test", "wb") != NULL);`**

* **`assert(fopen("d:\\filed.test", "wb") != NULL);`**

This works, as expected.

* **`//assert(fopen("C:\\fileC.test", "wb") != NULL); // Broken`**
* **`//assert(fopen("D:\\fileD.test", "wb") != NULL); // Broken`**

Not sure what's happening here - did you close the file streams / reboot before? We don't open with FILE_SHARE_WRITE, so there can only be one file stream with write access to the file.

* **`//assert(fopen("c:/filecf.test", "wb") != NULL); // Broken`**
* **`//assert(fopen("C:/fileCf.test", "wb") != NULL); // Broken`**
* **`//assert(fopen("d:/filedf.test", "wb") != NULL); // Broken`**
* **`//assert(fopen("D:/fileDf.test", "wb") != NULL); // Broken`**

These were unexpected, because the MSDN even says:

The name of the file or device to be created or opened. You may use either forward slashes (/) or backslashes () in this name.

I did not check if this is also an issue with CreateFile and other winapi functions, but I'd assume so (hence the quote from MSDN).

If the kernel doesn't handle this, then we'll probably need a translation function in our winapi.

Yeah that's a shortcoming of our current implementation in winapi. CreateFileA should handle those slashes.

* **`//assert(fopen(".\\filedot.test", "wb") != NULL); // Broken`**
* **`//assert(fopen("./filedotf.test", "wb") != NULL); // Broken`**
* **`//assert(fopen("file.test", "wb") != NULL); // Broken`**

These do not work. This was somewhat expected (I think). However, this is also severly limiting. A lot of the software I've seen uses such filepaths. I don't have a Windows machine to test these, but I'd assume their CreateFile to handle this, or at the very least I'd assume that their libc handles this for portability.

(I'm aware that D:\\ was advocated for Xbox, but I'm not sure if we should really expect people to keep using that; I'm also not sure how the XDK handled relative paths)

Handling working directories seems to usually involve quite a bit of kernel assistance, which is not provided on the Xbox (that's why the OpenXDK code had such a hack for it). The XDK solution (I'm deducing this from publicly available source such as XBMC) seems to be to just not support it at all, and use absolute paths everywhere.

I thought I could get by without touching the path strings in the winapi functions, but if I want to handle forward slashes I'll have to do that anyway (XDK probably doesn't support them? idk). I'll have to think about that for while to figure out how I would implement all this, and how it impacts stuff like MoveFileA.

[JayFoxRox]

Not sure what's happening here - did you close the file streams / reboot before? We don't open with FILE_SHARE_WRITE, so there can only be one file stream with write access to the file.

That does make a lot of sense. I forgot about this.

[...] touching the path strings in the winapi functions [...]. I'll have to think about that for while to figure out how I would implement all this [...].

I'm fine with this not being implemented in this PR, but we'll need issues for things that are expected to work, and probably also a nxdk wiki article or some other form of documentation which documents what works, what doesn't work (but should work = links to issues) and non-goals.

Because clearly a lot of our winapi will diverge from what the MSDN docments in unexpected ways, and win32 libc code will not be portable.

[...] but I do recommend everyone to test their code with this to make sure this doesn't break in some specific cases.

I'm sure that many applications will break. Given my results I skimmed over some nxdk code from https://github.com/XboxDev/nxdk/wiki/Projects-using-nxdk we can find a couple of spots in different projects very easily (I did not test these):

• https://github.com/Ryzee119/OpenXenium/blob/1e11059c5a759365e45ffc97ae1550b9c695678d/Xenium-Tools/main.c#L89
• https://github.com/Cxbx-Reloaded/xbox_kernel_test_suite/blob/c3dc177e47c4d1ffbe706ebb07a12d952164be3d/main.c#L110
• https://github.com/dracc/NevolutionX/blob/2aeb9ce77f68a3183f269bef196b05a309b0f88c/Includes/font.cpp#L9 via https://github.com/dracc/NevolutionX/blob/2aeb9ce77f68a3183f269bef196b05a309b0f88c/main.cpp#L68
• https://github.com/XboxDev/nxdk/blob/a944b38b1bdd1753dc484305e061f5eb98a05c74/samples/sdl_ttf/main.c#L55
• I'm also sure ports like DOOOM are affected, but I had trouble pinpointing a specific instance.

I'm not sure how to deal with this, because clearly this repository isn't gaining enough attention for this breaking-change (nobody had complained since November, despite obvious issues with backwards compatibility).

[JayFoxRox]

I think we should merge this as-is and iterate later.

However, first, we should create issues about this and add some documentation somewhere (nxdk wiki?), which lists what we do / do not support and what we plan to support / do not plan to support (for winapi and C stdio), including best practices how to avoid this. We'll probably also have to adapt some samples when updating the submodule.

If this breaks apps, then people should be able to look at the docs to decide a plan of action, or they can stick to an older version of nxdk for now. But clearly there is interest in advancing nxdk, and this PR is a step in that direction (even if it means dropping some defacto standard C features).

[thrimbor]

I suggest using error handling code from #22 it has more cases, is more generic and uses PDCLIB internal error codes like other functions in the lib do.

I'm not opposed to that idea, but I'd like to know where that mapping comes from first, and I'm also not a fan of the fallback_errno parameter.

[thrimbor]

Merging this will break relative path without notice. I suggest adding a #warning in stdio.h to let users know this at compile time.

I don't think that's a good solution. It would introduce warnings where nothing's actually wrong, and it also may bury those warnings, e.g. when users are using SDL to open a file, they won't get the warning where the actual problem is.

[sam-itt]

I'm not opposed to that idea, but I'd like to know where that mapping comes from first Part from cygnus, part from myself. , and I'm also not a fan of the fallback_errno parameter. You'll always have a fallback. At least here it's explicit.

[sam-itt]

. when users are using SDL to open a file, they won't get the warning where the actual problem is. Good point. So it's either

• a) merge as-is and fix code that breaks (i.e) relative path -b) Add some code in fopen or CreateFileA to handle relative paths before merging.

[thrimbor]

I've updated the PR with the error translations from #22. I did some minor changes to it, like formatting changes and changing the fallback (even with error checking in strerror I'm not comfortable with it just returning an out of bounds value). I also ended up squashing the changes into my commit because trying to rebase the changes into a meaningful history was quite annoying - I hope you don't mind @sam-itt.

[sam-itt]

I don't mind at all: Glad it's merged!

[JayFoxRox]

Last call for vetos, tests or reviews. I'll do a final pass tonight and merge it then (if necessary, untested or without another extensive review pass).

This sort of patch has been bikeshedded for too long already.

[JayFoxRox]

I'll merge this now, despite this not being ideal:

• Ideally we'd have had another rebase of pdclib first (to get rid of bugs before doing a larger breaking change), but I think we gave people enough time to review their code.
• Ideally we'd have fixed gmtime first; I also planned to do this last week but didn't find the time.

We'll have to remember to fix samples in nxdk (like samples/sdl_ttf) when updating the submodule in nxdk (which should happen as soon as possible, so that we don't group too many changes together).

Merged.