From a342e036adc631189b0d075cd9879cb77e1aa914 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Ekl=C3=B6f?= Date: Sun, 9 Jul 2023 10:56:30 +0200 Subject: [PATCH] =?UTF-8?q?module/battery:=20don=E2=80=99t=20reset=20poll?= =?UTF-8?q?=20timeout=20on=20irrelevant=20udev=20notifications?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We may receive udev notifications for the power-supply subsystem, that aren’t for us. Before this patch, this would result in a new poll() call, with timeout being set to m->poll_interval again. That is, the poll timeout was being reset. In theory, this means we could end up in a situation where the battery status is never updated. This patch fixes it by tracking how much time is left of the poll interval. The interval is reset either when the timeout has occurred, or when we receive an udev notification that _is_ for us. Hopefully closes #305 --- CHANGELOG.md | 4 +++ modules/battery.c | 88 ++++++++++++++++++++++++++++++++++++++++------- 2 files changed, 79 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fc05f95..30c9d47 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -54,6 +54,9 @@ * Crash when a yaml anchor has a value to already exists in the target yaml node ([#286][286]). * battery: Fix time conversion in battery estimation ([#303][303]). +* battery: poll timeout being reset when receiving irrelevant udev + notification (leading to battery status never updating, in worst + case) ([#305][305]). [239]: https://codeberg.org/dnkl/yambar/issues/239 [241]: https://codeberg.org/dnkl/yambar/issues/241 @@ -61,6 +64,7 @@ [253]: https://codeberg.org/dnkl/yambar/issues/253 [262]: https://codeberg.org/dnkl/yambar/issues/262 [286]: https://codeberg.org/dnkl/yambar/issues/286 +[305]: https://codeberg.org/dnkl/yambar/issues/305 ### Security diff --git a/modules/battery.c b/modules/battery.c index c7e7d58..a83f30e 100644 --- a/modules/battery.c +++ b/modules/battery.c @@ -1,6 +1,7 @@ #include #include #include +#include #include #include #include @@ -13,13 +14,15 @@ #include #define LOG_MODULE "battery" -#define LOG_ENABLE_DBG 0 +#define LOG_ENABLE_DBG 1 #include "../log.h" #include "../bar/bar.h" #include "../config.h" #include "../config-verify.h" #include "../plugin.h" +#define max(x, y) ((x) > (y) ? (x) : (y)) + static const long min_poll_interval = 250; static const long default_poll_interval = 60 * 1000; @@ -47,6 +50,22 @@ struct private { long time_to_full; }; +static void +timespec_sub(const struct timespec *a, const struct timespec *b, + struct timespec *res) +{ + const long one_sec_in_ns = 1000000000; + + res->tv_sec = a->tv_sec - b->tv_sec; + res->tv_nsec = a->tv_nsec - b->tv_nsec; + + /* tv_nsec may be negative */ + if (res->tv_nsec < 0) { + res->tv_sec--; + res->tv_nsec += one_sec_in_ns; + } +} + static void destroy(struct module *mod) { @@ -439,14 +458,25 @@ run(struct module *mod) bar->refresh(bar); + int timeout_left_ms = m->poll_interval; + while (true) { struct pollfd fds[] = { {.fd = mod->abort_fd, .events = POLLIN}, {.fd = udev_monitor_get_fd(mon), .events = POLLIN}, }; - if (poll(fds, sizeof(fds) / sizeof(fds[0]), - m->poll_interval > 0 ? m->poll_interval : -1) < 0) - { + + int timeout = m->poll_interval > 0 ? timeout_left_ms : -1; + + struct timespec time_before_poll; + if (clock_gettime(CLOCK_BOOTTIME, &time_before_poll) < 0) { + LOG_ERRNO("failed to get current time"); + break; + } + + const int poll_ret = poll(fds, sizeof(fds) / sizeof(fds[0]), timeout); + + if (poll_ret < 0) { if (errno == EINTR) continue; @@ -459,21 +489,53 @@ run(struct module *mod) break; } + bool udev_for_us = false; + if (fds[1].revents & POLLIN) { struct udev_device *dev = udev_monitor_receive_device(mon); - if (dev == NULL) - continue; + if (dev != NULL) { + const char *sysname = udev_device_get_sysname(dev); + udev_for_us = + sysname != NULL && strcmp(sysname, m->battery) == 0; - const char *sysname = udev_device_get_sysname(dev); - bool is_us = sysname != NULL && strcmp(sysname, m->battery) == 0; - udev_device_unref(dev); + if (!udev_for_us) { + LOG_DBG("udev notification not for us (%s != %s)", + m->battery, sysname != sysname ? sysname : "NULL"); + } - if (!is_us) - continue; + udev_device_unref(dev); + } } - if (update_status(mod)) - bar->refresh(bar); + if (udev_for_us || poll_ret == 0) { + if (update_status(mod)) + bar->refresh(bar); + } + + if (poll_ret == 0 || udev_for_us) { + LOG_DBG("resetting timeout-left to %ldms", m->poll_interval); + timeout_left_ms = m->poll_interval; + } else { + struct timespec time_after_poll; + if (clock_gettime(CLOCK_BOOTTIME, &time_after_poll) < 0) { + LOG_ERRNO("failed to get current time"); + break; + } + + struct timespec timeout_consumed; + timespec_sub(&time_after_poll, &time_before_poll, &timeout_consumed); + + const int timeout_consumed_ms = + timeout_consumed.tv_sec * 1000 + timeout_consumed.tv_nsec / 1000000; + + LOG_DBG("timeout-left before: %dms, consumed: %dms, updated: %dms", + timeout_left_ms, timeout_consumed_ms, + max(timeout_left_ms - timeout_consumed_ms, 0)); + + timeout_left_ms -= timeout_consumed_ms; + if (timeout_left_ms < 0) + timeout_left_ms = 0; + } } out: