bus-unit-util: rework UnitFreezer, explicitly thaw unit

Currently, we don't explicitly call unit_freezer_thaw(),
but rely on the destructor to thaw the frozen unit on
return. This has several problems though, one of them
being that we ignore the return value of ThawUnit(),
which is something we really shouldn't do here,
since such failure can easily leave the whole system
in unusable state. Moreover, the logging is kinda messy,
e.g. homed might log "Everything completed" yet immediately
followed by "Failed to thaw unit". Instead, we should log
consistently and at higher level, to make things more
debuggable.

Therefore, let's step away from the practice. Plus,
make UnitFreezer object heap-allocated, to match
with existing unit_freezer_new() and allow us to
use NULL to denote that the freezer is disabled.

diff --git a/src/home/homework.c b/src/home/homework.c
index e10922cefda3f167834b4de4e7fd8aa539430ab9..428f83291164ad542bc0dc371d4be55c5df26911 100644 (file)
--- a/src/home/homework.c
+++ b/src/home/homework.c
@@ -1866,10 +1866,13 @@ static int home_inspect(UserRecord *h, UserRecord **ret_home) {
         return 1;
 }
 
-static int user_session_freezer(uid_t uid, bool freeze_now, UnitFreezer *ret) {
+static int user_session_freezer(uid_t uid, bool freeze_now, UnitFreezer **ret) {
         _cleanup_free_ char *unit = NULL;
         int r;
 
+        assert(uid_is_valid(uid));
+        assert(ret);
+
         r = getenv_bool("SYSTEMD_HOME_LOCK_FREEZE_SESSION");
         if (r < 0 && r != -ENXIO)
                 log_warning_errno(r, "Cannot parse value of $SYSTEMD_HOME_LOCK_FREEZE_SESSION, ignoring.");
@@ -1878,7 +1881,8 @@ static int user_session_freezer(uid_t uid, bool freeze_now, UnitFreezer *ret) {
                         log_notice("Session remains unfrozen on explicit request ($SYSTEMD_HOME_LOCK_FREEZE_SESSION "
                                    "is set to false). This is not recommended, and might result in unexpected behavior "
                                    "including data loss!");
-                *ret = (UnitFreezer) {};
+
+                *ret = NULL;
                 return 0;
         }
 
@@ -1891,12 +1895,12 @@ static int user_session_freezer(uid_t uid, bool freeze_now, UnitFreezer *ret) {
                 r = unit_freezer_new(unit, ret);
         if (r < 0)
                 return r;
+
         return 1;
 }
 
 static int home_lock(UserRecord *h) {
         _cleanup_(home_setup_done) HomeSetup setup = HOME_SETUP_INIT;
-        _cleanup_(unit_freezer_done_thaw) UnitFreezer freezer = {};
         int r;
 
         assert(h);
@@ -1912,19 +1916,19 @@ static int home_lock(UserRecord *h) {
         if (r != USER_TEST_MOUNTED)
                 return log_error_errno(SYNTHETIC_ERRNO(ENOEXEC), "Home directory of %s is not mounted, can't lock.", h->user_name);
 
-        r = user_session_freezer(h->uid, /* freeze_now= */ true, &freezer);
-        if (r < 0)
-                log_warning_errno(r, "Failed to freeze user session, ignoring: %m");
-        else if (r == 0)
-                log_info("User session freeze disabled, skipping.");
-        else
-                log_info("Froze user session.");
+        _cleanup_(unit_freezer_freep) UnitFreezer *f = NULL;
 
-        r = home_lock_luks(h, &setup);
+        r = user_session_freezer(h->uid, /* freeze_now= */ true, &f);
         if (r < 0)
                 return r;
 
-        unit_freezer_done(&freezer); /* Don't thaw the user session. */
+        r = home_lock_luks(h, &setup);
+        if (r < 0) {
+                if (f)
+                        (void) unit_freezer_thaw(f);
+
+                return r;
+        }
 
         /* Explicitly flush any per-user key from the keyring */
         (void) keyring_flush(h);
@@ -1935,7 +1939,6 @@ static int home_lock(UserRecord *h) {
 
 static int home_unlock(UserRecord *h) {
         _cleanup_(home_setup_done) HomeSetup setup = HOME_SETUP_INIT;
-        _cleanup_(unit_freezer_done_thaw) UnitFreezer freezer = {};
         _cleanup_(password_cache_free) PasswordCache cache = {};
         int r;
 
@@ -1957,10 +1960,14 @@ static int home_unlock(UserRecord *h) {
         if (r < 0)
                 return r;
 
+        _cleanup_(unit_freezer_freep) UnitFreezer *f = NULL;
+
         /* We want to thaw the session only after it's safe to access $HOME */
-        r = user_session_freezer(h->uid, /* freeze_now= */ false, &freezer);
+        r = user_session_freezer(h->uid, /* freeze_now= */ false, &f);
+        if (r > 0)
+                r = unit_freezer_thaw(f);
         if (r < 0)
-                log_warning_errno(r, "Failed to recover freezer for user session, ignoring: %m");
+                return r;
 
         log_info("Everything completed.");
         return 1;

diff --git a/src/shared/bus-unit-util.c b/src/shared/bus-unit-util.c
index fd463328218697fdaf9b2817731a55d95098ea3f..8de9f374e339443bd024932e254afa50f48d2175 100644 (file)
--- a/src/shared/bus-unit-util.c
+++ b/src/shared/bus-unit-util.c
@@ -2,6 +2,7 @@
 
 #include "af-list.h"
 #include "alloc-util.h"
+#include "bus-common-errors.h"
 #include "bus-error.h"
 #include "bus-locator.h"
 #include "bus-unit-util.h"
@@ -2939,41 +2940,51 @@ int bus_service_manager_reload(sd_bus *bus) {
         return 0;
 }
 
+typedef struct UnitFreezer {
+        char *name;
+        sd_bus *bus;
+} UnitFreezer;
+
 /* Wait for 1.5 seconds at maximum for freeze operation */
 #define FREEZE_BUS_CALL_TIMEOUT (1500 * USEC_PER_MSEC)
 
-int unit_freezer_new(const char *name, UnitFreezer *ret) {
-        _cleanup_(sd_bus_flush_close_unrefp) sd_bus *bus = NULL;
-        _cleanup_free_ char *namedup = NULL;
+UnitFreezer* unit_freezer_free(UnitFreezer *f) {
+        if (!f)
+                return NULL;
+
+        free(f->name);
+        sd_bus_flush_close_unref(f->bus);
+
+        return mfree(f);
+}
+
+int unit_freezer_new(const char *name, UnitFreezer **ret) {
+        _cleanup_(unit_freezer_freep) UnitFreezer *f = NULL;
         int r;
 
         assert(name);
         assert(ret);
 
-        namedup = strdup(name);
-        if (!namedup)
-                return log_oom_debug();
+        f = new(UnitFreezer, 1);
+        if (!f)
+                return log_oom();
+
+        *f = (UnitFreezer) {
+                .name = strdup(name),
+        };
+        if (!f->name)
+                return log_oom();
 
-        r = bus_connect_system_systemd(&bus);
+        r = bus_connect_system_systemd(&f->bus);
         if (r < 0)
-                return log_debug_errno(r, "Failed to open connection to systemd: %m");
+                return log_error_errno(r, "Failed to open connection to systemd: %m");
 
-        (void) sd_bus_set_method_call_timeout(bus, FREEZE_BUS_CALL_TIMEOUT);
+        (void) sd_bus_set_method_call_timeout(f->bus, FREEZE_BUS_CALL_TIMEOUT);
 
-        *ret = (UnitFreezer) {
-                .name = TAKE_PTR(namedup),
-                .bus = TAKE_PTR(bus),
-        };
+        *ret = TAKE_PTR(f);
         return 0;
 }
 
-void unit_freezer_done(UnitFreezer *f) {
-        assert(f);
-
-        f->name = mfree(f->name);
-        f->bus = sd_bus_flush_close_unref(f->bus);
-}
-
 static int unit_freezer_action(UnitFreezer *f, bool freeze) {
         _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL;
         int r;
@@ -2988,11 +2999,22 @@ static int unit_freezer_action(UnitFreezer *f, bool freeze) {
                             /* reply = */ NULL,
                             "s",
                             f->name);
-        if (r < 0)
-                return log_debug_errno(r, "Failed to %s unit %s: %s",
+        if (r < 0) {
+                if (sd_bus_error_has_names(&error,
+                                           BUS_ERROR_NO_SUCH_UNIT,
+                                           BUS_ERROR_UNIT_INACTIVE,
+                                           SD_BUS_ERROR_NOT_SUPPORTED)) {
+
+                        log_debug_errno(r, "Skipping freezer for '%s': %s", f->name, bus_error_message(&error, r));
+                        return 0;
+                }
+
+                return log_error_errno(r, "Failed to %s unit '%s': %s",
                                        freeze ? "freeze" : "thaw", f->name, bus_error_message(&error, r));
+        }
 
-        return 0;
+        log_info("Successfully %s unit '%s'.", freeze ? "froze" : "thawed", f->name);
+        return 1;
 }
 
 int unit_freezer_freeze(UnitFreezer *f) {
@@ -3003,8 +3025,8 @@ int unit_freezer_thaw(UnitFreezer *f) {
         return unit_freezer_action(f, false);
 }
 
-int unit_freezer_new_freeze(const char *name, UnitFreezer *ret) {
-        _cleanup_(unit_freezer_done) UnitFreezer f = {};
+int unit_freezer_new_freeze(const char *name, UnitFreezer **ret) {
+        _cleanup_(unit_freezer_freep) UnitFreezer *f = NULL;
         int r;
 
         assert(name);
@@ -3014,20 +3036,10 @@ int unit_freezer_new_freeze(const char *name, UnitFreezer *ret) {
         if (r < 0)
                 return r;
 
-        r = unit_freezer_freeze(&f);
+        r = unit_freezer_freeze(f);
         if (r < 0)
                 return r;
 
-        *ret = TAKE_STRUCT(f);
+        *ret = TAKE_PTR(f);
         return 0;
 }
-
-void unit_freezer_done_thaw(UnitFreezer *f) {
-        assert(f);
-
-        if (!f->name)
-                return;
-
-        (void) unit_freezer_thaw(f);
-        unit_freezer_done(f);
-}

diff --git a/src/shared/bus-unit-util.h b/src/shared/bus-unit-util.h
index e4168a44254116ff24cc89f3a001db40afa3f5dc..ea4056c6910465fc1b5ec726384740662aa11bea 100644 (file)
--- a/src/shared/bus-unit-util.h
+++ b/src/shared/bus-unit-util.h
@@ -36,17 +36,14 @@ int unit_info_compare(const UnitInfo *a, const UnitInfo *b);
 
 int bus_service_manager_reload(sd_bus *bus);
 
-typedef struct UnitFreezer {
-        char *name;
-        sd_bus *bus;
-} UnitFreezer;
+typedef struct UnitFreezer UnitFreezer;
 
-int unit_freezer_new(const char *name, UnitFreezer *ret);
-void unit_freezer_done(UnitFreezer *f);
+UnitFreezer* unit_freezer_free(UnitFreezer *f);
+DEFINE_TRIVIAL_CLEANUP_FUNC(UnitFreezer*, unit_freezer_free);
+
+int unit_freezer_new(const char *name, UnitFreezer **ret);
 
 int unit_freezer_freeze(UnitFreezer *f);
 int unit_freezer_thaw(UnitFreezer *f);
 
-int unit_freezer_new_freeze(const char *name, UnitFreezer *ret);
-
-void unit_freezer_done_thaw(UnitFreezer *f);
+int unit_freezer_new_freeze(const char *name, UnitFreezer **ret);

diff --git a/src/sleep/sleep.c b/src/sleep/sleep.c
index c96207428dc5da10eca9a924cc6493a90f422d93..1911ef256e89d0a34cc14ed781feac01b89d9e9d 100644 (file)
--- a/src/sleep/sleep.c
+++ b/src/sleep/sleep.c
@@ -580,7 +580,7 @@ static int parse_argv(int argc, char *argv[]) {
 }
 
 static int run(int argc, char *argv[]) {
-        _cleanup_(unit_freezer_done_thaw) UnitFreezer user_slice_freezer = {};
+        _cleanup_(unit_freezer_freep) UnitFreezer *user_slice_freezer = NULL;
         _cleanup_(sleep_config_freep) SleepConfig *sleep_config = NULL;
         int r;
 
@@ -603,13 +603,9 @@ static int run(int argc, char *argv[]) {
         r = getenv_bool("SYSTEMD_SLEEP_FREEZE_USER_SESSIONS");
         if (r < 0 && r != -ENXIO)
                 log_warning_errno(r, "Cannot parse value of $SYSTEMD_SLEEP_FREEZE_USER_SESSIONS, ignoring.");
-        if (r != 0) {
-                r = unit_freezer_new_freeze(SPECIAL_USER_SLICE, &user_slice_freezer);
-                if (r < 0)
-                        log_warning_errno(r, "Failed to freeze user sessions, ignoring: %m");
-                else
-                        log_info("Froze user sessions");
-        } else
+        if (r != 0)
+                (void) unit_freezer_new_freeze(SPECIAL_USER_SLICE, &user_slice_freezer);
+        else
                 log_notice("User sessions remain unfrozen on explicit request "
                            "($SYSTEMD_SLEEP_FREEZE_USER_SESSIONS is set to false). This is not recommended, "
                            "and might result in unexpected behavior, particularly in suspend-then-hibernate "
@@ -641,6 +637,9 @@ static int run(int argc, char *argv[]) {
 
         }
 
+        if (user_slice_freezer)
+                RET_GATHER(r, unit_freezer_thaw(user_slice_freezer));
+
         return r;
 }