SymbX/systemd
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

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

Browse Source 
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.
This commit is contained in:
Mike Yuan
2024-05-30 21:51:48 +08:00
parent 759ac0dec3
commit 12904d5387
4 changed files with 87 additions and 72 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/home/homework.c
+22 -15
View File
@@ -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;
src/shared/bus-unit-util.c
+52 -40
View File
@@ -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();
r = bus_connect_system_systemd(&bus);
if (r < 0)
return log_debug_errno(r, "Failed to open connection to systemd: %m");
(void) sd_bus_set_method_call_timeout(bus, FREEZE_BUS_CALL_TIMEOUT);
*ret = (UnitFreezer) {
.name = TAKE_PTR(namedup),
.bus = TAKE_PTR(bus),
*f = (UnitFreezer) {
.name = strdup(name),
};
if (!f->name)
return log_oom();
r = bus_connect_system_systemd(&f->bus);
if (r < 0)
return log_error_errno(r, "Failed to open connection to systemd: %m");
(void) sd_bus_set_method_call_timeout(f->bus, FREEZE_BUS_CALL_TIMEOUT);
*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",
freeze ? "freeze" : "thaw", f->name, bus_error_message(&error, r));
if (r < 0) {
if (sd_bus_error_has_names(&error,
BUS_ERROR_NO_SUCH_UNIT,
BUS_ERROR_UNIT_INACTIVE,
SD_BUS_ERROR_NOT_SUPPORTED)) {
return 0;
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));
}
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);
}
src/shared/bus-unit-util.h
+6 -9
View File
@@ -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);
src/sleep/sleep.c
+7 -8
View File
@@ -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;
}