diff --git a/Common/File/FileUtil.cpp b/Common/File/FileUtil.cpp index f5da3c83c6..f52672d125 100644 --- a/Common/File/FileUtil.cpp +++ b/Common/File/FileUtil.cpp @@ -142,6 +142,46 @@ void InitLibretroVFS(const struct retro_vfs_interface_info *vfs) noexcept { } #endif +// For callers that look at errno, as they do with native paths. +static int ErrnoFromStorageError(StorageError error) { + switch (error) { + case StorageError::SUCCESS: return 0; + case StorageError::NOT_FOUND: return ENOENT; + case StorageError::DISK_FULL: return ENOSPC; + case StorageError::ALREADY_EXISTS: return EEXIST; + default: return EIO; + } +} + +// Opens a content URI for writing, creating the file if it's missing. Opening first saves the existence check +// when the file is there, the usual case. Creating never leaves a numbered duplicate: given a taken name, +// Android_CreateFile fails with ALREADY_EXISTS (someone created it meanwhile), and then we just open it. +static int OpenContentUriForWrite(const Path &path, Android_OpenContentUriMode mode) { + int fd = Android_OpenContentUriFd(path.ToString(), mode); + if (fd >= 0 || StorageErrorFromInt(fd) != StorageError::NOT_FOUND) { + if (fd < 0) { + errno = ErrnoFromStorageError(StorageErrorFromInt(fd)); + } + return fd; + } + if (!path.CanNavigateUp()) { + errno = ENOENT; + return -1; + } + const Path parent = path.NavigateUp(); + const StorageError created = Android_CreateFile(parent.ToString(), path.GetFilename()); + if (created != StorageError::SUCCESS && created != StorageError::ALREADY_EXISTS) { + WARN_LOG(Log::IO, "Failed to create '%s' in '%s': %s", path.GetFilename().c_str(), parent.c_str(), Android_ErrorToString(created)); + errno = ErrnoFromStorageError(created); + return -1; + } + fd = Android_OpenContentUriFd(path.ToString(), mode); + if (fd < 0) { + errno = ErrnoFromStorageError(StorageErrorFromInt(fd)); + } + return fd; +} + FILE *OpenCFile(const Path &path, const char *mode) { if (LOG_IO) { INFO_LOG(Log::IO, "OpenCFile %s, %s", path.c_str(), mode); @@ -160,31 +200,11 @@ FILE *OpenCFile(const Path &path, const char *mode) { // Read, let's support this - easy one. int descriptor = Android_OpenContentUriFd(path.ToString(), Android_OpenContentUriMode::READ); if (descriptor < 0) { + errno = ErrnoFromStorageError(StorageErrorFromInt(descriptor)); return nullptr; } return fdopen(descriptor, "rb"); } else if (!strcmp(mode, "w") || !strcmp(mode, "wb") || !strcmp(mode, "wt") || !strcmp(mode, "at") || !strcmp(mode, "a")) { - // Need to be able to create the file here if it doesn't exist. - // NOTE: The existance check is important, otherwise Android will create a numbered file by the side! - // This is also a terrible possible data race, ugh. Anyway... - // Not exactly sure which abstractions are best, let's start simple. - if (!File::Exists(path)) { - DEBUG_LOG(Log::IO, "OpenCFile(%s): Opening content file for write. Doesn't exist, creating empty and reopening.", path.c_str()); - std::string name = path.GetFilename(); - if (path.CanNavigateUp()) { - Path parent = path.NavigateUp(); - if (Android_CreateFile(parent.ToString(), name) != StorageError::SUCCESS) { - WARN_LOG(Log::IO, "Failed to create file '%s' in '%s'", name.c_str(), parent.c_str()); - return nullptr; - } - } else { - INFO_LOG(Log::IO, "Failed to navigate up to create file: %s", path.c_str()); - return nullptr; - } - } else { - DEBUG_LOG(Log::IO, "OpenCFile(%s): Opening existing content file for write (truncating). Requested mode: '%s'", path.c_str(), mode); - } - // TODO: Support append modes and stuff... For now let's go with the most common one. Android_OpenContentUriMode openMode = Android_OpenContentUriMode::READ_WRITE_TRUNCATE; const char *fmode = "wb"; @@ -192,7 +212,7 @@ FILE *OpenCFile(const Path &path, const char *mode) { openMode = Android_OpenContentUriMode::READ_WRITE; fmode = "ab"; } - int descriptor = Android_OpenContentUriFd(path.ToString(), openMode); + int descriptor = OpenContentUriForWrite(path, openMode); if (descriptor < 0) { INFO_LOG(Log::IO, "Opening '%s' for write failed", path.ToString().c_str()); return nullptr; @@ -284,29 +304,6 @@ int OpenFD(const Path &path, OpenFlag flags) { return -1; } - bool knownExists = false; - - if (flags & OPEN_CREATE) { - if (!File::Exists(path)) { - INFO_LOG(Log::IO, "OpenFD(%s): Creating file.", path.c_str()); - std::string name = path.GetFilename(); - if (path.CanNavigateUp()) { - Path parent = path.NavigateUp(); - if (Android_CreateFile(parent.ToString(), name) != StorageError::SUCCESS) { - WARN_LOG(Log::IO, "OpenFD: Failed to create file '%s' in '%s'", name.c_str(), parent.c_str()); - return -1; - } - knownExists = true; - } else { - INFO_LOG(Log::IO, "Failed to navigate up to create file: %s", path.c_str()); - return -1; - } - } else { - INFO_LOG(Log::IO, "OpenCFile(%s): Opening existing content file ('%s')", path.c_str(), OpenFlagToString(flags).c_str()); - knownExists = true; - } - } - Android_OpenContentUriMode mode; if (flags == OPEN_READ) { // Intentionally not a bitfield check. mode = Android_OpenContentUriMode::READ; @@ -324,14 +321,17 @@ int OpenFD(const Path &path, OpenFlag flags) { } DEBUG_LOG(Log::IO, "Android_OpenContentUriFd: %s (%s)", path.c_str(), OpenFlagToString(flags).c_str()); - int descriptor = Android_OpenContentUriFd(path.ToString(), mode); - if (descriptor < 0) { - // File probably just doesn't exist. No biggie. - if (knownExists) { - ERROR_LOG(Log::IO, "Android_OpenContentUriFd failed for existing file: '%s'", path.c_str()); - } else { - INFO_LOG(Log::IO, "Android_OpenContentUriFd failed, probably doesn't exist: '%s'", path.c_str()); + int descriptor; + if (flags & OPEN_CREATE) { + descriptor = OpenContentUriForWrite(path, mode); + } else { + descriptor = Android_OpenContentUriFd(path.ToString(), mode); + if (descriptor < 0) { + errno = ErrnoFromStorageError(StorageErrorFromInt(descriptor)); } + } + if (descriptor < 0) { + INFO_LOG(Log::IO, "Opening '%s' (%s) failed: %s", path.c_str(), OpenFlagToString(flags).c_str(), strerror(errno)); } else if (flags & OPEN_APPEND) { // Simply seek to the end of the file to simulate append mode. lseek(descriptor, 0, SEEK_END); diff --git a/Common/File/FileUtil.h b/Common/File/FileUtil.h index 0b66d4c898..3b844920ea 100644 --- a/Common/File/FileUtil.h +++ b/Common/File/FileUtil.h @@ -244,9 +244,8 @@ void InitLibretroVFS(const struct retro_vfs_interface_info *vfs) noexcept; // Whole-file reading/writing bool WriteStringToFile(bool textFile, std::string_view str, const Path &filename); bool WriteDataToFile(bool textFile, const void* data, size_t size, const Path &filename); -// For a file that's known not to exist yet. On Android content URIs, this saves the existence -// check WriteDataToFile makes, a round trip to the storage provider - but if the file does exist, -// the provider makes a renamed copy next to it instead of overwriting it. +// For a file that's known not to exist yet. On Android content URIs, this saves a round trip to the +// storage provider, and fails if the file does exist. bool WriteDataToNewFile(const void *data, size_t size, const Path &filename); bool ReadFileToStringOptions(bool textFile, bool allowShort, const Path &path, std::string *str); diff --git a/Core/FileSystems/DirectoryFileSystem.cpp b/Core/FileSystems/DirectoryFileSystem.cpp index 638d77080e..989407da1c 100644 --- a/Core/FileSystems/DirectoryFileSystem.cpp +++ b/Core/FileSystems/DirectoryFileSystem.cpp @@ -286,10 +286,15 @@ bool DirectoryFileHandle::Open(const Path &basePath, std::string &fileName, File // Success return true; } else { - ERROR_LOG(Log::FileSystem, "File::OpenFD returned an error"); - // TODO: Need better error codes from OpenFD so we can distinguish - // disk full. Just set not found for now. - error = SCE_KERNEL_ERROR_ERRNO_FILE_NOT_FOUND; + ERROR_LOG(Log::FileSystem, "File::OpenFD returned an error: %s", strerror(errno)); + // OpenFD sets errno, like the native path below. + if (errno == ENOSPC) { + auto err = GetI18NCategory(I18NCat::ERRORS); + g_OSD.Show(OSDType::MESSAGE_ERROR, err->T("Disk full while writing data"), 0.0f, "diskfull"); + error = SCE_KERNEL_ERROR_ERRNO_NO_PERM; + } else { + error = SCE_KERNEL_ERROR_ERRNO_FILE_NOT_FOUND; + } return false; } } diff --git a/Core/Util/StorageTest.cpp b/Core/Util/StorageTest.cpp index c16e2e3763..2067fe1180 100644 --- a/Core/Util/StorageTest.cpp +++ b/Core/Util/StorageTest.cpp @@ -19,6 +19,7 @@ #include #include "Common/Data/Text/Parsers.h" +#include "Common/File/AndroidStorage.h" #include "Common/File/DirListing.h" #include "Common/File/DiskFree.h" #include "Common/File/FileUtil.h" @@ -170,6 +171,61 @@ int RunStorageTests(const Path &root, const std::function std::string { + // Android's storage provider makes "name (1)" rather than fail, when asked to create something + // whose name is taken. Nothing may ever do that to the user's files. + const Path dir = scratch / "dup"; + CHECK(File::CreateFullPath(dir / "sub")); + CHECK(Write(dir / "a.txt", "a")); + CHECK(Write(dir / "b.txt", "b")); + CHECK(Write(dir / "sub/a.txt", "old")); + auto duplicates = [](const Path &folder) { + std::vector files; + File::GetFilesInDir(folder, &files); + std::string found; + for (const File::FileInfo &f : files) { + if (f.name.find(" (") != std::string::npos) { + found += f.name + " "; + } + } + return found; + }; + std::vector outcomes; + + // The memory stick move merges into existing data with this, so it has to overwrite. + const bool copied = File::Copy(dir / "a.txt", dir / "sub/a.txt"); + CHECK(duplicates(dir / "sub").empty()); + CHECK(copied && ReadAll(dir / "sub/a.txt") == "a"); + + // Rename and move onto a taken name differ between platforms (POSIX overwrites, Windows refuses), so + // only duplicates are wrong. Note what happened. + const bool renamed = File::Rename(dir / "b.txt", dir / "a.txt"); + CHECK(duplicates(dir).empty()); + outcomes.push_back(std::string("rename ") + (renamed ? (ReadAll(dir / "a.txt") == "b" ? "overwrote" : "?") : "refused")); + if (!renamed) { + CHECK(ReadAll(dir / "a.txt") == "a" && ReadAll(dir / "b.txt") == "b"); + } + + CHECK(Write(dir / "c.txt", "c")); + const bool moved = File::MoveIfFast(dir / "c.txt", dir / "sub/a.txt"); + CHECK(duplicates(dir / "sub").empty()); + outcomes.push_back(std::string("move ") + (moved ? (ReadAll(dir / "sub/a.txt") == "c" ? "overwrote" : "?") : "refused")); + + // Creating what's there already, below the File functions (which check first). + if (contentURI) { + const StorageError file = Android_CreateFile(dir.ToString(), "sub"); + const StorageError folder = Android_CreateDirectory(dir.ToString(), "sub"); + CHECK(duplicates(dir).empty()); + outcomes.push_back(StringFromFormat("create file %s, folder %s", Android_ErrorToString(file), Android_ErrorToString(folder))); + } + note = ""; + for (const std::string &o : outcomes) { + note += (note.empty() ? "" : ", ") + o; + } + CHECK(File::DeleteDirRecursively(dir)); + return ""; + }); + run("Names", [&]() -> std::string { // Characters that need escaping in a content URI, and outside ASCII. const char *name = "100% #1 & \xC3\xA4.txt"; diff --git a/android/src/org/ppsspp/ppsspp/ContentUri.java b/android/src/org/ppsspp/ppsspp/ContentUri.java index b31b01e796..0aeb7c36c7 100644 --- a/android/src/org/ppsspp/ppsspp/ContentUri.java +++ b/android/src/org/ppsspp/ppsspp/ContentUri.java @@ -45,6 +45,33 @@ public class ContentUri { private static final int STORAGE_ERROR_DISK_FULL = -3; private static final int STORAGE_ERROR_ALREADY_EXISTS = -4; + // So the native side can tell a missing file or a full disk from other failures. + private static int errorFromException(Exception e) { + if (e instanceof java.io.FileNotFoundException) { + return STORAGE_ERROR_NOT_FOUND; + } + final String message = e.getMessage(); + if (message != null && (message.contains("ENOSPC") || message.contains("No space left"))) { + return STORAGE_ERROR_DISK_FULL; + } + return STORAGE_ERROR_UNKNOWN; + } + + private static final String EXTERNAL_STORAGE_AUTHORITY = "com.android.externalstorage.documents"; + + // The name a document has. For shared storage it's the last part of the document ID ("primary:PSP/SAVEDATA"), + // other providers' IDs can be anything, so those are asked. + private static String documentName(Context context, Uri documentUri) { + if (EXTERNAL_STORAGE_AUTHORITY.equals(documentUri.getAuthority())) { + final String id = DocumentsContract.getDocumentId(documentUri); + final int slash = id.lastIndexOf('/'); + return id.substring(slash >= 0 ? slash + 1 : id.indexOf(':') + 1); + } + try (Cursor c = context.getContentResolver().query(documentUri, new String[] { DocumentsContract.Document.COLUMN_DISPLAY_NAME }, null, null, null)) { + return c != null && c.moveToFirst() ? c.getString(0) : null; + } + } + // Without a client of our own, every query and DocumentsContract call acquires the storage provider and // releases it again, and when our reference count drops to zero, each of those is another binder call, // to the system. Holding one keeps the count up. Unstable, so the provider dying doesn't take our process @@ -110,21 +137,21 @@ public class ContentUri { // I'd expect an exception to happen before we get here, so this is probably // never reached. Log.e(TAG, "Failed to get file descriptor for " + uriString); - return -1; + return STORAGE_ERROR_UNKNOWN; } // Review note: We intentionally "leak" the fd here, the caller takes ownership and closes // it when done with it. return filePfd.detachFd(); } - } catch (java.lang.IllegalArgumentException e) { - // This exception is long and ugly and really just means file not found. + } catch (java.io.FileNotFoundException | IllegalArgumentException e) { + // The IllegalArgumentException is long and ugly and really just means file not found too. // We don't log anything (the caller can log). - return -1; + return STORAGE_ERROR_NOT_FOUND; } catch (Exception e) { // Don't know when this might happen. Let's log. Still, the result is just a // failure that the caller may additionally log. Log.e(TAG, "Unexpected openContentUri exception: " + e); - return -1; + return errorFromException(e); } } @@ -269,46 +296,42 @@ public class ContentUri { } } - @Keep - @SuppressWarnings("unused") - public static int contentUriCreateDirectory(Activity activity, String rootTreeUri, String dirName) { + // Given a taken name, the provider makes "name (1)" instead of failing. Nothing may leave such a duplicate of + // the user's files, so that's undone here and reported as ALREADY_EXISTS. + private static int createDocument(Activity activity, String parentUriString, String mimeType, String name) { try { - Uri uri = Uri.parse(rootTreeUri); - DocumentFile documentFile = DocumentFile.fromTreeUri(activity, uri); - if (documentFile != null) { - DocumentFile createdDir = documentFile.createDirectory(dirName); - return createdDir != null ? STORAGE_ERROR_SUCCESS : STORAGE_ERROR_UNKNOWN; - } else { - Log.e(TAG, "contentUriCreateDirectory: fromTreeUri returned null"); + Uri parentUri = Uri.parse(parentUriString); + if (!DocumentsContract.isDocumentUri(activity, parentUri)) { + parentUri = DocumentsContract.buildDocumentUriUsingTree(parentUri, DocumentsContract.getTreeDocumentId(parentUri)); + } + final ContentResolver resolver = activity.getContentResolver(); + final Uri created = DocumentsContract.createDocument(resolver, parentUri, mimeType, name); + if (created == null) { return STORAGE_ERROR_UNKNOWN; } + if (!name.equals(documentName(activity, created))) { + Log.w(TAG, "createDocument: '" + name + "' is taken, removing the renamed copy " + created); + DocumentsContract.deleteDocument(resolver, created); + return STORAGE_ERROR_ALREADY_EXISTS; + } + return STORAGE_ERROR_SUCCESS; } catch (Exception e) { - Log.e(TAG, "contentUriCreateDirectory exception: " + e); - return STORAGE_ERROR_UNKNOWN; + Log.e(TAG, "createDocument exception: " + e); + return errorFromException(e); } } @Keep @SuppressWarnings("unused") - // NOTE: If the file already exists, this will NOT return an error, instead the OS will create a new - // file appending an incrementing number, for example (1), to the filename. + public static int contentUriCreateDirectory(Activity activity, String rootTreeUri, String dirName) { + return createDocument(activity, rootTreeUri, DocumentsContract.Document.MIME_TYPE_DIR, dirName); + } + + // Fails with ALREADY_EXISTS if the name is taken. + @Keep + @SuppressWarnings("unused") public static int contentUriCreateFile(Activity activity, String rootTreeUri, String fileName) { - try { - Uri uri = Uri.parse(rootTreeUri); - DocumentFile documentFile = DocumentFile.fromTreeUri(activity, uri); - if (documentFile != null) { - // TODO: Check the file extension and choose MIME type appropriately. - // Or actually, let's not bother. - DocumentFile createdFile = documentFile.createFile("application/octet-stream", fileName); - return createdFile != null ? STORAGE_ERROR_SUCCESS : STORAGE_ERROR_UNKNOWN; - } else { - Log.e(TAG, "contentUriCreateFile: fromTreeUri returned null"); - return STORAGE_ERROR_UNKNOWN; - } - } catch (Exception e) { - Log.e(TAG, "contentUriCreateFile exception: " + e); - return STORAGE_ERROR_UNKNOWN; - } + return createDocument(activity, rootTreeUri, "application/octet-stream", fileName); } @Keep @@ -324,7 +347,7 @@ public class ContentUri { } } catch (Exception e) { Log.e(TAG, "contentUriRemoveFile exception: " + e); - return STORAGE_ERROR_UNKNOWN; + return errorFromException(e); } } @@ -341,10 +364,22 @@ public class ContentUri { try { Uri srcUri = Uri.parse(srcFileUri); Uri dstParentUri = Uri.parse(dstParentDirUri); - return DocumentsContract.copyDocument(activity.getContentResolver(), srcUri, dstParentUri) != null ? STORAGE_ERROR_SUCCESS : STORAGE_ERROR_UNKNOWN; + final ContentResolver resolver = activity.getContentResolver(); + final Uri copy = DocumentsContract.copyDocument(resolver, srcUri, dstParentUri); + if (copy == null) { + return STORAGE_ERROR_UNKNOWN; + } + // A taken name gets a renamed copy, see createDocument. + final String name = documentName(activity, srcUri); + if (name != null && !name.equals(documentName(activity, copy))) { + Log.w(TAG, "contentUriCopyFile: '" + name + "' is taken, removing the renamed copy " + copy); + DocumentsContract.deleteDocument(resolver, copy); + return STORAGE_ERROR_ALREADY_EXISTS; + } + return STORAGE_ERROR_SUCCESS; } catch (Exception e) { Log.e(TAG, "contentUriCopyFile exception: " + e); - return STORAGE_ERROR_UNKNOWN; + return errorFromException(e); } } @@ -368,7 +403,7 @@ public class ContentUri { return result; } catch (Exception e) { Log.e(TAG, "contentUriMoveFile exception: " + e); - return STORAGE_ERROR_UNKNOWN; + return errorFromException(e); } } @@ -380,13 +415,19 @@ public class ContentUri { // Due to a design flaw, we can't use DocumentFile.renameTo(). // Instead we use the DocumentsContract API directly. // See https://stackoverflow.com/questions/37168200/android-5-0-new-sd-card-access-api-documentfile-renameto-unsupportedoperation. - Uri newUri = DocumentsContract.renameDocument(activity.getContentResolver(), uri, newName); - // NOTE: we don't use the returned newUri for anything right now. + final ContentResolver resolver = activity.getContentResolver(); + final String oldName = documentName(activity, uri); + final Uri newUri = DocumentsContract.renameDocument(resolver, uri, newName); + // A taken name gets "name (1)" instead, see createDocument. Put the old name back. + if (newUri != null && oldName != null && !newName.equals(documentName(activity, newUri))) { + Log.w(TAG, "contentUriRenameFileTo: '" + newName + "' is taken, renaming back to '" + oldName + "'"); + DocumentsContract.renameDocument(resolver, newUri, oldName); + return STORAGE_ERROR_ALREADY_EXISTS; + } return STORAGE_ERROR_SUCCESS; } catch (Exception e) { - // TODO: More detailed exception processing. Log.e(TAG, "contentUriRenameFile exception: " + e); - return STORAGE_ERROR_UNKNOWN; + return errorFromException(e); } }