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

Proposed by Arty
Status: Merged
Merged at revision: 8921
Proposed branch: lp:~widelands-dev/widelands/filesystem-errors
Merge into: lp:widelands
Diff against target: 361 lines (+85/-88)
4 files modified
src/io/filesystem/disk_filesystem.cc (+69/-66)
src/io/filesystem/filesystem.cc (+3/-3)
src/io/filesystem/filesystem_exceptions.h (+0/-12)
src/io/filesystem/zip_filesystem.cc (+13/-7)
To merge this branch: bzr merge lp:~widelands-dev/widelands/filesystem-errors
Reviewer Review Type Date Requested Status
GunChleoc Approve
Review via email: mp+358219@code.launchpad.net

Commit message

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

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

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.

8899. By artydent

made codecheck happy

Revision history for this message
bunnybot (widelandsofficial) wrote :

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.

8900. By GunChleoc

Merged trunk.

8901. By GunChleoc

Merged trunk.

8902. By GunChleoc

Code review

Revision history for this message
GunChleoc (gunchleoc) wrote :

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

Not tested yet.

review: Approve
8903. By GunChleoc

Fix compiler warning

Revision history for this message
Arty (artydent) wrote :

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?

Revision history for this message
GunChleoc (gunchleoc) wrote :

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.

Revision history for this message
Arty (artydent) wrote :

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?

Revision history for this message
kaputtnik (franku) wrote :

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

Revision history for this message
Arty (artydent) wrote :

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.

Revision history for this message
GunChleoc (gunchleoc) wrote :

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.

Revision history for this message
GunChleoc (gunchleoc) wrote :

Tested.

@bunnybot merge

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/io/filesystem/disk_filesystem.cc'
2--- src/io/filesystem/disk_filesystem.cc 2018-04-07 16:59:00 +0000
3+++ src/io/filesystem/disk_filesystem.cc 2018-11-12 08:55:49 +0000
4@@ -167,15 +167,13 @@
5 }
6
7 /**
8- * Create a sub filesystem out of this filesystem
9+ * Make a sub filesystem out of this filesystem
10 */
11 FileSystem* RealFSImpl::make_sub_file_system(const std::string& path) {
12 FileSystemPath fspath(canonicalize_name(path));
13
14 if (!fspath.exists_) {
15- throw wexception("RealFSImpl: unable to create sub filesystem, path does not exist for '%s'"
16- " in directory '%s'",
17- fspath.c_str(), directory_.c_str());
18+ throw FileNotFoundError("RealFSImpl::make_sub_file_system", fspath);
19 }
20
21 if (fspath.is_directory_)
22@@ -190,10 +188,7 @@
23 FileSystem* RealFSImpl::create_sub_file_system(const std::string& path, Type const fs) {
24 FileSystemPath fspath(canonicalize_name(path));
25 if (fspath.exists_)
26- throw wexception(
27- "path '%s'' already exists in directory '%s', can not create a filesystem from it",
28- path.c_str(), directory_.c_str());
29-
30+ throw FileError("RealFSImpl::create_sub_file_system", fspath, "path already exists, cannot create new filesystem from it");
31 if (fs == FileSystem::DIR) {
32 ensure_directory_exists(path);
33 return new RealFSImpl(fspath);
34@@ -221,20 +216,22 @@
35 void RealFSImpl::unlink_file(const std::string& file) {
36 FileSystemPath fspath(canonicalize_name(file));
37 if (!fspath.exists_) {
38- throw wexception(
39- "RealFSImpl: unable to unlink file, path does not exist for '%s' in directory '%s'",
40- fspath.c_str(), directory_.c_str());
41+ throw FileNotFoundError("RealFSImpl::unlink_file", fspath);
42 }
43 if (fspath.is_directory_) {
44- throw wexception(
45- "RealFSImpl: unable to unlink file, path '%s' in directory '%s' is a directory",
46- fspath.c_str(), directory_.c_str());
47+ throw FileTypeError("RealFSImpl::unlink_file", fspath, "path is a directory");
48 }
49-
50 #ifndef _WIN32
51- unlink(fspath.c_str());
52+ if (unlink(fspath.c_str()) == -1)
53+ throw FileError("RealFSImpl::unlink_file", fspath, strerror(errno));
54 #else
55- DeleteFile(fspath.c_str());
56+ // Note: We could possibly replace this with _unlink()
57+ // which would then also work with errno instead of GetLastError(),
58+ // but I am not sure if this is available (and works properly)
59+ // on all windows platforms or only with Visual Studio.
60+ if (!DeleteFile(fspath.c_str()))
61+ throw FileError("RealFSImpl::unlink_file", fspath, std::string("file error (Windows error code ")+std::to_string(GetLastError())+")");
62+ // TODO(Arty): generate proper system message from GetLastError() via FormatMessage
63 #endif
64 }
65
66@@ -244,14 +241,10 @@
67 void RealFSImpl::unlink_directory(const std::string& file) {
68 FileSystemPath fspath(canonicalize_name(file));
69 if (!fspath.exists_) {
70- throw wexception("RealFSImpl: unable to unlink directory, path does not exist for '%s'"
71- " in directory '%s'",
72- fspath.c_str(), directory_.c_str());
73+ throw FileNotFoundError("RealFSImpl::unlink_directory", fspath);
74 }
75 if (!fspath.is_directory_) {
76- throw wexception("RealFSImpl: unable to unlink directory, path '%s' in directory '%s'"
77- " is not a directory",
78- fspath.c_str(), directory_.c_str());
79+ throw FileTypeError("RealFSImpl::unlink_directory", fspath, "path is not a directory");
80 }
81
82 FilenameSet files = list_directory(file);
83@@ -268,14 +261,18 @@
84 unlink_file(*pname);
85 }
86
87-// NOTE: this might fail if this directory contains CVS dir,
88-// so no error checking here
89+// NOTE: this might fail if this directory contains CVS dir
90 #ifndef _WIN32
91- rmdir(fspath.c_str());
92+ if (rmdir(fspath.c_str()) == -1)
93+ throw FileError("RealFSImpl::unlink_directory", fspath, strerror(errno));
94 #else
95+ // Note: We could possibly replace this with _rmdir()
96+ // which would then also work with errno instead of GetLastError(),
97+ // but I am not sure if this is available (and works properly)
98+ // on all windows platforms or only with Visual Studio.
99 if (!RemoveDirectory(fspath.c_str()))
100- throw wexception(
101- "'%s' could not be deleted in directory '%s'.", fspath.c_str(), directory_.c_str());
102+ throw FileError("RealFSImpl::unlink_directory", fspath, std::string("file error (Windows error code ")+std::to_string(GetLastError())+")");
103+ // TODO(Arty): generate proper system message from GetLastError() via FormatMessage
104 #endif
105 }
106
107@@ -292,25 +289,23 @@
108 boost::replace_all(clean_dirname, "\\", "/");
109 #endif
110
111- try {
112- std::string::size_type it = 0;
113- while (it < clean_dirname.size()) {
114- it = clean_dirname.find('/', it);
115-
116- FileSystemPath fspath(canonicalize_name(clean_dirname.substr(0, it)));
117- if (fspath.exists_ && !fspath.is_directory_)
118- throw wexception("'%s' in directory '%s' exists and is not a directory",
119- clean_dirname.substr(0, it).c_str(), directory_.c_str());
120- if (!fspath.exists_)
121- make_directory(clean_dirname.substr(0, it));
122-
123- if (it == std::string::npos)
124- break;
125- ++it;
126- }
127- } catch (const std::exception& e) {
128- throw wexception("RealFSImpl::ensure_directory_exists(%s) in directory '%s': %s",
129- clean_dirname.c_str(), directory_.c_str(), e.what());
130+ std::string::size_type it = 0;
131+ while (it < clean_dirname.size()) {
132+ it = clean_dirname.find('/', it);
133+
134+ FileSystemPath fspath(canonicalize_name(clean_dirname.substr(0, it)));
135+ if (fspath.exists_ && !fspath.is_directory_) {
136+ throw FileTypeError("RealFSImpl::ensure_directory_exists",
137+ fspath, "path is not a directory");
138+ }
139+ if (!fspath.exists_) {
140+ make_directory(clean_dirname.substr(0, it));
141+ }
142+
143+ if (it == std::string::npos) {
144+ break;
145+ }
146+ ++it;
147 }
148 }
149
150@@ -318,33 +313,37 @@
151 * Create this directory, throw an error if it already exists or
152 * if a file is in the way or if the creation fails.
153 *
154- * Pleas note, this function does not honor parents,
155+ * Please note, this function does not honor parents,
156 * make_directory("onedir/otherdir/onemoredir") will fail
157 * if either onedir or otherdir is missing
158 */
159 void RealFSImpl::make_directory(const std::string& dirname) {
160 FileSystemPath fspath(canonicalize_name(dirname));
161 if (fspath.exists_)
162- throw wexception("a file/directory with the name '%s' already exists in directory '%s'",
163- dirname.c_str(), directory_.c_str());
164+ throw FileError("RealFSImpl::make_directory", fspath, "a file or directory with that name already exists");
165
166- if
167-#ifdef _WIN32
168- (!CreateDirectory(fspath.c_str(), NULL))
169+#ifndef _WIN32
170+ if (mkdir(fspath.c_str(), 0x1FF) == -1)
171+ throw FileError("RealFSImpl::make_directory", fspath, strerror(errno));
172 #else
173- (mkdir(fspath.c_str(), 0x1FF) == -1)
174+ // Note: We could possibly replace this with _mkdir()
175+ // which would then also work with errno instead of GetLastError(),
176+ // but I am not sure if this is available (and works properly)
177+ // on all windows platforms or only with Visual Studio.
178+ if (!CreateDirectory(fspath.c_str(), NULL))
179+ throw FileError("RealFSImpl::make_directory", fspath, std::string("file error (Windows error code ")+std::to_string(GetLastError())+")");
180+ // TODO(Arty): generate proper system message from GetLastError() via FormatMessage
181 #endif
182- throw DirectoryCannotCreateError("RealFSImpl::make_directory", dirname, strerror(errno));
183 }
184
185 /**
186- * Read the given file into alloced memory; called by FileRead::open.
187+ * Read the given file into allocated memory; called by FileRead::open.
188 * Throws an exception if the file couldn't be opened.
189 */
190 void* RealFSImpl::load(const std::string& fname, size_t& length) {
191 const std::string fullname = canonicalize_name(fname);
192 if (is_directory(fullname)) {
193- throw FileError("RealFSImpl::load", fullname);
194+ throw FileTypeError("RealFSImpl::load", fullname, "path is a directory");
195 }
196
197 FILE* file = nullptr;
198@@ -353,7 +352,7 @@
199 try {
200 file = fopen(fullname.c_str(), "rb");
201 if (!file)
202- throw FileError("RealFSImpl::load", fullname);
203+ throw FileError("RealFSImpl::load", fullname, "could not open file for reading");
204
205 // determine the size of the file (rather quirky, but it doesn't require
206 // potentially unportable functions)
207@@ -371,6 +370,9 @@
208
209 // allocate a buffer and read the entire file into it
210 data = malloc(size + 1); // TODO(unknown): memory leak!
211+ if (!data)
212+ throw wexception("RealFSImpl::load: memory allocation failed for reading file %s (%s) with size %" PRIuS "",
213+ fname.c_str(), fullname.c_str(), size);
214 int result = fread(data, size, 1, file);
215 if (size && (result != 1)) {
216 throw wexception("RealFSImpl::load: read failed for %s (%s) with size %" PRIuS "",
217@@ -383,10 +385,12 @@
218
219 length = size;
220 } catch (...) {
221- if (file) {
222+ if (file) {
223 fclose(file);
224- }
225- free(data);
226+ }
227+ if (data) {
228+ free(data);
229+ }
230 throw;
231 }
232
233@@ -410,7 +414,7 @@
234
235 FILE* const f = fopen(fullname.c_str(), append ? "a" : "wb");
236 if (!f)
237- throw wexception("could not open %s (%s) for writing", fname.c_str(), fullname.c_str());
238+ throw FileError("RealFSImpl::write", fullname, "could not open file for writing");
239
240 size_t const c = fwrite(data, length, 1, f);
241 fclose(f);
242@@ -424,8 +428,7 @@
243 const std::string fullname1 = canonicalize_name(old_name);
244 const std::string fullname2 = canonicalize_name(new_name);
245 if (rename(fullname1.c_str(), fullname2.c_str()) != 0)
246- throw wexception("DiskFileSystem: unable to rename %s to %s: %s", fullname1.c_str(),
247- fullname2.c_str(), strerror(errno));
248+ throw FileError("RealFSImpl::fs_rename", fullname1, std::string("unable to rename file to ") + fullname2 + ", " + strerror(errno));
249 }
250
251 /*****************************************************************************
252@@ -439,7 +442,7 @@
253 struct RealFSStreamRead : public StreamRead {
254 explicit RealFSStreamRead(const std::string& fname) : file_(fopen(fname.c_str(), "rb")) {
255 if (!file_)
256- throw wexception("could not open %s for reading", fname.c_str());
257+ throw FileError("RealFSStreamRead::RealFSStreamRead", fname, "could not open file for reading");
258 }
259
260 ~RealFSStreamRead() override {
261@@ -477,7 +480,7 @@
262 explicit RealFSStreamWrite(const std::string& fname) : filename_(fname) {
263 file_ = fopen(fname.c_str(), "wb");
264 if (!file_)
265- throw wexception("could not open %s for writing", fname.c_str());
266+ throw FileError("RealFSStreamWrite::RealFSStreamWrite", fname, "could not open file for writing");
267 }
268
269 ~RealFSStreamWrite() override {
270
271=== modified file 'src/io/filesystem/filesystem.cc'
272--- src/io/filesystem/filesystem.cc 2018-07-12 04:41:20 +0000
273+++ src/io/filesystem/filesystem.cc 2018-11-12 08:55:49 +0000
274@@ -306,7 +306,7 @@
275 std::list<std::string>::iterator i;
276
277 #ifdef _WIN32
278- // remove all slashes with backslashes so following can work.
279+ // replace all slashes with backslashes so following can work.
280 for (uint32_t j = 0; j < path.size(); ++j) {
281 if (path[j] == '/')
282 path[j] = '\\';
283@@ -463,8 +463,8 @@
284 fs.ensure_directory_exists(".widelands");
285 fs.fs_unlink(".widelands");
286 return true;
287- } catch (...) {
288- log("Directory %s is not writeable - next try\n", path);
289+ } catch (const FileError& e) {
290+ log("Directory %s is not writeable - next try: %s\n", path, e.what());
291 }
292
293 return false;
294
295=== modified file 'src/io/filesystem/filesystem_exceptions.h'
296--- src/io/filesystem/filesystem_exceptions.h 2018-04-07 16:59:00 +0000
297+++ src/io/filesystem/filesystem_exceptions.h 2018-11-12 08:55:49 +0000
298@@ -76,16 +76,4 @@
299 }
300 };
301
302-/**
303- * The directory cannot be created
304- */
305-class DirectoryCannotCreateError : public FileError {
306-public:
307- explicit DirectoryCannotCreateError(const std::string& thrower,
308- const std::string& dirname,
309- const std::string& message = "cannot create directory")
310-
311- : FileError(thrower, dirname, message) {
312- }
313-};
314 #endif // end of include guard: WL_IO_FILESYSTEM_FILESYSTEM_EXCEPTIONS_H
315
316=== modified file 'src/io/filesystem/zip_filesystem.cc'
317--- src/io/filesystem/zip_filesystem.cc 2018-10-21 22:21:34 +0000
318+++ src/io/filesystem/zip_filesystem.cc 2018-11-12 08:55:49 +0000
319@@ -268,12 +268,14 @@
320 if (!file_exists(path)) {
321 throw wexception(
322 "ZipFilesystem::make_sub_file_system: The path '%s' does not exist in zip file '%s'.",
323- path.c_str(), zip_file_->path().c_str());
324+ (basedir_in_zip_file_.empty() ? path : basedir_in_zip_file_ + "/" + path).c_str(),
325+ zip_file_->path().c_str());
326 }
327 if (!is_directory(path)) {
328- throw wexception("ZipFilesystem::make_sub_file_system: "
329- "The path '%s' needs to be a directory in zip file '%s'.",
330- path.c_str(), zip_file_->path().c_str());
331+ throw wexception(
332+ "ZipFilesystem::make_sub_file_system: The path '%s' needs to be a directory in zip file '%s'.",
333+ (basedir_in_zip_file_.empty() ? path : basedir_in_zip_file_ + "/" + path).c_str(),
334+ zip_file_->path().c_str());
335 }
336
337 std::string localpath = path;
338@@ -291,7 +293,10 @@
339 // see Filesystem::create
340 FileSystem* ZipFilesystem::create_sub_file_system(const std::string& path, Type const type) {
341 if (file_exists(path)) {
342- throw wexception("ZipFilesystem::create_sub_file_system: Sub file system already exists.");
343+ throw wexception(
344+ "ZipFilesystem::create_sub_file_system: Path '%s' already exists in zip file %s.",
345+ (basedir_in_zip_file_.empty() ? path : basedir_in_zip_file_ + "/" + path).c_str(),
346+ zip_file_->path().c_str());
347 }
348
349 if (type != FileSystem::DIR)
350@@ -438,8 +443,9 @@
351 "ZipFilesystem::write", complete_filename,
352 (boost::format("in path '%s'', Error") % zip_file_->path() % strerror(errno)).str());
353 default:
354- throw FileError("ZipFilesystem::write", complete_filename,
355- (boost::format("in path '%s'") % zip_file_->path()).str());
356+ throw FileError(
357+ "ZipFilesystem::write", complete_filename,
358+ (boost::format("in path '%s'") % zip_file_->path()).str());
359 }
360
361 zipCloseFileInZip(zip_file_->write_handle());

Subscribers

People subscribed via source and target branches

to status/vote changes: