Merge pull request #19438 from hrydgard/android-folder-move-fixes

Android memstick folder move: Minor logging and robustness improvements

Common/File/AndroidStorage.cpp

@@ -154,20 +154,26 @@ static bool ParseFileInfo(const std::string &line, File::FileInfo *fileInfo) {
 	std::vector<std::string> parts;
 	SplitString(line, '|', parts);
 	if (parts.size() != 4) {
-		ERROR_LOG(Log::FileSystem, "Bad format: %s", line.c_str());
+		ERROR_LOG(Log::FileSystem, "Bad format (1): %s", line.c_str());
 		return false;
 	}
 	fileInfo->name = std::string(parts[2]);
 	fileInfo->isDirectory = parts[0][0] == 'D';
 	fileInfo->exists = true;
-	sscanf(parts[1].c_str(), "%" PRIu64, &fileInfo->size);
+	if (1 != sscanf(parts[1].c_str(), "%" PRIu64, &fileInfo->size)) {
+		ERROR_LOG(Log::FileSystem, "Bad format (2): %s", line.c_str());
+		return false;
+	}
 	fileInfo->isWritable = true;  // TODO: Should be passed as part of the string.
 	// TODO: For read-only mappings, reflect that here, similarly as with isWritable.
 	// Directories are normally executable (0111) which means they're traversable.
 	fileInfo->access = fileInfo->isDirectory ? 0777 : 0666;
 
 	uint64_t lastModifiedMs = 0;
-	sscanf(parts[3].c_str(), "%" PRIu64, &lastModifiedMs);
+	if (1 != sscanf(parts[3].c_str(), "%" PRIu64, &lastModifiedMs)) {
+		ERROR_LOG(Log::FileSystem, "Bad format (3): %s", line.c_str());
+		return false;
+	}
 
 	// Convert from milliseconds
 	uint32_t lastModified = lastModifiedMs / 1000;
@@ -229,7 +235,7 @@ std::vector<File::FileInfo> Android_ListContentUri(const std::string &path, bool
 		const char *charArray = env->GetStringUTFChars(str, 0);
 		if (charArray) {  // paranoia
 			std::string line = charArray;
-			File::FileInfo info;
+			File::FileInfo info{};
 			if (line == "X") {
 				// Indicates an exception thrown, path doesn't exist.
 				*exists = false;

Common/File/FileUtil.cpp

@@ -683,6 +683,7 @@ bool Copy(const Path &srcFilename, const Path &destFilename) {
 			if (Android_CopyFile(srcFilename.ToString(), destParent.ToString()) == StorageError::SUCCESS) {
 				return true;
 			}
+			INFO_LOG(Log::Common, "Android_CopyFile failed, falling back.");
 			// Else fall through, and try using file I/O.
 		}
 		break;
@@ -690,7 +691,7 @@ bool Copy(const Path &srcFilename, const Path &destFilename) {
 		return false;
 	}
 
-	INFO_LOG(Log::Common, "Copy: %s --> %s", srcFilename.c_str(), destFilename.c_str());
+	INFO_LOG(Log::Common, "Copy by OpenCFile: %s --> %s", srcFilename.c_str(), destFilename.c_str());
 #ifdef _WIN32
 #if PPSSPP_PLATFORM(UWP)
 	if (CopyFileFromAppW(srcFilename.ToWString().c_str(), destFilename.ToWString().c_str(), FALSE))
@@ -702,7 +703,7 @@ bool Copy(const Path &srcFilename, const Path &destFilename) {
 	ERROR_LOG(Log::Common, "Copy: failed %s --> %s: %s",
 			srcFilename.c_str(), destFilename.c_str(), GetLastErrorMsg().c_str());
 	return false;
-#else
+#else  // Non-Win32
 
 	// buffer size
 #define BSIZE 16384
@@ -726,6 +727,8 @@ bool Copy(const Path &srcFilename, const Path &destFilename) {
 		return false;
 	}
 
+	int bytesWritten = 0;
+
 	// copy loop
 	while (!feof(input)) {
 		// read input
@@ -751,7 +754,14 @@ bool Copy(const Path &srcFilename, const Path &destFilename) {
 			fclose(output);
 			return false;
 		}
+
+		bytesWritten += wnum;
 	}
+
+	if (bytesWritten == 0) {
+		WARN_LOG(Log::Common, "Copy: No bytes written (must mean that input was empty)");
+	}
+
 	// close flushes
 	fclose(input);
 	fclose(output);

Core/System.cpp

@@ -746,6 +746,5 @@ bool CreateSysDirectories() {
 			File::CreateEmptyFile(path / ".nomedia");
 		}
 	}
-
 	return true;
 }

Core/Util/MemStick.cpp

@@ -1,3 +1,6 @@
+#include <algorithm>
+#include <vector>
+
 #include "Common/File/Path.h"
 #include "Common/File/FileUtil.h"
 #include "Common/File/DirListing.h"
@@ -178,7 +181,7 @@ MoveResult *MoveDirectoryContentsSafe(Path moveSrc, Path moveDest, MoveProgressR
 
 	bool dryRun = false;  // Useful for debugging.
 
-	size_t failedFiles = 0;
+	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.
@@ -218,20 +221,27 @@ MoveResult *MoveDirectoryContentsSafe(Path moveSrc, Path moveDest, MoveProgressR
 		} else {
 			// Remove the "from" prefix from the path.
 			// We have to drop down to string operations for this.
+			if (File::Exists(to)) {
+				WARN_LOG(Log::System, "Target file '%s' already exists. Will be overwritten", to.c_str());
+			}
+
 			if (!File::Copy(from, to)) {
 				ERROR_LOG(Log::System, "Failed to copy file '%s' to '%s'", from.c_str(), to.c_str());
-				failedFiles++;
+				failedFileCount++;
 				// Should probably just bail?
 			} else {
-				INFO_LOG(Log::System, "Copied file '%s' to '%s'", from.c_str(), to.c_str());
+				INFO_LOG(Log::System, "Copied file '%s' to '%s' (size: %d)", from.c_str(), to.c_str(), (int)fileSuffix.fileSize);
 			}
 		}
 	}
 
-	if (failedFiles) {
+	if (failedFileCount) {
-		return new MoveResult{ false, "", failedFiles };
+		ERROR_LOG(Log::System, "Copy failed (%d files failed)", (int)failedFileCount);
+		return new MoveResult{ false, "", failedFileCount };
 	}
 
+	INFO_LOG(Log::System, "Done copying. Moving to verification.");
+
 	// After the whole move, verify that all the files arrived correctly.
 	// If there's a single error, we do not delete the source data.
 	bool ok = true;
@@ -247,22 +257,32 @@ MoveResult *MoveDirectoryContentsSafe(Path moveSrc, Path moveDest, MoveProgressR
 			break;
 		}
 
-		if (fileSuffix.fileSize != info.size) {
+		if (!info.exists) {
-			ERROR_LOG(Log::System, "Mismatched size in target file %s. Verification failed.", fileSuffix.suffix.c_str());
+			ERROR_LOG(Log::System, "File '%s' missing at target location '%s'.", fileSuffix.suffix.c_str(), to.c_str());
 			ok = false;
-			failedFiles++;
+			failedFileCount++;
+			break;
+		}
+
+		// More lenient handling for .nomedia files.
+		if (fileSuffix.fileSize != info.size && !endsWithNoCase(fileSuffix.suffix, ".nomedia")) {
+			ERROR_LOG(Log::System, "Mismatched size in target file %s (expected %d, was %d bytes). Verification failed.", fileSuffix.suffix.c_str(), (int)fileSuffix.fileSize, (int)info.size);
+			ok = false;
+			failedFileCount++;
 			break;
 		}
 	}
 
 	if (!ok) {
-		return new MoveResult{ false, "", failedFiles };
+		return new MoveResult{ false, "", failedFileCount };
 	}
 
 	INFO_LOG(Log::System, "Verification complete");
 
 	// Delete all the old, now hopefully empty, directories.
 	// Hopefully DeleteDir actually fails if it contains a file...
+	// Reverse the order to delete subfolders before parents.
+	std::reverse(directorySuffixesToCreate.begin(), directorySuffixesToCreate.end());
 	for (size_t i = 0; i < directorySuffixesToCreate.size(); i++) {
 		const auto &dirSuffix = directorySuffixesToCreate[i];
 		Path dir = moveSrc / dirSuffix;

UI/MemStickScreen.cpp

@@ -302,7 +302,7 @@ UI::EventReturn MemStickScreen::OnConfirmClick(UI::EventParams &params) {
 }
 
 UI::EventReturn MemStickScreen::SetFolderManually(UI::EventParams &params) {
-	// The old way, from before scoped storage.
+	// The old way, from before scoped storage. Write in the full path.
 #if PPSSPP_PLATFORM(ANDROID) || PPSSPP_PLATFORM(SWITCH)
 	auto sy = GetI18NCategory(I18NCat::SYSTEM);
 	System_InputBoxGetString(GetRequesterToken(), sy->T("Memory Stick Folder"), g_Config.memStickDirectory.ToString(), [&](const std::string &value, int) {