From 40f4dc9ecf26a8d9cd8d33b9bc88ff4dd6b3e508 Mon Sep 17 00:00:00 2001 From: Aleksei Bavshin Date: Mon, 1 Feb 2021 18:50:45 -0800 Subject: [PATCH 1/6] fix(rfkill): accept events larger than v1 event size Kernel 5.11 added one more field to the `struct rfkill_event` and broke unnecessarily strict check in `rfkill.cpp`. According to `linux/rfkill.h`, we must accept events at least as large as v1 event size and should be prepared to get additional fields at the end of a v1 event structure. --- src/util/rfkill.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/rfkill.cpp b/src/util/rfkill.cpp index 82d29e91..e968ca18 100644 --- a/src/util/rfkill.cpp +++ b/src/util/rfkill.cpp @@ -61,7 +61,7 @@ void waybar::util::Rfkill::waitForEvent() { break; } - if (len != RFKILL_EVENT_SIZE_V1) { + if (len < RFKILL_EVENT_SIZE_V1) { throw std::runtime_error("Wrong size of RFKILL event"); continue; } From 38c29fc2426557b43af91e7557c40f7880aaa0b8 Mon Sep 17 00:00:00 2001 From: Aleksei Bavshin Date: Tue, 2 Feb 2021 19:17:06 -0800 Subject: [PATCH 2/6] refactor(rfkill): poll rfkill events from Glib main loop Open rfkill device only once per module. Remove rfkill threads and use `Glib::signal_io` as a more efficient way to poll the rfkill device. Handle runtime errors from rfkill and stop polling of the device instead of crashing waybar. --- include/modules/bluetooth.hpp | 16 ++++---- include/modules/network.hpp | 2 - include/util/rfkill.hpp | 15 +++++-- src/modules/bluetooth.cpp | 8 +--- src/modules/network.cpp | 17 ++++---- src/util/rfkill.cpp | 73 ++++++++++++++++++----------------- 6 files changed, 64 insertions(+), 67 deletions(-) diff --git a/include/modules/bluetooth.hpp b/include/modules/bluetooth.hpp index 04c213da..716df0eb 100644 --- a/include/modules/bluetooth.hpp +++ b/include/modules/bluetooth.hpp @@ -1,11 +1,11 @@ #pragma once -#include -#include "ALabel.hpp" - #include -#include "util/sleeper_thread.hpp" +#include + +#include "ALabel.hpp" #include "util/rfkill.hpp" +#include "util/sleeper_thread.hpp" namespace waybar::modules { @@ -16,11 +16,9 @@ class Bluetooth : public ALabel { auto update() -> void; private: - std::string status_; - util::SleeperThread thread_; - util::SleeperThread intervall_thread_; - - util::Rfkill rfkill_; + std::string status_; + util::SleeperThread thread_; + util::Rfkill rfkill_; }; } // namespace waybar::modules diff --git a/include/modules/network.hpp b/include/modules/network.hpp index c02d3c5e..539f4583 100644 --- a/include/modules/network.hpp +++ b/include/modules/network.hpp @@ -75,8 +75,6 @@ class Network : public ALabel { util::SleeperThread thread_; util::SleeperThread thread_timer_; #ifdef WANT_RFKILL - util::SleeperThread thread_rfkill_; - util::Rfkill rfkill_; #endif }; diff --git a/include/util/rfkill.hpp b/include/util/rfkill.hpp index ac3d406b..5d519cae 100644 --- a/include/util/rfkill.hpp +++ b/include/util/rfkill.hpp @@ -1,19 +1,26 @@ #pragma once +#include #include +#include +#include namespace waybar::util { -class Rfkill { +class Rfkill : public sigc::trackable { public: Rfkill(enum rfkill_type rfkill_type); - ~Rfkill() = default; - void waitForEvent(); + ~Rfkill(); bool getState() const; + sigc::signal on_update; + private: enum rfkill_type rfkill_type_; - int state_ = 0; + bool state_ = false; + int fd_ = -1; + + bool on_event(Glib::IOCondition cond); }; } // namespace waybar::util diff --git a/src/modules/bluetooth.cpp b/src/modules/bluetooth.cpp index b390976a..9939cc19 100644 --- a/src/modules/bluetooth.cpp +++ b/src/modules/bluetooth.cpp @@ -1,17 +1,11 @@ #include "modules/bluetooth.hpp" -#include "util/rfkill.hpp" -#include -#include waybar::modules::Bluetooth::Bluetooth(const std::string& id, const Json::Value& config) : ALabel(config, "bluetooth", id, "{icon}", 10), status_("disabled"), rfkill_{RFKILL_TYPE_BLUETOOTH} { + rfkill_.on_update.connect(sigc::hide(sigc::mem_fun(*this, &Bluetooth::update))); thread_ = [this] { - dp.emit(); - rfkill_.waitForEvent(); - }; - intervall_thread_ = [this] { auto now = std::chrono::system_clock::now(); auto timeout = std::chrono::floor(now + interval_); auto diff = std::chrono::seconds(timeout.time_since_epoch().count() % interval_.count()); diff --git a/src/modules/network.cpp b/src/modules/network.cpp index 7d9da8b5..41bc9d51 100644 --- a/src/modules/network.cpp +++ b/src/modules/network.cpp @@ -212,18 +212,15 @@ void waybar::modules::Network::worker() { thread_timer_.sleep_for(interval_); }; #ifdef WANT_RFKILL - thread_rfkill_ = [this] { - rfkill_.waitForEvent(); - { - std::lock_guard lock(mutex_); - if (ifid_ > 0) { - getInfo(); - dp.emit(); - } + rfkill_.on_update.connect([this](auto &) { + std::lock_guard lock(mutex_); + if (ifid_ > 0) { + getInfo(); + dp.emit(); } - }; + }); #else - spdlog::warn("Waybar has been built without rfkill support."); + spdlog::warn("Waybar has been built without rfkill support."); #endif thread_ = [this] { std::array events{}; diff --git a/src/util/rfkill.cpp b/src/util/rfkill.cpp index e968ca18..d3eb516a 100644 --- a/src/util/rfkill.cpp +++ b/src/util/rfkill.cpp @@ -19,60 +19,63 @@ #include "util/rfkill.hpp" #include +#include #include -#include -#include +#include #include #include -#include -#include -waybar::util::Rfkill::Rfkill(const enum rfkill_type rfkill_type) : rfkill_type_(rfkill_type) {} - -void waybar::util::Rfkill::waitForEvent() { - struct rfkill_event event; - struct pollfd p; - ssize_t len; - int fd, n; - - fd = open("/dev/rfkill", O_RDONLY); - if (fd < 0) { - throw std::runtime_error("Can't open RFKILL control device"); +waybar::util::Rfkill::Rfkill(const enum rfkill_type rfkill_type) : rfkill_type_(rfkill_type) { + fd_ = open("/dev/rfkill", O_RDONLY); + if (fd_ < 0) { + spdlog::error("Can't open RFKILL control device"); return; } + int rc = fcntl(fd_, F_SETFL, O_NONBLOCK); + if (rc < 0) { + spdlog::error("Can't set RFKILL control device to non-blocking: {}", errno); + close(fd_); + fd_ = -1; + return; + } + Glib::signal_io().connect( + sigc::mem_fun(*this, &Rfkill::on_event), fd_, Glib::IO_IN | Glib::IO_ERR | Glib::IO_HUP); +} - memset(&p, 0, sizeof(p)); - p.fd = fd; - p.events = POLLIN | POLLHUP; +waybar::util::Rfkill::~Rfkill() { + if (fd_ >= 0) { + close(fd_); + } +} - while (1) { - n = poll(&p, 1, -1); - if (n < 0) { - throw std::runtime_error("Failed to poll RFKILL control device"); - break; - } +bool waybar::util::Rfkill::on_event(Glib::IOCondition cond) { + if (cond & Glib::IO_IN) { + struct rfkill_event event; + ssize_t len; - if (n == 0) continue; - - len = read(fd, &event, sizeof(event)); + len = read(fd_, &event, sizeof(event)); if (len < 0) { - throw std::runtime_error("Reading of RFKILL events failed"); - break; + spdlog::error("Reading of RFKILL events failed: {}", errno); + return false; } if (len < RFKILL_EVENT_SIZE_V1) { - throw std::runtime_error("Wrong size of RFKILL event"); - continue; + if (errno != EAGAIN) { + spdlog::error("Wrong size of RFKILL event: {}", len); + } + return true; } - if (event.type == rfkill_type_ && event.op == RFKILL_OP_CHANGE) { + if (event.type == rfkill_type_ && (event.op == RFKILL_OP_ADD || event.op == RFKILL_OP_CHANGE)) { state_ = event.soft || event.hard; - break; + on_update.emit(event); } + return true; + } else { + spdlog::error("Failed to poll RFKILL control device"); + return false; } - - close(fd); } bool waybar::util::Rfkill::getState() const { return state_; } From ecc32ddd185b112b101891200d127dc319a58ca5 Mon Sep 17 00:00:00 2001 From: Aleksei Bavshin Date: Tue, 2 Feb 2021 20:01:01 -0800 Subject: [PATCH 3/6] refactor(bluetooth): remove Bluetooth::status_ The string was always overwritten in `update()`; don't need to store temporary value in the class. --- include/modules/bluetooth.hpp | 4 ---- src/modules/bluetooth.cpp | 21 +++++++-------------- 2 files changed, 7 insertions(+), 18 deletions(-) diff --git a/include/modules/bluetooth.hpp b/include/modules/bluetooth.hpp index 716df0eb..4d7b7c84 100644 --- a/include/modules/bluetooth.hpp +++ b/include/modules/bluetooth.hpp @@ -1,8 +1,5 @@ #pragma once -#include -#include - #include "ALabel.hpp" #include "util/rfkill.hpp" #include "util/sleeper_thread.hpp" @@ -16,7 +13,6 @@ class Bluetooth : public ALabel { auto update() -> void; private: - std::string status_; util::SleeperThread thread_; util::Rfkill rfkill_; }; diff --git a/src/modules/bluetooth.cpp b/src/modules/bluetooth.cpp index 9939cc19..0df404d3 100644 --- a/src/modules/bluetooth.cpp +++ b/src/modules/bluetooth.cpp @@ -1,9 +1,9 @@ #include "modules/bluetooth.hpp" +#include + waybar::modules::Bluetooth::Bluetooth(const std::string& id, const Json::Value& config) - : ALabel(config, "bluetooth", id, "{icon}", 10), - status_("disabled"), - rfkill_{RFKILL_TYPE_BLUETOOTH} { + : ALabel(config, "bluetooth", id, "{icon}", 10), rfkill_{RFKILL_TYPE_BLUETOOTH} { rfkill_.on_update.connect(sigc::hide(sigc::mem_fun(*this, &Bluetooth::update))); thread_ = [this] { auto now = std::chrono::system_clock::now(); @@ -15,25 +15,18 @@ waybar::modules::Bluetooth::Bluetooth(const std::string& id, const Json::Value& } auto waybar::modules::Bluetooth::update() -> void { - if (rfkill_.getState()) { - status_ = "disabled"; - } else { - status_ = "enabled"; - } + std::string status = rfkill_.getState() ? "disabled" : "enabled"; label_.set_markup( - fmt::format( - format_, - fmt::arg("status", status_), - fmt::arg("icon", getIcon(0, status_)))); + fmt::format(format_, fmt::arg("status", status), fmt::arg("icon", getIcon(0, status)))); if (tooltipEnabled()) { if (config_["tooltip-format"].isString()) { auto tooltip_format = config_["tooltip-format"].asString(); - auto tooltip_text = fmt::format(tooltip_format, status_); + auto tooltip_text = fmt::format(tooltip_format, status); label_.set_tooltip_text(tooltip_text); } else { - label_.set_tooltip_text(status_); + label_.set_tooltip_text(status); } } } From 52dd3d2446a99ff822ae4fd913bdab3dc2c06d1c Mon Sep 17 00:00:00 2001 From: Aleksei Bavshin Date: Tue, 2 Feb 2021 20:10:27 -0800 Subject: [PATCH 4/6] refactor(bluetooth): remove `interval` and timer thread The timer thread was always reading the same value from Rfkill state. --- include/modules/bluetooth.hpp | 4 +--- man/waybar-bluetooth.5.scd | 6 ------ src/modules/bluetooth.cpp | 7 ------- 3 files changed, 1 insertion(+), 16 deletions(-) diff --git a/include/modules/bluetooth.hpp b/include/modules/bluetooth.hpp index 4d7b7c84..87845c95 100644 --- a/include/modules/bluetooth.hpp +++ b/include/modules/bluetooth.hpp @@ -2,7 +2,6 @@ #include "ALabel.hpp" #include "util/rfkill.hpp" -#include "util/sleeper_thread.hpp" namespace waybar::modules { @@ -13,8 +12,7 @@ class Bluetooth : public ALabel { auto update() -> void; private: - util::SleeperThread thread_; - util::Rfkill rfkill_; + util::Rfkill rfkill_; }; } // namespace waybar::modules diff --git a/man/waybar-bluetooth.5.scd b/man/waybar-bluetooth.5.scd index 0cd9386a..d4ecb1d1 100644 --- a/man/waybar-bluetooth.5.scd +++ b/man/waybar-bluetooth.5.scd @@ -12,11 +12,6 @@ The *bluetooth* module displays information about the status of the device's blu Addressed by *bluetooth* -*interval*: ++ - typeof: integer ++ - default: 60 ++ - The interval in which the bluetooth state gets updated. - *format*: ++ typeof: string ++ default: *{icon}* ++ @@ -88,7 +83,6 @@ Addressed by *bluetooth* "bluetooth": { "format": "{icon}", "format-alt": "bluetooth: {status}", - "interval": 30, "format-icons": { "enabled": "", "disabled": "" diff --git a/src/modules/bluetooth.cpp b/src/modules/bluetooth.cpp index 0df404d3..88526845 100644 --- a/src/modules/bluetooth.cpp +++ b/src/modules/bluetooth.cpp @@ -5,13 +5,6 @@ waybar::modules::Bluetooth::Bluetooth(const std::string& id, const Json::Value& config) : ALabel(config, "bluetooth", id, "{icon}", 10), rfkill_{RFKILL_TYPE_BLUETOOTH} { rfkill_.on_update.connect(sigc::hide(sigc::mem_fun(*this, &Bluetooth::update))); - thread_ = [this] { - auto now = std::chrono::system_clock::now(); - auto timeout = std::chrono::floor(now + interval_); - auto diff = std::chrono::seconds(timeout.time_since_epoch().count() % interval_.count()); - thread_.sleep_until(timeout - diff); - dp.emit(); - }; } auto waybar::modules::Bluetooth::update() -> void { From 6d5afdaa5fee87304fceb4b9d978b992bad32126 Mon Sep 17 00:00:00 2001 From: Aleksei Bavshin Date: Tue, 2 Feb 2021 20:56:00 -0800 Subject: [PATCH 5/6] fix(network): don't block the main thread on rfkill update Moving rfkill to the main event loop had unexpected side-effects. Notably, the network module mutex can block all the main thread events for several seconds while the network worker thread is sleeping. Instead of waiting for the mutex let's hope that the worker thread succeeds and schedule timer thread wakeup just in case. --- src/modules/network.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/modules/network.cpp b/src/modules/network.cpp index 41bc9d51..a8aaffaf 100644 --- a/src/modules/network.cpp +++ b/src/modules/network.cpp @@ -213,11 +213,11 @@ void waybar::modules::Network::worker() { }; #ifdef WANT_RFKILL rfkill_.on_update.connect([this](auto &) { - std::lock_guard lock(mutex_); - if (ifid_ > 0) { - getInfo(); - dp.emit(); - } + /* If we are here, it's likely that the network thread already holds the mutex and will be + * holding it for a next few seconds. + * Let's delegate the update to the timer thread instead of blocking the main thread. + */ + thread_timer_.wake_up(); }); #else spdlog::warn("Waybar has been built without rfkill support."); From e786ea601ed0aba54338bb7ea1563f2f3ebd5ae0 Mon Sep 17 00:00:00 2001 From: Aleksei Bavshin Date: Wed, 10 Feb 2021 08:22:22 -0800 Subject: [PATCH 6/6] fix(rfkill): handle EAGAIN correctly --- src/util/rfkill.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/util/rfkill.cpp b/src/util/rfkill.cpp index d3eb516a..7400135e 100644 --- a/src/util/rfkill.cpp +++ b/src/util/rfkill.cpp @@ -56,14 +56,15 @@ bool waybar::util::Rfkill::on_event(Glib::IOCondition cond) { len = read(fd_, &event, sizeof(event)); if (len < 0) { + if (errno == EAGAIN) { + return true; + } spdlog::error("Reading of RFKILL events failed: {}", errno); return false; } if (len < RFKILL_EVENT_SIZE_V1) { - if (errno != EAGAIN) { - spdlog::error("Wrong size of RFKILL event: {}", len); - } + spdlog::error("Wrong size of RFKILL event: {} < {}", len, RFKILL_EVENT_SIZE_V1); return true; }