IOS: Refactor the filesystem code #6421

leoetlino · 2018-03-06T09:00:50Z

Add a new FileSystem class that can be used to perform operations
on the emulated Wii filesystem.

This will allow separating the IPC code (reading from and writing to
memory to handle IOS FS commands) and the actual filesystem code
in a much better way.

This also paves the way for implementing another filesystem backend
in the future -- NAND images for more complete emulation, including
filesystem metadata like permissions or file orders, which some games
and homebrew actually care about -- without needing to make any more
changes to the other parts of the codebase, in addition to making
filesystem behaviour tests easier to write.

BhaaLseN · 2018-03-06T17:58:06Z

The bulk of it looks good in general; the only thing that bothers me a little is the missed opportunity to address the style issues in HostFileSystem (moved code from the old FS parts). Mostly first-upper/mixed-case locals; perhaps more as I gave up half-way after noticing how many there are...
Or would you rather push this to a later point (with the option of forgetting about it)?

CreateVirtualFATFilesystem sounds somewhat familiar, it might as well be not necessary at all today. Some time back (before proper or even partial/broken FS support) we used to close/reopen files on every access which slowed things down a lot, giving birth to this workaround. I don't suppose you've tried without that method?

leoetlino · 2018-03-06T20:07:26Z

Sure, might as well fix them.

Regarding CreateVirtualFATFilesystem, IIRC I tried removing it some time ago and finding out the system menu does still take ages to create that file. I think it's some kind of emulated FS timing issue. I'll investigate it at some point :D

Source/Core/Common/Swap.h

+template <typename value_type>
+struct BigEndianValue
+{
+  static_assert(std::is_arithmetic<value_type>::value, "value_type must be an arithmetic type");


Source/Core/Core/IOS/FS/FileSystem.cpp

+
+namespace IOS::HLE::FS
+{
+std::unique_ptr<FileSystem> MakeFileSystem(IOSC& iosc)


leoetlino · 2018-03-12T17:29:00Z

I've added two more commits to futureproof this: one to avoid forgetting to close FDs, the other to make it possible to have two FS instances, each for a different NAND root. This will be useful for copying files between the configured NAND and the session NAND (for movie and netplay).

Source/Core/Core/IOS/FS/FileSystem.cpp

+
+namespace IOS::HLE::FS
+{
+std::unique_ptr<FileSystem> MakeFileSystem(bool use_session_path)


Source/Core/Common/Result.h

+private:
+  std::variant<ResultCode, T> m_variant;
+};
+}  // namespace Common


This adds a lightweight, easy to use std::variant wrapper intended to be used as a return type for functions that can return either a result or an error code.

Add a new FileSystem class that can be used to perform operations on the emulated Wii filesystem. This will allow separating the IPC code (reading from and writing to memory to handle IOS FS commands) and the actual filesystem code in a much better way. This also paves the way for implementing another filesystem backend in the future -- NAND images for more complete emulation, including filesystem metadata like permissions or file orders, which some games and homebrew actually care about -- without needing to make any more changes to the other parts of the codebase, in addition to making filesystem behaviour tests easier to write.

Extract the existing FS code into a HostBackend implementing the filesystem interface. Compared to the original code, this uses less static state. The open host files map is now a member variable as it should have been. Filesystem handles are now also easier to savestate. Some variable names and log messages were cleaned up. Nothing else has been changed.

Now that we have a proper filesystem interface, it makes more sense to return it instead of the emulated IOS device (which isn't really usable for any purpose other than emulated IPC).

It has nothing to do in the filesystem code. (It also smells like a workaround for some kind of timing issue.)

This is the large change in the branch. This lets us use either the host filesystem or (in the future) a NAND image exactly the same way, and make sure the IPC emulation code behaves identically. Less duplicated code. Note that "FileIO" and "FS" were merged, because it actually doesn't make a lot of sense to split them: IOS handles requests for both /dev/fs and files in the same resource manager, and as it turns out, /dev/fs commands can *also* be sent to non /dev/fs file descriptors! If we kept /dev/fs and files split, there would be no way to emulate that correctly. I'm not aware of anything that does that (yet?) but I think it's important to be correct.

Let's use the actual name of the system module. Also, the FS code is not all about files.

Return a FileHandle which will automatically close the FD when the handle goes out of scope. For the rare cases where this behaviour is undesirable, the FD can be released from the handle.

The FS IPC code already logs all commands, so keeping the log messages in the backend is redundant.

Cleaner than having the backend figure out which NAND root to use.

leoetlino · 2018-04-04T09:25:45Z

If no one objects to it, I am going to merge this PR in 2 days (one month after the PR was opened).

Since all FS access will go through the new FS interface (PR dolphin-emu#6421) in order to keep track of metadata properly, there is no need to return absolute paths anymore. In fact, returning host paths is a roadblock to using the FS interface. This starts the migration work by adding a way to get paths that are relative to the Wii NAND instead of always getting absolute paths on the host FS. To prepare for future changes, this commit also makes returned paths canonical by removing the trailing slash when it's unneeded. Eventually, once everything has been migrated to the new interface, we can remove the "from" parameter.

leoetlino force-pushed the fs-interface branch 5 times, most recently from ac38814 to d3de225 Compare March 6, 2018 17:04

leoetlino force-pushed the fs-interface branch from d3de225 to 1c5db03 Compare March 6, 2018 20:05

lioncash reviewed Mar 7, 2018

View reviewed changes

leoetlino force-pushed the fs-interface branch 6 times, most recently from 1a8de0c to a9056de Compare March 8, 2018 17:37

leoetlino added the WIP / do not merge Work in progress (do not merge) label Mar 8, 2018

leoetlino force-pushed the fs-interface branch from a9056de to 687fd33 Compare March 8, 2018 19:17

leoetlino removed the WIP / do not merge Work in progress (do not merge) label Mar 8, 2018

leoetlino force-pushed the fs-interface branch 3 times, most recently from 2cdff4c to 99cb729 Compare March 10, 2018 21:32

lioncash reviewed Mar 13, 2018

View reviewed changes

Source/Core/Core/IOS/FS/FileSystem.cpp Outdated

namespace IOS::HLE::FS

{

std::unique_ptr<FileSystem> MakeFileSystem(bool use_session_path)

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

leoetlino force-pushed the fs-interface branch 5 times, most recently from f60137b to a166b58 Compare March 18, 2018 10:26

abhiaagarwal reviewed Mar 18, 2018

View reviewed changes

Source/Core/Common/Result.h

private:

std::variant<ResultCode, T> m_variant;

};

} // namespace Common

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

leoetlino force-pushed the fs-interface branch from a166b58 to 709fb1a Compare March 21, 2018 09:39

leoetlino mentioned this pull request Mar 30, 2018

IOS: Emulate filesystem timings #6565

Merged

leoetlino added 4 commits March 31, 2018 10:45

Common: Add a Result class

1bdfedf

This adds a lightweight, easy to use std::variant wrapper intended to be used as a return type for functions that can return either a result or an error code.

IOS: Change GetFS() to return a FileSystem

fcfe4e2

Now that we have a proper filesystem interface, it makes more sense to return it instead of the emulated IOS device (which isn't really usable for any purpose other than emulated IPC).

leoetlino force-pushed the fs-interface branch 2 times, most recently from 88d14a3 to c662cef Compare March 31, 2018 08:54

leoetlino added 6 commits March 31, 2018 10:58

Boot: Move CreateVirtualFATFilesystem to Boot_WiiWAD

78478a6

It has nothing to do in the filesystem code. (It also smells like a workaround for some kind of timing issue.)

Log: Rename IOS_FILEIO to IOS_FS

de15e09

Let's use the actual name of the system module. Also, the FS code is not all about files.

IOS/FS: Make it harder to forget closing FDs

2d8be6a

Return a FileHandle which will automatically close the FD when the handle goes out of scope. For the rare cases where this behaviour is undesirable, the FD can be released from the handle.

IOS/FS: Remove some redundant log messages

c56f319

The FS IPC code already logs all commands, so keeping the log messages in the backend is redundant.

IOS/FS: Make the NAND root path a parameter

4e54777

Cleaner than having the backend figure out which NAND root to use.

leoetlino force-pushed the fs-interface branch from c662cef to 4e54777 Compare March 31, 2018 08:58

leoetlino merged commit a957bd1 into dolphin-emu:master Apr 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IOS: Refactor the filesystem code #6421

IOS: Refactor the filesystem code #6421

leoetlino commented Mar 6, 2018

BhaaLseN commented Mar 6, 2018

leoetlino commented Mar 6, 2018

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

leoetlino commented Mar 12, 2018

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

leoetlino commented Apr 4, 2018

IOS: Refactor the filesystem code #6421

IOS: Refactor the filesystem code #6421

Conversation

leoetlino commented Mar 6, 2018

BhaaLseN commented Mar 6, 2018

leoetlino commented Mar 6, 2018

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

leoetlino commented Mar 12, 2018

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

leoetlino commented Apr 4, 2018