Merge lp:~widelands-dev/widelands/bug-1746270-rolling-autosave-file-permissions into lp:widelands

Proposed by GunChleoc
Status: Merged
Merged at revision: 8854
Proposed branch: lp:~widelands-dev/widelands/bug-1746270-rolling-autosave-file-permissions
Merge into: lp:widelands
Diff against target: 66 lines (+37/-5)
1 file modified
src/logic/save_handler.cc (+37/-5)
To merge this branch: bzr merge lp:~widelands-dev/widelands/bug-1746270-rolling-autosave-file-permissions
Reviewer Review Type Date Requested Status
Klaus Halfmann testplay on windows Approve
Review via email: mp+353758@code.launchpad.net

Commit message

Handle file permissions problems during autosave.

- When a file can't be renamed, try to find an unused filename instead
- Skip the autosave interval as a last resort

To post a comment you must log in.
Revision history for this message
bunnybot (widelandsofficial) wrote :

Continuous integration builds have changed state:

Travis build 3836. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/420887444.
Appveyor build 3634. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_bug_1746270_rolling_autosave_file_permissions-3634.

8798. By GunChleoc

Merged trunk.

Revision history for this message
Klaus Halfmann (klaus-halfmann) wrote :

Looks like Its time to switch on the debugger.
Will compile this and think about all kind of nasty ways to break this :-)

Could we inform the player if this finally failes?

Code LGTM.

Revision history for this message
Klaus Halfmann (klaus-halfmann) wrote :

Tested this on OSX, but found the original Problem was on Windows.
The code works as expected but I did not trigger the original Problem.
e.g. when I make the complete save dir unreadable the save completly
fails, well.

OSX and *nixes behave differnt compared Windows when it comes to open files.

Alas I approve this I will do another round of testing on Windows, tomorrow.

review: Approve (review, compile, debug)
8799. By Klaus Halfman \<<email address hidden>\>

Merged trunk to recycle autosaves

Revision history for this message
Klaus Halfmann (klaus-halfmann) wrote :

OK, tested along the original Bug, works as desigend.

@bunnybot merge

review: Approve (testplay on windows)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/logic/save_handler.cc'
2--- src/logic/save_handler.cc 2018-07-18 15:01:33 +0000
3+++ src/logic/save_handler.cc 2018-09-25 18:47:39 +0000
4@@ -68,9 +68,14 @@
5 const std::string filename_next =
6 create_file_name(kSaveDir, (boost::format("%s_%02d") % filename % rolls).str());
7 if (g_fs->file_exists(filename_next)) {
8- g_fs->fs_rename(
9- filename_next, filename_previous); // e.g. wl_autosave_08 -> wl_autosave_09
10- log("Autosave: Rolled %s to %s\n", filename_next.c_str(), filename_previous.c_str());
11+ try {
12+ g_fs->fs_rename(
13+ filename_next, filename_previous); // e.g. wl_autosave_08 -> wl_autosave_09
14+ log("Autosave: Rolled %s to %s\n", filename_next.c_str(), filename_previous.c_str());
15+ } catch (const std::exception& e) {
16+ log("Autosave: Unable to rename file %s to %s: %s\n", filename_previous.c_str(),
17+ filename_next.c_str(), e.what());
18+ }
19 }
20 filename_previous = filename_next;
21 rolls--;
22@@ -167,7 +172,7 @@
23 }
24
25 // Saving now
26- const std::string complete_filename = create_file_name(kSaveDir, filename);
27+ std::string complete_filename = create_file_name(kSaveDir, filename);
28 std::string backup_filename;
29
30 // always overwrite a file
31@@ -177,7 +182,34 @@
32 if (g_fs->file_exists(backup_filename)) {
33 g_fs->fs_unlink(backup_filename);
34 }
35- g_fs->fs_rename(complete_filename, backup_filename);
36+ if (save_requested_) {
37+ // Exceptions (e.g. no file permissions) will be handled in UI
38+ g_fs->fs_rename(complete_filename, backup_filename);
39+ } else {
40+ // We're autosaving, try to handle file permissions issues
41+ try {
42+ g_fs->fs_rename(complete_filename, backup_filename);
43+ } catch (const std::exception& e) {
44+ log("Autosave: Unable to rename file %s to %s: %s\n", complete_filename.c_str(),
45+ backup_filename.c_str(), e.what());
46+ // See if we can find a file that doesn't exist and save to that
47+ int current_autosave = 0;
48+ do {
49+ filename = create_file_name(
50+ kSaveDir,
51+ (boost::format("%s_0%d") % autosave_filename_ % (++current_autosave)).str());
52+ } while (current_autosave < 9 && g_fs->file_exists(filename));
53+ complete_filename = filename;
54+ log("Autosave: saving as %s instead\n", complete_filename.c_str());
55+ try {
56+ g_fs->fs_rename(complete_filename, backup_filename);
57+ } catch (const std::exception&) {
58+ log("Autosave failed, skipping this interval\n");
59+ saving_next_tick_ = check_next_tick(game, realtime);
60+ return;
61+ }
62+ }
63+ }
64 }
65
66 if (!save_and_handle_error(game, complete_filename, backup_filename)) {

Subscribers

People subscribed via source and target branches

to status/vote changes: