diff --git a/src/bluez/horipad_steam/horipad_steam.cc b/src/bluez/horipad_steam/horipad_steam.cc index 17b2bd8..0187a6a 100644 --- a/src/bluez/horipad_steam/horipad_steam.cc +++ b/src/bluez/horipad_steam/horipad_steam.cc @@ -45,7 +45,6 @@ HoripadSteam::HoripadSteam(sdbus::IConnection& connection) sub_system ? sub_system : ""); if (std::strcmp(sub_system, "hidraw") == 0) { if (std::strcmp(action, "remove") == 0) { - std::scoped_lock reader_lock(input_reader_mutex_); if (input_reader_) { input_reader_->stop(); input_reader_.reset(); @@ -67,7 +66,6 @@ HoripadSteam::HoripadSteam(sdbus::IConnection& connection) } HoripadSteam::~HoripadSteam() { - stop(); unregisterProxy(); } @@ -82,7 +80,6 @@ void HoripadSteam::onInterfacesAdded( continue; } if (interface == org::bluez::Adapter1_proxy::INTERFACE_NAME) { - std::scoped_lock lock(adapters_mutex_); if (!adapters_.contains(objectPath)) { if (resource_limits::IsAtCapacity(adapters_.size(), resource_limits::kMaxAdapters)) { @@ -122,7 +119,6 @@ void HoripadSteam::onInterfacesAdded( std::string hidraw_device_key; { - std::scoped_lock lock(devices_mutex_); if (devices_.contains(objectPath)) { continue; } @@ -156,7 +152,6 @@ void HoripadSteam::onInterfacesAdded( if (const std::string hidraw_device = FindHidDevice(hidraw_device_key); !hidraw_device.empty()) { LOG_INFO("Adding hidraw device: {}", hidraw_device_key); - std::scoped_lock reader_lock(input_reader_mutex_); if (!input_reader_) { input_reader_ = std::make_unique(hidraw_device); input_reader_->start(); @@ -164,7 +159,6 @@ void HoripadSteam::onInterfacesAdded( } } } else if (interface == org::bluez::Input1_proxy::INTERFACE_NAME) { - std::lock_guard lock(input1_mutex_); if (!input1_.contains(objectPath)) { if (resource_limits::IsAtCapacity(input1_.size(), resource_limits::kMaxInputEntries)) { @@ -187,19 +181,16 @@ void HoripadSteam::onInterfacesRemoved( const std::vector& interfaces) { for (const auto& interface : interfaces) { if (interface == org::bluez::Adapter1_proxy::INTERFACE_NAME) { - std::scoped_lock lock(adapters_mutex_); if (adapters_.contains(objectPath)) { adapters_[objectPath].reset(); adapters_.erase(objectPath); } } else if (interface == org::bluez::Device1_proxy::INTERFACE_NAME) { - std::scoped_lock devices_lock(devices_mutex_); if (devices_.contains(objectPath)) { devices_[objectPath].reset(); devices_.erase(objectPath); } } else if (interface == org::bluez::Input1_proxy::INTERFACE_NAME) { - std::lock_guard lock(input1_mutex_); if (input1_.contains(objectPath)) { input1_[objectPath].reset(); input1_.erase(objectPath); diff --git a/src/bluez/horipad_steam/horipad_steam.h b/src/bluez/horipad_steam/horipad_steam.h index 7ad88d0..3894aea 100644 --- a/src/bluez/horipad_steam/horipad_steam.h +++ b/src/bluez/horipad_steam/horipad_steam.h @@ -45,22 +45,12 @@ class HoripadSteam final static constexpr auto INTROSPECTABLE_INTERFACE_NAME = "org.freedesktop.DBus.Introspectable"; - // Locking policy: avoid nested locking where possible. - // If nested locking is required, always acquire in this order: - // adapters_mutex_ -> devices_mutex_ -> input1_mutex_. - std::mutex adapters_mutex_; + // All of the state below is only ever touched from the single EventLoop + // thread: the D-Bus object-manager callbacks and the UdevMonitor callback + // are both dispatched by the loop, so no locking is required. std::map> adapters_; - - std::mutex devices_mutex_; std::map> devices_; - - std::mutex input1_mutex_; std::map> input1_; - - // Guards input_reader_, which is created/started from onInterfacesAdded - // (D-Bus event-loop or main thread) and stopped/reset from the udev monitor - // worker thread. - std::mutex input_reader_mutex_; std::unique_ptr input_reader_; void onInterfacesAdded( diff --git a/src/bluez/horipad_steam/main.cc b/src/bluez/horipad_steam/main.cc index c775ee3..f473439 100644 --- a/src/bluez/horipad_steam/main.cc +++ b/src/bluez/horipad_steam/main.cc @@ -12,34 +12,36 @@ // See the License for the specific language governing permissions and // limitations under the License. -#include "../../utils/signal_handler.h" +#include + +#include "../../utils/event_loop.h" +#include "../../utils/signal_source.h" #include "horipad_steam.h" int main() { try { - installSignalHandlers(); + // Single-threaded loop: it drives the D-Bus connection, the udev monitor, + // and signal delivery, so every callback runs on this thread. Construct the + // SignalSource first so SIGINT/SIGTERM are blocked before any thread is + // started (e.g. spdlog's periodic flush thread) and can only be delivered + // via the loop's signalfd. + EventLoop loop; + SignalSource signals(loop); + loop.add(&signals); spdlog::set_level(spdlog::level::debug); - spdlog::flush_every(kLogFlushInterval); + spdlog::flush_every(std::chrono::seconds(5)); const auto connection = sdbus::createSystemBusConnection(); - connection->enterEventLoopAsync(); HoripadSteam client(*connection); + loop.add(&client); // HoripadSteam is a UdevMonitor (an EventSource) LOG_INFO("HoriPad Steam client running - Press Ctrl+C to exit"); - // Monitor loop with shared connection health timing defaults - auto result = monitorLoop(*connection); - - if (result) { - LOG_ERROR("Exiting due to: {}", *result); - } else { - LOG_INFO("Shutting down..."); - } - - connection->leaveEventLoop(); - return result ? 1 : 0; + const int rc = loop.run(*connection); + LOG_INFO("Shutting down..."); + return rc; } catch (const sdbus::Error& e) { LOG_ERROR("D-Bus error: {} - {}", e.getName(), e.getMessage()); diff --git a/src/bluez/ps5_dual_sense/dual_sense.cc b/src/bluez/ps5_dual_sense/dual_sense.cc index eaa7ad1..622b8fa 100644 --- a/src/bluez/ps5_dual_sense/dual_sense.cc +++ b/src/bluez/ps5_dual_sense/dual_sense.cc @@ -44,7 +44,6 @@ DualSense::DualSense(sdbus::IConnection& connection) sub_system ? sub_system : ""); if (std::strcmp(sub_system, "hidraw") == 0) { if (std::strcmp(action, "remove") == 0) { - std::scoped_lock reader_lock(input_reader_mutex_); if (input_reader_) { input_reader_->stop(); input_reader_.reset(); @@ -66,7 +65,6 @@ DualSense::DualSense(sdbus::IConnection& connection) } DualSense::~DualSense() { - stop(); unregisterProxy(); } @@ -81,7 +79,6 @@ void DualSense::onInterfacesAdded( continue; } if (interface == org::bluez::Adapter1_proxy::INTERFACE_NAME) { - std::scoped_lock lock(adapters_mutex_); if (!adapters_.contains(objectPath)) { if (resource_limits::IsAtCapacity(adapters_.size(), resource_limits::kMaxAdapters)) { @@ -120,7 +117,6 @@ void DualSense::onInterfacesAdded( std::string power_path_to_add; std::string hidraw_device_key; { - std::scoped_lock lock(devices_mutex_); if (devices_.contains(objectPath)) { continue; } @@ -158,7 +154,6 @@ void DualSense::onInterfacesAdded( if (const std::string hidraw_device = FindHidDevice(hidraw_device_key); !hidraw_device.empty()) { LOG_INFO("Adding hidraw device: {}", hidraw_device_key); - std::scoped_lock reader_lock(input_reader_mutex_); if (!input_reader_) { input_reader_ = std::make_unique(hidraw_device); input_reader_->start(); @@ -166,10 +161,7 @@ void DualSense::onInterfacesAdded( } } - // Avoid nested locking with devices_mutex_ + - // upower_display_devices_mutex_. if (!power_path_to_add.empty()) { - std::scoped_lock power_lock(upower_display_devices_mutex_); if (!upower_clients_.contains(power_path_to_add)) { if (resource_limits::IsAtCapacity( upower_clients_.size(), resource_limits::kMaxUPowerClients)) { @@ -185,7 +177,6 @@ void DualSense::onInterfacesAdded( } } } else if (interface == org::bluez::Input1_proxy::INTERFACE_NAME) { - std::lock_guard lock(input1_mutex_); if (!input1_.contains(objectPath)) { if (resource_limits::IsAtCapacity(input1_.size(), resource_limits::kMaxInputEntries)) { @@ -208,7 +199,6 @@ void DualSense::onInterfacesRemoved( const std::vector& interfaces) { for (const auto& interface : interfaces) { if (interface == org::bluez::Adapter1_proxy::INTERFACE_NAME) { - std::scoped_lock lock(adapters_mutex_); if (adapters_.contains(objectPath)) { adapters_[objectPath].reset(); adapters_.erase(objectPath); @@ -216,7 +206,6 @@ void DualSense::onInterfacesRemoved( } else if (interface == org::bluez::Device1_proxy::INTERFACE_NAME) { std::string power_path_to_remove; { - std::scoped_lock devices_lock(devices_mutex_); if (devices_.contains(objectPath)) { auto& device = devices_[objectPath]; if (auto props = device->GetProperties(); @@ -233,7 +222,6 @@ void DualSense::onInterfacesRemoved( } if (!power_path_to_remove.empty()) { - std::scoped_lock power_lock(upower_display_devices_mutex_); if (upower_clients_.contains(power_path_to_remove)) { LOG_INFO("[Remove] UPower Display Device: {}", power_path_to_remove); auto& power_device = upower_clients_[power_path_to_remove]; @@ -242,7 +230,6 @@ void DualSense::onInterfacesRemoved( } } } else if (interface == org::bluez::Input1_proxy::INTERFACE_NAME) { - std::lock_guard lock(input1_mutex_); if (input1_.contains(objectPath)) { input1_[objectPath].reset(); input1_.erase(objectPath); diff --git a/src/bluez/ps5_dual_sense/dual_sense.h b/src/bluez/ps5_dual_sense/dual_sense.h index 3034273..98166b0 100644 --- a/src/bluez/ps5_dual_sense/dual_sense.h +++ b/src/bluez/ps5_dual_sense/dual_sense.h @@ -46,26 +46,13 @@ class DualSense final static constexpr auto INTROSPECTABLE_INTERFACE_NAME = "org.freedesktop.DBus.Introspectable"; - // Locking policy: avoid nested locking where possible. - // If nested locking is required, always acquire in this order: - // adapters_mutex_ -> devices_mutex_ -> input1_mutex_ -> - // upower_display_devices_mutex_. - std::mutex adapters_mutex_; + // All of the state below is only ever touched from the single EventLoop + // thread: the D-Bus object-manager callbacks and the UdevMonitor callback + // are both dispatched by the loop, so no locking is required. std::map> adapters_; - - std::mutex devices_mutex_; std::map> devices_; - - std::mutex input1_mutex_; std::map> input1_; - - std::mutex upower_display_devices_mutex_; std::map> upower_clients_; - - // Guards input_reader_, which is created/started from onInterfacesAdded - // (D-Bus event-loop or main thread) and stopped/reset from the udev monitor - // worker thread. - std::mutex input_reader_mutex_; std::unique_ptr input_reader_; void onInterfacesAdded( diff --git a/src/bluez/ps5_dual_sense/main.cc b/src/bluez/ps5_dual_sense/main.cc index 883e662..1179f71 100644 --- a/src/bluez/ps5_dual_sense/main.cc +++ b/src/bluez/ps5_dual_sense/main.cc @@ -12,34 +12,36 @@ // See the License for the specific language governing permissions and // limitations under the License. -#include "../../utils/signal_handler.h" +#include + +#include "../../utils/event_loop.h" +#include "../../utils/signal_source.h" #include "dual_sense.h" int main() { try { - installSignalHandlers(); + // Single-threaded loop: it drives the D-Bus connection, the udev monitor, + // and signal delivery, so every callback runs on this thread. Construct the + // SignalSource first so SIGINT/SIGTERM are blocked before any thread is + // started (e.g. spdlog's periodic flush thread) and can only be delivered + // via the loop's signalfd. + EventLoop loop; + SignalSource signals(loop); + loop.add(&signals); spdlog::set_level(spdlog::level::debug); - spdlog::flush_every(kLogFlushInterval); + spdlog::flush_every(std::chrono::seconds(5)); const auto connection = sdbus::createSystemBusConnection(); - connection->enterEventLoopAsync(); DualSense client(*connection); + loop.add(&client); // DualSense is a UdevMonitor (an EventSource) LOG_INFO("PS5 DualSense client running - Press Ctrl+C to exit"); - // Monitor loop with shared connection health timing defaults - auto result = monitorLoop(*connection); - - if (result) { - LOG_ERROR("Exiting due to: {}", *result); - } else { - LOG_INFO("Shutting down..."); - } - - connection->leaveEventLoop(); - return result ? 1 : 0; + const int rc = loop.run(*connection); + LOG_INFO("Shutting down..."); + return rc; } catch (const sdbus::Error& e) { LOG_ERROR("D-Bus error: {} - {}", e.getName(), e.getMessage()); diff --git a/src/bluez/udev_monitor.hpp b/src/bluez/udev_monitor.hpp index 6e9a7bb..656fa5b 100644 --- a/src/bluez/udev_monitor.hpp +++ b/src/bluez/udev_monitor.hpp @@ -15,20 +15,15 @@ #ifndef SRC_BLUEZ_UDEV_MONITOR_HPP_ #define SRC_BLUEZ_UDEV_MONITOR_HPP_ -#include -#include -#include #include #include #include -#include #include #include #include -#include -#include +#include "../utils/event_loop.h" #include "../utils/logging.h" // --------------------------------------------------------------------------- @@ -49,195 +44,77 @@ struct UdevDeviceDeleter { }; using UdevDevicePtr = std::unique_ptr; -/// Minimal RAII wrapper for an epoll file descriptor. -struct EpollFd { - explicit EpollFd(const int fd) noexcept : fd_(fd) {} - ~EpollFd() { - if (fd_ >= 0) - ::close(fd_); - } - EpollFd(const EpollFd&) = delete; - EpollFd& operator=(const EpollFd&) = delete; - [[nodiscard]] bool valid() const noexcept { return fd_ >= 0; } - [[nodiscard]] int get() const noexcept { return fd_; } - - private: - int fd_; -}; - -class UdevMonitor { +/// Monitors udev for device add/remove events as an EventSource. The netlink +/// socket fd is polled by an EventLoop; dispatch() drains the pending device +/// events and invokes the callback for each. Single-threaded: no internal +/// thread, so device callbacks run on the same loop thread as the D-Bus +/// callbacks and need no cross-thread synchronisation. +class UdevMonitor : public EventSource { public: - UdevMonitor(std::vector sub_systems, - const std::function& - callback) - : sub_systems_(std::move(sub_systems)), callback_(callback) { - if (pipe(pipe_fds_) == -1) { - LOG_ERROR("Failed to create pipe: {} ({})", std::strerror(errno), errno); - pipe_fds_[0] = -1; - pipe_fds_[1] = -1; - return; - } - - // Start a thread (not detached, so we can join it) - worker_thread_ = std::thread(&UdevMonitor::run, this); - } - - // Delete copy operations - this class manages a thread and file descriptor - UdevMonitor(const UdevMonitor&) = delete; - UdevMonitor& operator=(const UdevMonitor&) = delete; - - // Move operations deleted - moving a class with a running thread is complex - // and not needed for this use case. If move semantics are required in the - // future, would need to: - // 1. Stop the thread in the moved-from object - // 2. Transfer ownership of file descriptors - // 3. Start a new thread in the moved-to object - UdevMonitor(UdevMonitor&&) = delete; - UdevMonitor& operator=(UdevMonitor&&) = delete; - - virtual ~UdevMonitor() { - stop(); - - // Wait for worker thread to finish before closing pipes - if (worker_thread_.joinable()) { - worker_thread_.join(); - } - - // Now safe to close pipes - if (pipe_fds_[0] != -1) { - close(pipe_fds_[0]); - pipe_fds_[0] = -1; - } - if (pipe_fds_[1] != -1) { - close(pipe_fds_[1]); - pipe_fds_[1] = -1; - } - } - - void stop() { - // Only execute stop logic once - if (bool expected = true; - !is_running_.compare_exchange_strong(expected, false)) { - return; // Already stopped - } - - // Signal the worker thread to exit - if (pipe_fds_[1] != -1) { - if (const ssize_t wrote = write(pipe_fds_[1], "x", 1); wrote == -1) { - LOG_ERROR("Failed to write to stop pipe: {} ({})", std::strerror(errno), - errno); - } - } - } - - private: - std::vector sub_systems_; - std::atomic is_running_{true}; - int pipe_fds_[2]{-1, -1}; - std::function callback_; - std::thread worker_thread_; - - void run() { - // RAII: udev context — automatically udev_unref'd on scope exit - const UdevPtr udev(udev_new()); - if (!udev) { + UdevMonitor( + const std::vector& sub_systems, + std::function callback) + : callback_(std::move(callback)) { + udev_ = UdevPtr(udev_new()); + if (!udev_) { LOG_ERROR("Failed to create udev context"); - is_running_ = false; return; } - // RAII: udev monitor — automatically udev_monitor_unref'd on scope exit - const UdevMonitorPtr mon(udev_monitor_new_from_netlink(udev.get(), "udev")); - if (!mon) { + mon_ = UdevMonitorPtr(udev_monitor_new_from_netlink(udev_.get(), "udev")); + if (!mon_) { LOG_ERROR("Failed to create udev monitor"); - is_running_ = false; return; } - for (const auto& sub_system : sub_systems_) { - if (int res = udev_monitor_filter_add_match_subsystem_devtype( - mon.get(), sub_system.c_str(), nullptr); + for (const auto& sub_system : sub_systems) { + if (const int res = udev_monitor_filter_add_match_subsystem_devtype( + mon_.get(), sub_system.c_str(), nullptr); res != 0) { LOG_ERROR( "udev_monitor_filter_add_match_subsystem_devtype failed on {} = {}", sub_system, res); } } - udev_monitor_enable_receiving(mon.get()); - const auto fd = udev_monitor_get_fd(mon.get()); - - // RAII: epoll fd — automatically closed on scope exit - const EpollFd epoll_fd(epoll_create1(0)); - if (!epoll_fd.valid()) { - LOG_ERROR("Failed to create epoll: {} ({})", std::strerror(errno), errno); - is_running_ = false; - return; - } + udev_monitor_enable_receiving(mon_.get()); + fd_ = udev_monitor_get_fd(mon_.get()); + } - epoll_event ev{}; - ev.events = EPOLLIN; - ev.data.fd = fd; - if (epoll_ctl(epoll_fd.get(), EPOLL_CTL_ADD, fd, &ev) == -1) { - LOG_ERROR("Failed to add udev fd to epoll: {} ({})", std::strerror(errno), - errno); - is_running_ = false; - return; - } + UdevMonitor(const UdevMonitor&) = delete; + UdevMonitor& operator=(const UdevMonitor&) = delete; + UdevMonitor(UdevMonitor&&) = delete; + UdevMonitor& operator=(UdevMonitor&&) = delete; + ~UdevMonitor() override = default; - ev.data.fd = pipe_fds_[0]; - if (epoll_ctl(epoll_fd.get(), EPOLL_CTL_ADD, pipe_fds_[0], &ev) == -1) { - LOG_ERROR("Failed to add pipe fd to epoll: {} ({})", std::strerror(errno), - errno); - is_running_ = false; - return; - } + [[nodiscard]] int fd() const override { return fd_; } - while (is_running_) { - epoll_event events[2]; - const int triggered_event_count = - epoll_wait(epoll_fd.get(), events, 2, -1); - if (triggered_event_count == -1) { - if (errno == EINTR) { - continue; // Interrupted by signal, retry - } - LOG_ERROR("epoll_wait failed: {} ({})", std::strerror(errno), errno); - break; + void dispatch(short /*revents*/) override { + // Drain every device event queued on the netlink socket. + while (UdevDevicePtr dev{udev_monitor_receive_device(mon_.get())}) { + if (!callback_) { + continue; } - - for (int n = 0; n < triggered_event_count; ++n) { - if (events[n].data.fd == pipe_fds_[0]) { - LOG_INFO("Pipe was written to, exiting the loop"); - is_running_ = false; - break; - } - - if (events[n].data.fd == fd) { - // RAII: device handle — automatically udev_device_unref'd - if (UdevDevicePtr dev(udev_monitor_receive_device(mon.get())); dev) { - if (callback_) { - const char* action = udev_device_get_action(dev.get()); - const char* devnode = udev_device_get_devnode(dev.get()); - const char* subsystem = udev_device_get_subsystem(dev.get()); - - if (action && subsystem) { - callback_(action, devnode, subsystem); - } else { - LOG_DEBUG( - "Skipping callback for device with missing properties: " - "action={}, devnode={}, subsystem={}", - action ? action : "null", devnode ? devnode : "null", - subsystem ? subsystem : "null"); - } - } - // dev destructor calls udev_device_unref automatically - } - } + const char* action = udev_device_get_action(dev.get()); + const char* devnode = udev_device_get_devnode(dev.get()); + const char* subsystem = udev_device_get_subsystem(dev.get()); + + if (action && subsystem) { + callback_(action, devnode, subsystem); + } else { + LOG_DEBUG( + "Skipping callback for device with missing properties: " + "action={}, devnode={}, subsystem={}", + action ? action : "null", devnode ? devnode : "null", + subsystem ? subsystem : "null"); } } - - // All resources (epoll_fd, mon, udev) are released by their destructors. - LOG_DEBUG("UdevMonitor worker thread exiting"); } + + private: + std::function callback_; + UdevPtr udev_; + UdevMonitorPtr mon_; + int fd_ = -1; }; #endif // SRC_BLUEZ_UDEV_MONITOR_HPP_ diff --git a/src/bluez/xbox_controller/main.cc b/src/bluez/xbox_controller/main.cc index 6fcc231..eabd84d 100644 --- a/src/bluez/xbox_controller/main.cc +++ b/src/bluez/xbox_controller/main.cc @@ -12,30 +12,28 @@ // See the License for the specific language governing permissions and // limitations under the License. -#include "../../utils/signal_handler.h" +#include "../../utils/event_loop.h" +#include "../../utils/signal_source.h" #include "xbox_controller.h" int main() { try { - installSignalHandlers(); - const auto connection = sdbus::createSystemBusConnection(); - connection->enterEventLoopAsync(); + + // Single-threaded loop: it drives the D-Bus connection, the udev monitor, + // and signal delivery, so every callback runs on this thread. + EventLoop loop; + SignalSource signals(loop); + loop.add(&signals); XboxController client(*connection); + loop.add(&client); // XboxController is a UdevMonitor (an EventSource) LOG_INFO("Xbox controller client running - Press Ctrl+C to exit"); - auto result = monitorLoop(*connection); - - if (result) { - LOG_ERROR("Exiting due to: {}", *result); - } else { - LOG_INFO("Shutting down..."); - } - - connection->leaveEventLoop(); - return result ? 1 : 0; + const int rc = loop.run(*connection); + LOG_INFO("Shutting down..."); + return rc; } catch (const sdbus::Error& e) { LOG_ERROR("D-Bus error: {} - {}", e.getName(), e.getMessage()); diff --git a/src/bluez/xbox_controller/xbox_controller.cc b/src/bluez/xbox_controller/xbox_controller.cc index 827251c..ee23853 100644 --- a/src/bluez/xbox_controller/xbox_controller.cc +++ b/src/bluez/xbox_controller/xbox_controller.cc @@ -46,7 +46,6 @@ XboxController::XboxController(sdbus::IConnection& connection) sub_system ? sub_system : ""); if (std::strcmp(sub_system, "hidraw") == 0) { if (std::strcmp(action, "remove") == 0) { - std::scoped_lock reader_lock(input_reader_mutex_); if (input_reader_) { input_reader_->stop(); input_reader_.reset(); @@ -67,7 +66,6 @@ XboxController::XboxController(sdbus::IConnection& connection) } XboxController::~XboxController() { - stop(); unregisterProxy(); } @@ -82,7 +80,6 @@ void XboxController::onInterfacesAdded( continue; } if (interface == org::bluez::Adapter1_proxy::INTERFACE_NAME) { - std::scoped_lock lock(adapters_mutex_); if (!adapters_.contains(objectPath)) { if (resource_limits::IsAtCapacity(adapters_.size(), resource_limits::kMaxAdapters)) { @@ -121,7 +118,6 @@ void XboxController::onInterfacesAdded( std::string power_path_to_add; std::string hidraw_device_key; { - std::scoped_lock lock(devices_mutex_); if (devices_.contains(objectPath)) { continue; } @@ -158,7 +154,6 @@ void XboxController::onInterfacesAdded( if (const std::string hidraw_device = FindHidDevice(hidraw_device_key); !hidraw_device.empty()) { LOG_INFO("Adding hidraw device: {}", hidraw_device_key); - std::scoped_lock reader_lock(input_reader_mutex_); if (!input_reader_) { input_reader_ = std::make_unique(hidraw_device); input_reader_->start(); @@ -166,10 +161,7 @@ void XboxController::onInterfacesAdded( } } - // Avoid nested locking with devices_mutex_ + - // upower_display_devices_mutex_. if (!power_path_to_add.empty()) { - std::scoped_lock power_lock(upower_display_devices_mutex_); if (!upower_clients_.contains(power_path_to_add)) { if (resource_limits::IsAtCapacity( upower_clients_.size(), resource_limits::kMaxUPowerClients)) { @@ -185,7 +177,6 @@ void XboxController::onInterfacesAdded( } } } else if (interface == org::bluez::Input1_proxy::INTERFACE_NAME) { - std::lock_guard lock(input1_mutex_); if (!input1_.contains(objectPath)) { if (resource_limits::IsAtCapacity(input1_.size(), resource_limits::kMaxInputEntries)) { @@ -208,7 +199,6 @@ void XboxController::onInterfacesRemoved( const std::vector& interfaces) { for (const auto& interface : interfaces) { if (interface == org::bluez::Adapter1_proxy::INTERFACE_NAME) { - std::scoped_lock lock(adapters_mutex_); if (adapters_.contains(objectPath)) { adapters_[objectPath].reset(); adapters_.erase(objectPath); @@ -216,7 +206,6 @@ void XboxController::onInterfacesRemoved( } else if (interface == org::bluez::Device1_proxy::INTERFACE_NAME) { std::string power_path_to_remove; { - std::scoped_lock devices_lock(devices_mutex_); if (devices_.contains(objectPath)) { auto& device = devices_[objectPath]; if (auto props = device->GetProperties(); @@ -235,7 +224,6 @@ void XboxController::onInterfacesRemoved( } if (!power_path_to_remove.empty()) { - std::scoped_lock power_lock(upower_display_devices_mutex_); if (upower_clients_.contains(power_path_to_remove)) { LOG_INFO("[Remove] UPower Display Device: {}", power_path_to_remove); auto& power_device = upower_clients_[power_path_to_remove]; @@ -244,7 +232,6 @@ void XboxController::onInterfacesRemoved( } } } else if (interface == org::bluez::Input1_proxy::INTERFACE_NAME) { - std::lock_guard lock(input1_mutex_); if (input1_.contains(objectPath)) { input1_[objectPath].reset(); input1_.erase(objectPath); diff --git a/src/bluez/xbox_controller/xbox_controller.h b/src/bluez/xbox_controller/xbox_controller.h index b4f9afb..78e6d02 100644 --- a/src/bluez/xbox_controller/xbox_controller.h +++ b/src/bluez/xbox_controller/xbox_controller.h @@ -47,26 +47,13 @@ class XboxController final static constexpr auto INTROSPECTABLE_INTERFACE_NAME = "org.freedesktop.DBus.Introspectable"; - // Locking policy: avoid nested locking where possible. - // If nested locking is required, always acquire in this order: - // adapters_mutex_ -> devices_mutex_ -> input1_mutex_ -> - // upower_display_devices_mutex_. - std::mutex adapters_mutex_; + // All of the state below is only ever touched from the single EventLoop + // thread: the D-Bus object-manager callbacks and the UdevMonitor callback + // are both dispatched by the loop, so no locking is required. std::map> adapters_; - - std::mutex devices_mutex_; std::map> devices_; - - std::mutex input1_mutex_; std::map> input1_; - - std::mutex upower_display_devices_mutex_; std::map> upower_clients_; - - // Guards input_reader_, which is created/started from onInterfacesAdded - // (D-Bus event-loop or main thread) and stopped/reset from the udev monitor - // worker thread. - std::mutex input_reader_mutex_; std::unique_ptr input_reader_; void onInterfacesAdded( diff --git a/src/utils/signal_source.h b/src/utils/signal_source.h new file mode 100644 index 0000000..29a686d --- /dev/null +++ b/src/utils/signal_source.h @@ -0,0 +1,70 @@ +// Copyright (c) 2026 Joel Winarske +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#ifndef SRC_UTILS_SIGNAL_SOURCE_H +#define SRC_UTILS_SIGNAL_SOURCE_H + +#include +#include +#include +#include + +#include +#include + +#include "event_loop.h" +#include "logging.h" +#include "unique_fd.h" + +/// Turns POSIX signals into an EventLoop stop(). The signals are blocked +/// process-wide and delivered through a signalfd, so they are handled +/// synchronously on the loop thread (via dispatch()) rather than through an +/// async signal handler. Construct this before starting any worker threads so +/// they inherit the blocked mask and never take the default disposition. +class SignalSource final : public EventSource { + public: + explicit SignalSource(EventLoop& loop, + const std::initializer_list signals = {SIGINT, + SIGTERM}) + : loop_(loop) { + sigemptyset(&mask_); + for (const int signum : signals) { + sigaddset(&mask_, signum); + } + if (sigprocmask(SIG_BLOCK, &mask_, nullptr) < 0) { + LOG_ERROR("SignalSource: sigprocmask failed: {}", strerror(errno)); + } + fd_ = UniqueFd(::signalfd(-1, &mask_, SFD_CLOEXEC | SFD_NONBLOCK)); + if (!fd_.valid()) { + LOG_ERROR("SignalSource: signalfd failed: {}", strerror(errno)); + } + } + + [[nodiscard]] int fd() const override { return fd_.get(); } + + void dispatch(short /*revents*/) override { + signalfd_siginfo info{}; + while (::read(fd_.get(), &info, sizeof(info)) == sizeof(info)) { + LOG_INFO("SignalSource: received signal {}, stopping", info.ssi_signo); + } + loop_.stop(0); + } + + private: + EventLoop& loop_; + sigset_t mask_{}; + UniqueFd fd_; +}; + +#endif // SRC_UTILS_SIGNAL_SOURCE_H