Merge lp:~widelands-dev/widelands/filesystem-errors into lp:widelands

[Arty]

Improved error checking in filesystem functions
- some filesystem error checking added
- more specific filesystem exceptions thrown

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 4186. State: failed. Details: https://travis-ci.org/widelands/widelands/builds/449820523.
Appveyor build 3983. State: success. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_filesystem_errors-3983.

[bunnybot (widelandsofficial)]

Continuous integration builds have changed state:

Travis build 4199. State: passed. Details: https://travis-ci.org/widelands/widelands/builds/452677304.
Appveyor build 3995. State: failed. Details: https://ci.appveyor.com/project/widelands-dev/widelands/build/_widelands_dev_widelands_filesystem_errors-3995.

[GunChleoc (gunchleoc)]

I have done a code review with my last commit - please have a look at the diff.

Not tested yet.

review: Approve

[Arty (artydent)]

regarding code review:

There're plenty more long lines and if-blocks without curly braces in there. I'll fix this tonight.

Do we generally follow a strict rule of curly braces even for short single line blocks?

[GunChleoc (gunchleoc)]

Our policy is to always use curly braces, as the lack of them can cause bugs. We still have tons of them missing though, so we only fix them if the code is being changed anyway. Too many merge conflicts otherwise.

[Arty (artydent)]

Oh okay, then I'll leave it.

I am still somewhat fuzzy about some specifics with bazaar, specifically merge conflicts.
Let's say for examply I'd fix all those little things in disk_filesystem.cc, then merge the result into my two child branches (which itself should be fine because those don't make any changes to the file themselve). Then - if I got this right - the only conflicts would arise if someone else is doing other changes to a version of the branches from before the merge (which of course is easily possible during the review) or if someone is changing the file in some other branch. Right?

[kaputtnik (franku)]

No, then you will have 'diverged' branches. I had those a lot in the past... Mostly i deleted the branch and pulled it again to get a clean copy.

Merge conflicts arise if the logic of the merge command wasn't able to merge branch A into branch B. This can happen if the changes made in branch A are too complicated for the merge logic.

Try in terminal:

bzr help conflicts
bzr help conflict-types
bzr help diverged-branches

[Arty (artydent)]

Good to know, thanks. So I guess I shouldn't even make any child branches at all before the parent branch is fully revised and scheduled for merging into trunk. Or maybe just not push them online or at least not bother anyone else with them via merge requests; and then later resolve merge conflicts on my own (possibly just copying it over into a new branch) before making everything available. I feel that this kinda destroys what I thought to be a big advantage of this whole branching concept.

[GunChleoc (gunchleoc)]

You get diverged branches if somebody else pushed to the online version of the branch. You can rescue your changes with:

bzr uncommit
bzr shelve

Then pull the changes:

bzr pull lp:~widelands-dev/widelands/filesystem-errors

Then retrieve the shelved changes:

bzr unshelve

If you getting merge conflicts during the unshelve or then merging trunk, you'll have to manually edit the files to get them in a good state again, then call

bzr resolve

If it doesn't autoresolve:

bzr resolve <filename>

Compile again to make sure that you haven't made any mistakes with the merge, then commit.

[GunChleoc (gunchleoc)]

Tested.

@bunnybot merge

review: Approve

=== modified file 'src/io/filesystem/disk_filesystem.cc'
--- src/io/filesystem/disk_filesystem.cc	2018-04-07 16:59:00 +0000
+++ src/io/filesystem/disk_filesystem.cc	2018-11-12 08:55:49 +0000
@@ -167,15 +167,13 @@
 }
 
 /**
- * Create a sub filesystem out of this filesystem
+ * Make a sub filesystem out of this filesystem
  */
 FileSystem* RealFSImpl::make_sub_file_system(const std::string& path) {
 	FileSystemPath fspath(canonicalize_name(path));
 
 	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());
+		throw FileNotFoundError("RealFSImpl::make_sub_file_system", fspath);
 	}
 
 	if (fspath.is_directory_)
@@ -190,10 +188,7 @@
 FileSystem* RealFSImpl::create_sub_file_system(const std::string& path, Type const fs) {
 	FileSystemPath fspath(canonicalize_name(path));
 	if (fspath.exists_)
-		throw wexception(
-		   "path '%s'' already exists in directory '%s', can not create a filesystem from it",
-		   path.c_str(), directory_.c_str());
-
+		throw FileError("RealFSImpl::create_sub_file_system", fspath, "path already exists, cannot create new filesystem from it");
 	if (fs == FileSystem::DIR) {
 		ensure_directory_exists(path);
 		return new RealFSImpl(fspath);
@@ -221,20 +216,22 @@
 void RealFSImpl::unlink_file(const std::string& file) {
 	FileSystemPath fspath(canonicalize_name(file));
 	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());
+		throw FileNotFoundError("RealFSImpl::unlink_file", fspath);
 	}
 	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());
+		throw FileTypeError("RealFSImpl::unlink_file", fspath, "path is a directory");
 	}
-
 #ifndef _WIN32
-	unlink(fspath.c_str());
+	if (unlink(fspath.c_str()) == -1)
+		throw FileError("RealFSImpl::unlink_file", fspath, strerror(errno));
 #else
-	DeleteFile(fspath.c_str());
+	// Note: We could possibly replace this with _unlink()
+	//       which would then also work with errno instead of GetLastError(),
+	//       but I am not sure if this is available (and works properly)
+	//       on all windows platforms or only with Visual Studio.
+	if (!DeleteFile(fspath.c_str()))
+		throw FileError("RealFSImpl::unlink_file", fspath, std::string("file error (Windows error code ")+std::to_string(GetLastError())+")");
+		// TODO(Arty): generate proper system message from GetLastError() via FormatMessage
 #endif
 }
 
@@ -244,14 +241,10 @@
 void RealFSImpl::unlink_directory(const std::string& file) {
 	FileSystemPath fspath(canonicalize_name(file));
 	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());
+		throw FileNotFoundError("RealFSImpl::unlink_directory", fspath);
 	}
 	if (!fspath.is_directory_) {
-		throw wexception("RealFSImpl: unable to unlink directory, path '%s' in directory '%s'"
-		                 " is not a directory",
-		                 fspath.c_str(), directory_.c_str());
+		throw FileTypeError("RealFSImpl::unlink_directory", fspath, "path is not a directory");
 	}
 
 	FilenameSet files = list_directory(file);
@@ -268,14 +261,18 @@
 			unlink_file(*pname);
 	}
 
-// NOTE: this might fail if this directory contains CVS dir,
-// so no error checking here
+// NOTE: this might fail if this directory contains CVS dir
 #ifndef _WIN32
-	rmdir(fspath.c_str());
+	if (rmdir(fspath.c_str()) == -1)
+		throw FileError("RealFSImpl::unlink_directory", fspath, strerror(errno));
 #else
+	// Note: We could possibly replace this with _rmdir()
+	//       which would then also work with errno instead of GetLastError(),
+	//       but I am not sure if this is available (and works properly)
+	//       on all windows platforms or only with Visual Studio.
 	if (!RemoveDirectory(fspath.c_str()))
-		throw wexception(
-		   "'%s' could not be deleted in directory '%s'.", fspath.c_str(), directory_.c_str());
+		throw FileError("RealFSImpl::unlink_directory", fspath, std::string("file error (Windows error code ")+std::to_string(GetLastError())+")");
+		// TODO(Arty): generate proper system message from GetLastError() via FormatMessage
 #endif
 }
 
@@ -292,25 +289,23 @@
 	boost::replace_all(clean_dirname, "\\", "/");
 #endif
 
-	try {
-		std::string::size_type it = 0;
-		while (it < clean_dirname.size()) {
-			it = clean_dirname.find('/', it);
-
-			FileSystemPath fspath(canonicalize_name(clean_dirname.substr(0, it)));
-			if (fspath.exists_ && !fspath.is_directory_)
-				throw wexception("'%s' in directory '%s' exists and is not a directory",
-				                 clean_dirname.substr(0, it).c_str(), directory_.c_str());
-			if (!fspath.exists_)
-				make_directory(clean_dirname.substr(0, it));
-
-			if (it == std::string::npos)
-				break;
-			++it;
-		}
-	} catch (const std::exception& e) {
-		throw wexception("RealFSImpl::ensure_directory_exists(%s) in directory '%s': %s",
-		                 clean_dirname.c_str(), directory_.c_str(), e.what());
+	std::string::size_type it = 0;
+	while (it < clean_dirname.size()) {
+		it = clean_dirname.find('/', it);
+
+		FileSystemPath fspath(canonicalize_name(clean_dirname.substr(0, it)));
+        if (fspath.exists_ && !fspath.is_directory_) {
+			throw FileTypeError("RealFSImpl::ensure_directory_exists",
+			   fspath, "path is not a directory");
+        }
+        if (!fspath.exists_) {
+			make_directory(clean_dirname.substr(0, it));
+        }
+
+        if (it == std::string::npos) {
+			break;
+        }
+		++it;
 	}
 }
 
@@ -318,33 +313,37 @@
  * Create this directory, throw an error if it already exists or
  * if a file is in the way or if the creation fails.
  *
- * Pleas note, this function does not honor parents,
+ * Please note, this function does not honor parents,
  * make_directory("onedir/otherdir/onemoredir") will fail
  * if either onedir or otherdir is missing
  */
 void RealFSImpl::make_directory(const std::string& dirname) {
 	FileSystemPath fspath(canonicalize_name(dirname));
 	if (fspath.exists_)
-		throw wexception("a file/directory with the name '%s' already exists in directory '%s'",
-		                 dirname.c_str(), directory_.c_str());
+		throw FileError("RealFSImpl::make_directory", fspath, "a file or directory with that name already exists");
 
-	if
-#ifdef _WIN32
-	   (!CreateDirectory(fspath.c_str(), NULL))
+#ifndef _WIN32
+	if (mkdir(fspath.c_str(), 0x1FF) == -1)
+		throw FileError("RealFSImpl::make_directory", fspath, strerror(errno));
 #else
-	   (mkdir(fspath.c_str(), 0x1FF) == -1)
+	// Note: We could possibly replace this with _mkdir()
+	//       which would then also work with errno instead of GetLastError(),
+	//       but I am not sure if this is available (and works properly)
+	//       on all windows platforms or only with Visual Studio.
+	if (!CreateDirectory(fspath.c_str(), NULL))
+		throw FileError("RealFSImpl::make_directory", fspath, std::string("file error (Windows error code ")+std::to_string(GetLastError())+")");
+		// TODO(Arty): generate proper system message from GetLastError() via FormatMessage
 #endif
-		throw DirectoryCannotCreateError("RealFSImpl::make_directory", dirname, strerror(errno));
 }
 
 /**
- * Read the given file into alloced memory; called by FileRead::open.
+ * Read the given file into allocated memory; called by FileRead::open.
  * Throws an exception if the file couldn't be opened.
  */
 void* RealFSImpl::load(const std::string& fname, size_t& length) {
 	const std::string fullname = canonicalize_name(fname);
 	if (is_directory(fullname)) {
-		throw FileError("RealFSImpl::load", fullname);
+		throw FileTypeError("RealFSImpl::load", fullname, "path is a directory");
 	}
 
 	FILE* file = nullptr;
@@ -353,7 +352,7 @@
 	try {
 		file = fopen(fullname.c_str(), "rb");
 		if (!file)
-			throw FileError("RealFSImpl::load", fullname);
+			throw FileError("RealFSImpl::load", fullname, "could not open file for reading");
 
 		// determine the size of the file (rather quirky, but it doesn't require
 		// potentially unportable functions)
@@ -371,6 +370,9 @@
 
 		// allocate a buffer and read the entire file into it
 		data = malloc(size + 1);  //  TODO(unknown): memory leak!
+		if (!data)
+			throw wexception("RealFSImpl::load: memory allocation failed for reading file %s (%s) with size %" PRIuS "",
+			                 fname.c_str(), fullname.c_str(), size);
 		int result = fread(data, size, 1, file);
 		if (size && (result != 1)) {
 			throw wexception("RealFSImpl::load: read failed for %s (%s) with size %" PRIuS "",
@@ -383,10 +385,12 @@
 
 		length = size;
 	} catch (...) {
-		if (file) {
+        if (file) {
 			fclose(file);
-		}
-		free(data);
+        }
+        if (data) {
+			free(data);
+        }
 		throw;
 	}
 
@@ -410,7 +414,7 @@
 
 	FILE* const f = fopen(fullname.c_str(), append ? "a" : "wb");
 	if (!f)
-		throw wexception("could not open %s (%s) for writing", fname.c_str(), fullname.c_str());
+		throw FileError("RealFSImpl::write", fullname, "could not open file for writing");
 
 	size_t const c = fwrite(data, length, 1, f);
 	fclose(f);
@@ -424,8 +428,7 @@
 	const std::string fullname1 = canonicalize_name(old_name);
 	const std::string fullname2 = canonicalize_name(new_name);
 	if (rename(fullname1.c_str(), fullname2.c_str()) != 0)
-		throw wexception("DiskFileSystem: unable to rename %s to %s: %s", fullname1.c_str(),
-		                 fullname2.c_str(), strerror(errno));
+		throw FileError("RealFSImpl::fs_rename", fullname1, std::string("unable to rename file to ") + fullname2 + ", " + strerror(errno));
 }
 
 /*****************************************************************************
@@ -439,7 +442,7 @@
 struct RealFSStreamRead : public StreamRead {
 	explicit RealFSStreamRead(const std::string& fname) : file_(fopen(fname.c_str(), "rb")) {
 		if (!file_)
-			throw wexception("could not open %s for reading", fname.c_str());
+			throw FileError("RealFSStreamRead::RealFSStreamRead", fname, "could not open file for reading");
 	}
 
 	~RealFSStreamRead() override {
@@ -477,7 +480,7 @@
 	explicit RealFSStreamWrite(const std::string& fname) : filename_(fname) {
 		file_ = fopen(fname.c_str(), "wb");
 		if (!file_)
-			throw wexception("could not open %s for writing", fname.c_str());
+			throw FileError("RealFSStreamWrite::RealFSStreamWrite", fname, "could not open file for writing");
 	}
 
 	~RealFSStreamWrite() override {
 
=== modified file 'src/io/filesystem/filesystem.cc'
--- src/io/filesystem/filesystem.cc	2018-07-12 04:41:20 +0000
+++ src/io/filesystem/filesystem.cc	2018-11-12 08:55:49 +0000
@@ -306,7 +306,7 @@
 	std::list<std::string>::iterator i;
 
 #ifdef _WIN32
-	// remove all slashes with backslashes so following can work.
+	// replace all slashes with backslashes so following can work.
 	for (uint32_t j = 0; j < path.size(); ++j) {
 		if (path[j] == '/')
 			path[j] = '\\';
@@ -463,8 +463,8 @@
 		fs.ensure_directory_exists(".widelands");
 		fs.fs_unlink(".widelands");
 		return true;
-	} catch (...) {
-		log("Directory %s is not writeable - next try\n", path);
+	} catch (const FileError& e) {
+		log("Directory %s is not writeable - next try: %s\n", path, e.what());
 	}
 
 	return false;
 
=== modified file 'src/io/filesystem/filesystem_exceptions.h'
--- src/io/filesystem/filesystem_exceptions.h	2018-04-07 16:59:00 +0000
+++ src/io/filesystem/filesystem_exceptions.h	2018-11-12 08:55:49 +0000
@@ -76,16 +76,4 @@
 	}
 };
 
-/**
- * The directory cannot be created
- */
-class DirectoryCannotCreateError : public FileError {
-public:
-	explicit DirectoryCannotCreateError(const std::string& thrower,
-	                                    const std::string& dirname,
-	                                    const std::string& message = "cannot create directory")
-
-	   : FileError(thrower, dirname, message) {
-	}
-};
 #endif  // end of include guard: WL_IO_FILESYSTEM_FILESYSTEM_EXCEPTIONS_H
 
=== modified file 'src/io/filesystem/zip_filesystem.cc'
--- src/io/filesystem/zip_filesystem.cc	2018-10-21 22:21:34 +0000
+++ src/io/filesystem/zip_filesystem.cc	2018-11-12 08:55:49 +0000
@@ -268,12 +268,14 @@
 	if (!file_exists(path)) {
 		throw wexception(
 		   "ZipFilesystem::make_sub_file_system: The path '%s' does not exist in zip file '%s'.",
-		   path.c_str(), zip_file_->path().c_str());
+		   (basedir_in_zip_file_.empty() ? path : basedir_in_zip_file_ + "/" + path).c_str(),
+		   zip_file_->path().c_str());
 	}
 	if (!is_directory(path)) {
-		throw wexception("ZipFilesystem::make_sub_file_system: "
-		                 "The path '%s' needs to be a directory in zip file '%s'.",
-		                 path.c_str(), zip_file_->path().c_str());
+		throw wexception(
+		   "ZipFilesystem::make_sub_file_system: The path '%s' needs to be a directory in zip file '%s'.",
+		   (basedir_in_zip_file_.empty() ? path : basedir_in_zip_file_ + "/" + path).c_str(),
+		   zip_file_->path().c_str());
 	}
 
 	std::string localpath = path;
@@ -291,7 +293,10 @@
 // see Filesystem::create
 FileSystem* ZipFilesystem::create_sub_file_system(const std::string& path, Type const type) {
 	if (file_exists(path)) {
-		throw wexception("ZipFilesystem::create_sub_file_system: Sub file system already exists.");
+		throw wexception(
+		   "ZipFilesystem::create_sub_file_system: Path '%s' already exists in zip file %s.",
+		   (basedir_in_zip_file_.empty() ? path : basedir_in_zip_file_ + "/" + path).c_str(),
+		   zip_file_->path().c_str());
 	}
 
 	if (type != FileSystem::DIR)
@@ -438,8 +443,9 @@
 		   "ZipFilesystem::write", complete_filename,
 		   (boost::format("in path '%s'', Error") % zip_file_->path() % strerror(errno)).str());
 	default:
-		throw FileError("ZipFilesystem::write", complete_filename,
-		                (boost::format("in path '%s'") % zip_file_->path()).str());
+		throw FileError(
+		   "ZipFilesystem::write", complete_filename,
+		   (boost::format("in path '%s'") % zip_file_->path()).str());
 	}
 
 	zipCloseFileInZip(zip_file_->write_handle());