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

[GunChleoc]

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

[bunnybot (widelandsofficial)]

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.

[Klaus Halfmann (klaus-halfmann)]

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.

[Klaus Halfmann (klaus-halfmann)]

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)

[Klaus Halfmann (klaus-halfmann)]

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

@bunnybot merge

review: Approve (testplay on windows)

=== modified file 'src/logic/save_handler.cc'
--- src/logic/save_handler.cc	2018-07-18 15:01:33 +0000
+++ src/logic/save_handler.cc	2018-09-25 18:47:39 +0000
@@ -68,9 +68,14 @@
 		const std::string filename_next =
 		   create_file_name(kSaveDir, (boost::format("%s_%02d") % filename % rolls).str());
 		if (g_fs->file_exists(filename_next)) {
-			g_fs->fs_rename(
-			   filename_next, filename_previous);  // e.g. wl_autosave_08 -> wl_autosave_09
-			log("Autosave: Rolled %s to %s\n", filename_next.c_str(), filename_previous.c_str());
+			try {
+				g_fs->fs_rename(
+				   filename_next, filename_previous);  // e.g. wl_autosave_08 -> wl_autosave_09
+				log("Autosave: Rolled %s to %s\n", filename_next.c_str(), filename_previous.c_str());
+			} catch (const std::exception& e) {
+				log("Autosave: Unable to rename file %s to %s: %s\n", filename_previous.c_str(),
+				    filename_next.c_str(), e.what());
+			}
 		}
 		filename_previous = filename_next;
 		rolls--;
@@ -167,7 +172,7 @@
 		}
 
 		// Saving now
-		const std::string complete_filename = create_file_name(kSaveDir, filename);
+		std::string complete_filename = create_file_name(kSaveDir, filename);
 		std::string backup_filename;
 
 		// always overwrite a file
@@ -177,7 +182,34 @@
 			if (g_fs->file_exists(backup_filename)) {
 				g_fs->fs_unlink(backup_filename);
 			}
-			g_fs->fs_rename(complete_filename, backup_filename);
+			if (save_requested_) {
+				// Exceptions (e.g. no file permissions) will be handled in UI
+				g_fs->fs_rename(complete_filename, backup_filename);
+			} else {
+				// We're autosaving, try to handle file permissions issues
+				try {
+					g_fs->fs_rename(complete_filename, backup_filename);
+				} catch (const std::exception& e) {
+					log("Autosave: Unable to rename file %s to %s: %s\n", complete_filename.c_str(),
+					    backup_filename.c_str(), e.what());
+					// See if we can find a file that doesn't exist and save to that
+					int current_autosave = 0;
+					do {
+						filename = create_file_name(
+						   kSaveDir,
+						   (boost::format("%s_0%d") % autosave_filename_ % (++current_autosave)).str());
+					} while (current_autosave < 9 && g_fs->file_exists(filename));
+					complete_filename = filename;
+					log("Autosave: saving as %s instead\n", complete_filename.c_str());
+					try {
+						g_fs->fs_rename(complete_filename, backup_filename);
+					} catch (const std::exception&) {
+						log("Autosave failed, skipping this interval\n");
+						saving_next_tick_ = check_next_tick(game, realtime);
+						return;
+					}
+				}
+			}
 		}
 
 		if (!save_and_handle_error(game, complete_filename, backup_filename)) {