Clean up some filesys code

src/database/database-files.cpp

@@ -196,7 +196,7 @@ bool PlayerDatabaseFiles::removePlayer(const std::string &name)
 		is.close();
 
 		if (temp_player.getName() == name) {
-			fs::DeleteSingleFileOrEmptyDirectory(path);
+			fs::DeleteSingleFileOrEmptyDirectory(path, true);
 			return true;
 		}
 

src/filesys.cpp

@@ -62,46 +62,39 @@ std::vector<DirListNode> GetDirListing(const std::string &pathstring)
 {
 	std::vector<DirListNode> listing;
 
-	WIN32_FIND_DATA FindFileData;
+	WIN32_FIND_DATA FindFileData{};
-	HANDLE hFind = INVALID_HANDLE_VALUE;
 	DWORD dwError;
 
 	std::string dirSpec = pathstring + "\\*";
 
 	// Find the first file in the directory.
-	hFind = FindFirstFile(dirSpec.c_str(), &FindFileData);
+	HANDLE hFind = FindFirstFile(dirSpec.c_str(), &FindFileData);
 
 	if (hFind == INVALID_HANDLE_VALUE) {
 		dwError = GetLastError();
 		if (dwError != ERROR_FILE_NOT_FOUND && dwError != ERROR_PATH_NOT_FOUND) {
-			errorstream << "GetDirListing: FindFirstFile error."
+			errorstream << "GetDirListing: FindFirstFile error code "
-					<< " Error is " << dwError << std::endl;
+					<< dwError << std::endl;
 		}
 	} else {
 		// NOTE:
 		// Be very sure to not include '..' in the results, it will
 		// result in an epic failure when deleting stuff.
 
-		DirListNode node;
+		// do-while because the first file is already in the struct
-		node.name = FindFileData.cFileName;
+		do {
-		node.dir = FindFileData.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY;
-		if (node.name != "." && node.name != "..")
-			listing.push_back(node);
-
-		// List all the other files in the directory.
-		while (FindNextFile(hFind, &FindFileData) != 0) {
 			DirListNode node;
 			node.name = FindFileData.cFileName;
 			node.dir = FindFileData.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY;
 			if(node.name != "." && node.name != "..")
-				listing.push_back(node);
+				listing.push_back(std::move(node));
-		}
+		} while (FindNextFile(hFind, &FindFileData) != 0);
 
 		dwError = GetLastError();
 		FindClose(hFind);
 		if (dwError != ERROR_NO_MORE_FILES) {
-			errorstream << "GetDirListing: FindNextFile error."
+			errorstream << "GetDirListing: FindNextFile error code "
-					<< " Error is " << dwError << std::endl;
+					<< dwError << std::endl;
 			listing.clear();
 			return listing;
 		}
@@ -111,10 +104,8 @@ std::vector<DirListNode> GetDirListing(const std::string &pathstring)
 
 bool CreateDir(const std::string &path)
 {
-	bool r = CreateDirectory(path.c_str(), NULL);
+	bool r = CreateDirectory(path.c_str(), nullptr);
-	if(r == true)
+	if (r || GetLastError() == ERROR_ALREADY_EXISTS)
-		return true;
-	if(GetLastError() == ERROR_ALREADY_EXISTS)
 		return true;
 	return false;
 }
@@ -182,25 +173,31 @@ bool RecursiveDelete(const std::string &path)
 	return true;
 }
 
-bool DeleteSingleFileOrEmptyDirectory(const std::string &path)
+bool DeleteSingleFileOrEmptyDirectory(const std::string &path, bool log_error)
 {
-	if (!IsDir(path))
+	if (!IsDir(path)) {
-		return DeleteFile(path.c_str());
+		bool ok = DeleteFile(path.c_str()) != 0;
-	return RemoveDirectory(path.c_str());
+		if (!ok && log_error)
+			errorstream << "DeleteFile failed: " << LAST_OS_ERROR() << std::endl;
+	}
+	bool ok = RemoveDirectory(path.c_str()) != 0;
+	if (!ok && log_error)
+		errorstream << "RemoveDirectory failed: " << LAST_OS_ERROR() << std::endl;
+	return ok;
 }
 
 std::string TempPath()
 {
-	DWORD bufsize = GetTempPath(0, NULL);
+	DWORD bufsize = GetTempPath(0, nullptr);
-	if(bufsize == 0){
+	if (bufsize == 0) {
-		errorstream<<"GetTempPath failed, error = "<<GetLastError()<<std::endl;
+		errorstream << "GetTempPath failed: " << LAST_OS_ERROR() << std::endl;
 		return "";
 	}
 	std::string buf;
 	buf.resize(bufsize);
 	DWORD len = GetTempPath(bufsize, &buf[0]);
-	if(len == 0 || len > bufsize){
+	if (len == 0 || len > bufsize) {
-		errorstream<<"GetTempPath failed, error = "<<GetLastError()<<std::endl;
+		errorstream << "GetTempPath failed: " << LAST_OS_ERROR() << std::endl;
 		return "";
 	}
 	buf.resize(len);
@@ -235,7 +232,7 @@ bool CopyFileContents(const std::string &source, const std::string &target)
 		nullptr, COPY_FILE_ALLOW_DECRYPTED_DESTINATION);
 	if (!ok) {
 		errorstream << "copying " << source << " to " << target
-			<< " failed: " << GetLastError() << std::endl;
+			<< " failed: " << LAST_OS_ERROR() << std::endl;
 		return false;
 	}
 
@@ -265,13 +262,12 @@ std::vector<DirListNode> GetDirListing(const std::string &pathstring)
 	std::vector<DirListNode> listing;
 
 	DIR *dp;
-	struct dirent *dirp;
+	if((dp = opendir(pathstring.c_str())) == nullptr) {
-	if((dp = opendir(pathstring.c_str())) == NULL) {
-		//infostream<<"Error("<<errno<<") opening "<<pathstring<<std::endl;
 		return listing;
 	}
 
-	while ((dirp = readdir(dp)) != NULL) {
+	struct dirent *dirp;
+	while ((dirp = readdir(dp)) != nullptr) {
 		// NOTE:
 		// Be very sure to not include '..' in the results, it will
 		// result in an epic failure when deleting stuff.
@@ -301,12 +297,13 @@ std::vector<DirListNode> GetDirListing(const std::string &pathstring)
 		*/
 		if(isdir == -1) {
 			struct stat statbuf{};
-			if (stat((pathstring + "/" + node.name).c_str(), &statbuf))
+			if (stat((pathstring + DIR_DELIM + node.name).c_str(), &statbuf))
 				continue;
 			isdir = ((statbuf.st_mode & S_IFDIR) == S_IFDIR);
 		}
 		node.dir = isdir;
-		listing.push_back(node);
+
+		listing.push_back(std::move(node));
 	}
 	closedir(dp);
 
@@ -315,7 +312,7 @@ std::vector<DirListNode> GetDirListing(const std::string &pathstring)
 
 bool CreateDir(const std::string &path)
 {
-	int r = mkdir(path.c_str(), S_IRWXU | S_IRWXG | S_IROTH | S_IXOTH);
+	int r = mkdir(path.c_str(), 0775);
 	if (r == 0) {
 		return true;
 	}
@@ -407,40 +404,35 @@ bool RecursiveDelete(const std::string &path)
 	}
 }
 
-bool DeleteSingleFileOrEmptyDirectory(const std::string &path)
+bool DeleteSingleFileOrEmptyDirectory(const std::string &path, bool log_error)
 {
 	if (IsDir(path)) {
-		bool did = (rmdir(path.c_str()) == 0);
+		bool did = rmdir(path.c_str()) == 0;
-		if (!did)
+		if (!did && log_error)
-			errorstream << "rmdir errno: " << errno << ": " << strerror(errno)
+			errorstream << "rmdir error: " << LAST_OS_ERROR() << std::endl;
-					<< std::endl;
 		return did;
 	}
 
-	bool did = (unlink(path.c_str()) == 0);
+	bool did = unlink(path.c_str()) == 0;
-	if (!did)
+	if (!did && log_error)
-		errorstream << "unlink errno: " << errno << ": " << strerror(errno)
+		errorstream << "unlink error: " << LAST_OS_ERROR() << std::endl;
-				<< std::endl;
 	return did;
 }
 
 std::string TempPath()
 {
-	/*
-		Should the environment variables TMPDIR, TMP and TEMP
-		and the macro P_tmpdir (if defined by stdio.h) be checked
-		before falling back on /tmp?
-
-		Probably not, because this function is intended to be
-		compatible with lua's os.tmpname which under the default
-		configuration hardcodes mkstemp("/tmp/lua_XXXXXX").
-	*/
-
 #ifdef __ANDROID__
 	return porting::path_cache;
+#else
+	const char *env_tmpdir = getenv("TMPDIR");
+	if (env_tmpdir && env_tmpdir[0] == DIR_DELIM_CHAR)
+		return env_tmpdir;
+#ifdef P_tmpdir
+	return P_tmpdir;
 #else
 	return DIR_DELIM "tmp";
 #endif
+#endif
 }
 
 std::string CreateTempFile()
@@ -560,7 +552,7 @@ bool CopyFileContents(const std::string &source, const std::string &target)
 
 void GetRecursiveDirs(std::vector<std::string> &dirs, const std::string &dir)
 {
-	static const std::set<char> chars_to_ignore = { '_', '.' };
+	constexpr std::string_view chars_to_ignore = "_.";
 	if (dir.empty() || !IsDir(dir))
 		return;
 	dirs.push_back(dir);
@@ -575,39 +567,23 @@ std::vector<std::string> GetRecursiveDirs(const std::string &dir)
 }
 
 void GetRecursiveSubPaths(const std::string &path,
-		  std::vector<std::string> &dst,
+		std::vector<std::string> &dst,
-		  bool list_files,
+		bool list_files,
-		  const std::set<char> &ignore)
+		std::string_view ignore)
 {
 	std::vector<DirListNode> content = GetDirListing(path);
 	for (const auto &n : content) {
 		std::string fullpath = path + DIR_DELIM + n.name;
-		if (ignore.count(n.name[0]))
+		if (ignore.find(n.name[0]) != std::string_view::npos)
 			continue;
 		if (list_files || n.dir)
 			dst.push_back(fullpath);
+		// Note: this is probably vulnerable to a symlink infinite loop trap
 		if (n.dir)
 			GetRecursiveSubPaths(fullpath, dst, list_files, ignore);
 	}
 }
 
-bool RecursiveDeleteContent(const std::string &path)
-{
-	infostream<<"Removing content of \""<<path<<"\""<<std::endl;
-	std::vector<DirListNode> list = GetDirListing(path);
-	for (const DirListNode &dln : list) {
-		if(trim(dln.name) == "." || trim(dln.name) == "..")
-			continue;
-		std::string childpath = path + DIR_DELIM + dln.name;
-		bool r = RecursiveDelete(childpath);
-		if(!r) {
-			errorstream << "Removing \"" << childpath << "\" failed" << std::endl;
-			return false;
-		}
-	}
-	return true;
-}
-
 bool CreateAllDirs(const std::string &path)
 {
 	std::vector<std::string> tocreate;
@@ -618,8 +594,8 @@ bool CreateAllDirs(const std::string &path)
 		if (removed.empty())
 			break;
 	}
-	for(int i=tocreate.size()-1;i>=0;i--)
+	for (auto it = tocreate.rbegin(); it != tocreate.rend(); ++it)
-		if(!CreateDir(tocreate[i]))
+		if(!CreateDir(*it))
 			return false;
 	return true;
 }

src/filesys.h

@@ -5,7 +5,6 @@
 #pragma once
 
 #include "config.h"
-#include <set>
 #include <string>
 #include <string_view>
 #include <vector>
@@ -60,7 +59,7 @@ bool CreateDir(const std::string &path);
 // Only pass full paths to this one. returns true on success.
 bool RecursiveDelete(const std::string &path);
 
-bool DeleteSingleFileOrEmptyDirectory(const std::string &path);
+bool DeleteSingleFileOrEmptyDirectory(const std::string &path, bool log_error = false);
 
 /// Returns path to temp directory.
 /// You probably don't want to use this directly, see `CreateTempFile` or `CreateTempDir`.
@@ -79,23 +78,19 @@ bool DeleteSingleFileOrEmptyDirectory(const std::string &path);
        hidden directories (whose names start with . or _)
 */
 void GetRecursiveDirs(std::vector<std::string> &dirs, const std::string &dir);
+
 [[nodiscard]]
 std::vector<std::string> GetRecursiveDirs(const std::string &dir);
 
-/* Multiplatform */
-
 /* The path itself not included, returns a list of all subpaths.
    dst - vector that contains all the subpaths.
    list files - include files in the list of subpaths.
-   ignore - paths that start with these charcters will not be listed.
+   ignore - paths that start with one of these charcters will not be listed.
 */
 void GetRecursiveSubPaths(const std::string &path,
-		  std::vector<std::string> &dst,
+		std::vector<std::string> &dst,
-		  bool list_files,
+		bool list_files,
-		  const std::set<char> &ignore = {});
+		std::string_view ignore = {});
-
-// Only pass full paths to this one. True on success.
-bool RecursiveDeleteContent(const std::string &path);
 
 // Create all directories on the given path that don't already exist.
 bool CreateAllDirs(const std::string &path);

src/gui/guiEngine.cpp

@@ -615,7 +615,7 @@ bool GUIEngine::downloadFile(const std::string &url, const std::string &target)
 
 	if (!completed || !fetch_result.succeeded) {
 		target_file.close();
-		fs::DeleteSingleFileOrEmptyDirectory(target);
+		fs::DeleteSingleFileOrEmptyDirectory(target, true);
 		return false;
 	}
 	// TODO: directly stream the response data into the file instead of first

src/server.cpp

@@ -416,7 +416,7 @@ Server::~Server()
 	// Clean up files
 	for (auto &it : m_media) {
 		if (it.second.delete_at_shutdown) {
-			fs::DeleteSingleFileOrEmptyDirectory(it.second.path);
+			fs::DeleteSingleFileOrEmptyDirectory(it.second.path, true);
 		}
 	}
 
@@ -2913,7 +2913,7 @@ void Server::stepPendingDynMediaCallbacks(float dtime)
 			assert(m_media.count(name));
 			sanity_check(m_media[name].ephemeral);
 
-			fs::DeleteSingleFileOrEmptyDirectory(m_media[name].path);
+			fs::DeleteSingleFileOrEmptyDirectory(m_media[name].path, true);
 			m_media.erase(name);
 		}
 		getScriptIface()->freeDynamicMediaCallback(token);

src/settings.cpp

@@ -16,6 +16,7 @@
 #include "filesys.h"
 #include "noise.h"
 #include <cctype>
+#include <set>
 #include <algorithm>
 
 Settings *g_settings = nullptr;

src/unittest/test_filesys.cpp

@@ -5,6 +5,7 @@
 #include "test.h"
 
 #include <sstream>
+#include <algorithm>
 
 #include "log.h"
 #include "serialization.h"
@@ -29,6 +30,7 @@ public:
 	void testCopyFileContents();
 	void testNonExist();
 	void testRecursiveDelete();
+	void testGetRecursiveSubPaths();
 };
 
 static TestFileSys g_test_instance;
@@ -45,6 +47,7 @@ void TestFileSys::runTests(IGameDef *gamedef)
 	TEST(testCopyFileContents);
 	TEST(testNonExist);
 	TEST(testRecursiveDelete);
+	TEST(testGetRecursiveSubPaths);
 }
 
 ////////////////////////////////////////////////////////////////////////////////
@@ -413,3 +416,50 @@ void TestFileSys::testRecursiveDelete()
 	for (auto &it : files)
 		UASSERT(!fs::IsFile(it));
 }
+
+void TestFileSys::testGetRecursiveSubPaths()
+{
+	const auto dir_path = getTestTempDirectory() + DIR_DELIM "recursivetest";
+	UASSERT(fs::CreateAllDirs(dir_path));
+
+	std::string dirs[] = {
+		dir_path + DIR_DELIM "d1",
+		dir_path + DIR_DELIM "d1" DIR_DELIM "d2",
+		dir_path + DIR_DELIM "_d3"
+	};
+	std::string files[] = {
+		dirs[0] + DIR_DELIM "f1",
+		dirs[1] + DIR_DELIM "f2",
+		dirs[0] + DIR_DELIM ".f3",
+	};
+
+	for (auto &it : dirs)
+		fs::CreateDir(it);
+	for (auto &it : files)
+		open_ofstream(it.c_str(), false).close();
+
+	std::vector<std::string> dst;
+	fs::GetRecursiveSubPaths(dir_path, dst, false);
+	UASSERT(CONTAINS(dst, dirs[0]));
+	UASSERT(CONTAINS(dst, dirs[1]));
+	UASSERT(CONTAINS(dst, dirs[2]));
+	UASSERTEQ(size_t, dst.size(), 3);
+
+	dst.clear();
+	fs::GetRecursiveSubPaths(dir_path, dst, true);
+	UASSERT(CONTAINS(dst, dirs[0]));
+	UASSERT(CONTAINS(dst, dirs[1]));
+	UASSERT(CONTAINS(dst, dirs[2]));
+	UASSERT(CONTAINS(dst, files[0]));
+	UASSERT(CONTAINS(dst, files[1]));
+	UASSERT(CONTAINS(dst, files[2]));
+	UASSERTEQ(size_t, dst.size(), 3+3);
+
+	dst.clear();
+	fs::GetRecursiveSubPaths(dir_path, dst, true, "_zzzabczzzz.");
+	UASSERT(CONTAINS(dst, dirs[0]));
+	UASSERT(CONTAINS(dst, dirs[1]));
+	UASSERT(CONTAINS(dst, files[0]));
+	UASSERT(CONTAINS(dst, files[1]));
+	UASSERTEQ(size_t, dst.size(), 2+2);
+}