From f267ec7681a59a4a6c4215d9559e4905e50137e7 Mon Sep 17 00:00:00 2001 From: "Ryan C. Gordon" Date: Thu, 26 Sep 2024 19:25:34 -0400 Subject: [PATCH] filesystem: Improve docs, make some promises about overwrites and file caches. Note that SDL_FlushIO() doesn't make promises about file data sync but that is intended to be changed in the IOStream code in a later commit. Fixes #10886. --- include/SDL3/SDL_filesystem.h | 45 ++++++++++++++++++++++++++- include/SDL3/SDL_iostream.h | 18 +++++++++-- src/filesystem/posix/SDL_sysfsops.c | 5 +++ src/filesystem/windows/SDL_sysfsops.c | 2 +- 4 files changed, 65 insertions(+), 5 deletions(-) diff --git a/include/SDL3/SDL_filesystem.h b/include/SDL3/SDL_filesystem.h index cdbde2924b..908cc1c71f 100644 --- a/include/SDL3/SDL_filesystem.h +++ b/include/SDL3/SDL_filesystem.h @@ -289,7 +289,10 @@ extern SDL_DECLSPEC bool SDLCALL SDL_EnumerateDirectory(const char *path, SDL_En /** * Remove a file or an empty directory. * - * \param path the path of the directory to enumerate. + * Directories that are not empty will fail; this function will not recursely + * delete directory trees. + * + * \param path the path to remove from the filesystem. * \returns true on success or false on failure; call SDL_GetError() for more * information. * @@ -300,6 +303,17 @@ extern SDL_DECLSPEC bool SDLCALL SDL_RemovePath(const char *path); /** * Rename a file or directory. * + * If the file at `newpath` already exists, it will replaced. + * + * Note that this will not copy files across filesystems/drives/volumes, as + * that is a much more complicated (and possibly time-consuming) operation. + * + * Which is to say, if this function fails, SDL_CopyFile() to a temporary + * file in the same directory as `newpath`, then SDL_RenamePath() from the + * temporary file to `newpath` and SDL_RemovePath() on `oldpath` might work + * for files. Renaming a non-empty directory across filesystems is + * dramatically more complex, however. + * * \param oldpath the old path. * \param newpath the new path. * \returns true on success or false on failure; call SDL_GetError() for more @@ -312,6 +326,35 @@ extern SDL_DECLSPEC bool SDLCALL SDL_RenamePath(const char *oldpath, const char /** * Copy a file. * + * If the file at `newpath` already exists, it will be overwritten with the + * contents of the file at `oldpath`. + * + * This function will block until the copy is complete, which might be a + * significant time for large files on slow disks. On some platforms, the + * copy can be handed off to the OS itself, but on others SDL might just open + * both paths, and read from one and write to the other. + * + * Note that this is not an atomic operation! If something tries to read from + * `newpath` while the copy is in progress, it will see an incomplete copy + * of the data, and if the calling thread terminates (or the power goes out) + * during the copy, `oldpath`'s previous contents will be gone, replaced with + * an incomplete copy of the data. To avoid this risk, it is recommended that + * the app copy to a temporary file in the same directory as `newpath`, and + * if the copy is successful, use SDL_RenamePath() to replace `newpath` with + * the temporary file. This will ensure that reads of `newpath` will either + * see a complete copy of the data, or it will see the pre-copy state of + * `newpath`. + * + * This function attempts to synchronize the newly-copied data to disk before + * returning, if the platform allows it, so that the renaming trick will not + * have a problem in a system crash or power failure, where the file could + * be renamed but the contents never made it from the system file cache to + * the physical disk. + * + * If the copy fails for any reason, the state of `newpath` is undefined. It + * might be half a copy, it might be the untouched data of what was already + * there, or it might be a zero-byte file, etc. + * * \param oldpath the old path. * \param newpath the new path. * \returns true on success or false on failure; call SDL_GetError() for more diff --git a/include/SDL3/SDL_iostream.h b/include/SDL3/SDL_iostream.h index 3bfdd34b72..5c5c31f1e9 100644 --- a/include/SDL3/SDL_iostream.h +++ b/include/SDL3/SDL_iostream.h @@ -147,8 +147,11 @@ typedef struct SDL_IOStreamInterface /** * Close and free any allocated resources. * + * This does not guarantee file writes will sync to physical media; they + * can be in the system's file cache, waiting to go to disk. + * * The SDL_IOStream is still destroyed even if this fails, so clean up anything - * even if flushing to disk returns an error. + * even if flushing buffers, etc, returns an error. * * \return true if successful or false on write error when flushing data. */ @@ -406,8 +409,17 @@ extern SDL_DECLSPEC SDL_IOStream * SDLCALL SDL_OpenIO(const SDL_IOStreamInterfac * returns true on success, or false if the stream failed to flush to its * output (e.g. to disk). * - * Note that if this fails to flush the stream to disk, this function reports - * an error, but the SDL_IOStream is still invalid once this function returns. + * Note that if this fails to flush the stream for any reason, this function + * reports an error, but the SDL_IOStream is still invalid once this function + * returns. + * + * This call flushes any buffered writes to the operating system, but there + * are no guarantees that those writes have gone to physical media; they might + * be in the OS's file cache, waiting to go to disk later. If it's absolutely + * crucial that writes go to disk immediately, so they are definitely stored + * even if the power fails before the file cache would have caught up, one + * should call SDL_FlushIO() before closing. Note that flushing takes time and + * makes the system and your app operate less efficiently, so do so sparingly. * * \param context SDL_IOStream structure to close. * \returns true on success or false on failure; call SDL_GetError() for more diff --git a/src/filesystem/posix/SDL_sysfsops.c b/src/filesystem/posix/SDL_sysfsops.c index ff04b2c8bc..e589f438b6 100644 --- a/src/filesystem/posix/SDL_sysfsops.c +++ b/src/filesystem/posix/SDL_sysfsops.c @@ -121,7 +121,12 @@ bool SDL_SYS_CopyFile(const char *oldpath, const char *newpath) SDL_CloseIO(input); input = NULL; + if (!SDL_FlushIO(output)) { + goto done; + } + if (!SDL_CloseIO(output)) { + output = NULL; // it's gone, even if it failed. goto done; } output = NULL; diff --git a/src/filesystem/windows/SDL_sysfsops.c b/src/filesystem/windows/SDL_sysfsops.c index 743eca681d..f3210648d9 100644 --- a/src/filesystem/windows/SDL_sysfsops.c +++ b/src/filesystem/windows/SDL_sysfsops.c @@ -151,7 +151,7 @@ bool SDL_SYS_CopyFile(const char *oldpath, const char *newpath) return false; } - const BOOL rc = CopyFileW(woldpath, wnewpath, TRUE); + const BOOL rc = CopyFileExW(woldpath, wnewpath, NULL, NULL, NULL, COPY_FILE_ALLOW_DECRYPTED_DESTINATION|COPY_FILE_NO_BUFFERING); SDL_free(wnewpath); SDL_free(woldpath); if (!rc) {