diff --git a/src/miral/config_file.cpp b/src/miral/config_file.cpp index de9bef0d8c2..914bfd0785f 100644 --- a/src/miral/config_file.cpp +++ b/src/miral/config_file.cpp @@ -66,11 +66,11 @@ auto watch_descriptor(mir::Fd const& inotify_fd, std::optional const& path return inotify_add_watch(inotify_fd, path.value().c_str(), IN_CLOSE_WRITE | IN_CREATE | IN_MOVED_TO); } -class Watcher +class Watcher : public std::enable_shared_from_this { public: using Loader = miral::ConfigFile::Loader; - Watcher(miral::MirRunner& runner, path file, miral::ConfigFile::Loader load_config); + Watcher(path file, miral::ConfigFile::Loader load_config); mir::Fd const inotify_fd; Loader const load_config; @@ -79,6 +79,8 @@ class Watcher std::optional const directory_watch_descriptor; void register_handler(miral::MirRunner& runner); + void handler(int) const; + std::unique_ptr fd_handle; }; } @@ -89,18 +91,16 @@ class miral::ConfigFile::Self Self(MirRunner& runner, path file, Mode mode, Loader load_config); private: - std::unique_ptr watcher; + std::shared_ptr watcher; }; -Watcher::Watcher(miral::MirRunner& runner, path file, miral::ConfigFile::Loader load_config) : +Watcher::Watcher(path file, miral::ConfigFile::Loader load_config) : inotify_fd{inotify_init1(IN_CLOEXEC)}, load_config{load_config}, filename{file.filename()}, directory{config_directory(file)}, directory_watch_descriptor{watch_descriptor(inotify_fd, directory)} { - register_handler(runner); - if (directory_watch_descriptor.has_value()) { mir::log_debug("Monitoring %s for configuration changes", (directory.value()/filename).c_str()); @@ -152,7 +152,8 @@ miral::ConfigFile::Self::Self(MirRunner& runner, path file, Mode mode, Loader lo break; case Mode::reload_on_change: - watcher = std::make_unique(runner, file, std::move(load_config)); + watcher = std::make_shared(file, std::move(load_config)); + watcher->register_handler(runner); break; } } @@ -168,54 +169,67 @@ void Watcher::register_handler(miral::MirRunner& runner) { if (directory_watch_descriptor) { - fd_handle = runner.register_fd_handler(inotify_fd, [this] (int) + auto const weak_this = weak_from_this(); + fd_handle = runner.register_fd_handler(inotify_fd, [weak_this] (int fd) { - static size_t const sizeof_inotify_event = sizeof(inotify_event); - - // A union ensures buffer is aligned for inotify_event - union + if (auto const strong_this = weak_this.lock()) { - char buffer[sizeof_inotify_event + NAME_MAX + 1]; - inotify_event unused [[maybe_unused]]; - } inotify_magic; - - auto const readsize = read(inotify_fd, &inotify_magic, sizeof(inotify_magic)); - if (readsize < static_cast(sizeof_inotify_event)) + strong_this->handler(fd); + } + else { - return; + mir::log_debug("Watcher has been removed, but handler invoked"); } + }); + } +} + +void Watcher::handler(int) const +{ + static size_t const sizeof_inotify_event = sizeof(inotify_event); + + // A union ensures buffer is aligned for inotify_event + union + { + char buffer[sizeof_inotify_event + NAME_MAX + 1]; + inotify_event unused [[maybe_unused]]; + } inotify_magic; + + auto const readsize = read(inotify_fd, &inotify_magic, sizeof(inotify_magic)); + if (readsize < static_cast(sizeof_inotify_event)) + { + return; + } - auto raw_buffer = inotify_magic.buffer; - while (raw_buffer != inotify_magic.buffer + readsize) + auto raw_buffer = inotify_magic.buffer; + while (raw_buffer != inotify_magic.buffer + readsize) + { + // This is safe because inotify_magic.buffer is aligned and event.len includes padding for alignment + auto& event = reinterpret_cast(*raw_buffer); + if (event.mask & (IN_CLOSE_WRITE | IN_MOVED_TO) && event.wd == directory_watch_descriptor.value()) + try { - // This is safe because inotify_magic.buffer is aligned and event.len includes padding for alignment - auto& event = reinterpret_cast(*raw_buffer); - if (event.mask & (IN_CLOSE_WRITE | IN_MOVED_TO) && event.wd == directory_watch_descriptor.value()) - try + if (event.name == filename) { - if (event.name == filename) + auto const& file = directory.value() / filename; + + if (std::ifstream config_file{file}) { - auto const& file = directory.value() / filename; - - if (std::ifstream config_file{file}) - { - load_config(config_file, file); - mir::log_debug("(Re)loaded %s", file.c_str()); - } - else - { - mir::log_debug("Failed to open %s", file.c_str()); - } + load_config(config_file, file); + mir::log_debug("(Re)loaded %s", file.c_str()); + } + else + { + mir::log_debug("Failed to open %s", file.c_str()); } } - catch (...) - { - mir::log(mir::logging::Severity::warning, MIR_LOG_COMPONENT, std::current_exception(), - "Failed to reload configuration"); - } - - raw_buffer += sizeof_inotify_event+event.len; } - }); + catch (...) + { + mir::log(mir::logging::Severity::warning, MIR_LOG_COMPONENT, std::current_exception(), + "Failed to reload configuration"); + } + + raw_buffer += sizeof_inotify_event+event.len; } } diff --git a/tests/miral/config_file.cpp b/tests/miral/config_file.cpp index f56bd6ad474..a532dcd33d2 100644 --- a/tests/miral/config_file.cpp +++ b/tests/miral/config_file.cpp @@ -66,7 +66,7 @@ class PendingLoad bool pending_loads = false; }; -struct TestConfigFile : miral::TestServer, PendingLoad +struct TestConfigFile : PendingLoad, miral::TestServer { TestConfigFile(); MOCK_METHOD(void, load, (std::istream& in, std::filesystem::path path), ()); @@ -92,6 +92,12 @@ struct TestConfigFile : miral::TestServer, PendingLoad miral::TestServer::SetUp(); ON_CALL(*this, load(testing::_, testing::_)).WillByDefault([this]{ notify_load(); }); } + + void TearDown() override + { + reloading_config_file.reset(); + miral::TestServer::TearDown(); + } }; char const* const home = "/tmp/test_reloading_config_file/home";