WiiSave: Refactor the import/export code #6988

leoetlino · 2018-05-27T14:55:48Z

The current WiiSave code is extremely messy, as it exposes all kinds of implementation details in the header (including internal struct definitions and magic numbers that don't have to be public).

The read/write code is intermingled, so it's hard to tell which members are used, or when/where they are set at all.

It also implicitly relies on some functions being called in a specific order since it doesn't seek manually every time, which makes the code even more fragile.

The logic is also hardcoded to only support bin->nand or nand->bin, even though it would be useful to support nand->nand (for the Movie save copying code, for example).

This commit attempts to solve these problems by getting rid of the WiiSave class:

Read/write code is moved to new Storage classes (NandStorage and DataBinStorage) with small, clear functions that do one and only one thing.
The import/export logic was refactored into a generic Copy function that takes two storages as parameters.
The existing import and export functions are now just small wrappers that call Copy with the appropriate storages.

Because tons of code was moved, the diff may seem pretty large even though most of the code is the same, so I recommend using WiiBrew as a reference for the save code.

The current WiiSave code is extremely messy, as it exposes all kinds of implementation details in the header (including internal struct definitions and magic numbers that don't have to be). The read/write code is intermingled, so it's hard to tell which members are used, or when/where they are set at all. It also implicitly relies on some functions being called in a specific order since it doesn't seek manually every time, which makes the code even more fragile. The logic is also hardcoded to only support bin->nand or nand->bin, even though it would be useful to support nand->nand (for the Movie save copying code, for example). This commit attempts to solve these problems by getting rid of the WiiSave class: * Read/write code is moved to new Storage classes (NandStorage and DataBinStorage) with small, clear functions that do one and only one thing. * The import/export logic was refactored into a generic Copy function that takes two storages as parameters. * The existing import and export functions are now just small wrappers that call Copy with the appropriate storages.

WriteHeader should just write the header and not do anything else.

Source/Core/Core/HW/WiiSave.cpp

+    std::transform(m_files_list.begin(), m_files_list.end(), ret.begin(), [this](const auto& path) {
+      const File::FileInfo file_info{path};
+      SaveFile save_file;
+      save_file.mode = 0x3c;


Source/Core/Core/HW/WiiSave.cpp

-        raw_save_file.WriteBytes(file_data.data(), file_hdr_tmp.size);
+    for (u32 i = 0; i < directories.size(); ++i)
+    {
+      if (i != 0)


Source/Core/Core/HW/WiiSave.cpp

+      SaveFile save_file;
+      save_file.mode = 0x3c;
+      save_file.attributes = 0;
+      save_file.type = file_info.IsDirectory() ? 2 : 1;


Source/Core/Core/HW/WiiSave.cpp

+      file_hdr.permissions = save_file.mode;
+      file_hdr.attrib = save_file.attributes;
+      file_hdr.type = save_file.type;
+      if (save_file.path.length() > 0x44)


Source/Core/Core/HW/WiiSave.cpp

+    // Read data to sign.
+    const u32 data_size = bk_header->size_of_files + 0x80;
+    auto data = std::make_unique<u8[]>(data_size);
+    m_file.Seek(0xf0c0, SEEK_SET);


Source/Core/Core/HW/WiiSave.cpp

-  else
+  const auto nand = MakeNandStorage(ios.GetFS().get(), header->hdr.tid);
+  if (nand->SaveExists() && !AskYesNoT("Save data for this title already exists. Consider backing "
+                                       "up the current data before overwriting.\nOverwrite now?"))


Paths can never exceed 0x40 characters as this is the maximum length that is allowed by IOS (and probably the common FS library too).

leoetlino · 2018-06-01T18:47:37Z

@BhaaLseN fixed (except first and second comments, since the code will be replaced very soon, and the 0xf0c0 comment since I don't know what the values actually are)

BhaaLseN

I guess we can leave it at that. I tried reviewing it a couple of times and only now came thru; and even with that I can only say it looks good, but I haven't really tested it. My NAND seems to be in an abysmal state, so it probably wouldn't help me anyways.

Some other reviews (or some actual testing with a good NAND) would be nice.

Source/Core/Core/HW/WiiSave.cpp

+    // Read data to sign.
+    const u32 data_size = bk_header->size_of_files + 0x80;
+    auto data = std::make_unique<u8[]>(data_size);
+    m_file.Seek(0xf0c0, SEEK_SET);


It would make sense for 0x80 and 0xf0c0 to be respectively sizeof(BkHeader) and sizeof(Header) as Nintendo is signing anything that comes after the header, including the BkHeader.

DataBinHeader is not used anywhere in the code other than via Header, so let's merge them to reduce noise when accessing header fields (currently we have to do header.hdr which looks silly).

BhaaLseN · 2018-06-02T13:45:04Z

Nice, that struct size actually matches those magic numbers 👍

leoetlino force-pushed the wii-save-refactor branch 11 times, most recently from 9e271d6 to 3d1eacd Compare May 31, 2018 18:41

leoetlino force-pushed the wii-save-refactor branch from 3d1eacd to a46a8dd Compare May 31, 2018 18:52

WiiSave: Move overwrite prompt to Import

9495367

WriteHeader should just write the header and not do anything else.

BhaaLseN reviewed Jun 1, 2018

View reviewed changes

WiiSave: Use an enum class for save file type

8af16fd

leoetlino force-pushed the wii-save-refactor branch from 359d537 to ec778ca Compare June 1, 2018 18:44

leoetlino added 2 commits June 1, 2018 20:46

WiiSave: Use the correct length for paths

4df266f

Paths can never exceed 0x40 characters as this is the maximum length that is allowed by IOS (and probably the common FS library too).

WiiSave: Move user interaction to UI frontends

8eafd19

leoetlino force-pushed the wii-save-refactor branch from ec778ca to 8eafd19 Compare June 1, 2018 18:46

BhaaLseN approved these changes Jun 1, 2018

View reviewed changes

Source/Core/Core/HW/WiiSave.cpp Outdated

// Read data to sign.

const u32 data_size = bk_header->size_of_files + 0x80;

auto data = std::make_unique<u8[]>(data_size);

m_file.Seek(0xf0c0, SEEK_SET);

This comment was marked as off-topic.

Sign in to view

leoetlino added 2 commits June 1, 2018 23:44

WiiSave: Get rid of some magic numbers

2103778

It would make sense for 0x80 and 0xf0c0 to be respectively sizeof(BkHeader) and sizeof(Header) as Nintendo is signing anything that comes after the header, including the BkHeader.

WiiSave: Merge Header and DataBinHeader to reduce noise

69abaf3

DataBinHeader is not used anywhere in the code other than via Header, so let's merge them to reduce noise when accessing header fields (currently we have to do header.hdr which looks silly).

leoetlino merged commit 1e51e26 into dolphin-emu:master Jun 3, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WiiSave: Refactor the import/export code #6988

WiiSave: Refactor the import/export code #6988

leoetlino commented May 27, 2018 •

edited

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.

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.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

leoetlino commented Jun 1, 2018

BhaaLseN left a comment

This comment was marked as off-topic.

BhaaLseN commented Jun 2, 2018

WiiSave: Refactor the import/export code #6988

WiiSave: Refactor the import/export code #6988

Conversation

leoetlino commented May 27, 2018 • edited

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.

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.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

leoetlino commented Jun 1, 2018

BhaaLseN left a comment

Choose a reason for hiding this comment

This comment was marked as off-topic.

BhaaLseN commented Jun 2, 2018

leoetlino commented May 27, 2018 •

edited