From 234621685353763ac3bea63a916df757bf06a8a6 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 24 Apr 2018 18:13:12 +0200 Subject: [PATCH 1/5] logind: modernize Manager object allocation and freeing Let's propagate errors correctly, and stick to the usual naming and behaviour of these functions. Or in other words, make this closer to the matching code in machined. --- src/login/logind-core.c | 2 ++ src/login/logind.c | 59 ++++++++++++++++++++--------------------- 2 files changed, 31 insertions(+), 30 deletions(-) diff --git a/src/login/logind-core.c b/src/login/logind-core.c index d21fa10b48..66c0796e97 100644 --- a/src/login/logind-core.c +++ b/src/login/logind-core.c @@ -26,6 +26,8 @@ #include "user-util.h" void manager_reset_config(Manager *m) { + assert(m); + m->n_autovts = 6; m->reserve_vt = 6; m->remove_ipc = true; diff --git a/src/login/logind.c b/src/login/logind.c index 3b097e1def..410f6074c6 100644 --- a/src/login/logind.c +++ b/src/login/logind.c @@ -30,15 +30,18 @@ #include "strv.h" #include "udev-util.h" -static void manager_free(Manager *m); +static Manager* manager_unref(Manager *m); +DEFINE_TRIVIAL_CLEANUP_FUNC(Manager*, manager_unref); -static Manager *manager_new(void) { - Manager *m; +static int manager_new(Manager **ret) { + _cleanup_(manager_unrefp) Manager *m = NULL; int r; + assert(ret); + m = new0(Manager, 1); if (!m) - return NULL; + return -ENOMEM; m->console_active_fd = -1; m->reserve_vt_fd = -1; @@ -56,28 +59,25 @@ static Manager *manager_new(void) { m->session_units = hashmap_new(&string_hash_ops); if (!m->devices || !m->seats || !m->sessions || !m->users || !m->inhibitors || !m->buttons || !m->user_units || !m->session_units) - goto fail; + return -ENOMEM; m->udev = udev_new(); if (!m->udev) - goto fail; + return -errno; r = sd_event_default(&m->event); if (r < 0) - goto fail; + return r; - sd_event_set_watchdog(m->event, true); + (void) sd_event_set_watchdog(m->event, true); manager_reset_config(m); - return m; - -fail: - manager_free(m); - return NULL; + *ret = TAKE_PTR(m); + return 0; } -static void manager_free(Manager *m) { +static Manager* manager_unref(Manager *m) { Session *session; User *u; Device *d; @@ -86,7 +86,7 @@ static void manager_free(Manager *m) { Button *b; if (!m) - return; + return NULL; while ((session = hashmap_first(m->sessions))) session_free(session); @@ -155,7 +155,8 @@ static void manager_free(Manager *m) { free(m->scheduled_shutdown_tty); free(m->wall_message); free(m->action_job); - free(m); + + return mfree(m); } static int manager_enumerate_devices(Manager *m) { @@ -1192,7 +1193,7 @@ static int manager_run(Manager *m) { } int main(int argc, char *argv[]) { - Manager *m = NULL; + _cleanup_(manager_unrefp) Manager *m = NULL; int r; log_set_target(LOG_TARGET_AUTO); @@ -1223,13 +1224,13 @@ int main(int argc, char *argv[]) { mkdir_label("/run/systemd/users", 0755); mkdir_label("/run/systemd/sessions", 0755); - m = manager_new(); - if (!m) { - r = log_oom(); + r = manager_new(&m); + if (r < 0) { + log_error_errno(r, "Failed to allocate manager object: %m"); goto finish; } - manager_parse_config_file(m); + (void) manager_parse_config_file(m); r = manager_startup(m); if (r < 0) { @@ -1239,20 +1240,18 @@ int main(int argc, char *argv[]) { log_debug("systemd-logind running as pid "PID_FMT, getpid_cached()); - sd_notify(false, - "READY=1\n" - "STATUS=Processing requests..."); + (void) sd_notify(false, + "READY=1\n" + "STATUS=Processing requests..."); r = manager_run(m); log_debug("systemd-logind stopped as pid "PID_FMT, getpid_cached()); + (void) sd_notify(false, + "STOPPING=1\n" + "STATUS=Shutting down..."); + finish: - sd_notify(false, - "STOPPING=1\n" - "STATUS=Shutting down..."); - - manager_free(m); - return r < 0 ? EXIT_FAILURE : EXIT_SUCCESS; } From fcfa765d189f4a9b0e30aa448cf0cd3d41a6a2a6 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 24 Apr 2018 18:14:25 +0200 Subject: [PATCH 2/5] logind: terminate cleanly on SIGTERM/SIGINT Let's properly terminate on SIGTERM or SIGINT. Previously we'd just rely on the implicit process clean-up logic on UNIX. By shutting down properly on SIGTERM/SIGINT we make it easier to track down memory leaks by employing valgrind. --- src/login/logind.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/login/logind.c b/src/login/logind.c index 410f6074c6..b0fd3f9cc5 100644 --- a/src/login/logind.c +++ b/src/login/logind.c @@ -69,6 +69,14 @@ static int manager_new(Manager **ret) { if (r < 0) return r; + r = sd_event_add_signal(m->event, NULL, SIGINT, NULL, NULL); + if (r < 0) + return r; + + r = sd_event_add_signal(m->event, NULL, SIGTERM, NULL, NULL); + if (r < 0) + return r; + (void) sd_event_set_watchdog(m->event, true); manager_reset_config(m); @@ -1084,8 +1092,6 @@ static int manager_startup(Manager *m) { assert(m); - assert_se(sigprocmask_many(SIG_SETMASK, NULL, SIGHUP, -1) >= 0); - r = sd_event_add_signal(m->event, NULL, SIGHUP, manager_dispatch_reload_signal, m); if (r < 0) return log_error_errno(r, "Failed to register SIGHUP handler: %m"); @@ -1224,6 +1230,8 @@ int main(int argc, char *argv[]) { mkdir_label("/run/systemd/users", 0755); mkdir_label("/run/systemd/sessions", 0755); + assert_se(sigprocmask_many(SIG_BLOCK, NULL, SIGHUP, SIGTERM, SIGINT, -1) >= 0); + r = manager_new(&m); if (r < 0) { log_error_errno(r, "Failed to allocate manager object: %m"); From 90b8a009a7a532b4a60ecb114466b1704156cf53 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 24 Apr 2018 18:16:14 +0200 Subject: [PATCH 3/5] logind: (void)ify all things we knowingly ignore --- src/login/logind.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/src/login/logind.c b/src/login/logind.c index b0fd3f9cc5..385c977399 100644 --- a/src/login/logind.c +++ b/src/login/logind.c @@ -1221,14 +1221,12 @@ int main(int argc, char *argv[]) { goto finish; } - /* Always create the directories people can create inotify - * watches in. Note that some applications might check for the - * existence of /run/systemd/seats/ to determine whether - * logind is available, so please always make sure this check - * stays in. */ - mkdir_label("/run/systemd/seats", 0755); - mkdir_label("/run/systemd/users", 0755); - mkdir_label("/run/systemd/sessions", 0755); + /* Always create the directories people can create inotify watches in. Note that some applications might check + * for the existence of /run/systemd/seats/ to determine whether logind is available, so please always make + * sure these directories are created early on and unconditionally. */ + (void) mkdir_label("/run/systemd/seats", 0755); + (void) mkdir_label("/run/systemd/users", 0755); + (void) mkdir_label("/run/systemd/sessions", 0755); assert_se(sigprocmask_many(SIG_BLOCK, NULL, SIGHUP, SIGTERM, SIGINT, -1) >= 0); From c8f054361b653d7cd8bf8f01ce29b8ba11e5939e Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 24 Apr 2018 18:16:44 +0200 Subject: [PATCH 4/5] machined: minor code cleanups, such as voidifying calls --- src/machine/machine.c | 1 - src/machine/machined.c | 13 +++++++++---- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/src/machine/machine.c b/src/machine/machine.c index 06dda19c43..008bbbc3f4 100644 --- a/src/machine/machine.c +++ b/src/machine/machine.c @@ -236,7 +236,6 @@ static void machine_unlink(Machine *m) { assert(m); if (m->unit) { - char *sl; sl = strjoina("/run/systemd/machines/unit:", m->unit); diff --git a/src/machine/machined.c b/src/machine/machined.c index 3577c809a4..41b4cdad6d 100644 --- a/src/machine/machined.c +++ b/src/machine/machined.c @@ -67,7 +67,8 @@ static int manager_new(Manager **ret) { static Manager* manager_unref(Manager *m) { Machine *machine; - assert(m); + if (!m) + return NULL; while (m->operations) operation_free(m->operations); @@ -387,14 +388,18 @@ int main(int argc, char *argv[]) { log_debug("systemd-machined running as pid "PID_FMT, getpid_cached()); - sd_notify(false, - "READY=1\n" - "STATUS=Processing requests..."); + (void) sd_notify(false, + "READY=1\n" + "STATUS=Processing requests..."); r = manager_run(m); log_debug("systemd-machined stopped as pid "PID_FMT, getpid_cached()); + (void) sd_notify(false, + "STOPPING=1\n" + "STATUS=Shutting down..."); + finish: return r < 0 ? EXIT_FAILURE : EXIT_SUCCESS; } From ee45b8d9ef20b4f6435b9b04718b9d5854102c99 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 24 Apr 2018 18:23:29 +0200 Subject: [PATCH 5/5] update TODO --- TODO | 1 - 1 file changed, 1 deletion(-) diff --git a/TODO b/TODO index b576521798..a772b110f1 100644 --- a/TODO +++ b/TODO @@ -626,7 +626,6 @@ Features: - logind: wakelock/opportunistic suspend support - Add pretty name for seats in logind - logind: allow showing logout dialog from system? - - we should probably handle SIGTERM/SIGINT to not leave dot files around, just in case - session scopes/user unit: add RequiresMountsFor for the home directory of the user - add Suspend() bus calls which take timestamps to fix double suspend issues when somebody hits suspend and closes laptop quickly. - if pam_systemd is invoked by su from a process that is outside of a