Merge pull request #26159 from keszybz/capacity-to-crash

Resolve crash in systemd-sleep and other minor fixes

4 files changed, 46 insertions, 20 deletions

diff --git a/src/shared/sleep-config.c b/src/shared/sleep-config.c
index 9c5602d617..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();
@@ -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);
@@ -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;
@@ -495,7 +493,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;
 
@@ -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/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/sleep/sleep.c b/src/sleep/sleep.c
index 20d3a94704..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;
@@ -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);
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");