widelands-dev team mailing list archive

[GunChleoc <fios@xxxxxxxxxxxxxxxxxxx>]

GunChleoc has proposed merging lp:~widelands-dev/widelands/fix-lan-autosave into lp:widelands.

Commit message:
Fixed crash with identical autosave filenames when LAN game is run with multiple instances of Widelands on a single computer. Also, more informative error messages in disk_filesystem.cc.

Requested reviews:
  Widelands Developers (widelands-dev)

For more details, see:
https://code.launchpad.net/~widelands-dev/widelands/fix-lan-autosave/+merge/286071

Fixed crash with identical autosave filenames when LAN game is run with multiple instances of Widelands on a single computer. Also, more informative error messages in disk_filesystem.cc.
-- 
Your team Widelands Developers is requested to review the proposed merge of lp:~widelands-dev/widelands/fix-lan-autosave into lp:widelands.

=== modified file 'src/io/filesystem/disk_filesystem.cc'
--- src/io/filesystem/disk_filesystem.cc	2016-01-10 13:22:20 +0000
+++ src/io/filesystem/disk_filesystem.cc	2016-02-15 15:23:38 +0000
@@ -51,16 +51,16 @@
 
 struct FileSystemPath: public std::string
 {
-	bool m_exists;
-	bool m_isDirectory;
+	bool exists_;
+	bool is_directory_;
 
 	FileSystemPath(const std::string & path)
 	: std::string(path)
 	{
 		struct stat st;
 
-		m_exists = (stat(c_str(), &st) != -1);
-		m_isDirectory = m_exists && S_ISDIR(st.st_mode);
+		exists_ = (stat(c_str(), &st) != -1);
+		is_directory_ = exists_ && S_ISDIR(st.st_mode);
 	}
 };
 
@@ -68,10 +68,10 @@
  * Initialize the real file-system
  */
 RealFSImpl::RealFSImpl(const std::string & Directory)
-: m_directory(Directory)
+: directory_(Directory)
 {
 	// TODO(unknown): check OS permissions on whether the directory is writable!
-	m_root = canonicalize_name(Directory);
+	root_ = canonicalize_name(Directory);
 }
 
 
@@ -105,9 +105,9 @@
 	long hFile;
 
 	if (path.size())
-		buf = m_directory + '\\' + path + "\\*";
+		buf = directory_ + '\\' + path + "\\*";
 	else
-		buf = m_directory + "\\*";
+		buf = directory_ + "\\*";
 
 	std::set<std::string> results;
 
@@ -145,12 +145,12 @@
 			buf = path + "/*";
 			ofs = 0;
 		} else {
-			buf = m_directory + '/' + path + "/*";
-			ofs = m_directory.length() + 1;
+			buf = directory_ + '/' + path + "/*";
+			ofs = directory_.length() + 1;
 		}
 	} else {
-		buf = m_directory + "/*";
-		ofs = m_directory.length() + 1;
+		buf = directory_ + "/*";
+		ofs = directory_.length() + 1;
 	}
 	std::set<std::string> results;
 
@@ -159,7 +159,7 @@
 
 	for (size_t i = 0; i < gl.gl_pathc; ++i) {
 		const std::string filename(canonicalize_name(&gl.gl_pathv[i][ofs]));
-		results.insert(filename.substr(m_root.size() + 1));
+		results.insert(filename.substr(root_.size() + 1));
 	}
 
 	globfree(&gl);
@@ -175,7 +175,7 @@
  */
 // TODO(unknown): Can this be rewritten to just using exceptions? Should it?
 bool RealFSImpl::file_exists(const std::string & path) {
-	return FileSystemPath(canonicalize_name(path)).m_exists;
+	return FileSystemPath(canonicalize_name(path)).exists_;
 }
 
 /**
@@ -184,7 +184,7 @@
  * \e can't exist then)
  */
 bool RealFSImpl::is_directory(const std::string & path) {
-	return FileSystemPath(canonicalize_name(path)).m_isDirectory;
+	return FileSystemPath(canonicalize_name(path)).is_directory_;
 }
 
 /**
@@ -192,9 +192,14 @@
  */
 FileSystem * RealFSImpl::make_sub_file_system(const std::string & path) {
 	FileSystemPath fspath(canonicalize_name(path));
-	assert(fspath.m_exists); //TODO(unknown): throw an exception instead
-
-	if (fspath.m_isDirectory)
+
+	if (!fspath.exists_) {
+		throw wexception("RealFSImpl: unable to create sub filesystem, path does not exist for '%s'"
+							  " in directory '%s'",
+							  fspath.c_str(), directory_.c_str());
+	}
+
+	if (fspath.is_directory_)
 		return new RealFSImpl   (fspath);
 	else
 		return new ZipFilesystem(fspath);
@@ -206,10 +211,10 @@
 FileSystem * RealFSImpl::create_sub_file_system(const std::string & path, Type const fs)
 {
 	FileSystemPath fspath(canonicalize_name(path));
-	if (fspath.m_exists)
+	if (fspath.exists_)
 		throw wexception
-			("path %s already exists, can not create a filesystem from it",
-			 path.c_str());
+			("path '%s'' already exists in directory '%s', can not create a filesystem from it",
+			 path.c_str(), directory_.c_str());
 
 	if (fs == FileSystem::DIR) {
 		ensure_directory_exists(path);
@@ -223,22 +228,28 @@
  */
 void RealFSImpl::fs_unlink(const std::string & file) {
 	FileSystemPath fspath(canonicalize_name(file));
-	if (!fspath.m_exists)
+	if (!fspath.exists_)
 		return;
 
-	if (fspath.m_isDirectory)
-		m_unlink_directory(file);
+	if (fspath.is_directory_)
+		unlink_directory(file);
 	else
-		m_unlink_file(file);
+		unlink_file(file);
 }
 
 /**
  * Remove a single directory or file
  */
-void RealFSImpl::m_unlink_file(const std::string & file) {
+void RealFSImpl::unlink_file(const std::string & file) {
 	FileSystemPath fspath(canonicalize_name(file));
-	assert(fspath.m_exists);  //TODO(unknown): throw an exception instead
-	assert(!fspath.m_isDirectory); //TODO(unknown): throw an exception instead
+	if (!fspath.exists_) {
+		throw wexception("RealFSImpl: unable to unlink file, path does not exist for '%s' in directory '%s'",
+							  fspath.c_str(), directory_.c_str());
+	}
+	if (fspath.is_directory_) {
+		throw wexception("RealFSImpl: unable to unlink file, path '%s' in directory '%s' is a directory",
+							  fspath.c_str(), directory_.c_str());
+	}
 
 #ifndef _WIN32
 	unlink(fspath.c_str());
@@ -250,10 +261,17 @@
 /**
  * Recursively remove a directory
  */
-void RealFSImpl::m_unlink_directory(const std::string & file) {
+void RealFSImpl::unlink_directory(const std::string & file) {
 	FileSystemPath fspath(canonicalize_name(file));
-	assert(fspath.m_exists);  //TODO(unknown): throw an exception instead
-	assert(fspath.m_isDirectory);  //TODO(unknown): throw an exception instead
+	if (!fspath.exists_) {
+		throw wexception("RealFSImpl: unable to unlink directory, path does not exist for '%s'"
+							  " in directory '%s'",
+							  fspath.c_str(), directory_.c_str());
+	}
+	if (!fspath.is_directory_) {
+		throw wexception("RealFSImpl: unable to unlink directoy, path '%s' in directory '%s'"
+							  " is not a directory", fspath.c_str(), directory_.c_str());
+	}
 
 	FilenameSet files = list_directory(file);
 	for
@@ -268,9 +286,9 @@
 			continue;
 
 		if (is_directory(*pname))
-			m_unlink_directory(*pname);
+			unlink_directory(*pname);
 		else
-			m_unlink_file(*pname);
+			unlink_file(*pname);
 	}
 
 	// NOTE: this might fail if this directory contains CVS dir,
@@ -279,7 +297,7 @@
 	rmdir(fspath.c_str());
 #else
 	if (!RemoveDirectory(fspath.c_str()))
-		throw wexception("%s could not be deleted.", fspath.c_str());
+		throw wexception("'%s' could not be deleted in directory '%s'.", fspath.c_str(), directory_.c_str());
 #endif
 }
 
@@ -295,11 +313,11 @@
 			it = dirname.find(file_separator(), it);
 
 			FileSystemPath fspath(canonicalize_name(dirname.substr(0, it)));
-			if (fspath.m_exists && !fspath.m_isDirectory)
+			if (fspath.exists_ && !fspath.is_directory_)
 				throw wexception
-					("%s exists and is not a directory",
-					 dirname.substr(0, it).c_str());
-			if (!fspath.m_exists)
+					("'%s' in directory '%s' exists and is not a directory",
+					 dirname.substr(0, it).c_str(), directory_.c_str());
+			if (!fspath.exists_)
 				make_directory(dirname.substr(0, it));
 
 			if (it == std::string::npos)
@@ -308,8 +326,8 @@
 		}
 	} catch (const std::exception & e) {
 		throw wexception
-			("RealFSImpl::ensure_directory_exists(%s): %s",
-			 dirname.c_str(), e.what());
+			("RealFSImpl::ensure_directory_exists(%s) in directory '%s': %s",
+			 dirname.c_str(), directory_.c_str(), e.what());
 	}
 }
 
@@ -323,9 +341,10 @@
  */
 void RealFSImpl::make_directory(const std::string & dirname) {
 	FileSystemPath fspath(canonicalize_name(dirname));
-	if (fspath.m_exists)
+	if (fspath.exists_)
 		throw wexception
-			("a file with the name \"%s\" already exists", dirname.c_str());
+			("a file/directory with the name '%s' already exists in directory '%s'",
+			 dirname.c_str(), directory_.c_str());
 
 	if
 #ifdef _WIN32
@@ -442,28 +461,28 @@
 
 struct RealFSStreamRead : public StreamRead {
 	RealFSStreamRead(const std::string & fname)
-		: m_file(fopen(fname.c_str(), "rb"))
+		: file_(fopen(fname.c_str(), "rb"))
 	{
-		if (!m_file)
+		if (!file_)
 			throw wexception("could not open %s for reading", fname.c_str());
 	}
 
 	~RealFSStreamRead()
 	{
-		fclose(m_file);
+		fclose(file_);
 	}
 
 	size_t data(void * read_data, size_t const bufsize) override {
-		return fread(read_data, 1, bufsize, m_file);
+		return fread(read_data, 1, bufsize, file_);
 	}
 
 	bool end_of_file() const override
 	{
-		return feof(m_file);
+		return feof(file_);
 	}
 
 private:
-	FILE * m_file;
+	FILE * file_;
 };
 
 }
@@ -485,31 +504,31 @@
 
 struct RealFSStreamWrite : public StreamWrite {
 	RealFSStreamWrite(const std::string & fname)
-		: m_filename(fname)
+		: filename_(fname)
 	{
-		m_file = fopen(fname.c_str(), "wb");
-		if (!m_file)
+		file_ = fopen(fname.c_str(), "wb");
+		if (!file_)
 			throw wexception("could not open %s for writing", fname.c_str());
 	}
 
-	~RealFSStreamWrite() {fclose(m_file);}
+	~RealFSStreamWrite() {fclose(file_);}
 
 	void data(const void * const write_data, const size_t size) override
 	{
-		size_t ret = fwrite(write_data, 1, size, m_file);
+		size_t ret = fwrite(write_data, 1, size, file_);
 
 		if (ret != size)
-			throw wexception("Write to %s failed", m_filename.c_str());
+			throw wexception("Write to %s failed", filename_.c_str());
 	}
 
 	void flush() override
 	{
-		fflush(m_file);
+		fflush(file_);
 	}
 
 private:
-	std::string m_filename;
-	FILE * m_file;
+	std::string filename_;
+	FILE * file_;
 };
 
 }
@@ -525,14 +544,14 @@
 	ULARGE_INTEGER freeavailable;
 	return
 		GetDiskFreeSpaceEx
-			(canonicalize_name(m_directory).c_str(), &freeavailable, 0, 0)
+			(canonicalize_name(directory_).c_str(), &freeavailable, 0, 0)
 		?
 		//if more than 2G free space report that much
 		freeavailable.HighPart ? std::numeric_limits<unsigned long>::max() :
 		freeavailable.LowPart : 0;
 #else
 	struct statvfs svfs;
-	if (statvfs(canonicalize_name(m_directory).c_str(), &svfs) != -1) {
+	if (statvfs(canonicalize_name(directory_).c_str(), &svfs) != -1) {
 		return static_cast<unsigned long long>(svfs.f_bsize) * svfs.f_bavail;
 	}
 #endif

=== modified file 'src/io/filesystem/disk_filesystem.h'
--- src/io/filesystem/disk_filesystem.h	2014-09-29 13:30:46 +0000
+++ src/io/filesystem/disk_filesystem.h	2016-02-15 15:23:38 +0000
@@ -54,14 +54,14 @@
 	void fs_unlink(const std::string & file) override;
 	void fs_rename(const std::string & old_name, const std::string & new_name) override;
 
-	std::string get_basename() override {return m_directory;}
+	std::string get_basename() override {return directory_;}
 	unsigned long long disk_space() override;
 
 private:
-	void m_unlink_directory(const std::string & file);
-	void m_unlink_file     (const std::string & file);
+	void unlink_directory(const std::string & file);
+	void unlink_file     (const std::string & file);
 
-	std::string m_directory;
+	std::string directory_;
 };
 
 #endif  // end of include guard: WL_IO_FILESYSTEM_DISK_FILESYSTEM_H

=== modified file 'src/io/filesystem/filesystem.cc'
--- src/io/filesystem/filesystem.cc	2014-12-03 07:15:40 +0000
+++ src/io/filesystem/filesystem.cc	2016-02-15 15:23:38 +0000
@@ -62,7 +62,7 @@
 
 FileSystem::FileSystem()
 {
-	m_root = "";
+	root_ = "";
 }
 
 
@@ -72,15 +72,15 @@
  */
 bool FileSystem::is_path_absolute(const std::string & path) const {
 	std::string::size_type const path_size = path  .size();
-	std::string::size_type const root_size = m_root.size();
+	std::string::size_type const root_size = root_.size();
 
 	if (path_size < root_size)
 		return false;
 
 	if (path_size == root_size)
-		return path == m_root;
+		return path == root_;
 
-	if (path.compare(0, m_root.size(), m_root))
+	if (path.compare(0, root_.size(), root_))
 		return false;
 
 #ifdef _WIN32
@@ -262,7 +262,7 @@
 	} else if (!is_path_absolute(path))
 		//  make relative paths absolute (so that "../../foo" can work)
 		fs_tokenize
-			(m_root.empty() ? get_working_directory() : m_root, file_separator(),
+			(root_.empty() ? get_working_directory() : root_, file_separator(),
 			 std::inserter(components, components.begin()));
 
 	//clean up the path

=== modified file 'src/io/filesystem/filesystem.h'
--- src/io/filesystem/filesystem.h	2016-02-06 18:58:57 +0000
+++ src/io/filesystem/filesystem.h	2016-02-15 15:23:38 +0000
@@ -142,7 +142,7 @@
 
 	///How to address the fs' topmost component (e.g. "" on Unix, "D:" on win32)
 	///\warning This is should \e not contain filesep!
-	std::string m_root;
+	std::string root_;
 
 #ifdef _WIN32
 private:

=== modified file 'src/logic/save_handler.cc'
--- src/logic/save_handler.cc	2015-11-14 15:58:29 +0000
+++ src/logic/save_handler.cc	2016-02-15 15:23:38 +0000
@@ -44,7 +44,7 @@
 void SaveHandler::think(Widelands::Game & game) {
 	uint32_t realtime = SDL_GetTicks();
 	initialize(realtime);
-	std::string filename = "wl_autosave";
+	std::string filename = autosave_filename_;
 
 	if (!m_allow_saving) {
 		return;
@@ -97,7 +97,7 @@
 			filename_previous = filename_next;
 			number_of_rolls--;
 		}
-		filename = "wl_autosave_00";
+		filename = (boost::format("%s_00") % autosave_filename_).str();
 		log("Autosave: interval elapsed (%d s), saving %s\n", elapsed, filename.c_str());
 	}
 

=== modified file 'src/logic/save_handler.h'
--- src/logic/save_handler.h	2015-11-14 15:58:29 +0000
+++ src/logic/save_handler.h	2016-02-15 15:23:38 +0000
@@ -33,7 +33,7 @@
 class SaveHandler {
 public:
 	SaveHandler() : m_last_saved_realtime(0), m_initialized(false), m_allow_saving(true),
-		m_save_requested(false), m_save_filename("") {}
+		m_save_requested(false), m_save_filename(""), autosave_filename_("wl_autosave") {}
 	void think(Widelands::Game &);
 	std::string create_file_name(std::string dir, std::string filename);
 	bool save_game
@@ -44,6 +44,7 @@
 	static std::string get_base_dir() {return "save";}
 	const std::string get_cur_filename() {return m_current_filename;}
 	void set_current_filename(std::string filename) {m_current_filename = filename;}
+	void set_autosave_filename(std::string filename) {autosave_filename_ = filename;}
 	void set_allow_saving(bool t) {m_allow_saving = t;}
 	bool get_allow_saving() {return m_allow_saving;}
 	void request_save(std::string filename = "")
@@ -59,6 +60,7 @@
 	bool m_save_requested;
 	std::string m_save_filename;
 	std::string m_current_filename;
+	std::string autosave_filename_;
 
 	void initialize(uint32_t gametime);
 };

=== modified file 'src/network/netclient.cc'
--- src/network/netclient.cc	2016-02-15 15:04:34 +0000
+++ src/network/netclient.cc	2016-02-15 15:23:38 +0000
@@ -186,6 +186,7 @@
 		d->game = &game;
 		game.set_game_controller(this);
 		uint8_t const pn = d->settings.playernum + 1;
+		game.save_handler().set_autosave_filename((boost::format("wl_autosave_netclient%u") % static_cast<unsigned int>(pn)).str());
 		InteractiveGameBase* igb;
 		if (pn > 0)
 			igb = new InteractivePlayer(game, g_options.pull_section("global"), pn, true);

=== modified file 'src/network/nethost.cc'
--- src/network/nethost.cc	2016-02-15 15:04:34 +0000
+++ src/network/nethost.cc	2016-02-15 15:23:38 +0000
@@ -730,6 +730,7 @@
 		game.set_game_controller(this);
 		InteractiveGameBase* igb;
 		uint8_t pn = d->settings.playernum + 1;
+		game.save_handler().set_autosave_filename("wl_autosave_nethost");
 
 		if (d->settings.savegame) {
 			// Read and broadcast original win condition