diff --git a/Core/Util/MemStick.cpp b/Core/Util/MemStick.cpp index 5e0c9d3e83..fe79846b19 100644 --- a/Core/Util/MemStick.cpp +++ b/Core/Util/MemStick.cpp @@ -198,8 +198,9 @@ bool SwitchMemstickFolderTo(Path newMemstickFolder) { std::string str = newMemstickFolder.ToString(); if (!File::WriteDataToFile(true, str.c_str(), str.size(), memStickDirFile)) { + // Disk full? Without it, the next start would be back on the old folder. ERROR_LOG(Log::System, "Failed to write memstick path '%s' to '%s'", newMemstickFolder.c_str(), memStickDirFile.c_str()); - // Not sure what to do if this file can't be written. Disk full? + return false; } #if PPSSPP_PLATFORM(UWP) @@ -213,38 +214,35 @@ bool SwitchMemstickFolderTo(Path newMemstickFolder) { return true; } -// Keep the size with the file, so we can skip overly large ones in the move. -// The user will have to take care of them afterwards, it'll just take too long probably. +// Keep the size with the file, to verify the copy. struct FileSuffix { std::string suffix; u64 fileSize; }; +// Fails if any part of the tree can't be listed. The move deletes the source folders afterwards, and on +// content URIs that deletes whatever is still in them, so anything left out here would be lost. static bool ListFileSuffixesRecursively(const Path &root, const Path &folder, std::vector &dirSuffixes, std::vector &fileSuffixes, MoveProgressReporter &progressReporter) { std::vector files; if (!File::GetFilesInDir(folder, &files)) { + ERROR_LOG(Log::System, "Failed to list '%s'", folder.c_str()); return false; } for (auto &file : files) { + std::string suffix; + if (!root.ComputePathTo(file.fullName, suffix) || suffix.empty()) { + ERROR_LOG_REPORT(Log::System, "Failed to compute PathTo from '%s' to '%s'", root.c_str(), file.fullName.c_str()); + return false; + } if (file.isDirectory) { - std::string dirSuffix; - if (root.ComputePathTo(file.fullName, dirSuffix)) { - if (!dirSuffix.empty()) { - dirSuffixes.push_back(dirSuffix); - ListFileSuffixesRecursively(root, folder / file.name, dirSuffixes, fileSuffixes, progressReporter); - progressReporter.SetProgress(file.name, fileSuffixes.size(), (size_t)-1); - } - } else { - ERROR_LOG_REPORT(Log::System, "Failed to compute PathTo from '%s' to '%s'", root.c_str(), folder.c_str()); + dirSuffixes.push_back(suffix); + if (!ListFileSuffixesRecursively(root, folder / file.name, dirSuffixes, fileSuffixes, progressReporter)) { + return false; } + progressReporter.SetProgress(file.name, fileSuffixes.size(), (size_t)-1); } else { - std::string fileSuffix; - if (root.ComputePathTo(file.fullName, fileSuffix)) { - if (!fileSuffix.empty()) { - fileSuffixes.push_back(FileSuffix{ fileSuffix, file.size }); - } - } + fileSuffixes.push_back(FileSuffix{ suffix, file.size }); } } @@ -327,7 +325,6 @@ MoveResult *MoveDirectoryContentsSafe(Path moveSrc, Path moveDest, MoveProgressR // NOTE: It's correct to pass moveSrc twice here, it's to keep the root in the recursion. if (!ListFileSuffixesRecursively(moveSrc, moveSrc, directorySuffixesToCreate, fileSuffixesToMove, progressReporter)) { - // TODO: Handle failure listing files. INFO_LOG(Log::System, "Failed to read old directory"); progressReporter.SetProgress(ms->T("Failed to read old directory")); return new MoveResult{ false, "Failed to read old directory" }; @@ -337,10 +334,6 @@ MoveResult *MoveDirectoryContentsSafe(Path moveSrc, Path moveDest, MoveProgressR size_t failedFileCount = 0; - // We're not moving huge files like ISOs during this process, unless - // they can be directly moved, without rewriting the file. - const uint64_t BIG_FILE_THRESHOLD = 24 * 1024 * 1024; - if (moveSrc.empty()) { // Shouldn't happen. return new MoveResult{ true, "", }; @@ -433,8 +426,21 @@ MoveResult *MoveDirectoryContentsSafe(Path moveSrc, Path moveDest, MoveProgressR INFO_LOG(Log::System, "Verification complete"); - // Delete all the old, now hopefully empty, directories. - // Hopefully DeleteDir actually fails if it contains a file... + // Everything arrived, so delete the originals: the files, then the folders. On content URIs DeleteDir + // would take the files along anyway, but on native paths it only removes empty folders. + for (size_t i = 0; i < fileSuffixesToMove.size(); i++) { + const Path from = moveSrc / fileSuffixesToMove[i].suffix; + if (dryRun) { + INFO_LOG(Log::System, "dry run: Would have deleted '%s'", from.c_str()); + continue; + } + progressReporter.SetProgress(fileSuffixesToMove[i].suffix, i, fileSuffixesToMove.size()); + if (!File::Delete(from)) { + // It's safely copied, so this only leaves a duplicate behind. + WARN_LOG(Log::System, "Failed to delete '%s' after copying it", from.c_str()); + } + } + // Reverse the order to delete subfolders before parents. std::reverse(directorySuffixesToCreate.begin(), directorySuffixesToCreate.end()); for (size_t i = 0; i < directorySuffixesToCreate.size(); i++) { diff --git a/UI/MemStickScreen.cpp b/UI/MemStickScreen.cpp index 696c4fb293..5dbf161b01 100644 --- a/UI/MemStickScreen.cpp +++ b/UI/MemStickScreen.cpp @@ -371,11 +371,10 @@ void MemStickScreen::UseInternalStorage(UI::EventParams ¶ms) { if (initialSetup_) { // There's not gonna be any files here in this case since it's a fresh install. // Let's just accept it and move on. No need to move files either. - if (SwitchMemstickFolderTo(pendingMemStickFolder)) { - TriggerFinish(DialogResult::DR_OK); - } else { - // This can't really happen?? Not worth making an error message. + if (!SwitchMemstickFolderTo(pendingMemStickFolder)) { ERROR_LOG(Log::System, "Could not switch memstick path in setup (internal)"); + g_OSD.Show(OSDType::MESSAGE_ERROR, T(I18NCat::MEMSTICK, "That folder doesn't work as a memstick folder.")); + return; } // Don't have a confirmation dialog that would otherwise do it for us, need to just switch directly to the main screen. screenManager()->switchScreen(new MainScreen()); diff --git a/unittest/UnitTest.cpp b/unittest/UnitTest.cpp index 3e8cb845b6..5768ad928c 100644 --- a/unittest/UnitTest.cpp +++ b/unittest/UnitTest.cpp @@ -3007,6 +3007,43 @@ static bool TestMemstickFolderCheck() { return true; } +static bool TestMemstickMove() { + const Path root = UnitTestTempPath("unittest_memstick_move"); + File::DeleteDirRecursively(root); + const Path src = root / "old"; + const Path dest = root / "new"; + EXPECT_TRUE(File::CreateFullPath(src / "PSP/SAVEDATA/GAME1")); + EXPECT_TRUE(File::CreateFullPath(src / "PSP/SYSTEM")); + EXPECT_TRUE(File::WriteStringToFile(true, "save", src / "PSP/SAVEDATA/GAME1/DATA.BIN")); + EXPECT_TRUE(File::WriteStringToFile(true, "ini", src / "PSP/SYSTEM/ppsspp.ini")); + // A non-empty SAVEDATA already there can't be renamed over, which forces the copy path. + EXPECT_TRUE(File::CreateFullPath(dest / "PSP/SAVEDATA/GAME2")); + EXPECT_TRUE(File::WriteStringToFile(true, "other", dest / "PSP/SAVEDATA/GAME2/DATA.BIN")); + + MoveProgressReporter progress; + MoveResult *result = MoveDirectoryContentsSafe(src, dest, progress); + const bool success = result && result->success; + delete result; + EXPECT_TRUE(success); + EXPECT_TRUE(File::Exists(dest / "PSP/SAVEDATA/GAME1/DATA.BIN")); + EXPECT_TRUE(File::Exists(dest / "PSP/SAVEDATA/GAME2/DATA.BIN")); + EXPECT_TRUE(File::Exists(dest / "PSP/SYSTEM/ppsspp.ini")); + // Moved, not copied. + EXPECT_FALSE(File::Exists(src / "PSP/SAVEDATA/GAME1/DATA.BIN")); + EXPECT_FALSE(File::Exists(src / "PSP/SAVEDATA")); + EXPECT_FALSE(File::Exists(src / "PSP/SYSTEM")); + + // Into its own PSP folder: refused, and nothing touched. + result = MoveDirectoryContentsSafe(dest, dest / "PSP/SAVEDATA", progress); + const bool refused = result && !result->success; + delete result; + EXPECT_TRUE(refused); + EXPECT_TRUE(File::Exists(dest / "PSP/SAVEDATA/GAME1/DATA.BIN")); + + File::DeleteDirRecursively(root); + return true; +} + class UnitTestWordWrapper : public WordWrapper { public: UnitTestWordWrapper(std::string_view str, float maxW, int flags) @@ -4395,6 +4432,7 @@ TestItem availableTests[] = { TEST_ITEM(AndroidContentURI), TEST_ITEM(Storage), TEST_ITEM(MemstickFolderCheck), + TEST_ITEM(MemstickMove), TEST_ITEM(ThreadManager), TEST_ITEM(WrapText), TEST_ITEM(TinySet),