systemd-sleep: refactor sleep config parsing

remove verb from parse

refactor required fields checks

refactor allow settings

5 files changed, 140 insertions, 106 deletions

diff --git a/TODO b/TODO
index 4507c16878..1b91b7c799 100644
--- a/TODO
+++ b/TODO
@@ -34,8 +34,6 @@ Features:
   cgroup.
 
 * clean up sleep.c:
-  - Parse sleep.conf only once, and parse its whole contents so that we don't
-    have to parse it again and again in s2h
   - Make sure resume= and resume_offset= on the kernel cmdline always take
     precedence
 
diff --git a/src/shared/sleep-config.c b/src/shared/sleep-config.c
index cba7afdd11..32a22eebd2 100644
--- a/src/shared/sleep-config.c
+++ b/src/shared/sleep-config.c
@@ -32,16 +32,14 @@
 #include "strv.h"
 #include "time-util.h"
 
-int parse_sleep_config(const char *verb, bool *ret_allow, char ***ret_modes, char ***ret_states, usec_t *ret_delay) {
+int parse_sleep_config(SleepConfig **ret_sleep_config) {
+        _cleanup_(free_sleep_configp) SleepConfig *sc;
         int allow_suspend = -1, allow_hibernate = -1,
             allow_s2h = -1, allow_hybrid_sleep = -1;
-        bool allow;
-        _cleanup_strv_free_ char
-                **suspend_mode = NULL, **suspend_state = NULL,
-                **hibernate_mode = NULL, **hibernate_state = NULL,
-                **hybrid_mode = NULL, **hybrid_state = NULL;
-        _cleanup_strv_free_ char **modes, **states; /* always initialized below */
-        usec_t delay = 180 * USEC_PER_MINUTE;
+
+        sc = new0(SleepConfig, 1);
+        if (!sc)
+                return log_oom();
 
         const ConfigTableItem items[] = {
                 { "Sleep", "AllowSuspend",              config_parse_tristate, 0, &allow_suspend },
@@ -49,14 +47,14 @@ int parse_sleep_config(const char *verb, bool *ret_allow, char ***ret_modes, cha
                 { "Sleep", "AllowSuspendThenHibernate", config_parse_tristate, 0, &allow_s2h },
                 { "Sleep", "AllowHybridSleep",          config_parse_tristate, 0, &allow_hybrid_sleep },
 
-                { "Sleep", "SuspendMode",               config_parse_strv, 0, &suspend_mode  },
-                { "Sleep", "SuspendState",              config_parse_strv, 0, &suspend_state },
-                { "Sleep", "HibernateMode",             config_parse_strv, 0, &hibernate_mode  },
-                { "Sleep", "HibernateState",            config_parse_strv, 0, &hibernate_state },
-                { "Sleep", "HybridSleepMode",           config_parse_strv, 0, &hybrid_mode  },
-                { "Sleep", "HybridSleepState",          config_parse_strv, 0, &hybrid_state },
+                { "Sleep", "SuspendMode",               config_parse_strv, 0, &sc->suspend_modes  },
+                { "Sleep", "SuspendState",              config_parse_strv, 0, &sc->suspend_states },
+                { "Sleep", "HibernateMode",             config_parse_strv, 0, &sc->hibernate_modes  },
+                { "Sleep", "HibernateState",            config_parse_strv, 0, &sc->hibernate_states },
+                { "Sleep", "HybridSleepMode",           config_parse_strv, 0, &sc->hybrid_modes  },
+                { "Sleep", "HybridSleepState",          config_parse_strv, 0, &sc->hybrid_states },
 
-                { "Sleep", "HibernateDelaySec",         config_parse_sec,  0, &delay},
+                { "Sleep", "HibernateDelaySec",         config_parse_sec,  0, &sc->hibernate_delay_sec},
                 {}
         };
 
@@ -65,64 +63,33 @@ int parse_sleep_config(const char *verb, bool *ret_allow, char ***ret_modes, cha
                                         "Sleep\0", config_item_table_lookup, items,
                                         CONFIG_PARSE_WARN, NULL);
 
-        if (streq(verb, "suspend")) {
-                allow = allow_suspend != 0;
-
-                /* empty by default */
-                modes = TAKE_PTR(suspend_mode);
-
-                if (suspend_state)
-                        states = TAKE_PTR(suspend_state);
-                else
-                        states = strv_new("mem", "standby", "freeze");
-
-        } else if (streq(verb, "hibernate")) {
-                allow = allow_hibernate != 0;
-
-                if (hibernate_mode)
-                        modes = TAKE_PTR(hibernate_mode);
-                else
-                        modes = strv_new("platform", "shutdown");
-
-                if (hibernate_state)
-                        states = TAKE_PTR(hibernate_state);
-                else
-                        states = strv_new("disk");
-
-        } else if (streq(verb, "hybrid-sleep")) {
-                allow = allow_hybrid_sleep > 0 ||
-                        (allow_suspend != 0 && allow_hibernate != 0);
-
-                if (hybrid_mode)
-                        modes = TAKE_PTR(hybrid_mode);
-                else
-                        modes = strv_new("suspend", "platform", "shutdown");
-
-                if (hybrid_state)
-                        states = TAKE_PTR(hybrid_state);
-                else
-                        states = strv_new("disk");
-
-        } else if (streq(verb, "suspend-then-hibernate")) {
-                allow = allow_s2h > 0 ||
-                        (allow_suspend != 0 && allow_hibernate != 0);
-
-                modes = states = NULL;
-        } else
-                assert_not_reached("what verb");
-
-        if ((!modes && STR_IN_SET(verb, "hibernate", "hybrid-sleep")) ||
-            (!states && !streq(verb, "suspend-then-hibernate")))
+        /* use default values unless set */
+        sc->allow_suspend = allow_suspend != 0;
+        sc->allow_hibernate = allow_hibernate != 0;
+        sc->allow_hybrid_sleep = allow_hybrid_sleep > 0
+                || (allow_suspend != 0 && allow_hibernate != 0);
+        sc->allow_s2h = allow_s2h > 0
+                || (allow_suspend != 0 && allow_hibernate != 0);
+
+        if (!sc->suspend_states)
+                sc->suspend_states = strv_new("mem", "standby", "freeze");
+        if (!sc->hibernate_modes)
+                sc->hibernate_modes = strv_new("platform", "shutdown");
+        if (!sc->hibernate_states)
+                sc->hibernate_states = strv_new("disk");
+        if (!sc->hybrid_modes)
+                sc->hybrid_modes = strv_new("suspend", "platform", "shutdown");
+        if (!sc->hybrid_states)
+                sc->hybrid_states = strv_new("disk");
+        if (sc->hibernate_delay_sec == 0)
+                sc->hibernate_delay_sec = 180 * USEC_PER_MINUTE;
+
+        /* ensure values set for all required fields */
+        if (!sc->suspend_states || !sc->hibernate_modes
+            || !sc->hibernate_states || !sc->hybrid_modes || !sc->hybrid_states)
                 return log_oom();
 
-        if (ret_allow)
-                *ret_allow = allow;
-        if (ret_modes)
-                *ret_modes = TAKE_PTR(modes);
-        if (ret_states)
-                *ret_states = TAKE_PTR(states);
-        if (ret_delay)
-                *ret_delay = delay;
+        *ret_sleep_config = TAKE_PTR(sc);
 
         return 0;
 }
@@ -378,9 +345,9 @@ int read_fiemap(int fd, struct fiemap **ret) {
         return 0;
 }
 
-static int can_sleep_internal(const char *verb, bool check_allowed);
+static int can_sleep_internal(const char *verb, bool check_allowed, const SleepConfig *sleep_config);
 
-static bool can_s2h(void) {
+static bool can_s2h(const SleepConfig *sleep_config) {
         const char *p;
         int r;
 
@@ -391,7 +358,7 @@ static bool can_s2h(void) {
         }
 
         FOREACH_STRING(p, "suspend", "hibernate") {
-                r = can_sleep_internal(p, false);
+                r = can_sleep_internal(p, false, sleep_config);
                 if (IN_SET(r, 0, -ENOSPC, -EADV)) {
                         log_debug("Unable to %s system.", p);
                         return false;
@@ -403,14 +370,14 @@ static bool can_s2h(void) {
         return true;
 }
 
-static int can_sleep_internal(const char *verb, bool check_allowed) {
+static int can_sleep_internal(const char *verb, bool check_allowed, const SleepConfig *sleep_config) {
         bool allow;
-        _cleanup_strv_free_ char **modes = NULL, **states = NULL;
+        char **modes = NULL, **states = NULL;
         int r;
 
         assert(STR_IN_SET(verb, "suspend", "hibernate", "hybrid-sleep", "suspend-then-hibernate"));
 
-        r = parse_sleep_config(verb, &allow, &modes, &states, NULL);
+        r = sleep_settings(verb, sleep_config, &allow, &modes, &states);
         if (r < 0)
                 return false;
 
@@ -420,7 +387,7 @@ static int can_sleep_internal(const char *verb, bool check_allowed) {
         }
 
         if (streq(verb, "suspend-then-hibernate"))
-                return can_s2h();
+                return can_s2h(sleep_config);
 
         if (!can_sleep_state(states) || !can_sleep_disk(modes))
                 return false;
@@ -435,5 +402,58 @@ static int can_sleep_internal(const char *verb, bool check_allowed) {
 }
 
 int can_sleep(const char *verb) {
-        return can_sleep_internal(verb, true);
+        _cleanup_(free_sleep_configp) SleepConfig *sleep_config = NULL;
+        int r;
+
+        r = parse_sleep_config(&sleep_config);
+        if (r < 0)
+                return r;
+
+        return can_sleep_internal(verb, true, sleep_config);
+}
+
+int sleep_settings(const char *verb, const SleepConfig *sleep_config, bool *ret_allow, char ***ret_modes, char ***ret_states) {
+
+        assert(verb);
+        assert(sleep_config);
+        assert(STR_IN_SET(verb, "suspend", "hibernate", "hybrid-sleep", "suspend-then-hibernate"));
+
+        if (streq(verb, "suspend")) {
+                *ret_allow = sleep_config->allow_suspend;
+                *ret_modes = sleep_config->suspend_modes;
+                *ret_states = sleep_config->suspend_states;
+        } else if (streq(verb, "hibernate")) {
+                *ret_allow = sleep_config->allow_hibernate;
+                *ret_modes = sleep_config->hibernate_modes;
+                *ret_states = sleep_config->hibernate_states;
+        } else if (streq(verb, "hybrid-sleep")) {
+                *ret_allow = sleep_config->allow_hybrid_sleep;
+                *ret_modes = sleep_config->hybrid_modes;
+                *ret_states = sleep_config->hybrid_states;
+        } else if (streq(verb, "suspend-then-hibernate")) {
+                *ret_allow = sleep_config->allow_s2h;
+                *ret_modes = *ret_states = NULL;
+        }
+
+        /* suspend modes empty by default */
+        if ((!ret_modes && !streq(verb, "suspend")) || !ret_states)
+                return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "No modes or states set for %s; Check sleep.conf", verb);
+
+        return 0;
+}
+
+void free_sleep_config(SleepConfig *sc) {
+        if (!sc)
+                return;
+
+        strv_free(sc->suspend_modes);
+        strv_free(sc->suspend_states);
+
+        strv_free(sc->hibernate_modes);
+        strv_free(sc->hibernate_states);
+
+        strv_free(sc->hybrid_modes);
+        strv_free(sc->hybrid_states);
+
+        free(sc);
 }
diff --git a/src/shared/sleep-config.h b/src/shared/sleep-config.h
index c584f44d39..965fde93a2 100644
--- a/src/shared/sleep-config.h
+++ b/src/shared/sleep-config.h
@@ -4,8 +4,29 @@
 #include <linux/fiemap.h>
 #include "time-util.h"
 
+typedef struct SleepConfig {
+        bool allow_suspend;         /* AllowSuspend */
+        bool allow_hibernate;       /* AllowHibernation */
+        bool allow_s2h;             /* AllowSuspendThenHibernate */
+        bool allow_hybrid_sleep;    /* AllowHybridSleep */
+
+        char **suspend_modes;       /* SuspendMode */
+        char **suspend_states;      /* SuspendState */
+        char **hibernate_modes;     /* HibernateMode */
+        char **hibernate_states;    /* HibernateState */
+        char **hybrid_modes;        /* HybridSleepMode */
+        char **hybrid_states;       /* HybridSleepState */
+
+        usec_t hibernate_delay_sec; /* HibernateDelaySec */
+} SleepConfig;
+
+void free_sleep_config(SleepConfig *sc);
+DEFINE_TRIVIAL_CLEANUP_FUNC(SleepConfig*, free_sleep_config);
+
+int sleep_settings(const char *verb, const SleepConfig *sleep_config, bool *ret_allow, char ***ret_modes, char ***ret_states);
+
 int read_fiemap(int fd, struct fiemap **ret);
-int parse_sleep_config(const char *verb, bool *ret_allow, char ***ret_modes, char ***ret_states, usec_t *ret_delay);
+int parse_sleep_config(SleepConfig **sleep_config);
 int find_hibernate_location(char **device, char **type, size_t *size, size_t *used);
 
 int can_sleep(const char *verb);
diff --git a/src/sleep/sleep.c b/src/sleep/sleep.c
index 2510ccc72b..11757f2efa 100644
--- a/src/sleep/sleep.c
+++ b/src/sleep/sleep.c
@@ -199,37 +199,29 @@ static int execute(char **modes, char **states) {
         return r;
 }
 
-static int execute_s2h(usec_t hibernate_delay_sec) {
-        _cleanup_strv_free_ char **hibernate_modes = NULL, **hibernate_states = NULL,
-                                 **suspend_modes = NULL, **suspend_states = NULL;
+static int execute_s2h(const SleepConfig *sleep_config) {
         _cleanup_close_ int tfd = -1;
         char buf[FORMAT_TIMESPAN_MAX];
         struct itimerspec ts = {};
         struct pollfd fds;
         int r;
 
-        r = parse_sleep_config("suspend", NULL, &suspend_modes, &suspend_states, NULL);
-        if (r < 0)
-                return r;
-
-        r = parse_sleep_config("hibernate", NULL, &hibernate_modes, &hibernate_states, NULL);
-        if (r < 0)
-                return r;
+        assert(sleep_config);
 
         tfd = timerfd_create(CLOCK_BOOTTIME_ALARM, TFD_NONBLOCK|TFD_CLOEXEC);
         if (tfd < 0)
                 return log_error_errno(errno, "Error creating timerfd: %m");
 
         log_debug("Set timerfd wake alarm for %s",
-                  format_timespan(buf, sizeof(buf), hibernate_delay_sec, USEC_PER_SEC));
+                  format_timespan(buf, sizeof(buf), sleep_config->hibernate_delay_sec, USEC_PER_SEC));
 
-        timespec_store(&ts.it_value, hibernate_delay_sec);
+        timespec_store(&ts.it_value, sleep_config->hibernate_delay_sec);
 
         r = timerfd_settime(tfd, 0, &ts, NULL);
         if (r < 0)
                 return log_error_errno(errno, "Error setting hibernate timer: %m");
 
-        r = execute(suspend_modes, suspend_states);
+        r = execute(sleep_config->suspend_modes, sleep_config->suspend_states);
         if (r < 0)
                 return r;
 
@@ -248,11 +240,12 @@ static int execute_s2h(usec_t hibernate_delay_sec) {
 
         /* If woken up after alarm time, hibernate */
         log_debug("Attempting to hibernate after waking from %s timer",
-                  format_timespan(buf, sizeof(buf), hibernate_delay_sec, USEC_PER_SEC));
-        r = execute(hibernate_modes, hibernate_states);
+                  format_timespan(buf, sizeof(buf), sleep_config->hibernate_delay_sec, USEC_PER_SEC));
+
+        r = execute(sleep_config->hibernate_modes, sleep_config->hibernate_states);
         if (r < 0) {
                 log_notice("Couldn't hibernate, will try to suspend again.");
-                r = execute(suspend_modes, suspend_states);
+                r = execute(sleep_config->suspend_modes, sleep_config->suspend_states);
                 if (r < 0) {
                         log_notice("Could neither hibernate nor suspend again, giving up.");
                         return r;
@@ -335,8 +328,8 @@ static int parse_argv(int argc, char *argv[]) {
 
 static int run(int argc, char *argv[]) {
         bool allow;
-        _cleanup_strv_free_ char **modes = NULL, **states = NULL;
-        usec_t delay = 0;
+        char **modes = NULL, **states = NULL;
+        _cleanup_(free_sleep_configp) SleepConfig *sleep_config = NULL;
         int r;
 
         log_setup_service();
@@ -345,7 +338,11 @@ static int run(int argc, char *argv[]) {
         if (r <= 0)
                 return r;
 
-        r = parse_sleep_config(arg_verb, &allow, &modes, &states, &delay);
+        r = parse_sleep_config(&sleep_config);
+        if (r < 0)
+                return r;
+
+        r = sleep_settings(arg_verb, sleep_config, &allow, &modes, &states);
         if (r < 0)
                 return r;
 
@@ -355,7 +352,7 @@ static int run(int argc, char *argv[]) {
                                        arg_verb);
 
         if (streq(arg_verb, "suspend-then-hibernate"))
-                return execute_s2h(delay);
+                return execute_s2h(sleep_config);
         else
                 return execute(modes, states);
 }
diff --git a/src/test/test-sleep.c b/src/test/test-sleep.c
index 7e949154b1..a0830bc899 100644
--- a/src/test/test-sleep.c
+++ b/src/test/test-sleep.c
@@ -16,12 +16,10 @@
 #include "util.h"
 
 static void test_parse_sleep_config(void) {
-        const char *verb;
-
+        _cleanup_(free_sleep_configp) SleepConfig *sleep_config = NULL;
         log_info("/* %s */", __func__);
 
-        FOREACH_STRING(verb, "suspend", "hibernate", "hybrid-sleep", "suspend-then-hibernate")
-                assert_se(parse_sleep_config(verb, NULL, NULL, NULL, NULL) == 0);
+        assert(parse_sleep_config(&sleep_config) == 0);
 }
 
 static int test_fiemap(const char *path) {