From 7594b19644b52d473994cd97d02a5b723ae6d7e8 Mon Sep 17 00:00:00 2001 From: sfan5 Date: Thu, 20 Nov 2025 18:51:00 +0100 Subject: [PATCH] Clean up some filesys code --- src/database/database-files.cpp | 2 +- src/filesys.cpp | 138 +++++++++++++------------------- src/filesys.h | 17 ++-- src/gui/guiEngine.cpp | 2 +- src/server.cpp | 4 +- src/settings.cpp | 1 + src/unittest/test_filesys.cpp | 50 ++++++++++++ 7 files changed, 118 insertions(+), 96 deletions(-) diff --git a/src/database/database-files.cpp b/src/database/database-files.cpp index 52d6ab0e38..a9ce7dac24 100644 --- a/src/database/database-files.cpp +++ b/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; } diff --git a/src/filesys.cpp b/src/filesys.cpp index ba8661462e..1391e346d7 100644 --- a/src/filesys.cpp +++ b/src/filesys.cpp @@ -62,46 +62,39 @@ std::vector GetDirListing(const std::string &pathstring) { std::vector listing; - WIN32_FIND_DATA FindFileData; - HANDLE hFind = INVALID_HANDLE_VALUE; + WIN32_FIND_DATA FindFileData{}; 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." - << " Error is " << dwError << std::endl; + errorstream << "GetDirListing: FindFirstFile error code " + << 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; - node.name = FindFileData.cFileName; - 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) { + // do-while because the first file is already in the struct + do { 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." - << " Error is " << dwError << std::endl; + errorstream << "GetDirListing: FindNextFile error code " + << dwError << std::endl; listing.clear(); return listing; } @@ -111,10 +104,8 @@ std::vector GetDirListing(const std::string &pathstring) bool CreateDir(const std::string &path) { - bool r = CreateDirectory(path.c_str(), NULL); - if(r == true) - return true; - if(GetLastError() == ERROR_ALREADY_EXISTS) + bool r = CreateDirectory(path.c_str(), nullptr); + if (r || 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)) - return DeleteFile(path.c_str()); - return RemoveDirectory(path.c_str()); + if (!IsDir(path)) { + bool ok = DeleteFile(path.c_str()) != 0; + 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); - if(bufsize == 0){ - errorstream<<"GetTempPath failed, error = "< bufsize){ - errorstream<<"GetTempPath failed, error = "< bufsize) { + 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 GetDirListing(const std::string &pathstring) std::vector listing; DIR *dp; - struct dirent *dirp; - if((dp = opendir(pathstring.c_str())) == NULL) { - //infostream<<"Error("< 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 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); - if (!did) - errorstream << "rmdir errno: " << errno << ": " << strerror(errno) - << std::endl; + bool did = rmdir(path.c_str()) == 0; + if (!did && log_error) + errorstream << "rmdir error: " << LAST_OS_ERROR() << std::endl; return did; } - bool did = (unlink(path.c_str()) == 0); - if (!did) - errorstream << "unlink errno: " << errno << ": " << strerror(errno) - << std::endl; + bool did = unlink(path.c_str()) == 0; + if (!did && log_error) + errorstream << "unlink error: " << LAST_OS_ERROR() << 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 &dirs, const std::string &dir) { - static const std::set 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 GetRecursiveDirs(const std::string &dir) } void GetRecursiveSubPaths(const std::string &path, - std::vector &dst, - bool list_files, - const std::set &ignore) + std::vector &dst, + bool list_files, + std::string_view ignore) { std::vector 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 \""< 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 tocreate; @@ -618,8 +594,8 @@ bool CreateAllDirs(const std::string &path) if (removed.empty()) break; } - for(int i=tocreate.size()-1;i>=0;i--) - if(!CreateDir(tocreate[i])) + for (auto it = tocreate.rbegin(); it != tocreate.rend(); ++it) + if(!CreateDir(*it)) return false; return true; } diff --git a/src/filesys.h b/src/filesys.h index 5ae1f1234e..3d682e47a0 100644 --- a/src/filesys.h +++ b/src/filesys.h @@ -5,7 +5,6 @@ #pragma once #include "config.h" -#include #include #include #include @@ -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 &dirs, const std::string &dir); + [[nodiscard]] std::vector 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 &dst, - bool list_files, - const std::set &ignore = {}); - -// Only pass full paths to this one. True on success. -bool RecursiveDeleteContent(const std::string &path); + std::vector &dst, + bool list_files, + std::string_view ignore = {}); // Create all directories on the given path that don't already exist. bool CreateAllDirs(const std::string &path); diff --git a/src/gui/guiEngine.cpp b/src/gui/guiEngine.cpp index 66232ed4af..e1517065b3 100644 --- a/src/gui/guiEngine.cpp +++ b/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 diff --git a/src/server.cpp b/src/server.cpp index fd866aa7f8..f585e77052 100644 --- a/src/server.cpp +++ b/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); diff --git a/src/settings.cpp b/src/settings.cpp index 97b7ca4f6e..2e55cff5da 100644 --- a/src/settings.cpp +++ b/src/settings.cpp @@ -16,6 +16,7 @@ #include "filesys.h" #include "noise.h" #include +#include #include Settings *g_settings = nullptr; diff --git a/src/unittest/test_filesys.cpp b/src/unittest/test_filesys.cpp index 8c93bdb22d..2da27e829e 100644 --- a/src/unittest/test_filesys.cpp +++ b/src/unittest/test_filesys.cpp @@ -5,6 +5,7 @@ #include "test.h" #include +#include #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 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); +}