Merge pull request OpenEnroth#1865 from captainurist/macos_fixes_605

Some cleanup in FileSystem & Logging
captainurist · Nov 3, 2024 · af34bad · af34bad
2 parents 5d86d75 + bdc19ca
commit af34bad
Show file tree

Hide file tree

Showing 60 changed files with 1,061 additions and 521 deletions.
diff --git a/Doxyfile b/Doxyfile
@@ -2295,7 +2295,7 @@ INCLUDE_FILE_PATTERNS  =
 # recursively expanded use the := operator instead of the = operator.
 # This tag requires that the tag ENABLE_PREPROCESSING is set to YES.
 
-PREDEFINED             =
+PREDEFINED             = __DOXYGEN__
 
 # If the MACRO_EXPANSION and EXPAND_ONLY_PREDEF tags are set to YES then this
 # tag can be used to specify a list of macro names that should be expanded. The

diff --git a/src/Application/GameConfig.h b/src/Application/GameConfig.h
@@ -94,7 +94,7 @@ class GameConfig : public Config {
         Bool NoMargaret = {this, "no_margareth", false, "Disable Margaret's tour messages on Emerald Island."};
 
         ConfigEntry<::LogLevel> LogLevel = {this, "log_level", LOG_INFO,
-                                            "Default log level. One of 'trace', 'debug', 'info', 'warning', 'error' and 'critical'."};
+                                            "Default log level. One of 'none', 'trace', 'debug', 'info', 'warning', 'error' and 'critical'."};
 
         // TODO(captainurist): move all Trace* options into a separate section.
 

diff --git a/src/Application/Startup/GameStarter.cpp b/src/Application/Startup/GameStarter.cpp
@@ -59,7 +59,6 @@ constexpr std::string_view configName = "openenroth.ini";
 
 GameStarter::GameStarter(GameStarterOptions options): _options(std::move(options)) {
     // Init logging.
-    _logStarter.initPrimary();
     Engine::LogEngineBuildInfo();
 
     try {
@@ -77,7 +76,6 @@ void GameStarter::initWithLogger() {
     // Resolve user path, create user fs & file logger.
     resolveUserPath(_environment.get(), &_options);
     _fsStarter.initUserFs(_options.ramFsUserData, _options.userPath);
-    _logStarter.initSecondary(ufs);
 
     // Resolve data path, create data fs.
     // TODO(captainurist): actually move datapath to config?
@@ -99,7 +97,7 @@ void GameStarter::initWithLogger() {
     }
 
     // Finish logger init now that we know the desired log level.
-    _logStarter.initFinal(_options.logLevel ? *_options.logLevel : _config->debug.LogLevel.value());
+    _logStarter.initialize(ufs, _options.logLevel ? *_options.logLevel : _config->debug.LogLevel.value());
 
     // Create platform.
     if (_options.headless) {

diff --git a/src/Application/Startup/LogStarter.cpp b/src/Application/Startup/LogStarter.cpp
@@ -5,28 +5,11 @@
 #include <memory>
 
 #include "Library/FileSystem/Interface/FileSystem.h"
-#include "Library/Logger/StreamLogSink.h"
+#include "Library/Logger/RotatingLogSink.h"
 #include "Library/Logger/DistLogSink.h"
 #include "Library/Logger/BufferLogSink.h"
 
-LogStarter::LogStarter() = default;
-
-LogStarter::~LogStarter() {
-    // We never got around to finding out the desired log level => flush everything at LOG_TRACE.
-    if (_stage != STAGE_FINAL && _stage != STAGE_INITIAL) {
-        _stage = STAGE_SECONDARY;
-        try {
-            initFinal(LOG_TRACE);
-        } catch (...) {
-            // Nothing we can do here.
-        }
-    }
-}
-
-void LogStarter::initPrimary() {
-    assert(_stage == STAGE_INITIAL);
-    _stage = STAGE_PRIMARY;
-
+LogStarter::LogStarter() {
     _rootLogSink = std::make_unique<DistLogSink>();
     _logger = std::make_unique<Logger>(LOG_TRACE, _rootLogSink.get());
 
@@ -35,31 +18,35 @@ void LogStarter::initPrimary() {
     _rootLogSink->addLogSink(_bufferLogSink.get());
 }
 
-void LogStarter::initSecondary(FileSystem *userFs) {
-    assert(_stage == STAGE_PRIMARY);
-    _stage = STAGE_SECONDARY;
-
-    try {
-        std::string path = fmt::format("logs/openenroth_{:%Y_%m_%d_%H_%M_%S}.log",
-                                       std::chrono::floor<std::chrono::seconds>(std::chrono::system_clock::now()));
-        _userLogStream = userFs->openForWriting(path);
-        _userLogSink = std::make_unique<StreamLogSink>(_userLogStream.get());
-    } catch (const std::exception &e) {
-        _logger->log(LOG_ERROR, "Could not open log file for writing: {}", e.what());
-        _userLogStream.reset();
-        _userLogSink.reset();
+LogStarter::~LogStarter() {
+    // We never got around to finding out the desired log level => flush everything at LOG_TRACE.
+    if (!_initialized) {
+        try {
+            initialize(nullptr, LOG_TRACE);
+        } catch (...) {
+            // Nothing we can do here.
+        }
     }
 }
 
-void LogStarter::initFinal(LogLevel logLevel) {
-    assert(_stage == STAGE_SECONDARY);
-    _stage = STAGE_FINAL;
+void LogStarter::initialize(FileSystem *userFs, LogLevel logLevel) {
+    assert(!_initialized);
+    _initialized = true;
 
-    // Create proper log sinks.
+    // Create & install default log sink.
     _defaultLogSink = LogSink::createDefaultSink();
     _rootLogSink->addLogSink(_defaultLogSink.get());
-    if (_userLogSink)
-        _rootLogSink->addLogSink(_userLogSink.get());
+
+    // Set up filesystem logging. We skip this on LOG_NONE b/c we don't want any FS changes in this case.
+    if (userFs && logLevel != LOG_NONE) {
+        try {
+            _userLogSink = std::make_unique<RotatingLogSink>("logs/openenroth.log", userFs);
+            _rootLogSink->addLogSink(_userLogSink.get());
+        } catch (const std::exception &e) {
+            _logger->log(LOG_ERROR, "Could not open log file for writing: {}", e.what());
+            _userLogSink.reset();
+        }
+    }
 
     // Then init log level & flush.
     _rootLogSink->removeLogSink(_bufferLogSink.get());

diff --git a/src/Application/Startup/LogStarter.h b/src/Application/Startup/LogStarter.h
@@ -9,35 +9,24 @@ class FileSystem;
 class LogSink;
 class DistLogSink;
 class BufferLogSink;
-class StreamLogSink;
+class RotatingLogSink;
 class Logger;
 
 class LogStarter {
  public:
     LogStarter();
     ~LogStarter();
 
-    void initPrimary(); // Can use global logger after this.
-    void initSecondary(FileSystem *userFs); // Start writing filesystem log.
-    void initFinal(LogLevel logLevel); // Set log level & finalize logger init.
+    void initialize(FileSystem *userFs, LogLevel logLevel); // Set log level & finalize logger init.
 
     DistLogSink *rootSink() const;
 
  private:
-    enum class Stage {
-        STAGE_INITIAL,
-        STAGE_PRIMARY,
-        STAGE_SECONDARY,
-        STAGE_FINAL
-    };
-    using enum Stage;
-
- private:
-    Stage _stage = STAGE_INITIAL;
+    bool _initialized = false;
+    std::unique_ptr<LogSink> _nullSink;
     std::unique_ptr<BufferLogSink> _bufferLogSink;
     std::unique_ptr<LogSink> _defaultLogSink;
-    std::unique_ptr<OutputStream> _userLogStream;
-    std::unique_ptr<StreamLogSink> _userLogSink;
+    std::unique_ptr<RotatingLogSink> _userLogSink;
     std::unique_ptr<DistLogSink> _rootLogSink;
     std::unique_ptr<Logger> _logger;
 };
diff --git a/src/Bin/OpenEnroth/OpenEnrothOptions.cpp b/src/Bin/OpenEnroth/OpenEnrothOptions.cpp
@@ -35,7 +35,7 @@ OpenEnrothOptions OpenEnrothOptions::parse(int argc, char **argv) {
         "If '.portable' file exists in the current folder, then this parameter defaults to 'true'.");
     app->add_option(
         "--log-level", result.logLevel,
-        "Log level, one of 'trace', 'debug', 'info', 'warning', 'error', 'critical'.")->option_text("LOG_LEVEL");
+        "Log level, one of 'none', 'trace', 'debug', 'info', 'warning', 'error', 'critical'.")->option_text("LOG_LEVEL");
     app->add_flag_callback(
         "-v,--verbose", [&] { result.logLevel = LOG_TRACE; },
         "Set log level to 'trace'.");

diff --git a/src/Library/FileSystem/Directory/DirectoryFileSystem.cpp b/src/Library/FileSystem/Directory/DirectoryFileSystem.cpp
@@ -22,14 +22,14 @@ DirectoryFileSystem::DirectoryFileSystem(std::string_view root) {
 
 DirectoryFileSystem::~DirectoryFileSystem() = default;
 
-bool DirectoryFileSystem::_exists(const FileSystemPath &path) const {
+bool DirectoryFileSystem::_exists(FileSystemPathView path) const {
     assert(!path.isEmpty());
 
     std::error_code ec;
     return std::filesystem::exists(makeBasePath(path), ec); // Returns false on error.
 }
 
-FileStat DirectoryFileSystem::_stat(const FileSystemPath &path) const {
+FileStat DirectoryFileSystem::_stat(FileSystemPathView path) const {
     assert(!path.isEmpty());
 
     std::filesystem::path basePath = makeBasePath(path);
@@ -53,7 +53,7 @@ FileStat DirectoryFileSystem::_stat(const FileSystemPath &path) const {
     result.size = size;
     return result;}
 
-void DirectoryFileSystem::_ls(const FileSystemPath &path, std::vector<DirectoryEntry> *entries) const {
+void DirectoryFileSystem::_ls(FileSystemPathView path, std::vector<DirectoryEntry> *entries) const {
     std::filesystem::path basePath = makeBasePath(path);
 
     // Handle the known errors first.
@@ -92,12 +92,12 @@ void DirectoryFileSystem::_ls(const FileSystemPath &path, std::vector<DirectoryE
     }
 }
 
-Blob DirectoryFileSystem::_read(const FileSystemPath &path) const {
+Blob DirectoryFileSystem::_read(FileSystemPathView path) const {
     assert(!path.isEmpty());
     return Blob::fromFile(makeBasePath(path).generic_string());
 }
 
-void DirectoryFileSystem::_write(const FileSystemPath &path, const Blob &data) {
+void DirectoryFileSystem::_write(FileSystemPathView path, const Blob &data) {
     assert(!path.isEmpty());
     std::filesystem::path basePath = makeBasePath(path);
     std::filesystem::create_directories(basePath.parent_path());
@@ -106,19 +106,19 @@ void DirectoryFileSystem::_write(const FileSystemPath &path, const Blob &data) {
     stream.close();
 }
 
-std::unique_ptr<InputStream> DirectoryFileSystem::_openForReading(const FileSystemPath &path) const {
+std::unique_ptr<InputStream> DirectoryFileSystem::_openForReading(FileSystemPathView path) const {
     assert(!path.isEmpty());
     return std::make_unique<FileInputStream>(makeBasePath(path).generic_string());
 }
 
-std::unique_ptr<OutputStream> DirectoryFileSystem::_openForWriting(const FileSystemPath &path) {
+std::unique_ptr<OutputStream> DirectoryFileSystem::_openForWriting(FileSystemPathView path) {
     assert(!path.isEmpty());
     std::filesystem::path basePath = makeBasePath(path);
     std::filesystem::create_directories(basePath.parent_path());
     return std::make_unique<FileOutputStream>(basePath.generic_string());
 }
 
-void DirectoryFileSystem::_rename(const FileSystemPath &srcPath, const FileSystemPath &dstPath) {
+void DirectoryFileSystem::_rename(FileSystemPathView srcPath, FileSystemPathView dstPath) {
     assert(!srcPath.isEmpty());
     assert(!dstPath.isEmpty());
 
@@ -133,15 +133,15 @@ void DirectoryFileSystem::_rename(const FileSystemPath &srcPath, const FileSyste
     std::filesystem::rename(srcBasePath, dstBasePath);
 }
 
-bool DirectoryFileSystem::_remove(const FileSystemPath &path) {
+bool DirectoryFileSystem::_remove(FileSystemPathView path) {
     assert(!path.isEmpty());
     return std::filesystem::remove_all(makeBasePath(path)) > 0;
 }
 
-std::string DirectoryFileSystem::_displayPath(const FileSystemPath &path) const {
+std::string DirectoryFileSystem::_displayPath(FileSystemPathView path) const {
     return makeBasePath(path).generic_string();
 }
 
-std::filesystem::path DirectoryFileSystem::makeBasePath(const FileSystemPath &path) const {
+std::filesystem::path DirectoryFileSystem::makeBasePath(FileSystemPathView path) const {
     return _root / path.string();
 }
diff --git a/src/Library/FileSystem/Directory/DirectoryFileSystem.h b/src/Library/FileSystem/Directory/DirectoryFileSystem.h
@@ -26,18 +26,18 @@ class DirectoryFileSystem : public FileSystem {
     virtual ~DirectoryFileSystem();
 
  private:
-    virtual bool _exists(const FileSystemPath &path) const override;
-    virtual FileStat _stat(const FileSystemPath &path) const override;
-    virtual void _ls(const FileSystemPath &path, std::vector<DirectoryEntry> *entries) const override;
-    virtual Blob _read(const FileSystemPath &path) const override;
-    virtual void _write(const FileSystemPath &path, const Blob &data) override;
-    virtual std::unique_ptr<InputStream> _openForReading(const FileSystemPath &path) const override;
-    virtual std::unique_ptr<OutputStream> _openForWriting(const FileSystemPath &path) override;
-    virtual void _rename(const FileSystemPath &srcPath, const FileSystemPath &dstPath) override;
-    virtual bool _remove(const FileSystemPath &path) override;
-    virtual std::string _displayPath(const FileSystemPath &path) const override;
+    virtual bool _exists(FileSystemPathView path) const override;
+    virtual FileStat _stat(FileSystemPathView path) const override;
+    virtual void _ls(FileSystemPathView path, std::vector<DirectoryEntry> *entries) const override;
+    virtual Blob _read(FileSystemPathView path) const override;
+    virtual void _write(FileSystemPathView path, const Blob &data) override;
+    virtual std::unique_ptr<InputStream> _openForReading(FileSystemPathView path) const override;
+    virtual std::unique_ptr<OutputStream> _openForWriting(FileSystemPathView path) override;
+    virtual void _rename(FileSystemPathView srcPath, FileSystemPathView dstPath) override;
+    virtual bool _remove(FileSystemPathView path) override;
+    virtual std::string _displayPath(FileSystemPathView path) const override;
 
-    std::filesystem::path makeBasePath(const FileSystemPath &path) const;
+    std::filesystem::path makeBasePath(FileSystemPathView path) const;
 
  private:
     std::string _originalRoot;

diff --git a/src/Library/FileSystem/Directory/Tests/DirectoryFileSystem_ut.cpp b/src/Library/FileSystem/Directory/Tests/DirectoryFileSystem_ut.cpp
@@ -110,3 +110,28 @@ UNIT_TEST(DirectoryFileSystem, DisplayPathSymmetry) {
     EXPECT_TRUE(std::filesystem::path(blob.displayPath()).is_absolute());
     EXPECT_EQ(blob.displayPath(), stream->displayPath());
 }
+
+UNIT_TEST(DirectoryFileSystem, EscapingPaths) {
+    TemporaryDir tmp("a");
+    ScopedTestFile tmp2("1.txt", "");
+    ScopedTestFile tmp3("a/1.txt", "");
+
+    DirectoryFileSystem fs("a");
+
+    EXPECT_FALSE(fs.exists(".."));
+    EXPECT_FALSE(fs.stat(".."));
+    EXPECT_ANY_THROW((void) fs.ls(".."));
+    EXPECT_ANY_THROW((void) fs.read("../1.txt"));
+    EXPECT_ANY_THROW((void) fs.openForReading("../1.txt"));
+    EXPECT_ANY_THROW(fs.write("../1.txt", Blob()));
+    EXPECT_ANY_THROW((void) fs.openForWriting("../1.txt"));
+    EXPECT_ANY_THROW(fs.remove("../1.txt"));
+    EXPECT_ANY_THROW(fs.rename("../1.txt", "2.txt"));
+    EXPECT_ANY_THROW(fs.rename("1.txt", "../2.txt"));
+}
+
+UNIT_TEST(DirectoryFileSystem, EscapingDisplayPath) {
+    DirectoryFileSystem fs("");
+
+    EXPECT_TRUE(fs.displayPath("..").ends_with(".."));
+}
diff --git a/src/Library/FileSystem/Dump/FileSystemDump.cpp b/src/Library/FileSystem/Dump/FileSystemDump.cpp
@@ -3,7 +3,6 @@
 #include <cstdio>
 #include <iomanip>
 #include <ranges>
-#include <tuple>
 #include <utility>
 #include <string>
 #include <vector>
@@ -27,11 +26,11 @@ class FileSystemDumper {
         if (_maxEntries == 0)
             return;
 
-        dump(FileSystemPath());
+        dump({});
     }
 
  private:
-    void dump(const FileSystemPath &path) {
+    void dump(FileSystemPathView path) {
         writeOutDir(path);
         if (_entries == _maxEntries)
             return;
@@ -40,7 +39,7 @@ class FileSystemDumper {
         std::ranges::sort(entries);
 
         for (const DirectoryEntry &entry : entries) {
-            FileSystemPath entryPath = path.appended(entry.name);
+            FileSystemPath entryPath = path / entry.name;
 
             if (entry.type == FILE_REGULAR) {
                 writeOutFile(entryPath);
@@ -54,7 +53,7 @@ class FileSystemDumper {
         }
     }
 
-    void writeOutDir(const FileSystemPath &path) {
+    void writeOutDir(FileSystemPathView path) {
         if (_target) {
             _target->push_back(FileSystemDumpEntry(path.string(), FILE_DIRECTORY));
         } else {
@@ -65,7 +64,7 @@ class FileSystemDumper {
         assert(_entries <= _maxEntries);
     }
 
-    void writeOutFile(const FileSystemPath &path) {
+    void writeOutFile(FileSystemPathView path) {
         Blob content;
         if (_flags & FILE_SYSTEM_DUMP_WITH_CONTENTS)
             content = _fs->read(path);

diff --git a/src/Library/FileSystem/Dump/FileSystemDump.h b/src/Library/FileSystem/Dump/FileSystemDump.h
@@ -26,12 +26,12 @@ struct FileSystemDumpEntry {
 
     FileSystemDumpEntry(const FileSystemDumpEntry &other): path(other.path), type(other.type), content(Blob::share(other.content)) {}
 
-    FileSystemDumpEntry(std::string path, FileType type, Blob content = {}): path(std::move(path)), type(type), content(std::move(content)) {
+    FileSystemDumpEntry(std::string_view path, FileType type, Blob content = {}): path(path), type(type), content(std::move(content)) {
         assert(type == FILE_REGULAR || type == FILE_DIRECTORY);
         assert(content.empty() || type == FILE_REGULAR);
     }
 
-    FileSystemDumpEntry(std::string path, FileType type, std::string content) : path(std::move(path)), type(type) {
+    FileSystemDumpEntry(std::string_view path, FileType type, std::string content) : path(path), type(type) {
         assert(type == FILE_REGULAR || type == FILE_DIRECTORY);
         assert(content.empty() || type == FILE_REGULAR);