preserve-emulation/ppsspp
1
0 Fork 0
mirror of https://github.com/hrydgard/ppsspp.git synced 2026-11-13 09:39:33 +01:00
Code Issues Packages Projects Releases Wiki Activity

MemStick: Fix data loss and leftovers in the copying move

Browse Source 
When moving data can't rename (another device, or merging into a folder
with data), it copies, verifies, and deletes the old folders. On content
URIs that delete is recursive, but a subfolder that failed to list, or a
file whose relative path couldn't be computed, was silently left out of
the copy, and so got deleted with its folder. Any listing failure now
aborts the move before anything is copied.

On native paths, the same fallback never deleted the old files, only the
then non-empty folders, so "Move Data" left a full copy behind. The
verified originals are now deleted first, on both.

SwitchMemstickFolderTo() fails if it can't write memstick_dir.txt, rather
than restarting onto the old, by then emptied, folder. "Skip for now" in
setup only leaves when the switch worked.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
Henrik RydgårdandClaude Opus 5.5 committed 2026-11-06 14:53:36 +01:00
1 parent 1832c9aced
commit 86a12a7df2
3 files changed
+72 -29
Download Patch File Download Diff File Expand all files Collapse all files

No files matched your search

Core/Util/MemStick.cpp
+31 -25
View File
@@ -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<std::string> &dirSuffixes, std::vector<FileSuffix> &fileSuffixes, MoveProgressReporter &progressReporter) {
std::vector<File::FileInfo> 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++) {
UI/MemStickScreen.cpp
+3 -4
View File
@@ -371,11 +371,10 @@ void MemStickScreen::UseInternalStorage(UI::EventParams &params) {
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());
unittest/UnitTest.cpp
+38
View File
@@ -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),