From bc9d1dbfc827c22ace2e5fb6755f7e7b93aba3b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 2 Jul 2020 13:44:43 +0200 Subject: [PATCH 1/6] update-utmp,initctl: drop ppid check Such checks make debugging harder but serve no useful purpose otherwise. We got rid of all the checks for root, let's kill those too. --- src/initctl/initctl.c | 5 ----- src/update-utmp/update-utmp.c | 3 --- 2 files changed, 8 deletions(-) diff --git a/src/initctl/initctl.c b/src/initctl/initctl.c index 7505512fe7..7b93de0d8a 100644 --- a/src/initctl/initctl.c +++ b/src/initctl/initctl.c @@ -341,11 +341,6 @@ int main(int argc, char *argv[]) { Server server; int r = EXIT_FAILURE, n; - if (getppid() != 1) { - log_error("This program should be invoked by init only."); - return EXIT_FAILURE; - } - if (argc > 1) { log_error("This program does not take arguments."); return EXIT_FAILURE; diff --git a/src/update-utmp/update-utmp.c b/src/update-utmp/update-utmp.c index fd24cdc789..28f1aadabc 100644 --- a/src/update-utmp/update-utmp.c +++ b/src/update-utmp/update-utmp.c @@ -223,9 +223,6 @@ static int run(int argc, char *argv[]) { }; int r; - if (getppid() != 1) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "This program should be invoked by init only."); if (argc != 2) return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "This program requires one argument."); From e9d9d50cefaae3d2db6d1f9873371290de2c7c36 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 2 Jul 2020 14:25:06 +0200 Subject: [PATCH 2/6] initctl,update-utmp: define iterator variable in loop --- src/initctl/initctl.c | 7 ++----- src/update-utmp/update-utmp.c | 3 +-- 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/src/initctl/initctl.c b/src/initctl/initctl.c index 7b93de0d8a..8e61572626 100644 --- a/src/initctl/initctl.c +++ b/src/initctl/initctl.c @@ -68,11 +68,9 @@ static const char *translate_runlevel(int runlevel, bool *isolate) { { '6', SPECIAL_REBOOT_TARGET, false }, }; - unsigned i; - assert(isolate); - for (i = 0; i < ELEMENTSOF(table); i++) + for (size_t i = 0; i < ELEMENTSOF(table); i++) if (table[i].runlevel == runlevel) { *isolate = table[i].isolate; if (runlevel == '6' && kexec_loaded()) @@ -241,7 +239,6 @@ static void server_done(Server *s) { static int server_init(Server *s, unsigned n_sockets) { int r; - unsigned i; assert(s); assert(n_sockets > 0); @@ -256,7 +253,7 @@ static int server_init(Server *s, unsigned n_sockets) { goto fail; } - for (i = 0; i < n_sockets; i++) { + for (unsigned i = 0; i < n_sockets; i++) { Fifo *f; int fd; diff --git a/src/update-utmp/update-utmp.c b/src/update-utmp/update-utmp.c index 28f1aadabc..47354d5012 100644 --- a/src/update-utmp/update-utmp.c +++ b/src/update-utmp/update-utmp.c @@ -84,11 +84,10 @@ static int get_current_runlevel(Context *c) { _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; int r; - unsigned i; assert(c); - for (i = 0; i < ELEMENTSOF(table); i++) { + for (size_t i = 0; i < ELEMENTSOF(table); i++) { _cleanup_free_ char *state = NULL, *path = NULL; path = unit_dbus_path_from_name(table[i].special); From cc090ca7fec93cd6b41bd7a756cd5fe32df44764 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 2 Jul 2020 14:50:29 +0200 Subject: [PATCH 3/6] initctl: use _cleanup_ and run() --- src/initctl/initctl.c | 133 +++++++++++++++--------------------------- 1 file changed, 46 insertions(+), 87 deletions(-) diff --git a/src/initctl/initctl.c b/src/initctl/initctl.c index 8e61572626..7eee197665 100644 --- a/src/initctl/initctl.c +++ b/src/initctl/initctl.c @@ -14,12 +14,14 @@ #include "alloc-util.h" #include "bus-error.h" #include "bus-util.h" +#include "daemon-util.h" #include "def.h" #include "fd-util.h" #include "format-util.h" #include "initreq.h" #include "list.h" #include "log.h" +#include "main-func.h" #include "memory-util.h" #include "process-util.h" #include "special.h" @@ -226,6 +228,7 @@ static void fifo_free(Fifo *f) { free(f); } +DEFINE_TRIVIAL_CLEANUP_FUNC(Fifo*, fifo_free); static void server_done(Server *s) { assert(s); @@ -240,77 +243,48 @@ static void server_done(Server *s) { static int server_init(Server *s, unsigned n_sockets) { int r; + /* This function will leave s partially initialized on failure. Caller needs to clean up. */ + assert(s); assert(n_sockets > 0); - *s = (struct Server) { - .epoll_fd = epoll_create1(EPOLL_CLOEXEC), - }; - - if (s->epoll_fd < 0) { - r = log_error_errno(errno, - "Failed to create epoll object: %m"); - goto fail; - } + s->epoll_fd = epoll_create1(EPOLL_CLOEXEC); + if (s->epoll_fd < 0) + return log_error_errno(errno, "Failed to create epoll object: %m"); for (unsigned i = 0; i < n_sockets; i++) { - Fifo *f; - int fd; - - fd = SD_LISTEN_FDS_START+i; + _cleanup_(fifo_freep) Fifo *f = NULL; + int fd = SD_LISTEN_FDS_START + i; r = sd_is_fifo(fd, NULL); - if (r < 0) { - log_error_errno(r, "Failed to determine file descriptor type: %m"); - goto fail; - } - - if (!r) { - log_error("Wrong file descriptor type."); - r = -EINVAL; - goto fail; - } + if (r < 0) + return log_error_errno(r, "Failed to determine file descriptor type: %m"); + if (!r) + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "Wrong file descriptor type."); f = new0(Fifo, 1); - if (!f) { - r = -ENOMEM; - log_error_errno(errno, "Failed to create fifo object: %m"); - goto fail; - } - - f->fd = -1; + if (!f) + return log_oom(); struct epoll_event ev = { .events = EPOLLIN, .data.ptr = f, }; - if (epoll_ctl(s->epoll_fd, EPOLL_CTL_ADD, fd, &ev) < 0) { - r = -errno; - fifo_free(f); - log_error_errno(errno, "Failed to add fifo fd to epoll object: %m"); - goto fail; - } + if (epoll_ctl(s->epoll_fd, EPOLL_CTL_ADD, fd, &ev) < 0) + return log_error_errno(errno, "Failed to add fifo fd to epoll object: %m"); f->fd = fd; - LIST_PREPEND(fifo, s->fifos, f); f->server = s; + LIST_PREPEND(fifo, s->fifos, TAKE_PTR(f)); s->n_fifos++; } r = bus_connect_system_systemd(&s->bus); - if (r < 0) { - log_error_errno(r, "Failed to get D-Bus connection: %m"); - r = -EIO; - goto fail; - } + if (r < 0) + return log_error_errno(r, "Failed to get D-Bus connection: %m"); return 0; - -fail: - server_done(s); - - return r; } static int process_event(Server *s, struct epoll_event *ev) { @@ -334,38 +308,33 @@ static int process_event(Server *s, struct epoll_event *ev) { return 0; } -int main(int argc, char *argv[]) { - Server server; - int r = EXIT_FAILURE, n; +static int run(int argc, char *argv[]) { + _cleanup_(server_done) Server server = { .epoll_fd = -1 }; + _cleanup_(notify_on_cleanup) const char *notify_stop = NULL; + int r, n; - if (argc > 1) { - log_error("This program does not take arguments."); - return EXIT_FAILURE; - } + if (argc > 1) + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), + "This program does not take arguments."); log_setup_service(); umask(0022); n = sd_listen_fds(true); - if (n < 0) { - log_error_errno(r, "Failed to read listening file descriptors from environment: %m"); - return EXIT_FAILURE; - } + if (n < 0) + return log_error_errno(errno, + "Failed to read listening file descriptors from environment: %m"); - if (n <= 0 || n > SERVER_FD_MAX) { - log_error("No or too many file descriptors passed."); - return EXIT_FAILURE; - } + if (n <= 0 || n > SERVER_FD_MAX) + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), + "No or too many file descriptors passed."); - if (server_init(&server, (unsigned) n) < 0) - return EXIT_FAILURE; + r = server_init(&server, (unsigned) n); + if (r < 0) + return r; - log_debug("systemd-initctl running as pid "PID_FMT, getpid_cached()); - - sd_notify(false, - "READY=1\n" - "STATUS=Processing requests..."); + notify_stop = notify_start(NOTIFY_READY, NOTIFY_STOPPING); while (!server.quit) { struct epoll_event event; @@ -375,27 +344,17 @@ int main(int argc, char *argv[]) { if (k < 0) { if (errno == EINTR) continue; - log_error_errno(errno, "epoll_wait() failed: %m"); - goto fail; + return log_error_errno(errno, "epoll_wait() failed: %m"); } - - if (k <= 0) + if (k == 0) break; - if (process_event(&server, &event) < 0) - goto fail; + r = process_event(&server, &event); + if (r < 0) + return r; } - r = EXIT_SUCCESS; - - log_debug("systemd-initctl stopped as pid "PID_FMT, getpid_cached()); - -fail: - sd_notify(false, - "STOPPING=1\n" - "STATUS=Shutting down..."); - - server_done(&server); - - return r; + return 0; } + +DEFINE_MAIN_FUNCTION(run); From 272ac70a21165b6220f177a5f5db3c7863f7729c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 2 Jul 2020 14:56:34 +0200 Subject: [PATCH 4/6] various daemons: emit Stopping... notification before destructing the manager object This is mostly cosmetic, but let's reorder the destructors so that we do the final sd_notify() call before we run the destructor for the manager object. --- src/home/homed.c | 2 +- src/journal-remote/journal-remote-main.c | 2 +- src/journal-remote/journal-upload.c | 2 +- src/network/networkd.c | 2 +- src/network/wait-online/wait-online.c | 2 +- src/resolve/resolved.c | 2 +- src/timesync/timesyncd.c | 2 +- src/userdb/userdbd.c | 2 +- 8 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/home/homed.c b/src/home/homed.c index 2fd3b65d89..ed8404d7e3 100644 --- a/src/home/homed.c +++ b/src/home/homed.c @@ -13,8 +13,8 @@ #include "signal-util.h" static int run(int argc, char *argv[]) { - _cleanup_(notify_on_cleanup) const char *notify_stop = NULL; _cleanup_(manager_freep) Manager *m = NULL; + _cleanup_(notify_on_cleanup) const char *notify_stop = NULL; int r; log_setup_service(); diff --git a/src/journal-remote/journal-remote-main.c b/src/journal-remote/journal-remote-main.c index f82d188a8c..273fdf9196 100644 --- a/src/journal-remote/journal-remote-main.c +++ b/src/journal-remote/journal-remote-main.c @@ -1104,8 +1104,8 @@ static int load_certificates(char **key, char **cert, char **trust) { } static int run(int argc, char **argv) { - _cleanup_(notify_on_cleanup) const char *notify_message = NULL; _cleanup_(journal_remote_server_destroy) RemoteServer s = {}; + _cleanup_(notify_on_cleanup) const char *notify_message = NULL; _cleanup_free_ char *key = NULL, *cert = NULL, *trust = NULL; int r; diff --git a/src/journal-remote/journal-upload.c b/src/journal-remote/journal-upload.c index e169a27eb3..13ca90f957 100644 --- a/src/journal-remote/journal-upload.c +++ b/src/journal-remote/journal-upload.c @@ -815,8 +815,8 @@ static int open_journal(sd_journal **j) { } static int run(int argc, char **argv) { - _cleanup_(notify_on_cleanup) const char *notify_message = NULL; _cleanup_(destroy_uploader) Uploader u = {}; + _cleanup_(notify_on_cleanup) const char *notify_message = NULL; bool use_journal; int r; diff --git a/src/network/networkd.c b/src/network/networkd.c index 35f22fea80..445aee16ad 100644 --- a/src/network/networkd.c +++ b/src/network/networkd.c @@ -17,8 +17,8 @@ #include "user-util.h" static int run(int argc, char *argv[]) { - _cleanup_(notify_on_cleanup) const char *notify_message = NULL; _cleanup_(manager_freep) Manager *m = NULL; + _cleanup_(notify_on_cleanup) const char *notify_message = NULL; int r; log_setup_service(); diff --git a/src/network/wait-online/wait-online.c b/src/network/wait-online/wait-online.c index 17ed5d38cf..cfd9093f1a 100644 --- a/src/network/wait-online/wait-online.c +++ b/src/network/wait-online/wait-online.c @@ -183,8 +183,8 @@ static int parse_argv(int argc, char *argv[]) { } static int run(int argc, char *argv[]) { - _cleanup_(notify_on_cleanup) const char *notify_message = NULL; _cleanup_(manager_freep) Manager *m = NULL; + _cleanup_(notify_on_cleanup) const char *notify_message = NULL; int r; log_setup_service(); diff --git a/src/resolve/resolved.c b/src/resolve/resolved.c index 566b950a63..16477f28d6 100644 --- a/src/resolve/resolved.c +++ b/src/resolve/resolved.c @@ -22,8 +22,8 @@ #include "user-util.h" static int run(int argc, char *argv[]) { - _cleanup_(notify_on_cleanup) const char *notify_stop = NULL; _cleanup_(manager_freep) Manager *m = NULL; + _cleanup_(notify_on_cleanup) const char *notify_stop = NULL; int r; log_setup_service(); diff --git a/src/timesync/timesyncd.c b/src/timesync/timesyncd.c index e56e09ca8c..4f514d536d 100644 --- a/src/timesync/timesyncd.c +++ b/src/timesync/timesyncd.c @@ -89,8 +89,8 @@ settime: } static int run(int argc, char *argv[]) { - _cleanup_(notify_on_cleanup) const char *notify_message = NULL; _cleanup_(manager_freep) Manager *m = NULL; + _cleanup_(notify_on_cleanup) const char *notify_message = NULL; const char *user = "systemd-timesync"; uid_t uid, uid_current; gid_t gid; diff --git a/src/userdb/userdbd.c b/src/userdb/userdbd.c index 978fd1d788..dbc285e61a 100644 --- a/src/userdb/userdbd.c +++ b/src/userdb/userdbd.c @@ -20,8 +20,8 @@ */ static int run(int argc, char *argv[]) { - _cleanup_(notify_on_cleanup) const char *notify_stop = NULL; _cleanup_(manager_freep) Manager *m = NULL; + _cleanup_(notify_on_cleanup) const char *notify_stop = NULL; int r; log_setup_service(); From c2a99093777aaa2e639261b66feddd1405f236a3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 2 Jul 2020 16:24:04 +0200 Subject: [PATCH 5/6] Define _cleanup_ helper for setutxent()+endutxent() --- src/login/logind-core.c | 24 ++++++++---------------- src/shared/utmp-wtmp.c | 37 +++++++++++-------------------------- src/shared/utmp-wtmp.h | 11 +++++++++++ 3 files changed, 30 insertions(+), 42 deletions(-) diff --git a/src/login/logind-core.c b/src/login/logind-core.c index 475759c79b..480ec1927b 100644 --- a/src/login/logind-core.c +++ b/src/login/logind-core.c @@ -4,9 +4,6 @@ #include #include #include -#if ENABLE_UTMP -#include -#endif #include "sd-device.h" @@ -29,6 +26,7 @@ #include "udev-util.h" #include "user-util.h" #include "userdb.h" +#include "utmp-wtmp.h" void manager_reset_config(Manager *m) { assert(m); @@ -685,13 +683,14 @@ bool manager_all_buttons_ignored(Manager *m) { int manager_read_utmp(Manager *m) { #if ENABLE_UTMP int r; + _cleanup_(utxent_cleanup) bool utmpx = false; assert(m); if (utmpxname(_PATH_UTMPX) < 0) return log_error_errno(errno, "Failed to set utmp path to " _PATH_UTMPX ": %m"); - setutxent(); + utmpx = utxent_start(); for (;;) { _cleanup_free_ char *t = NULL; @@ -704,8 +703,7 @@ int manager_read_utmp(Manager *m) { if (!u) { if (errno != 0) log_warning_errno(errno, "Failed to read " _PATH_UTMPX ", ignoring: %m"); - r = 0; - break; + return 0; } if (u->ut_type != USER_PROCESS) @@ -715,18 +713,14 @@ int manager_read_utmp(Manager *m) { continue; t = strndup(u->ut_line, sizeof(u->ut_line)); - if (!t) { - r = log_oom(); - break; - } + if (!t) + return log_oom(); c = path_startswith(t, "/dev/"); if (c) { r = free_and_strdup(&t, c); - if (r < 0) { - log_oom(); - break; - } + if (r < 0) + return log_oom(); } if (isempty(t)) @@ -756,8 +750,6 @@ int manager_read_utmp(Manager *m) { log_debug("Acquired TTY information '%s' from utmp for session '%s'.", s->tty, s->id); } - endutxent(); - return r; #else return 0; #endif diff --git a/src/shared/utmp-wtmp.c b/src/shared/utmp-wtmp.c index 9debb54390..096f99ec1a 100644 --- a/src/shared/utmp-wtmp.c +++ b/src/shared/utmp-wtmp.c @@ -25,8 +25,8 @@ #include "utmp-wtmp.h" int utmp_get_runlevel(int *runlevel, int *previous) { + _cleanup_(utxent_cleanup) bool utmpx = false; struct utmpx *found, lookup = { .ut_type = RUN_LVL }; - int r; const char *e; assert(runlevel); @@ -58,27 +58,17 @@ int utmp_get_runlevel(int *runlevel, int *previous) { if (utmpxname(_PATH_UTMPX) < 0) return -errno; - setutxent(); + utmpx = utxent_start(); found = getutxid(&lookup); if (!found) - r = -errno; - else { - int a, b; + return -errno; - a = found->ut_pid & 0xFF; - b = (found->ut_pid >> 8) & 0xFF; + *runlevel = found->ut_pid & 0xFF; + if (previous) + *previous = (found->ut_pid >> 8) & 0xFF; - *runlevel = a; - if (previous) - *previous = b; - - r = 0; - } - - endutxent(); - - return r; + return 0; } static void init_timestamp(struct utmpx *store, usec_t t) { @@ -106,7 +96,7 @@ static void init_entry(struct utmpx *store, usec_t t) { } static int write_entry_utmp(const struct utmpx *store) { - int r; + _cleanup_(utxent_cleanup) bool utmpx = false; assert(store); @@ -117,16 +107,11 @@ static int write_entry_utmp(const struct utmpx *store) { if (utmpxname(_PATH_UTMPX) < 0) return -errno; - setutxent(); + utmpx = utxent_start(); if (!pututxline(store)) - r = -errno; - else - r = 0; - - endutxent(); - - return r; + return -errno; + return 0; } static int write_entry_wtmp(const struct utmpx *store) { diff --git a/src/shared/utmp-wtmp.h b/src/shared/utmp-wtmp.h index 9e433cf73e..fe55bad12d 100644 --- a/src/shared/utmp-wtmp.h +++ b/src/shared/utmp-wtmp.h @@ -8,6 +8,8 @@ #include "util.h" #if ENABLE_UTMP +#include + int utmp_get_runlevel(int *runlevel, int *previous); int utmp_put_shutdown(void); @@ -24,6 +26,15 @@ int utmp_wall( bool (*match_tty)(const char *tty, void *userdata), void *userdata); +static inline bool utxent_start(void) { + setutxent(); + return true; +} +static inline void utxent_cleanup(bool *initialized) { + if (initialized) + endutxent(); +} + #else /* ENABLE_UTMP */ static inline int utmp_get_runlevel(int *runlevel, int *previous) { From ca1d199b4827c710769c4e0b16fd22a0c48b373b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 2 Jul 2020 16:35:52 +0200 Subject: [PATCH 6/6] update-utmp: do not fail on EROFS Right now systemd-update-utmp.service would fail on read-only /var because it was not able to write the wtmp record. But it still writes the utmp record just fine, so runtime information is OK. I don't think we need to make too much fuss about not being able to save wtmp info. --- src/shared/utmp-wtmp.c | 36 ++++++++++++++++++++---------------- 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/src/shared/utmp-wtmp.c b/src/shared/utmp-wtmp.c index 096f99ec1a..9edcd8302a 100644 --- a/src/shared/utmp-wtmp.c +++ b/src/shared/utmp-wtmp.c @@ -35,8 +35,7 @@ int utmp_get_runlevel(int *runlevel, int *previous) { * precedence. Presumably, sysvinit does this to work around a * race condition that would otherwise exist where we'd always * go to disk and hence might read runlevel data that might be - * very new and does not apply to the current script being - * executed. */ + * very new and not apply to the current script being executed. */ e = getenv("RUNLEVEL"); if (e && e[0] > 0) { @@ -109,19 +108,33 @@ static int write_entry_utmp(const struct utmpx *store) { utmpx = utxent_start(); - if (!pututxline(store)) - return -errno; - return 0; + if (pututxline(store)) + return 0; + if (errno == ENOENT) { + /* If utmp/wtmp have been disabled, that's a good thing, hence ignore the error. */ + log_debug_errno(errno, "Not writing utmp: %m"); + return 0; + } + return -errno; } static int write_entry_wtmp(const struct utmpx *store) { assert(store); /* wtmp is a simple append-only file where each entry is - simply appended to the end; i.e. basically a log. */ + * simply appended to the end; i.e. basically a log. */ errno = 0; updwtmpx(_PATH_WTMPX, store); + if (errno == ENOENT) { + /* If utmp/wtmp have been disabled, that's a good thing, hence ignore the error. */ + log_debug_errno(errno, "Not writing wtmp: %m"); + return 0; + } + if (errno == EROFS) { + log_warning_errno(errno, "Failed to write wtmp record, ignoring: %m"); + return 0; + } return -errno; } @@ -130,16 +143,7 @@ static int write_utmp_wtmp(const struct utmpx *store_utmp, const struct utmpx *s r = write_entry_utmp(store_utmp); s = write_entry_wtmp(store_wtmp); - - if (r >= 0) - r = s; - - /* If utmp/wtmp have been disabled, that's a good thing, hence - * ignore the errors */ - if (r == -ENOENT) - r = 0; - - return r; + return r < 0 ? r : s; } static int write_entry_both(const struct utmpx *store) {