WiiSave: (not so) Minor cleanup #6836

leoetlino · 2018-05-12T22:50:55Z

Naming fixes: get rid of the legacy "C" prefix and "Crypted" suffix -- we got rid of it for VolumeWiiCrypted a while ago. Also fix all of the struct names (including members).
Don't expose constants and keys that don't have to be.
Remove most of the swaps that were visually polluting the code read/write logic by using Common::BigEndianValue. This is more readable and much less error prone as it becomes impossible to accidentally forget to swap endianness somewhere.
Remove an unused, dummy Extract member function.
Clean up constructor and static functions:

Move the import/export operation into separate functions, as it doesn't really make sense for the constructor to do everything, even things like printing success/failure message boxes.

The existing constructor was split into two: one that takes a path, and another taking a title ID. This makes it more obvious what is actually done when a path/TID is passed. This also makes it clear what parameters should be passed. (No more magic 0 or "" value.)

There's still a significant amount of duplicated code (for instance, the title listing function should be using ES::GetInstalledTitles, the crypto stuff ES::Sign, etc.). I'll address these issues in a followup PR.

Source/Core/Core/HW/WiiSave.h

-  CWiiSaveCrypted(const std::string& filename, u64 title_id = 0);
-  ~CWiiSaveCrypted();
+  WiiSave();
+  WiiSave(const std::string& filename);


Source/Core/Core/HW/WiiSave.cpp

-    mbedtls_aes_setkey_dec(&m_aes_ctx, s_sd_key, 128);
+WiiSave::WiiSave(const std::string& filename) : WiiSave()
+{
+  m_encrypted_save_path = filename;


Source/Core/Core/HW/WiiSave.cpp

-  CWiiSaveCrypted export_save("", title_id);
-  if (export_save.m_valid)
+  WiiSave export_save{title_id};
+  if (export_save.Export())
  {
    SuccessAlertT("Successfully exported file to %s", export_save.m_encrypted_save_path.c_str());


Source/Core/Core/HW/WiiSave.cpp

      success++;
  }
  SuccessAlertT("Successfully exported %u save(s) to %s", success,
                (File::GetUserPath(D_USER_IDX) + "private/wii/title/").c_str());
 }

-CWiiSaveCrypted::CWiiSaveCrypted(const std::string& filename, u64 title_id)
-    : m_encrypted_save_path(filename), m_title_id(title_id)
+WiiSave::WiiSave()
 {
  memcpy(m_sd_iv, "\x21\x67\x12\xE6\xAA\x1F\x68\x9F\x95\xC5\xA2\x23\x24\xDC\x6A\x98", 0x10);


Source/Core/Core/HW/WiiSave.cpp

-const u32 CWiiSaveCrypted::s_ng_id = 0x0403AC68;
+constexpr u8 s_sd_key[16] = {0xAB, 0x01, 0xB9, 0xD8, 0xE1, 0x62, 0x2B, 0x08,
+                             0xAF, 0xBA, 0xD8, 0x4D, 0xBF, 0xC2, 0xA5, 0x5D};
+constexpr u8 s_md5_blanker[16] = {0x0E, 0x65, 0x37, 0x81, 0x99, 0xBE, 0x45, 0x17,


leoetlino · 2018-05-13T11:37:21Z

@lioncash @BhaaLseN done

BhaaLseN · 2018-05-13T12:25:28Z

Just wondering, should Export and ExportAll take a target path as parameter instead of returning a (what seems to be fairly hardcoded) path from them?

Source/Core/DolphinQt2/MenuBar.cpp

-  CWiiSaveCrypted::ExportAllSaves();
+  const std::pair<size_t, std::string> result = WiiSave::ExportAll();
+  QMessageBox::information(this, tr("Save Export"),
+                           tr("Exported %n save(s) to %1", "", static_cast<int>(result.first))


leoetlino · 2018-05-13T12:58:54Z

@BhaaLseN ~~we currently don't let the user select the path. I'm not sure if that's a good thing, but either way if we want to change this I think we should do it in another PR as it'd be mostly a UX change.~~

edit: ended up changing it

[15:00:12] <Bh44L> btw leoetlino, main argument for changing the path thing now is to make the ux change later easier
[15:00:22] <Bh44L> plus, you dont change a to b and in a later pr to c
[15:00:53] <Bh44L> (and you'd get rid of the pair :P)

Gets rid of the need to manually cast when reading/writing, which is error prone and repetitive.

Move the import/export operation into separate functions, as it doesn't really make sense for the constructor to do *everything*, including printing success/failure message boxes. The existing constructor was split into two: one that takes a path, and another taking a title ID. This makes it more obvious what is actually done when a path/TID is passed and also clarifies what parameters should be passed. (No more magic 0 or "" value.)

This moves the result dialogs to DolphinQt2, since WiiSave should not really be responsible for interacting with the user as a simple Wii save importing/exporting class. This also fixes Wii save import/export showing result dialogs twice, once from WiiSave, and another time from DolphinQt2.

Makes copying, comparing more readable

Export and ExportAll now open a directory picker (that defaults to the previous default directory, i.e. the Dolphin user dir). Also removes the need to return the path in the export functions since the user knows which path they chose.

lioncash reviewed May 13, 2018

View reviewed changes

BhaaLseN reviewed May 13, 2018

View reviewed changes

leoetlino force-pushed the wii-save-cleanup branch from 2d5ce0e to 11bf528 Compare May 13, 2018 11:37

leoetlino changed the title ~~WiiSave: Minor cleanup~~ WiiSave: (not so) Minor cleanup May 13, 2018

leoetlino force-pushed the wii-save-cleanup branch 2 times, most recently from 5ab7fe8 to 7336ab6 Compare May 13, 2018 11:49

BhaaLseN reviewed May 13, 2018

View reviewed changes

BhaaLseN approved these changes May 13, 2018

View reviewed changes

leoetlino force-pushed the wii-save-cleanup branch from 42fc23d to 24754e5 Compare May 13, 2018 13:45

leoetlino added 9 commits May 13, 2018 17:42

Rename CWiiSaveCrypted to WiiSave

00bc1f2

WiiSave: Do not expose internal constants

bae8bcd

WiiSave: Fix struct member naming

fb39efb

WiiSave: Use Common::BigEndianValue to simplify parsing

956df21

Gets rid of the need to manually cast when reading/writing, which is error prone and repetitive.

WiiSave: Remove unused Extract() member function

fbf36b8

WiiSave: Use std::array instead of C arrays

87e5cd7

Makes copying, comparing more readable

WiiSave: Allow users to specify export directory

80b3d7c

Export and ExportAll now open a directory picker (that defaults to the previous default directory, i.e. the Dolphin user dir). Also removes the need to return the path in the export functions since the user knows which path they chose.

leoetlino force-pushed the wii-save-cleanup branch from 24754e5 to 80b3d7c Compare May 13, 2018 15:43

leoetlino merged commit e828c24 into dolphin-emu:master May 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WiiSave: (not so) Minor cleanup #6836

WiiSave: (not so) Minor cleanup #6836

leoetlino commented May 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.

This comment was marked as off-topic.

leoetlino commented May 13, 2018

BhaaLseN commented May 13, 2018

This comment was marked as off-topic.

This comment was marked as off-topic.

leoetlino commented May 13, 2018 •

edited

WiiSave: (not so) Minor cleanup #6836

WiiSave: (not so) Minor cleanup #6836

Conversation

leoetlino commented May 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.

This comment was marked as off-topic.

leoetlino commented May 13, 2018

BhaaLseN commented May 13, 2018

This comment was marked as off-topic.

This comment was marked as off-topic.

leoetlino commented May 13, 2018 • edited

leoetlino commented May 13, 2018 •

edited