From b54a49eaaf81170fd312deaff67cfdaf09b3c7eb Mon Sep 17 00:00:00 2001 From: "Admiral H. Curtiss" Date: Sun, 21 Apr 2019 15:18:01 +0200 Subject: [PATCH 1/5] GCMemcard: Rework construction logic to better match our knowledge of the format, while providing better error reporting facilities. --- Source/Core/Core/HW/GCMemcard/GCMemcard.cpp | 532 +++++++++++------- Source/Core/Core/HW/GCMemcard/GCMemcard.h | 42 +- .../Core/HW/GCMemcard/GCMemcardDirectory.cpp | 6 +- Source/Core/DolphinQt/GCMemcardManager.cpp | 7 +- .../Core/DolphinQt/Settings/GameCubePane.cpp | 5 +- 5 files changed, 369 insertions(+), 223 deletions(-) diff --git a/Source/Core/Core/HW/GCMemcard/GCMemcard.cpp b/Source/Core/Core/HW/GCMemcard/GCMemcard.cpp index dad1b6f19b..434a8d892f 100644 --- a/Source/Core/Core/HW/GCMemcard/GCMemcard.cpp +++ b/Source/Core/Core/HW/GCMemcard/GCMemcard.cpp @@ -27,196 +27,249 @@ static void ByteSwap(u8* valueA, u8* valueB) *valueB = tmp; } -GCMemcard::GCMemcard(const std::string& filename, bool forceCreation, bool shift_jis) - : m_valid(false), m_filename(filename) +static constexpr std::optional BytesToMegabits(u64 bytes) { - // Currently there is a string freeze. instead of adding a new message about needing r/w - // open file read only, if write is denied the error will be reported at that point - File::IOFile mcdFile(m_filename, "rb"); - if (!mcdFile.IsOpen()) - { - if (!forceCreation) - { - if (!AskYesNoT("\"%s\" does not exist.\n Create a new 16MB Memory Card?", filename.c_str())) - return; - shift_jis = - AskYesNoT("Format as Shift JIS (Japanese)?\nChoose no for Windows-1252 (Western)"); - } - Format(shift_jis); - return; - } - else - { - // This function can be removed once more about hdr is known and we can check for a valid header - std::string fileType; - SplitPath(filename, nullptr, nullptr, &fileType); - if (strcasecmp(fileType.c_str(), ".raw") && strcasecmp(fileType.c_str(), ".gcp")) - { - PanicAlertT("File has the extension \"%s\".\nValid extensions are (.raw/.gcp)", - fileType.c_str()); - return; - } - auto size = mcdFile.GetSize(); - if (size < MC_FST_BLOCKS * BLOCK_SIZE) - { - PanicAlertT("%s failed to load as a memory card.\nFile is not large enough to be a valid " - "memory card file (0x%x bytes)", - filename.c_str(), (unsigned)size); - return; - } - if (size % BLOCK_SIZE) - { - PanicAlertT("%s failed to load as a memory card.\nCard file size is invalid (0x%x bytes)", - filename.c_str(), (unsigned)size); - return; - } - - m_size_mb = (u16)((size / BLOCK_SIZE) / MBIT_TO_BLOCKS); - switch (m_size_mb) - { - case MemCard59Mb: - case MemCard123Mb: - case MemCard251Mb: - case Memcard507Mb: - case MemCard1019Mb: - case MemCard2043Mb: - break; - default: - PanicAlertT("%s failed to load as a memory card.\nCard size is invalid (0x%x bytes)", - filename.c_str(), (unsigned)size); - return; - } - } - - mcdFile.Seek(0, SEEK_SET); - if (!mcdFile.ReadBytes(&m_header_block, BLOCK_SIZE)) - { - PanicAlertT("Failed to read header correctly\n(0x0000-0x1FFF)"); - return; - } - if (m_size_mb != m_header_block.m_size_mb) - { - PanicAlertT("Memory card file size does not match the header size"); - return; - } - - if (!mcdFile.ReadBytes(&m_directory_blocks[0], BLOCK_SIZE)) - { - PanicAlertT("Failed to read 1st directory block correctly\n(0x2000-0x3FFF)"); - return; - } - - if (!mcdFile.ReadBytes(&m_directory_blocks[1], BLOCK_SIZE)) - { - PanicAlertT("Failed to read 2nd directory block correctly\n(0x4000-0x5FFF)"); - return; - } - - if (!mcdFile.ReadBytes(&m_bat_blocks[0], BLOCK_SIZE)) - { - PanicAlertT("Failed to read 1st block allocation table block correctly\n(0x6000-0x7FFF)"); - return; - } - - if (!mcdFile.ReadBytes(&m_bat_blocks[1], BLOCK_SIZE)) - { - PanicAlertT("Failed to read 2nd block allocation table block correctly\n(0x8000-0x9FFF)"); - return; - } - - u32 csums = TestChecksums(); - - if (csums & 0x1) - { - // header checksum error! - // invalid files do not always get here - PanicAlertT("Header checksum failed"); - return; - } - - if (csums & 0x2) // 1st directory block checksum error! - { - if (csums & 0x4) - { - // 2nd block is also wrong! - PanicAlertT("Both directory block checksums are invalid"); - return; - } - else - { - // FIXME: This is probably incorrect behavior, confirm what actually happens on hardware here. - // The currently active directory block and currently active BAT block don't necessarily have - // to correlate. - - // 2nd block is correct, restore - m_directory_blocks[0] = m_directory_blocks[1]; - m_bat_blocks[0] = m_bat_blocks[1]; - - // update checksums - csums = TestChecksums(); - } - } - - if (csums & 0x8) // 1st BAT checksum error! - { - if (csums & 0x10) - { - // 2nd BAT is also wrong! - PanicAlertT("Both Block Allocation Table block checksums are invalid"); - return; - } - else - { - // FIXME: Same as above, this feels incorrect. - - // 2nd block is correct, restore - m_directory_blocks[0] = m_directory_blocks[1]; - m_bat_blocks[0] = m_bat_blocks[1]; - - // update checksums - csums = TestChecksums(); - } - } - - mcdFile.Seek(0xa000, SEEK_SET); - - m_size_blocks = (u32)m_size_mb * MBIT_TO_BLOCKS; - m_data_blocks.reserve(m_size_blocks - MC_FST_BLOCKS); - - m_valid = true; - for (u32 i = MC_FST_BLOCKS; i < m_size_blocks; ++i) - { - GCMBlock b; - if (mcdFile.ReadBytes(b.m_block.data(), b.m_block.size())) - { - m_data_blocks.push_back(b); - } - else - { - PanicAlertT("Failed to read block %u of the save data\nMemory card may be truncated\nFile " - "position: 0x%" PRIx64, - i, mcdFile.Tell()); - m_valid = false; - break; - } - } - - mcdFile.Close(); - - InitActiveDirBat(); + const u64 factor = ((1024 * 1024) / 8); + const u64 megabits = bytes / factor; + const u64 remainder = bytes % factor; + if (remainder != 0) + return std::nullopt; + return megabits; } -void GCMemcard::InitActiveDirBat() +bool GCMemcardErrorCode::HasCriticalErrors() const { - if (m_directory_blocks[0].m_update_counter > m_directory_blocks[1].m_update_counter) - m_active_directory = 0; - else - m_active_directory = 1; + return Test(GCMemcardValidityIssues::FAILED_TO_OPEN) || Test(GCMemcardValidityIssues::IO_ERROR) || + Test(GCMemcardValidityIssues::INVALID_CARD_SIZE) || + Test(GCMemcardValidityIssues::INVALID_CHECKSUM) || + Test(GCMemcardValidityIssues::MISMATCHED_CARD_SIZE) || + Test(GCMemcardValidityIssues::FREE_BLOCK_MISMATCH); +} - if (m_bat_blocks[0].m_update_counter > m_bat_blocks[1].m_update_counter) - m_active_bat = 0; +bool GCMemcardErrorCode::Test(GCMemcardValidityIssues code) const +{ + return m_errors.test(static_cast(code)); +} + +void GCMemcardErrorCode::Set(GCMemcardValidityIssues code) +{ + m_errors.set(static_cast(code)); +} + +GCMemcardErrorCode& GCMemcardErrorCode::operator|=(const GCMemcardErrorCode& other) +{ + this->m_errors |= other.m_errors; + return *this; +} + +GCMemcard::GCMemcard() + : m_valid(false), m_size_blocks(0), m_size_mb(0), m_active_directory(0), m_active_bat(0) +{ +} + +std::optional GCMemcard::Create(std::string filename, u16 size_mbits, bool shift_jis) +{ + GCMemcard card; + card.m_filename = std::move(filename); + + // TODO: Format() not only formats the card but also writes it to disk at m_filename. + // Those tasks should probably be separated. + if (!card.Format(shift_jis, size_mbits)) + return std::nullopt; + + return std::move(card); +} + +std::pair> GCMemcard::Open(std::string filename) +{ + GCMemcardErrorCode error_code; + File::IOFile file(filename, "rb"); + if (!file.IsOpen()) + { + error_code.Set(GCMemcardValidityIssues::FAILED_TO_OPEN); + return std::make_pair(error_code, std::nullopt); + } + + // check if the filesize is a valid memory card size + const u64 filesize = file.GetSize(); + const u64 filesize_megabits = BytesToMegabits(filesize).value_or(0); + const std::array valid_megabits = {{ + MemCard59Mb, + MemCard123Mb, + MemCard251Mb, + Memcard507Mb, + MemCard1019Mb, + MemCard2043Mb, + }}; + + if (!std::any_of(valid_megabits.begin(), valid_megabits.end(), + [filesize_megabits](u64 mbits) { return mbits == filesize_megabits; })) + { + error_code.Set(GCMemcardValidityIssues::INVALID_CARD_SIZE); + return std::make_pair(error_code, std::nullopt); + } + + const u16 card_size_mbits = static_cast(filesize_megabits); + + // read the entire card into memory + GCMemcard card; + file.Seek(0, SEEK_SET); + if (!file.ReadBytes(&card.m_header_block, BLOCK_SIZE) || + !file.ReadBytes(&card.m_directory_blocks[0], BLOCK_SIZE) || + !file.ReadBytes(&card.m_directory_blocks[1], BLOCK_SIZE) || + !file.ReadBytes(&card.m_bat_blocks[0], BLOCK_SIZE) || + !file.ReadBytes(&card.m_bat_blocks[1], BLOCK_SIZE)) + { + error_code.Set(GCMemcardValidityIssues::IO_ERROR); + return std::make_pair(error_code, std::nullopt); + } + + const u16 card_size_blocks = card_size_mbits * MBIT_TO_BLOCKS; + const u16 user_data_blocks = card_size_blocks - MC_FST_BLOCKS; + card.m_data_blocks.reserve(user_data_blocks); + for (u16 i = 0; i < user_data_blocks; ++i) + { + GCMBlock& block = card.m_data_blocks.emplace_back(); + if (!file.ReadArray(block.m_block.data(), BLOCK_SIZE)) + { + error_code.Set(GCMemcardValidityIssues::IO_ERROR); + return std::make_pair(error_code, std::nullopt); + } + } + + file.Close(); + + card.m_filename = std::move(filename); + card.m_size_blocks = card_size_blocks; + card.m_size_mb = card_size_mbits; + + // can return invalid card size, invalid checksum, data in unused area + // data in unused area is okay, otherwise fail + const GCMemcardErrorCode header_error_code = card.m_header_block.CheckForErrors(card_size_mbits); + error_code |= header_error_code; + if (header_error_code.HasCriticalErrors()) + return std::make_pair(error_code, std::nullopt); + + // The GC BIOS counts any card as corrupted as long as at least any two of [dir0, dir1, bat0, + // bat1] are corrupted. Yes, even if we have one valid dir and one valid bat, and even if those + // are both supposedly the newer ones. + // + // If both blocks of a single category are non-corrupted the used block depends on the update + // counter. If both blocks have the same update counter, it prefers block 0. Otherwise it prefers + // whichever block has the higher value. Essentially, if (0.update_ctr >= 1.update_ctr) { use 0 } + // else { use 1 }. + // + // If a single block of the four is corrupted, the non-corrupted one of the same category is + // immediately copied over the corrupted block with an incremented update counter. At this point + // both blocks contain the same data, so it's hard to tell which one is used, but presumably it + // uses the one with the now-higher update counter, same as it would have otherwise. + // + // This rule only applies for errors within a single block! That is, invalid checksums for both + // types, and free block mismatch for the BATs. Once two valid blocks have been selected but it + // later turns out they do not match eachother (eg. claimed block count of a file in the directory + // does not match the actual block count arrived at by following BAT), the card will be treated as + // corrupted, even if perhaps a different combination of the two blocks would result in a valid + // memory card. + + // can return invalid checksum, data in unused area + GCMemcardErrorCode dir_block_0_error_code = card.m_directory_blocks[0].CheckForErrors(); + GCMemcardErrorCode dir_block_1_error_code = card.m_directory_blocks[1].CheckForErrors(); + + // can return invalid card size, invalid checksum, data in unused area, free block mismatch + GCMemcardErrorCode bat_block_0_error_code = card.m_bat_blocks[0].CheckForErrors(card_size_mbits); + GCMemcardErrorCode bat_block_1_error_code = card.m_bat_blocks[1].CheckForErrors(card_size_mbits); + + const bool dir_block_0_valid = !dir_block_0_error_code.HasCriticalErrors(); + const bool dir_block_1_valid = !dir_block_1_error_code.HasCriticalErrors(); + const bool bat_block_0_valid = !bat_block_0_error_code.HasCriticalErrors(); + const bool bat_block_1_valid = !bat_block_1_error_code.HasCriticalErrors(); + + // if any two (at least) blocks are corrupted return failure + // TODO: Consider allowing a recovery option when there's still a valid one of each type. + int number_of_corrupted_dir_bat_blocks = 0; + if (!dir_block_0_valid) + ++number_of_corrupted_dir_bat_blocks; + if (!dir_block_1_valid) + ++number_of_corrupted_dir_bat_blocks; + if (!bat_block_0_valid) + ++number_of_corrupted_dir_bat_blocks; + if (!bat_block_1_valid) + ++number_of_corrupted_dir_bat_blocks; + + if (number_of_corrupted_dir_bat_blocks > 1) + { + error_code |= dir_block_0_error_code; + error_code |= dir_block_1_error_code; + error_code |= bat_block_0_error_code; + error_code |= bat_block_1_error_code; + return std::make_pair(error_code, std::nullopt); + } + + // if exactly one block is corrupted copy and update it over the non-corrupted block + if (number_of_corrupted_dir_bat_blocks == 1) + { + if (!dir_block_0_valid) + { + card.m_directory_blocks[0] = card.m_directory_blocks[1]; + card.m_directory_blocks[0].m_update_counter = card.m_directory_blocks[0].m_update_counter + 1; + card.m_directory_blocks[0].FixChecksums(); + dir_block_0_error_code = card.m_directory_blocks[0].CheckForErrors(); + } + else if (!dir_block_1_valid) + { + card.m_directory_blocks[1] = card.m_directory_blocks[0]; + card.m_directory_blocks[1].m_update_counter = card.m_directory_blocks[1].m_update_counter + 1; + card.m_directory_blocks[1].FixChecksums(); + dir_block_1_error_code = card.m_directory_blocks[1].CheckForErrors(); + } + else if (!bat_block_0_valid) + { + card.m_bat_blocks[0] = card.m_bat_blocks[1]; + card.m_bat_blocks[0].m_update_counter = card.m_bat_blocks[0].m_update_counter + 1; + card.m_bat_blocks[0].FixChecksums(); + bat_block_0_error_code = card.m_bat_blocks[0].CheckForErrors(card_size_mbits); + } + else if (!bat_block_1_valid) + { + card.m_bat_blocks[1] = card.m_bat_blocks[0]; + card.m_bat_blocks[1].m_update_counter = card.m_bat_blocks[1].m_update_counter + 1; + card.m_bat_blocks[1].FixChecksums(); + bat_block_1_error_code = card.m_bat_blocks[1].CheckForErrors(card_size_mbits); + } + else + { + // should never reach here + assert(0); + } + } + + error_code |= dir_block_0_error_code; + error_code |= dir_block_1_error_code; + error_code |= bat_block_0_error_code; + error_code |= bat_block_1_error_code; + + // select the in-use Dir and BAT blocks based on update counter + // TODO: Is there a special case for wraparound after 65535 block updates, or is it assumed that + // the hardware fails long before that anyway? + + if (card.m_directory_blocks[0].m_update_counter >= card.m_directory_blocks[1].m_update_counter) + card.m_active_directory = 0; else - m_active_bat = 1; + card.m_active_directory = 1; + + if (card.m_bat_blocks[0].m_update_counter >= card.m_bat_blocks[1].m_update_counter) + card.m_active_bat = 0; + else + card.m_active_bat = 1; + + // TODO: Do a card-wide corruption check here, such as cross-checking the active Dir's files with + // the active BAT's allocated sectors. The GC BIOS does at the very least recognize if a file's + // number of used blocks is inconsistent between the two, and will count a card as corrupted if + // that is the case. + + card.m_valid = true; + + return std::make_pair(error_code, std::move(card)); } const Directory& GCMemcard::GetActiveDirectory() const @@ -292,31 +345,6 @@ std::pair CalculateMemcardChecksums(const u8* data, size_t size) return std::make_pair(csum, inv_csum); } -u32 GCMemcard::TestChecksums() const -{ - const auto [csum_hdr, cinv_hdr] = m_header_block.CalculateChecksums(); - const auto [csum_dir0, cinv_dir0] = m_directory_blocks[0].CalculateChecksums(); - const auto [csum_dir1, cinv_dir1] = m_directory_blocks[1].CalculateChecksums(); - const auto [csum_bat0, cinv_bat0] = m_bat_blocks[0].CalculateChecksums(); - const auto [csum_bat1, cinv_bat1] = m_bat_blocks[1].CalculateChecksums(); - - u32 results = 0; - if ((m_header_block.m_checksum != csum_hdr) || (m_header_block.m_checksum_inv != cinv_hdr)) - results |= 1; - if ((m_directory_blocks[0].m_checksum != csum_dir0) || - (m_directory_blocks[0].m_checksum_inv != cinv_dir0)) - results |= 2; - if ((m_directory_blocks[1].m_checksum != csum_dir1) || - (m_directory_blocks[1].m_checksum_inv != cinv_dir1)) - results |= 4; - if ((m_bat_blocks[0].m_checksum != csum_bat0) || (m_bat_blocks[0].m_checksum_inv != cinv_bat0)) - results |= 8; - if ((m_bat_blocks[1].m_checksum != csum_bat1) || (m_bat_blocks[1].m_checksum_inv != cinv_bat1)) - results |= 16; - - return results; -} - bool GCMemcard::FixChecksums() { if (!m_valid) @@ -679,6 +707,50 @@ std::pair BlockAlloc::CalculateChecksums() const return CalculateMemcardChecksums(&raw[checksum_area_start], checksum_area_size); } +GCMemcardErrorCode BlockAlloc::CheckForErrors(u16 size_mbits) const +{ + GCMemcardErrorCode error_code; + + // verify checksums + const auto [checksum_sum, checksum_inv] = CalculateChecksums(); + if (checksum_sum != m_checksum || checksum_inv != m_checksum_inv) + error_code.Set(GCMemcardValidityIssues::INVALID_CHECKSUM); + + if (size_mbits > 0 && size_mbits <= 256) + { + // check if free block count matches the actual amount of free blocks in m_map + const u16 total_available_blocks = (size_mbits * MBIT_TO_BLOCKS) - MC_FST_BLOCKS; + assert(total_available_blocks <= m_map.size()); + u16 blocks_in_use = 0; + for (size_t i = 0; i < total_available_blocks; ++i) + { + if (m_map[i] != 0) + ++blocks_in_use; + } + const u16 free_blocks = total_available_blocks - blocks_in_use; + + if (free_blocks != m_free_blocks) + error_code.Set(GCMemcardValidityIssues::FREE_BLOCK_MISMATCH); + + // remaining blocks map to nothing on hardware and must be empty + for (size_t i = total_available_blocks; i < m_map.size(); ++i) + { + if (m_map[i] != 0) + { + error_code.Set(GCMemcardValidityIssues::DATA_IN_UNUSED_AREA); + break; + } + } + } + else + { + // card size is outside the range of blocks that can be addressed + error_code.Set(GCMemcardValidityIssues::INVALID_CARD_SIZE); + } + + return error_code; +} + GCMemcardGetSaveDataRetVal GCMemcard::GetSaveData(u8 index, std::vector& Blocks) const { if (!m_valid) @@ -1253,7 +1325,8 @@ bool GCMemcard::Format(bool shift_jis, u16 SizeMb) m_data_blocks.clear(); m_data_blocks.resize(m_size_blocks - MC_FST_BLOCKS); - InitActiveDirBat(); + m_active_directory = 0; + m_active_bat = 0; m_valid = true; return Save(); @@ -1473,6 +1546,29 @@ std::pair Header::CalculateChecksums() const return CalculateMemcardChecksums(&raw[checksum_area_start], checksum_area_size); } +GCMemcardErrorCode Header::CheckForErrors(u16 card_size_mbits) const +{ + GCMemcardErrorCode error_code; + + // total card size should match card size in header + if (m_size_mb != card_size_mbits) + error_code.Set(GCMemcardValidityIssues::MISMATCHED_CARD_SIZE); + + // unused areas, should always be filled with 0xFF + if (std::any_of(m_unused_1.begin(), m_unused_1.end(), [](u8 val) { return val != 0xFF; }) || + std::any_of(m_unused_2.begin(), m_unused_2.end(), [](u8 val) { return val != 0xFF; })) + { + error_code.Set(GCMemcardValidityIssues::DATA_IN_UNUSED_AREA); + } + + // verify checksums + const auto [checksum_sum, checksum_inv] = CalculateChecksums(); + if (checksum_sum != m_checksum || checksum_inv != m_checksum_inv) + error_code.Set(GCMemcardValidityIssues::INVALID_CHECKSUM); + + return error_code; +} + Directory::Directory() { memset(this, 0xFF, BLOCK_SIZE); @@ -1508,3 +1604,19 @@ std::pair Directory::CalculateChecksums() const constexpr size_t checksum_area_size = checksum_area_end - checksum_area_start; return CalculateMemcardChecksums(&raw[checksum_area_start], checksum_area_size); } + +GCMemcardErrorCode Directory::CheckForErrors() const +{ + GCMemcardErrorCode error_code; + + // verify checksums + const auto [checksum_sum, checksum_inv] = CalculateChecksums(); + if (checksum_sum != m_checksum || checksum_inv != m_checksum_inv) + error_code.Set(GCMemcardValidityIssues::INVALID_CHECKSUM); + + // unused area, should always be filled with 0xFF + if (std::any_of(m_padding.begin(), m_padding.end(), [](u8 val) { return val != 0xFF; })) + error_code.Set(GCMemcardValidityIssues::DATA_IN_UNUSED_AREA); + + return error_code; +} diff --git a/Source/Core/Core/HW/GCMemcard/GCMemcard.h b/Source/Core/Core/HW/GCMemcard/GCMemcard.h index 8d735ced1f..4f575dfe79 100644 --- a/Source/Core/Core/HW/GCMemcard/GCMemcard.h +++ b/Source/Core/Core/HW/GCMemcard/GCMemcard.h @@ -6,6 +6,7 @@ #include #include +#include #include #include @@ -79,6 +80,30 @@ enum class GCMemcardRemoveFileRetVal DELETE_FAIL, }; +enum class GCMemcardValidityIssues +{ + FAILED_TO_OPEN, + IO_ERROR, + INVALID_CARD_SIZE, + INVALID_CHECKSUM, + MISMATCHED_CARD_SIZE, + FREE_BLOCK_MISMATCH, + DATA_IN_UNUSED_AREA, + COUNT +}; + +class GCMemcardErrorCode +{ +public: + bool HasCriticalErrors() const; + bool Test(GCMemcardValidityIssues code) const; + void Set(GCMemcardValidityIssues code); + GCMemcardErrorCode& operator|=(const GCMemcardErrorCode& other); + +private: + std::bitset(GCMemcardValidityIssues::COUNT)> m_errors; +}; + // size of a single memory card block in bytes constexpr u32 BLOCK_SIZE = 0x2000; @@ -106,7 +131,7 @@ constexpr u16 BAT_SIZE = 0xFFB; constexpr u16 MemCard59Mb = 0x04; constexpr u16 MemCard123Mb = 0x08; constexpr u16 MemCard251Mb = 0x10; -constexpr u16 Memcard507Mb = 0x20; +constexpr u16 Memcard507Mb = 0x20; // FIXME: case constexpr u16 MemCard1019Mb = 0x40; constexpr u16 MemCard2043Mb = 0x80; @@ -192,6 +217,8 @@ struct Header void FixChecksums(); std::pair CalculateChecksums() const; + + GCMemcardErrorCode CheckForErrors(u16 card_size_mbits) const; }; static_assert(sizeof(Header) == BLOCK_SIZE); @@ -301,6 +328,8 @@ struct Directory void FixChecksums(); std::pair CalculateChecksums() const; + + GCMemcardErrorCode CheckForErrors() const; }; static_assert(sizeof(Directory) == BLOCK_SIZE); @@ -333,6 +362,8 @@ struct BlockAlloc void FixChecksums(); std::pair CalculateChecksums() const; + + GCMemcardErrorCode CheckForErrors(u16 size_mbits) const; }; static_assert(sizeof(BlockAlloc) == BLOCK_SIZE); #pragma pack(pop) @@ -354,8 +385,9 @@ private: int m_active_directory; int m_active_bat; + GCMemcard(); + GCMemcardImportFileRetVal ImportGciInternal(File::IOFile&& gci, const std::string& inputFile); - void InitActiveDirBat(); const Directory& GetActiveDirectory() const; const BlockAlloc& GetActiveBat() const; @@ -364,8 +396,9 @@ private: void UpdateBat(const BlockAlloc& bat); public: - explicit GCMemcard(const std::string& fileName, bool forceCreation = false, - bool shift_jis = false); + static std::optional Create(std::string filename, u16 size_mbits, bool shift_jis); + + static std::pair> Open(std::string filename); GCMemcard(const GCMemcard&) = delete; GCMemcard& operator=(const GCMemcard&) = delete; @@ -382,7 +415,6 @@ public: static s32 PSO_MakeSaveGameValid(const Header& cardheader, const DEntry& direntry, std::vector& FileBuffer); - u32 TestChecksums() const; bool FixChecksums(); // get number of file entries in the directory diff --git a/Source/Core/Core/HW/GCMemcard/GCMemcardDirectory.cpp b/Source/Core/Core/HW/GCMemcard/GCMemcardDirectory.cpp index 54fae94d24..6e03d63f3f 100644 --- a/Source/Core/Core/HW/GCMemcard/GCMemcardDirectory.cpp +++ b/Source/Core/Core/HW/GCMemcard/GCMemcardDirectory.cpp @@ -703,12 +703,12 @@ void MigrateFromMemcardFile(const std::string& directory_name, int card_index) Config::Get(Config::MAIN_MEMCARD_B_PATH); if (File::Exists(ini_memcard)) { - GCMemcard memcard(ini_memcard.c_str()); - if (memcard.IsValid()) + auto [error_code, memcard] = GCMemcard::Open(ini_memcard.c_str()); + if (!error_code.HasCriticalErrors() && memcard && memcard->IsValid()) { for (u8 i = 0; i < DIRLEN; i++) { - memcard.ExportGci(i, "", directory_name); + memcard->ExportGci(i, "", directory_name); } } } diff --git a/Source/Core/DolphinQt/GCMemcardManager.cpp b/Source/Core/DolphinQt/GCMemcardManager.cpp index 02d3adacab..7b60853f8d 100644 --- a/Source/Core/DolphinQt/GCMemcardManager.cpp +++ b/Source/Core/DolphinQt/GCMemcardManager.cpp @@ -241,13 +241,14 @@ void GCMemcardManager::UpdateActions() void GCMemcardManager::SetSlotFile(int slot, QString path) { - auto memcard = std::make_unique(path.toStdString()); + // TODO: Check error codes and give reasonable error messages. + auto [error_code, memcard] = GCMemcard::Open(path.toStdString()); - if (!memcard->IsValid()) + if (error_code.HasCriticalErrors() || !memcard || !memcard->IsValid()) return; m_slot_file_edit[slot]->setText(path); - m_slot_memcard[slot] = std::move(memcard); + m_slot_memcard[slot] = std::make_unique(std::move(*memcard)); UpdateSlotTable(slot); UpdateActions(); diff --git a/Source/Core/DolphinQt/Settings/GameCubePane.cpp b/Source/Core/DolphinQt/Settings/GameCubePane.cpp index 7d3ac2b8eb..46595fc3c3 100644 --- a/Source/Core/DolphinQt/Settings/GameCubePane.cpp +++ b/Source/Core/DolphinQt/Settings/GameCubePane.cpp @@ -212,9 +212,10 @@ void GameCubePane::OnConfigPressed(int slot) { if (File::Exists(filename.toStdString())) { - GCMemcard mc(filename.toStdString()); + // TODO: check error codes and give reasonable error messages + auto [error_code, mc] = GCMemcard::Open(filename.toStdString()); - if (!mc.IsValid()) + if (error_code.HasCriticalErrors() || !mc || !mc->IsValid()) { ModalMessageBox::critical(this, tr("Error"), tr("Cannot use that file as a memory card.\n%1\n" From 8fc2f0ff2d283e32404f7c34ec3fe30652f350dc Mon Sep 17 00:00:00 2001 From: "Admiral H. Curtiss" Date: Wed, 14 Aug 2019 23:31:43 +0200 Subject: [PATCH 2/5] GCMemcard: Rename MemCardXMb constants to MBIT_SIZE_MEMORY_CARD_X for consistency with other constants. --- .../Core/Core/HW/EXI/EXI_DeviceMemoryCard.cpp | 4 ++-- Source/Core/Core/HW/GCMemcard/GCMemcard.cpp | 12 +++++----- Source/Core/Core/HW/GCMemcard/GCMemcard.h | 24 ++++++++++--------- Source/Core/Core/HW/GCMemcard/GCMemcardRaw.h | 3 ++- 4 files changed, 23 insertions(+), 20 deletions(-) diff --git a/Source/Core/Core/HW/EXI/EXI_DeviceMemoryCard.cpp b/Source/Core/Core/HW/EXI/EXI_DeviceMemoryCard.cpp index 7c4f453c27..db171bf378 100644 --- a/Source/Core/Core/HW/EXI/EXI_DeviceMemoryCard.cpp +++ b/Source/Core/Core/HW/EXI/EXI_DeviceMemoryCard.cpp @@ -141,7 +141,7 @@ CEXIMemoryCard::CEXIMemoryCard(const int index, bool gciFolder) : card_index(ind bool useMC251; IniFile gameIni = SConfig::GetInstance().LoadGameIni(); gameIni.GetOrCreateSection("Core")->Get("MemoryCard251", &useMC251, false); - u16 sizeMb = useMC251 ? MemCard251Mb : MemCard2043Mb; + u16 sizeMb = useMC251 ? MBIT_SIZE_MEMORY_CARD_251 : MBIT_SIZE_MEMORY_CARD_2043; if (gciFolder) { @@ -241,7 +241,7 @@ void CEXIMemoryCard::SetupRawMemcard(u16 sizeMb) SConfig::GetDirectoryForRegion(SConfig::ToGameCubeRegion(SConfig::GetInstance().m_region)); MemoryCard::CheckPath(filename, region_dir, is_slot_a); - if (sizeMb == MemCard251Mb) + if (sizeMb == MBIT_SIZE_MEMORY_CARD_251) filename.insert(filename.find_last_of("."), ".251"); memorycard = std::make_unique(filename, card_index, sizeMb); diff --git a/Source/Core/Core/HW/GCMemcard/GCMemcard.cpp b/Source/Core/Core/HW/GCMemcard/GCMemcard.cpp index 434a8d892f..f610243238 100644 --- a/Source/Core/Core/HW/GCMemcard/GCMemcard.cpp +++ b/Source/Core/Core/HW/GCMemcard/GCMemcard.cpp @@ -94,12 +94,12 @@ std::pair> GCMemcard::Open(std::str const u64 filesize = file.GetSize(); const u64 filesize_megabits = BytesToMegabits(filesize).value_or(0); const std::array valid_megabits = {{ - MemCard59Mb, - MemCard123Mb, - MemCard251Mb, - Memcard507Mb, - MemCard1019Mb, - MemCard2043Mb, + MBIT_SIZE_MEMORY_CARD_59, + MBIT_SIZE_MEMORY_CARD_123, + MBIT_SIZE_MEMORY_CARD_251, + MBIT_SIZE_MEMORY_CARD_507, + MBIT_SIZE_MEMORY_CARD_1019, + MBIT_SIZE_MEMORY_CARD_2043, }}; if (!std::any_of(valid_megabits.begin(), valid_megabits.end(), diff --git a/Source/Core/Core/HW/GCMemcard/GCMemcard.h b/Source/Core/Core/HW/GCMemcard/GCMemcard.h index 4f575dfe79..8855a493de 100644 --- a/Source/Core/Core/HW/GCMemcard/GCMemcard.h +++ b/Source/Core/Core/HW/GCMemcard/GCMemcard.h @@ -128,17 +128,17 @@ constexpr u16 BAT_SIZE = 0xFFB; // possible sizes of memory cards in megabits // TODO: Do memory card sizes have to be power of two? // TODO: Are these all of them? A 4091 block card should work in theory at least. -constexpr u16 MemCard59Mb = 0x04; -constexpr u16 MemCard123Mb = 0x08; -constexpr u16 MemCard251Mb = 0x10; -constexpr u16 Memcard507Mb = 0x20; // FIXME: case -constexpr u16 MemCard1019Mb = 0x40; -constexpr u16 MemCard2043Mb = 0x80; +constexpr u16 MBIT_SIZE_MEMORY_CARD_59 = 0x04; +constexpr u16 MBIT_SIZE_MEMORY_CARD_123 = 0x08; +constexpr u16 MBIT_SIZE_MEMORY_CARD_251 = 0x10; +constexpr u16 MBIT_SIZE_MEMORY_CARD_507 = 0x20; +constexpr u16 MBIT_SIZE_MEMORY_CARD_1019 = 0x40; +constexpr u16 MBIT_SIZE_MEMORY_CARD_2043 = 0x80; class MemoryCardBase { public: - explicit MemoryCardBase(int card_index = 0, int size_mbits = MemCard2043Mb) + explicit MemoryCardBase(int card_index = 0, int size_mbits = MBIT_SIZE_MEMORY_CARD_2043) : m_card_index(card_index), m_nintendo_card_id(size_mbits) { } @@ -210,7 +210,8 @@ struct Header // 0x1e00 bytes at 0x0200: Unused (0xff) std::array m_unused_2; - explicit Header(int slot = 0, u16 size_mbits = MemCard2043Mb, bool shift_jis = false); + explicit Header(int slot = 0, u16 size_mbits = MBIT_SIZE_MEMORY_CARD_2043, + bool shift_jis = false); // Calculates the card serial numbers used for encrypting some save files. std::pair CalculateSerial() const; @@ -353,7 +354,7 @@ struct BlockAlloc // 0x1ff8 bytes at 0x000a: Map of allocated Blocks std::array, BAT_SIZE> m_map; - explicit BlockAlloc(u16 size_mbits = MemCard2043Mb); + explicit BlockAlloc(u16 size_mbits = MBIT_SIZE_MEMORY_CARD_2043); u16 GetNextBlock(u16 block) const; u16 NextFreeBlock(u16 max_block, u16 starting_block = MC_FST_BLOCKS) const; @@ -408,8 +409,9 @@ public: bool IsValid() const { return m_valid; } bool IsShiftJIS() const; bool Save(); - bool Format(bool shift_jis = false, u16 SizeMb = MemCard2043Mb); - static bool Format(u8* card_data, bool shift_jis = false, u16 SizeMb = MemCard2043Mb); + bool Format(bool shift_jis = false, u16 SizeMb = MBIT_SIZE_MEMORY_CARD_2043); + static bool Format(u8* card_data, bool shift_jis = false, + u16 SizeMb = MBIT_SIZE_MEMORY_CARD_2043); static s32 FZEROGX_MakeSaveGameValid(const Header& cardheader, const DEntry& direntry, std::vector& FileBuffer); static s32 PSO_MakeSaveGameValid(const Header& cardheader, const DEntry& direntry, diff --git a/Source/Core/Core/HW/GCMemcard/GCMemcardRaw.h b/Source/Core/Core/HW/GCMemcard/GCMemcardRaw.h index 0659e33d3a..727d625aa4 100644 --- a/Source/Core/Core/HW/GCMemcard/GCMemcardRaw.h +++ b/Source/Core/Core/HW/GCMemcard/GCMemcardRaw.h @@ -17,7 +17,8 @@ class PointerWrap; class MemoryCard : public MemoryCardBase { public: - MemoryCard(const std::string& filename, int card_index, u16 size_mbits = MemCard2043Mb); + MemoryCard(const std::string& filename, int card_index, + u16 size_mbits = MBIT_SIZE_MEMORY_CARD_2043); ~MemoryCard(); static void CheckPath(std::string& memcardPath, const std::string& gameRegion, bool isSlotA); void FlushThread(); From 7d4cabea07c48186688570df531b0c203dcb94d0 Mon Sep 17 00:00:00 2001 From: "Admiral H. Curtiss" Date: Wed, 14 Aug 2019 23:50:43 +0200 Subject: [PATCH 3/5] GCMemcard: The update counters are interpreted as signed values for the newer-than comparison. --- Source/Core/Core/HW/GCMemcard/GCMemcard.cpp | 7 +++++-- Source/Core/Core/HW/GCMemcard/GCMemcard.h | 5 ++--- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/Source/Core/Core/HW/GCMemcard/GCMemcard.cpp b/Source/Core/Core/HW/GCMemcard/GCMemcard.cpp index f610243238..adccd920fa 100644 --- a/Source/Core/Core/HW/GCMemcard/GCMemcard.cpp +++ b/Source/Core/Core/HW/GCMemcard/GCMemcard.cpp @@ -249,8 +249,11 @@ std::pair> GCMemcard::Open(std::str error_code |= bat_block_1_error_code; // select the in-use Dir and BAT blocks based on update counter - // TODO: Is there a special case for wraparound after 65535 block updates, or is it assumed that - // the hardware fails long before that anyway? + + // These are compared as signed values by the GC BIOS. There is no protection against overflow, so + // if one block is MAX_VAL and the other is MIN_VAL it still picks the MAX_VAL one as the active + // one, even if that results in a corrupted memory card. + // TODO: We could try to be smarter about this to rescue seemingly-corrupted cards. if (card.m_directory_blocks[0].m_update_counter >= card.m_directory_blocks[1].m_update_counter) card.m_active_directory = 0; diff --git a/Source/Core/Core/HW/GCMemcard/GCMemcard.h b/Source/Core/Core/HW/GCMemcard/GCMemcard.h index 8855a493de..50d0913787 100644 --- a/Source/Core/Core/HW/GCMemcard/GCMemcard.h +++ b/Source/Core/Core/HW/GCMemcard/GCMemcard.h @@ -311,8 +311,7 @@ struct Directory std::array m_padding; // 2 bytes at 0x1ffa: Update Counter - // TODO: What happens if this overflows? Is there a special case for preferring 0 over max value? - Common::BigEndianValue m_update_counter; + Common::BigEndianValue m_update_counter; // 2 bytes at 0x1ffc: Additive Checksum u16 m_checksum; @@ -343,7 +342,7 @@ struct BlockAlloc u16 m_checksum_inv; // 2 bytes at 0x0004: Update Counter - Common::BigEndianValue m_update_counter; + Common::BigEndianValue m_update_counter; // 2 bytes at 0x0006: Free Blocks Common::BigEndianValue m_free_blocks; From ba8ffd939159fd8fc9e9d2accb8aa0197249a0db Mon Sep 17 00:00:00 2001 From: "Admiral H. Curtiss" Date: Thu, 15 Aug 2019 02:06:29 +0200 Subject: [PATCH 4/5] GCMemcard: Check if the Directory's number-of-blocks claim for files matches the BAT, and report an error if it doesn't. --- Source/Core/Core/HW/GCMemcard/GCMemcard.cpp | 70 +++++++++++++++++++-- Source/Core/Core/HW/GCMemcard/GCMemcard.h | 5 ++ 2 files changed, 70 insertions(+), 5 deletions(-) diff --git a/Source/Core/Core/HW/GCMemcard/GCMemcard.cpp b/Source/Core/Core/HW/GCMemcard/GCMemcard.cpp index adccd920fa..45a2141353 100644 --- a/Source/Core/Core/HW/GCMemcard/GCMemcard.cpp +++ b/Source/Core/Core/HW/GCMemcard/GCMemcard.cpp @@ -43,7 +43,8 @@ bool GCMemcardErrorCode::HasCriticalErrors() const Test(GCMemcardValidityIssues::INVALID_CARD_SIZE) || Test(GCMemcardValidityIssues::INVALID_CHECKSUM) || Test(GCMemcardValidityIssues::MISMATCHED_CARD_SIZE) || - Test(GCMemcardValidityIssues::FREE_BLOCK_MISMATCH); + Test(GCMemcardValidityIssues::FREE_BLOCK_MISMATCH) || + Test(GCMemcardValidityIssues::DIR_BAT_INCONSISTENT); } bool GCMemcardErrorCode::Test(GCMemcardValidityIssues code) const @@ -265,10 +266,12 @@ std::pair> GCMemcard::Open(std::str else card.m_active_bat = 1; - // TODO: Do a card-wide corruption check here, such as cross-checking the active Dir's files with - // the active BAT's allocated sectors. The GC BIOS does at the very least recognize if a file's - // number of used blocks is inconsistent between the two, and will count a card as corrupted if - // that is the case. + // check for consistency between the active Dir and BAT + const GCMemcardErrorCode dir_bat_consistency_error_code = + card.GetActiveDirectory().CheckForErrorsWithBat(card.GetActiveBat()); + error_code |= dir_bat_consistency_error_code; + if (error_code.HasCriticalErrors()) + return std::make_pair(error_code, std::nullopt); card.m_valid = true; @@ -1623,3 +1626,60 @@ GCMemcardErrorCode Directory::CheckForErrors() const return error_code; } + +GCMemcardErrorCode Directory::CheckForErrorsWithBat(const BlockAlloc& bat) const +{ + GCMemcardErrorCode error_code; + + for (u8 i = 0; i < DIRLEN; ++i) + { + const DEntry& entry = m_dir_entries[i]; + if (entry.m_gamecode == DEntry::UNINITIALIZED_GAMECODE) + continue; + + // check if we end up with the same number of blocks when traversing through the BAT using the + // given first block + const u16 dir_number_of_blocks = entry.m_block_count; + const u16 dir_first_block = entry.m_first_block; + bool bat_block_count_matches = false; + { + u16 remaining_blocks = dir_number_of_blocks; + u16 current_block = dir_first_block; + while (true) + { + if (remaining_blocks == 0) + { + // we should be at the last block but haven't seen the last-block BAT indicator yet, file + // is larger according to BAT, so we're inconsistent + break; + } + --remaining_blocks; + const u16 next_block = bat.GetNextBlock(current_block); + if (next_block == 0) + { + // current block is out-of-range or next block is unallocated, this is definitely wrong + break; + } + if (next_block == 0xFFFF) + { + // we're at the final block according to the BAT + // if there are zero remaining blocks according to the directory we're consistent, + // otherwise the file is smaller according to the BAT and we're inconsistent + bat_block_count_matches = remaining_blocks == 0; + break; + } + current_block = next_block; + } + } + + if (!bat_block_count_matches) + { + error_code.Set(GCMemcardValidityIssues::DIR_BAT_INCONSISTENT); + break; + } + } + + // TODO: We could also check if every allocated BAT block is actually reachable with the files. + + return error_code; +} diff --git a/Source/Core/Core/HW/GCMemcard/GCMemcard.h b/Source/Core/Core/HW/GCMemcard/GCMemcard.h index 50d0913787..c173138028 100644 --- a/Source/Core/Core/HW/GCMemcard/GCMemcard.h +++ b/Source/Core/Core/HW/GCMemcard/GCMemcard.h @@ -88,6 +88,7 @@ enum class GCMemcardValidityIssues INVALID_CHECKSUM, MISMATCHED_CARD_SIZE, FREE_BLOCK_MISMATCH, + DIR_BAT_INCONSISTENT, DATA_IN_UNUSED_AREA, COUNT }; @@ -302,6 +303,8 @@ struct DEntry }; static_assert(sizeof(DEntry) == DENTRY_SIZE); +struct BlockAlloc; + struct Directory { // 127 files of 0x40 bytes each @@ -330,6 +333,8 @@ struct Directory std::pair CalculateChecksums() const; GCMemcardErrorCode CheckForErrors() const; + + GCMemcardErrorCode CheckForErrorsWithBat(const BlockAlloc& bat) const; }; static_assert(sizeof(Directory) == BLOCK_SIZE); From 7b9d43a834d8cf6be6be9b4b0c582f9327d12184 Mon Sep 17 00:00:00 2001 From: "Admiral H. Curtiss" Date: Thu, 15 Aug 2019 04:08:20 +0200 Subject: [PATCH 5/5] Qt/GCMemcardManager and Qt/GameCubePane: Give detailed error messages for invalid memory cards. --- Source/Core/DolphinQt/GCMemcardManager.cpp | 54 ++++++++++++++++--- Source/Core/DolphinQt/GCMemcardManager.h | 4 ++ .../Core/DolphinQt/Settings/GameCubePane.cpp | 12 ++--- 3 files changed, 58 insertions(+), 12 deletions(-) diff --git a/Source/Core/DolphinQt/GCMemcardManager.cpp b/Source/Core/DolphinQt/GCMemcardManager.cpp index 7b60853f8d..12e9e5d48c 100644 --- a/Source/Core/DolphinQt/GCMemcardManager.cpp +++ b/Source/Core/DolphinQt/GCMemcardManager.cpp @@ -17,6 +17,8 @@ #include #include #include +#include +#include #include #include @@ -241,14 +243,20 @@ void GCMemcardManager::UpdateActions() void GCMemcardManager::SetSlotFile(int slot, QString path) { - // TODO: Check error codes and give reasonable error messages. auto [error_code, memcard] = GCMemcard::Open(path.toStdString()); - if (error_code.HasCriticalErrors() || !memcard || !memcard->IsValid()) - return; - - m_slot_file_edit[slot]->setText(path); - m_slot_memcard[slot] = std::make_unique(std::move(*memcard)); + if (!error_code.HasCriticalErrors() && memcard && memcard->IsValid()) + { + m_slot_file_edit[slot]->setText(path); + m_slot_memcard[slot] = std::make_unique(std::move(*memcard)); + } + else + { + m_slot_memcard[slot] = nullptr; + ModalMessageBox::critical( + this, tr("Error"), + tr("Failed opening memory card:\n%1").arg(GetErrorMessagesForErrorCode(error_code))); + } UpdateSlotTable(slot); UpdateActions(); @@ -524,3 +532,37 @@ std::vector GCMemcardManager::GetIconFromSaveFile(int file_index, int s return frame_pixmaps; } + +QString GCMemcardManager::GetErrorMessagesForErrorCode(const GCMemcardErrorCode& code) +{ + QStringList sl; + + if (code.Test(GCMemcardValidityIssues::FAILED_TO_OPEN)) + sl.push_back(tr("Couldn't open file.")); + + if (code.Test(GCMemcardValidityIssues::IO_ERROR)) + sl.push_back(tr("Couldn't read file.")); + + if (code.Test(GCMemcardValidityIssues::INVALID_CARD_SIZE)) + sl.push_back(tr("Filesize does not match any known GameCube Memory Card size.")); + + if (code.Test(GCMemcardValidityIssues::MISMATCHED_CARD_SIZE)) + sl.push_back(tr("Filesize in header mismatches actual card size.")); + + if (code.Test(GCMemcardValidityIssues::INVALID_CHECKSUM)) + sl.push_back(tr("Invalid checksums.")); + + if (code.Test(GCMemcardValidityIssues::FREE_BLOCK_MISMATCH)) + sl.push_back(tr("Mismatch between free block count in header and actually unused blocks.")); + + if (code.Test(GCMemcardValidityIssues::DIR_BAT_INCONSISTENT)) + sl.push_back(tr("Mismatch between internal data structures.")); + + if (code.Test(GCMemcardValidityIssues::DATA_IN_UNUSED_AREA)) + sl.push_back(tr("Data in area of file that should be unused.")); + + if (sl.empty()) + return QStringLiteral("No errors."); + + return sl.join(QStringLiteral("\n")); +} diff --git a/Source/Core/DolphinQt/GCMemcardManager.h b/Source/Core/DolphinQt/GCMemcardManager.h index e1579abe11..5e25989221 100644 --- a/Source/Core/DolphinQt/GCMemcardManager.h +++ b/Source/Core/DolphinQt/GCMemcardManager.h @@ -12,6 +12,7 @@ #include class GCMemcard; +class GCMemcardErrorCode; class QDialogButtonBox; class QGroupBox; @@ -19,6 +20,7 @@ class QLabel; class QLineEdit; class QPixmap; class QPushButton; +class QString; class QTableWidget; class QTimer; @@ -29,6 +31,8 @@ public: explicit GCMemcardManager(QWidget* parent = nullptr); ~GCMemcardManager(); + static QString GetErrorMessagesForErrorCode(const GCMemcardErrorCode& code); + private: void CreateWidgets(); void ConnectWidgets(); diff --git a/Source/Core/DolphinQt/Settings/GameCubePane.cpp b/Source/Core/DolphinQt/Settings/GameCubePane.cpp index 46595fc3c3..169d670a35 100644 --- a/Source/Core/DolphinQt/Settings/GameCubePane.cpp +++ b/Source/Core/DolphinQt/Settings/GameCubePane.cpp @@ -28,6 +28,7 @@ #include "Core/HW/GCMemcard/GCMemcard.h" #include "DolphinQt/Config/Mapping/MappingWindow.h" +#include "DolphinQt/GCMemcardManager.h" #include "DolphinQt/QtUtils/ModalMessageBox.h" enum @@ -212,16 +213,15 @@ void GameCubePane::OnConfigPressed(int slot) { if (File::Exists(filename.toStdString())) { - // TODO: check error codes and give reasonable error messages auto [error_code, mc] = GCMemcard::Open(filename.toStdString()); if (error_code.HasCriticalErrors() || !mc || !mc->IsValid()) { - ModalMessageBox::critical(this, tr("Error"), - tr("Cannot use that file as a memory card.\n%1\n" - "is not a valid GameCube memory card file") - .arg(filename)); - + ModalMessageBox::critical( + this, tr("Error"), + tr("The file\n%1\nis either corrupted or not a GameCube memory card file.\n%2") + .arg(filename) + .arg(GCMemcardManager::GetErrorMessagesForErrorCode(error_code))); return; } }