From 7ebbe4a5ce431a2bf03066ec85513c693836550b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 23 Jan 2023 15:43:35 +0100 Subject: [PATCH 1/4] sleep: do not abort if we try to query capacity of missing battery Fixes #25584. From the issue: Assertion 'capacity >= 0' failed at src/shared/sleep-config.c:58, function PTR_TO_CAPACITY(). Aborting. (gdb) bt The problem is that PTR_TO_CAPACITY(hashmap_get(last_capacity, battery_name)) will abort if it's called with a name not present in the hashmap. We want to skip the device silently in this case instead. --- src/shared/sleep-config.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/shared/sleep-config.c b/src/shared/sleep-config.c index 9c5602d617..84082214ae 100644 --- a/src/shared/sleep-config.c +++ b/src/shared/sleep-config.c @@ -495,7 +495,7 @@ int get_total_suspend_interval(Hashmap *last_capacity, usec_t *ret) { continue; } - battery_last_capacity = PTR_TO_CAPACITY(hashmap_get(last_capacity, battery_name)); + battery_last_capacity = get_capacity_by_name(last_capacity, battery_name); if (battery_last_capacity <= 0) continue; From 31f62bdd79472c32d52408956d5c82e9991ca425 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 23 Jan 2023 15:44:33 +0100 Subject: [PATCH 2/4] test-sleep: add a very simple test that prints battery suspend estimates --- src/shared/sleep-config.c | 2 +- src/shared/sleep-config.h | 1 + src/test/test-sleep.c | 26 ++++++++++++++++++++++++++ 3 files changed, 28 insertions(+), 1 deletion(-) diff --git a/src/shared/sleep-config.c b/src/shared/sleep-config.c index 84082214ae..d7581960f9 100644 --- a/src/shared/sleep-config.c +++ b/src/shared/sleep-config.c @@ -150,7 +150,7 @@ static int battery_enumerator_new(sd_device_enumerator **ret) { return 0; } -static int get_capacity_by_name(Hashmap *capacities_by_name, const char *name) { +int get_capacity_by_name(Hashmap *capacities_by_name, const char *name) { void *p; assert(capacities_by_name); diff --git a/src/shared/sleep-config.h b/src/shared/sleep-config.h index 6645c3e596..af6947d982 100644 --- a/src/shared/sleep-config.h +++ b/src/shared/sleep-config.h @@ -60,6 +60,7 @@ int can_sleep_state(char **types); int battery_is_low(void); int get_total_suspend_interval(Hashmap *last_capacity, usec_t *ret); int fetch_batteries_capacity_by_name(Hashmap **ret_current_capacity); +int get_capacity_by_name(Hashmap *capacities_by_name, const char *name); int estimate_battery_discharge_rate_per_hour( Hashmap *last_capacity, Hashmap *current_capacity, diff --git a/src/test/test-sleep.c b/src/test/test-sleep.c index abaae43e72..2290e7c302 100644 --- a/src/test/test-sleep.c +++ b/src/test/test-sleep.c @@ -117,6 +117,32 @@ TEST(sleep) { log_info("Suspend-then-Hibernate configured and possible: %s", r >= 0 ? yes_no(r) : STRERROR(r)); } +TEST(fetch_batteries_capacity_by_name) { + _cleanup_hashmap_free_ Hashmap *capacity = NULL; + int r; + + assert_se(fetch_batteries_capacity_by_name(&capacity) >= 0); + log_debug("fetch_batteries_capacity_by_name: %u entries", hashmap_size(capacity)); + + const char *name; + void *cap; + HASHMAP_FOREACH_KEY(cap, name, capacity) { + assert(cap); /* Anything non-null is fine. */ + log_info("Battery %s: capacity = %i", name, get_capacity_by_name(capacity, name)); + } + + for (int i = 0; i < 5; i++) { + usec_t interval; + + sleep(1); + + r = get_total_suspend_interval(capacity, &interval); + assert_se(r >= 0 || r == -ENOENT); + log_info("%d: get_total_suspend_interval: %s", i, + r < 0 ? STRERROR(r) : FORMAT_TIMESPAN(interval, USEC_PER_SEC)); + } +} + static int intro(void) { if (getuid() != 0) log_warning("This program is unlikely to work for unprivileged users"); From 099810a65b8d7e7e83098edff144643b77011a85 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 23 Jan 2023 15:47:58 +0100 Subject: [PATCH 3/4] sleep: reduce double logging and improve messages and comments a bit read_battery_capacity_percentage() was already logging, but with a slightly different wording. More could be done, I just touched the most noticable places. Especially in debug messages, it is much more useful to be direct about what couldn't be accessed or parsed, instead of providing "descriptive names" which are not useful to the user at all, who then needs to read the code to figure out what was the actual property name. --- src/shared/sleep-config.c | 30 ++++++++++++++---------------- src/sleep/sleep.c | 3 ++- 2 files changed, 16 insertions(+), 17 deletions(-) diff --git a/src/shared/sleep-config.c b/src/shared/sleep-config.c index d7581960f9..caf94b455c 100644 --- a/src/shared/sleep-config.c +++ b/src/shared/sleep-config.c @@ -116,7 +116,7 @@ int parse_sleep_config(SleepConfig **ret_sleep_config) { if (sc->hibernate_delay_sec == 0) sc->hibernate_delay_sec = 2 * USEC_PER_HOUR; - /* ensure values set for all required fields */ + /* Ensure values set for all required fields */ if (!sc->states[SLEEP_SUSPEND] || !sc->modes[SLEEP_HIBERNATE] || !sc->states[SLEEP_HIBERNATE] || !sc->modes[SLEEP_HYBRID_SLEEP] || !sc->states[SLEEP_HYBRID_SLEEP]) return log_oom(); @@ -172,11 +172,11 @@ static int read_battery_capacity_percentage(sd_device *dev) { r = sd_device_get_property_value(dev, "POWER_SUPPLY_CAPACITY", &power_supply_capacity); if (r < 0) - return log_device_debug_errno(dev, r, "Failed to read battery capacity: %m"); + return log_device_debug_errno(dev, r, "Failed to get property POWER_SUPPLY_CAPACITY: %m"); r = safe_atoi(power_supply_capacity, &battery_capacity); if (r < 0) - return log_device_debug_errno(dev, r, "Failed to parse battery capacity: %m"); + return log_device_debug_errno(dev, r, "Failed to parse property POWER_SUPPLY_CAPACITY: %m"); if (battery_capacity < 0 || battery_capacity > 100) return log_device_debug_errno(dev, SYNTHETIC_ERRNO(ERANGE), "Invalid battery capacity"); @@ -184,14 +184,14 @@ static int read_battery_capacity_percentage(sd_device *dev) { return battery_capacity; } -/* If battery percentage capacity is less than equal to 5% return success */ +/* If battery percentage capacity is <= 5%, return success */ int battery_is_low(void) { _cleanup_(sd_device_enumerator_unrefp) sd_device_enumerator *e = NULL; sd_device *dev; int r; /* We have not used battery capacity_level since value is set to full - * or Normal in case acpi is not working properly. In case of no battery + * or Normal in case ACPI is not working properly. In case of no battery * 0 will be returned and system will be suspended for 1st cycle then hibernated */ r = battery_enumerator_new(&e); @@ -234,14 +234,12 @@ int fetch_batteries_capacity_by_name(Hashmap **ret) { int battery_capacity; battery_capacity = r = read_battery_capacity_percentage(dev); - if (r < 0) { - log_device_debug_errno(dev, r, "Failed to get battery capacity, ignoring: %m"); + if (r < 0) continue; - } r = sd_device_get_property_value(dev, "POWER_SUPPLY_NAME", &battery_name); if (r < 0) { - log_device_debug_errno(dev, r, "Failed to read battery name, ignoring: %m"); + log_device_debug_errno(dev, r, "Failed to get POWER_SUPPLY_NAME property, ignoring: %m"); continue; } @@ -272,11 +270,11 @@ static int get_battery_identifier(sd_device *dev, const char *property, struct s r = sd_device_get_property_value(dev, property, &x); if (r == -ENOENT) - log_device_debug_errno(dev, r, "battery device property %s is unavailable, ignoring: %m", property); + log_device_debug_errno(dev, r, "Battery device property %s is unavailable, ignoring: %m", property); else if (r < 0) - return log_device_debug_errno(dev, r, "Failed to read battery device property %s: %m", property); + return log_device_debug_errno(dev, r, "Failed to get battery device property %s: %m", property); else if (isempty(x)) - log_device_debug(dev, "battery device property '%s' is null.", property); + log_device_debug(dev, "Battery device property '%s' is empty.", property); else siphash24_compress_string(x, state); @@ -319,7 +317,7 @@ static int get_system_battery_identifier_hash(sd_device *dev, uint64_t *ret) { return 0; } -/* battery percentage discharge rate per hour is in range 1-199 then return success */ +/* Return success if battery percentage discharge rate per hour is in the range 1–199 */ static bool battery_discharge_rate_is_valid(int battery_discharge_rate) { return battery_discharge_rate > 0 && battery_discharge_rate < 200; } @@ -470,7 +468,7 @@ int estimate_battery_discharge_rate_per_hour( return 0; } -/* calculate the suspend interval for each battery and then return the sum of it */ +/* Calculate the suspend interval for each battery and then return their sum */ int get_total_suspend_interval(Hashmap *last_capacity, usec_t *ret) { _cleanup_(sd_device_enumerator_unrefp) sd_device_enumerator *e = NULL; usec_t total_suspend_interval = 0; @@ -516,8 +514,8 @@ int get_total_suspend_interval(Hashmap *last_capacity, usec_t *ret) { total_suspend_interval = usec_add(total_suspend_interval, suspend_interval); } - /* The previous discharge rate is stored in per hour basis so converted to minutes. - * Subtracted 30 minutes from the result to keep a buffer of 30 minutes before battery gets critical */ + /* Previous discharge rate is stored in per hour basis converted to usec. + * Subtract 30 minutes from the result to keep a buffer of 30 minutes before battery gets critical */ total_suspend_interval = usec_sub_unsigned(total_suspend_interval, 30 * USEC_PER_MINUTE); if (total_suspend_interval == 0) return -ENOENT; diff --git a/src/sleep/sleep.c b/src/sleep/sleep.c index 20d3a94704..de5b9ae192 100644 --- a/src/sleep/sleep.c +++ b/src/sleep/sleep.c @@ -327,7 +327,8 @@ static int custom_timer_suspend(const SleepConfig *sleep_config) { } after_timestamp = now(CLOCK_BOOTTIME); - log_debug("Attempting to estimate battery discharge rate after wakeup from %s sleep", FORMAT_TIMESPAN(after_timestamp - before_timestamp, USEC_PER_HOUR)); + log_debug("Attempting to estimate battery discharge rate after wakeup from %s sleep", + FORMAT_TIMESPAN(after_timestamp - before_timestamp, USEC_PER_HOUR)); if (after_timestamp != before_timestamp) { r = estimate_battery_discharge_rate_per_hour(last_capacity, current_capacity, before_timestamp, after_timestamp); From de5d8b40eda84cfd546c9d969a191ce4615e6375 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Mon, 23 Jan 2023 15:50:36 +0100 Subject: [PATCH 4/4] sleep: fix memleak Those hashmaps are created anew in each iteration of the loop. The leak wasn't really a problem, because the loop is bounded and the hashmaps were not huge, but it's nicer to be correct. --- src/sleep/sleep.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sleep/sleep.c b/src/sleep/sleep.c index de5b9ae192..c2c8c779ad 100644 --- a/src/sleep/sleep.c +++ b/src/sleep/sleep.c @@ -267,12 +267,12 @@ static int execute( } static int custom_timer_suspend(const SleepConfig *sleep_config) { - _cleanup_hashmap_free_ Hashmap *last_capacity = NULL, *current_capacity = NULL; int r; assert(sleep_config); while (battery_is_low() == 0) { + _cleanup_hashmap_free_ Hashmap *last_capacity = NULL, *current_capacity = NULL; _cleanup_close_ int tfd = -EBADF; struct itimerspec ts = {}; usec_t suspend_interval = sleep_config->hibernate_delay_sec, before_timestamp = 0, after_timestamp = 0, total_suspend_interval;