diff --git a/content.h b/content.h index 4746f080da..a0941e58c3 100644 --- a/content.h +++ b/content.h @@ -46,6 +46,11 @@ bool content_load_state_from_ram(void); /* Save a state to memory. */ bool content_save_state_to_ram(void); +/* Finishes a save written to filestream_atomic_temp_name(@path): moved + * over @path if @ok, deleted otherwise. A temporary file that could not + * be moved is kept only when it holds the one complete copy left. */ +bool content_finish_tmp(const char *path, bool ok); + /* Save a ram state from memory to disk. */ bool content_ram_state_to_file(const char *path); diff --git a/disk_index_file.c b/disk_index_file.c index 9ceed1b271..d04b5b98fb 100644 --- a/disk_index_file.c +++ b/disk_index_file.c @@ -429,7 +429,9 @@ bool disk_index_file_save(disk_index_file_t *disk_index_file) /* NULL means the writer hit an error while serialising */ buf = rjsonwriter_get_memory_buffer(writer, &_len); - if (!buf || !filestream_write_file(file_path, buf, _len)) + /* Atomic write: a process killed mid-save would otherwise leave a + * truncated disk index, which makes multi-disc games revert to disc 1. */ + if (!buf || !filestream_write_file_atomic(file_path, buf, _len)) { RARCH_ERR("[Disk index file] Error writing disk index file: \"%s\".\n", file_path); rjsonwriter_free(writer); diff --git a/libretro-common/include/streams/file_stream.h b/libretro-common/include/streams/file_stream.h index a0cdfc00cf..8959f596a0 100644 --- a/libretro-common/include/streams/file_stream.h +++ b/libretro-common/include/streams/file_stream.h @@ -337,6 +337,28 @@ RFILE *filestream_open_atomic(const char *path); */ int filestream_commit_atomic(RFILE *file, const char *path, bool ok); +/** + * The name of the sibling temporary file an atomic write of \c path + * goes through, for a writer that opens files by name. + * + * @param s Where the name is written. + * @param len The size of \c s. + * @param path Path of the file that will be replaced. + * @return \c false if the name does not fit in \c s. + */ +bool filestream_atomic_temp_name(char *s, size_t len, const char *path); + +/** + * Finishes an atomic write of \c path whose temporary file, named by + * filestream_atomic_temp_name(), was written and closed by other means: + * as filestream_commit_atomic() does after its close. + * + * @param path Path of the file that will be replaced. + * @param ok Whether the temporary file holds everything it should. + * @return As filestream_commit_atomic(). + */ +int filestream_finish_atomic(const char *path, bool ok); + /** * Writes a single character to the given file. * diff --git a/libretro-common/streams/file_stream.c b/libretro-common/streams/file_stream.c index 8d92481df1..826980ef86 100644 --- a/libretro-common/streams/file_stream.c +++ b/libretro-common/streams/file_stream.c @@ -2253,18 +2253,23 @@ RFILE *filestream_open_atomic(const char *path) return file; } +bool filestream_atomic_temp_name(char *s, size_t len, const char *path) +{ + size_t path_len; + if (!s || !path || !*path) + return false; + path_len = strlen(path); + if (path_len + sizeof(".tmp") > len) + return false; + memcpy(s, path, path_len); + memcpy(s + path_len, ".tmp", sizeof(".tmp")); + return true; +} + int filestream_commit_atomic(RFILE *file, const char *path, bool ok) { - char *temp_path; - if (!file) return -1; - if (!path || !*path || !(temp_path = filestream_atomic_temp_path(path))) - { - if (filestream_close(file) != 0) - free(file); - return -1; - } /* A buffered write reports a full disk at close, not at write, * so both have to agree before the rename goes ahead. */ @@ -2274,6 +2279,16 @@ int filestream_commit_atomic(RFILE *file, const char *path, bool ok) ok = false; } + return filestream_finish_atomic(path, ok); +} + +int filestream_finish_atomic(const char *path, bool ok) +{ + char *temp_path; + + if (!path || !*path || !(temp_path = filestream_atomic_temp_path(path))) + return -1; + if (!ok) { filestream_delete(temp_path); diff --git a/samples/tasks/cloudsync/cloudsync_pacing_test.c b/samples/tasks/cloudsync/cloudsync_pacing_test.c index 87c03c5c89..08f61640f5 100644 --- a/samples/tasks/cloudsync/cloudsync_pacing_test.c +++ b/samples/tasks/cloudsync/cloudsync_pacing_test.c @@ -561,6 +561,7 @@ static void walk_lane(void) snprintf(path, sizeof(path), "%s/.hidden/h.srm", a); write_file(path, 4, 1); snprintf(path, sizeof(path), "%s/.dot.srm", a); write_file(path, 4, 1); snprintf(path, sizeof(path), "%s/core2/p.srm.rafetching", a); write_file(path, 4, 1); + snprintf(path, sizeof(path), "%s/core2/p.srm.tmp", a); write_file(path, 4, 1); snprintf(path, sizeof(path), "%s/.DS_Store", b); write_file(path, 4, 1); nkeys = reference_keys(a, "saves", keys, 0); @@ -584,7 +585,8 @@ static void walk_lane(void) same = 0; check("walk: the same files dir_list_new() lists, hidden ones too", same); for (i = 0; same && i < nkeys; i++) - if (strstr(keys[i], ".rafetching") || strstr(keys[i], ".DS_Store")) + if ( strstr(keys[i], ".rafetching") || strstr(keys[i], ".DS_Store") + || strstr(keys[i], ".tmp")) same = 0; check("walk: ignored names left out", same); task_cloud_sync_cleanup(task); diff --git a/samples/tasks/save_state/save_state_io_test.c b/samples/tasks/save_state/save_state_io_test.c index 74d5f8e270..3a82c9e36d 100644 --- a/samples/tasks/save_state/save_state_io_test.c +++ b/samples/tasks/save_state/save_state_io_test.c @@ -532,6 +532,75 @@ static void test_serialize_failure(void) filestream_delete(path); } +/* ----------------------------------------------------------------- + * A save is written beside its slot and moved in only once whole: a + * save that fails over an existing state leaves that state as it was, + * and none leaves its temporary file behind. + * ----------------------------------------------------------------- */ +static bool tmp_exists(const char *path) +{ + char tmp[256]; + snprintf(tmp, sizeof(tmp), "%s.tmp", path); + return file_size(tmp) >= 0; +} + +/* The slot no longer holds @old: a write into it has begun. */ +static bool written_into(const char *path, const void *old, int64_t n_old) +{ + void *now = NULL; + int64_t n = 0; + bool changed; + if (!filestream_read_file(path, &now, &n)) + return true; + changed = n != n_old || memcmp(now, old, (size_t)n_old); + free(now); + return changed; +} + +static void test_failed_save_keeps_old(void) +{ + const char *path = "sst_keep.state"; + size_t sz = 12 * TEST_SAVE_STATE_CHUNK; + void *before = NULL, *after = NULL; + int64_t n_before = 0, n_after = 0; + + frontend_reset(); + core_fill(sz); + filestream_delete(path); + content_save_state(path, true); + pump(1000); + okf(!tmp_exists(path), "a save leaves nothing beside its slot"); + if (!filestream_read_file(path, &before, &n_before)) + n_before = -1; + + /* A quantum a tick, cancelled two ticks into the write: whatever + * the save does first (keeping the old state for undo), the write + * has begun once the slot is open, beside it or in it. */ + memset(core_mem, 0x5A, sz); + clock_step = TEST_TICK_BUDGET_US; + content_save_state(path, true); + { + int i; + for (i = 0; i < 1000 && !tmp_exists(path) && !written_into(path, before, n_before); i++) + task_queue_check(); + } + task_queue_check(); + task_queue_check(); + task_queue_reset(); + pump(1000); + clock_step = 0; + + if (!filestream_read_file(path, &after, &n_after)) + n_after = -2; + okf(n_before > 0 && n_after == n_before && !memcmp(before, after, (size_t)n_before), + "a save cancelled part-way over a state leaves that state as it was"); + okf(!tmp_exists(path), "a cancelled save leaves nothing beside its slot"); + + free(before); + free(after); + filestream_delete(path); +} + /* ----------------------------------------------------------------- * A background save defers only the core's serialize to the handler * - that deferral is the whole point of the core's @@ -1181,6 +1250,7 @@ static int run_default_lane(void) "round-trip is byte-exact at one quantum per tick"); test_close_waits_for_save(); test_serialize_failure(); + test_failed_save_keeps_old(); test_background_serialize_timing(); test_open_failure(); test_truncated_state(); diff --git a/save.c b/save.c index 7c996bb28a..d760790441 100644 --- a/save.c +++ b/save.c @@ -566,17 +566,21 @@ static bool content_save_ram_file(unsigned slot, bool compress) msg_hash_to_str(MSG_TO), ram.path); + /* Write via a temporary file, so a crash or power loss mid-save + * leaves the previous save intact instead of a truncated one. */ #if defined(HAVE_COMPRESSION) if (compress) { - if (!rzipstream_write_file( - ram.path, mem_info.data, mem_info.size)) + char tmp_path[PATH_MAX_LENGTH]; + if ( !filestream_atomic_temp_name(tmp_path, sizeof(tmp_path), ram.path) + || !content_finish_tmp(ram.path, rzipstream_write_file( + tmp_path, mem_info.data, mem_info.size))) goto fail; } else #endif { - if (!filestream_write_file( + if (!filestream_write_file_atomic( ram.path, mem_info.data, mem_info.size)) goto fail; } diff --git a/tasks/task_cloudsync.c b/tasks/task_cloudsync.c index 0c141dc7f5..82f7e76a51 100644 --- a/tasks/task_cloudsync.c +++ b/tasks/task_cloudsync.c @@ -507,6 +507,11 @@ static bool task_cloud_sync_should_ignore_file(const char *filename) if (string_ends_with(filename, ".rafetching")) return true; + /* a save, state or disk index still being written, or left by one + * that never finished (filestream_atomic_temp_name) */ + if (string_ends_with(filename, ".tmp")) + return true; + return false; } diff --git a/tasks/task_save.c b/tasks/task_save.c index 1d63ec1ee2..e07abaa622 100644 --- a/tasks/task_save.c +++ b/tasks/task_save.c @@ -412,6 +412,18 @@ static void undo_save_state_cb(retro_task_t *task, free(state); } +bool content_finish_tmp(const char *path, bool ok) +{ + int ret = filestream_finish_atomic(path, ok); + if (ret == 1 && path_is_valid(path)) + { + char tmp_path[PATH_MAX_LENGTH]; + if (filestream_atomic_temp_name(tmp_path, sizeof(tmp_path), path)) + filestream_delete(tmp_path); + } + return ret == 0; +} + /** * task_save_handler_finished: * @task : the task to finish @@ -430,7 +442,13 @@ static void task_save_handler_finished(retro_task_t *task, * (serialize failure, or the open itself). */ if (state->file) { - intfstream_close(state->file); + /* rzip writes its last chunk at close, and a buffered write + * reports a full disk there, so a failed close is a failed save + * and the temporary file must not replace the old state. */ + if ( intfstream_close(state->file) != 0 + && !task_get_error(task)) + task_set_error(task, strdup( + msg_hash_to_str(MSG_FAILED_TO_SAVE_STATE_TO))); free(state->file); state->file = NULL; } @@ -440,6 +458,25 @@ static void task_save_handler_finished(retro_task_t *task, if (!task_get_error(task) && ((flg & RETRO_TASK_FLG_CANCELLED) > 0)) task_set_error(task, strdup("Task canceled")); + /* The state was written to ".tmp", so a crash, cancel or + * short write leaves the existing file intact. Move it into place + * on success, discard it otherwise. */ + { + char tmp_path[PATH_MAX_LENGTH]; + + if (!filestream_atomic_temp_name(tmp_path, sizeof(tmp_path), state->path)) + { + /* Nothing was written: the open refused the same path. */ + if (!task_get_error(task)) + task_set_error(task, strdup( + msg_hash_to_str(MSG_FAILED_TO_SAVE_STATE_TO))); + } + else if (!content_finish_tmp(state->path, !task_get_error(task)) + && !task_get_error(task)) + task_set_error(task, strdup( + msg_hash_to_str(MSG_FAILED_TO_SAVE_STATE_TO))); + } + if (state->data) { if ( (state->flags & SAVE_TASK_FLAG_UNDO_SAVE) @@ -838,12 +875,20 @@ static void task_save_handler(retro_task_t *task) if (!state->file) { - if (state->flags & SAVE_TASK_FLAG_COMPRESS_FILES) + char tmp_path[PATH_MAX_LENGTH]; + + /* Write to ".tmp"; task_save_handler_finished moves it + * into place on success or deletes it on failure or cancel. + * A path too long to take the suffix is refused rather than + * written in place. */ + if (!filestream_atomic_temp_name(tmp_path, sizeof(tmp_path), state->path)) + state->file = NULL; + else if (state->flags & SAVE_TASK_FLAG_COMPRESS_FILES) state->file = intfstream_open_rzip_file( - state->path, RETRO_VFS_FILE_ACCESS_WRITE); + tmp_path, RETRO_VFS_FILE_ACCESS_WRITE); else state->file = intfstream_open_file( - state->path, RETRO_VFS_FILE_ACCESS_WRITE, + tmp_path, RETRO_VFS_FILE_ACCESS_WRITE, RETRO_VFS_FILE_ACCESS_HINT_NONE); if (!state->file) @@ -1899,9 +1944,11 @@ static bool task_push_load_and_save_state(const char *path, void *data, bool content_auto_save_state(const char *path) { size_t _len; + int _close_ret; settings_t *settings = config_get_ptr(); void *serial_data = NULL; intfstream_t *file = NULL; + char tmp_path[PATH_MAX_LENGTH]; if (!core_info_current_supports_savestate()) { @@ -1918,12 +1965,20 @@ bool content_auto_save_state(const char *path) if (!serial_data) return false; + /* Write to ".tmp" and move it into place, so a crash or power + * loss mid-save leaves the previous state intact. */ + if (!filestream_atomic_temp_name(tmp_path, sizeof(tmp_path), path)) + { + free(serial_data); + return false; + } + #if defined(HAVE_COMPRESSION) if (settings->bools.savestate_file_compression) - file = intfstream_open_rzip_file(path, RETRO_VFS_FILE_ACCESS_WRITE); + file = intfstream_open_rzip_file(tmp_path, RETRO_VFS_FILE_ACCESS_WRITE); else #endif - file = intfstream_open_file(path, RETRO_VFS_FILE_ACCESS_WRITE, + file = intfstream_open_file(tmp_path, RETRO_VFS_FILE_ACCESS_WRITE, RETRO_VFS_FILE_ACCESS_HINT_NONE); if (!file) @@ -1937,13 +1992,18 @@ bool content_auto_save_state(const char *path) intfstream_close(file); free(serial_data); free(file); + filestream_delete(tmp_path); return false; } - intfstream_close(file); + /* A failed close (rzip's last chunk, a full disk) is a failed save. */ + _close_ret = intfstream_close(file); free(serial_data); free(file); + if (!content_finish_tmp(path, _close_ret == 0)) + return false; + #ifdef HAVE_SCREENSHOTS if (settings->bools.savestate_thumbnail_enable) { @@ -2424,21 +2484,25 @@ bool content_ram_state_to_file(const char *path) && ram_buf.state_buf.data && ram_buf.to_write_file) { + /* Write via a temporary file, so a crash or power loss mid-save + * leaves the previous file intact instead of a truncated one. */ + bool written = false; #if defined(HAVE_COMPRESSION) settings_t *settings = config_get_ptr(); if (settings->bools.save_file_compression) { - if (rzipstream_write_file( - path, ram_buf.state_buf.data, ram_buf.state_buf.size)) - goto success; + char tmp_path[PATH_MAX_LENGTH]; + if (filestream_atomic_temp_name(tmp_path, sizeof(tmp_path), path)) + written = content_finish_tmp(path, rzipstream_write_file(tmp_path, + ram_buf.state_buf.data, ram_buf.state_buf.size)); } else #endif - { - if (filestream_write_file( - path, ram_buf.state_buf.data, ram_buf.state_buf.size)) - goto success; - } + written = filestream_write_file_atomic(path, + ram_buf.state_buf.data, ram_buf.state_buf.size); + + if (written) + goto success; } return false;