From 8f41e6b63608f5dcd320655d7cbc000179ae798f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 13 Jul 2022 14:49:42 +0200 Subject: [PATCH 01/15] core: wrap long comments and capitalize sentences --- src/core/manager.c | 73 ++++++++++++++++++++++++---------------------- 1 file changed, 38 insertions(+), 35 deletions(-) diff --git a/src/core/manager.c b/src/core/manager.c index e7e077dcf0..45f6dccf4a 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -2530,14 +2530,14 @@ static int manager_dispatch_notify_fd(sd_event_source *source, int fd, uint32_t return 0; } - /* As extra safety check, let's make sure the string we get doesn't contain embedded NUL bytes. We permit one - * trailing NUL byte in the message, but don't expect it. */ + /* As extra safety check, let's make sure the string we get doesn't contain embedded NUL bytes. + * We permit one trailing NUL byte in the message, but don't expect it. */ if (n > 1 && memchr(buf, 0, n-1)) { log_warning("Received notify message with embedded NUL bytes. Ignoring."); return 0; } - /* Make sure it's NUL-terminated, then parse it to obtain the tags list */ + /* Make sure it's NUL-terminated, then parse it to obtain the tags list. */ buf[n] = 0; tags = strv_split_newlines(buf); if (!tags) { @@ -2545,7 +2545,7 @@ static int manager_dispatch_notify_fd(sd_event_source *source, int fd, uint32_t return 0; } - /* possibly a barrier fd, let's see */ + /* Possibly a barrier fd, let's see. */ if (manager_process_barrier_fd(tags, fds)) return 0; @@ -2566,8 +2566,8 @@ static int manager_dispatch_notify_fd(sd_event_source *source, int fd, uint32_t if (!array_copy) log_oom(); } - /* And now invoke the per-unit callbacks. Note that manager_invoke_notify_message() will handle duplicate units - * make sure we only invoke each unit's handler once. */ + /* And now invoke the per-unit callbacks. Note that manager_invoke_notify_message() will handle + * duplicate units make sure we only invoke each unit's handler once. */ if (u1) { manager_invoke_notify_message(m, u1, ucred, tags, fds); found = true; @@ -2620,8 +2620,8 @@ static int manager_dispatch_sigchld(sd_event_source *source, void *userdata) { assert(source); assert(m); - /* First we call waitid() for a PID and do not reap the zombie. That way we can still access /proc/$PID for it - * while it is a zombie. */ + /* First we call waitid() for a PID and do not reap the zombie. That way we can still access + * /proc/$PID for it while it is a zombie. */ if (waitid(P_ALL, 0, &si, WEXITED|WNOHANG|WNOWAIT) < 0) { @@ -2988,15 +2988,15 @@ static int manager_dispatch_idle_pipe_fd(sd_event_source *source, int fd, uint32 assert(m); assert(m->idle_pipe[2] == fd); - /* There's at least one Type=idle child that just gave up on us waiting for the boot process to complete. Let's - * now turn off any further console output if there's at least one service that needs console access, so that - * from now on our own output should not spill into that service's output anymore. After all, we support - * Type=idle only to beautify console output and it generally is set on services that want to own the console - * exclusively without our interference. */ + /* There's at least one Type=idle child that just gave up on us waiting for the boot process to + * complete. Let's now turn off any further console output if there's at least one service that needs + * console access, so that from now on our own output should not spill into that service's output + * anymore. After all, we support Type=idle only to beautify console output and it generally is set + * on services that want to own the console exclusively without our interference. */ m->no_console_output = m->n_on_console > 0; - /* Acknowledge the child's request, and let all all other children know too that they shouldn't wait any longer - * by closing the pipes towards them, which is what they are waiting for. */ + /* Acknowledge the child's request, and let all all other children know too that they shouldn't wait + * any longer by closing the pipes towards them, which is what they are waiting for. */ manager_close_idle_pipe(m); return 0; @@ -3092,8 +3092,8 @@ int manager_load_unit_from_dbus_path(Manager *m, const char *s, sd_bus_error *e, if (r < 0) return r; - /* Permit addressing units by invocation ID: if the passed bus path is suffixed by a 128bit ID then we use it - * as invocation ID. */ + /* Permit addressing units by invocation ID: if the passed bus path is suffixed by a 128bit ID then + * we use it as invocation ID. */ r = sd_id128_from_string(n, &invocation_id); if (r >= 0) { u = hashmap_get(m->units_by_invocation_id, &invocation_id); @@ -3915,10 +3915,10 @@ int manager_set_default_rlimits(Manager *m, struct rlimit **default_rlimit) { void manager_recheck_dbus(Manager *m) { assert(m); - /* Connects to the bus if the dbus service and socket are running. If we are running in user mode this is all - * it does. In system mode we'll also connect to the system bus (which will most likely just reuse the - * connection of the API bus). That's because the system bus after all runs as service of the system instance, - * while in the user instance we can assume it's already there. */ + /* Connects to the bus if the dbus service and socket are running. If we are running in user mode + * this is all it does. In system mode we'll also connect to the system bus (which will most likely + * just reuse the connection of the API bus). That's because the system bus after all runs as service + * of the system instance, while in the user instance we can assume it's already there. */ if (MANAGER_IS_RELOADING(m)) return; /* don't check while we are reloading… */ @@ -3990,9 +3990,9 @@ void manager_recheck_journal(Manager *m) { if (MANAGER_IS_RELOADING(m)) return; - /* The journal is fully and entirely up? If so, let's permit logging to it, if that's configured. If the - * journal is down, don't ever log to it, otherwise we might end up deadlocking ourselves as we might trigger - * an activation ourselves we can't fulfill. */ + /* The journal is fully and entirely up? If so, let's permit logging to it, if that's configured. If + * the journal is down, don't ever log to it, otherwise we might end up deadlocking ourselves as we + * might trigger an activation ourselves we can't fulfill. */ log_set_prohibit_ipc(!manager_journal_is_running(m)); log_open(); } @@ -4264,13 +4264,14 @@ static void manager_unref_uid_internal( assert(uid_is_valid(uid)); assert(_clean_ipc); - /* A generic implementation, covering both manager_unref_uid() and manager_unref_gid(), under the assumption - * that uid_t and gid_t are actually defined the same way, with the same validity rules. + /* A generic implementation, covering both manager_unref_uid() and manager_unref_gid(), under the + * assumption that uid_t and gid_t are actually defined the same way, with the same validity rules. * - * We store a hashmap where the key is the UID/GID and the value is a 32bit reference counter, whose highest - * bit is used as flag for marking UIDs/GIDs whose IPC objects to remove when the last reference to the UID/GID - * is dropped. The flag is set to on, once at least one reference from a unit where RemoveIPC= is set is added - * on a UID/GID. It is reset when the UID's/GID's reference counter drops to 0 again. */ + * We store a hashmap where the key is the UID/GID and the value is a 32bit reference counter, whose + * highest bit is used as flag for marking UIDs/GIDs whose IPC objects to remove when the last + * reference to the UID/GID is dropped. The flag is set to on, once at least one reference from a + * unit where RemoveIPC= is set is added on a UID/GID. It is reset when the UID's/GID's reference + * counter drops to 0 again. */ assert_cc(sizeof(uid_t) == sizeof(gid_t)); assert_cc(UID_INVALID == (uid_t) GID_INVALID); @@ -4318,8 +4319,9 @@ static int manager_ref_uid_internal( assert(uid_refs); assert(uid_is_valid(uid)); - /* A generic implementation, covering both manager_ref_uid() and manager_ref_gid(), under the assumption - * that uid_t and gid_t are actually defined the same way, with the same validity rules. */ + /* A generic implementation, covering both manager_ref_uid() and manager_ref_gid(), under the + * assumption that uid_t and gid_t are actually defined the same way, with the same validity + * rules. */ assert_cc(sizeof(uid_t) == sizeof(gid_t)); assert_cc(UID_INVALID == (uid_t) GID_INVALID); @@ -4419,9 +4421,10 @@ int manager_dispatch_user_lookup_fd(sd_event_source *source, int fd, uint32_t re assert_se(source); assert_se(m); - /* Invoked whenever a child process succeeded resolving its user/group to use and sent us the resulting UID/GID - * in a datagram. We parse the datagram here and pass it off to the unit, so that it can add a reference to the - * UID/GID so that it can destroy the UID/GID's IPC objects when the reference counter drops to 0. */ + /* Invoked whenever a child process succeeded resolving its user/group to use and sent us the + * resulting UID/GID in a datagram. We parse the datagram here and pass it off to the unit, so that + * it can add a reference to the UID/GID so that it can destroy the UID/GID's IPC objects when the + * reference counter drops to 0. */ l = recv(fd, &buffer, sizeof(buffer), MSG_DONTWAIT); if (l < 0) { From 9cfc294fe0e2637d96f8e5c29143c10e2173daa3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 13 Jul 2022 15:02:39 +0200 Subject: [PATCH 02/15] man: fix formatting of "BARRIER=1" Whitespace inside of the field was propagated to the displayed form, causing strange indentation. --- man/sd_notify.xml | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/man/sd_notify.xml b/man/sd_notify.xml index 4a0a7b34dc..31388b9c3d 100644 --- a/man/sd_notify.xml +++ b/man/sd_notify.xml @@ -272,13 +272,14 @@ BARRIER=1 - Tells the service manager that the client is explicitly requesting synchronization by means of - closing the file descriptor sent with this command. The service manager guarantees that the processing of a - BARRIER=1 command will only happen after all previous notification messages sent before this command - have been processed. Hence, this command accompanied with a single file descriptor can be used to synchronize - against reception of all previous status messages. Note that this command cannot be mixed with other notifications, - and has to be sent in a separate message to the service manager, otherwise all assignments will be ignored. Note that - sending 0 or more than 1 file descriptor with this command is a violation of the protocol. + Tells the service manager that the client is explicitly requesting synchronization by + means of closing the file descriptor sent with this command. The service manager guarantees that the + processing of a BARRIER=1 command will only happen after all previous notification + messages sent before this command have been processed. Hence, this command accompanied with a single + file descriptor can be used to synchronize against reception of all previous status messages. Note + that this command cannot be mixed with other notifications, and has to be sent in a separate message + to the service manager, otherwise all assignments will be ignored. Note that sending 0 or more than 1 + file descriptor with this command is a violation of the protocol. @@ -341,7 +342,7 @@ sd_notify_barrier() allows the caller to synchronize against reception of previously sent notification messages - and uses the BARRIER=1 command. It takes a relative + and uses the BARRIER=1 command. It takes a relative timeout value in microseconds which is passed to ppoll2 . A value of UINT64_MAX is interpreted as infinite timeout. From 78f8ce6d9086b93d6a806de7035c3826f22319c7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 13 Jul 2022 18:07:24 +0200 Subject: [PATCH 03/15] environment-d-generator: use DEFINE_MAIN_FUNCTION() --- .../environment-d-generator.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/environment-d-generator/environment-d-generator.c b/src/environment-d-generator/environment-d-generator.c index 12aafbc7d9..e3fea1f2b0 100644 --- a/src/environment-d-generator/environment-d-generator.c +++ b/src/environment-d-generator/environment-d-generator.c @@ -8,6 +8,7 @@ #include "escape.h" #include "glyph-util.h" #include "log.h" +#include "main-func.h" #include "path-lookup.h" #include "strv.h" @@ -80,20 +81,19 @@ static int load_and_print(void) { return 0; } -int main(int argc, char *argv[]) { +static int run(int argc, char *argv[]) { int r; log_parse_environment(); log_open(); - if (argc > 1) { - log_error("This program takes no arguments."); - return EXIT_FAILURE; - } + if (argc > 1) + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "This program takes no arguments."); r = load_and_print(); if (r < 0) - log_error_errno(r, "Failed to load environment.d: %m"); - - return r < 0 ? EXIT_FAILURE : EXIT_SUCCESS; + return log_error_errno(r, "Failed to load environment.d: %m"); + return 0; } + +DEFINE_MAIN_FUNCTION(run); From b8110a3eb8cb8a54626ee0ae440da83096f2b07d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 13 Jul 2022 18:29:36 +0200 Subject: [PATCH 04/15] hibernate-resume-generator: use DEFINE_MAIN_GENERATOR_FUNCTION() DEFINE_MAIN_GENERATOR_FUNCTION() always sets dest*, so there should be no change in behaviour. --- src/hibernate-resume/hibernate-resume-generator.c | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/src/hibernate-resume/hibernate-resume-generator.c b/src/hibernate-resume/hibernate-resume-generator.c index ee320909ee..f591407f78 100644 --- a/src/hibernate-resume/hibernate-resume-generator.c +++ b/src/hibernate-resume/hibernate-resume-generator.c @@ -16,7 +16,7 @@ #include "string-util.h" #include "unit-name.h" -static const char *arg_dest = "/tmp"; +static const char *arg_dest = NULL; static char *arg_resume_device = NULL; static char *arg_resume_options = NULL; static char *arg_root_options = NULL; @@ -109,17 +109,10 @@ static int process_resume(void) { return 0; } -static int run(int argc, char *argv[]) { +static int run(const char *dest, const char *dest_early, const char *dest_late) { int r = 0; - log_setup_generator(); - - if (argc > 1 && argc != 4) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "This program takes three or no arguments."); - - if (argc > 1) - arg_dest = argv[1]; + arg_dest = ASSERT_PTR(dest); /* Don't even consider resuming outside of initramfs. */ if (!in_initrd()) { @@ -139,4 +132,4 @@ static int run(int argc, char *argv[]) { return process_resume(); } -DEFINE_MAIN_FUNCTION(run); +DEFINE_MAIN_GENERATOR_FUNCTION(run); From b711a9ee185dfffe8ef05d145cdbd6253986ba91 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 13 Jul 2022 18:35:42 +0200 Subject: [PATCH 05/15] bless-boot-generator: use DEFINE_MAIN_GENERATOR_FUNCTION() DEFINE_MAIN_GENERATOR_FUNCTION() always sets dest*, so there should be no change in behaviour. --- src/boot/bless-boot-generator.c | 50 +++++++++++---------------------- 1 file changed, 17 insertions(+), 33 deletions(-) diff --git a/src/boot/bless-boot-generator.c b/src/boot/bless-boot-generator.c index 7209b28e67..6adef5b3dc 100644 --- a/src/boot/bless-boot-generator.c +++ b/src/boot/bless-boot-generator.c @@ -1,7 +1,6 @@ /* SPDX-License-Identifier: LGPL-2.1-or-later */ #include -#include #include #include "efi-loader.h" @@ -10,62 +9,47 @@ #include "mkdir.h" #include "special.h" #include "string-util.h" -#include "util.h" #include "virt.h" -/* This generator pulls systemd-bless-boot.service into the initial transaction if the "LoaderBootCountPath" EFI - * variable is set, i.e. the system boots up with boot counting in effect, which means we should mark the boot as - * "good" if we manage to boot up far enough. */ +/* This generator pulls systemd-bless-boot.service into the initial transaction if the "LoaderBootCountPath" + * EFI variable is set, i.e. the system boots up with boot counting in effect, which means we should mark the + * boot as "good" if we manage to boot up far enough. */ -static const char *arg_dest = "/tmp"; - -int main(int argc, char *argv[]) { - const char *p; - - log_setup_generator(); - - if (argc > 1 && argc != 4) { - log_error("This program takes three or no arguments."); - return EXIT_FAILURE; - } - - if (argc > 1) - arg_dest = argv[2]; +static int run(const char *dest, const char *dest_early, const char *dest_late) { if (in_initrd() > 0) { log_debug("Skipping generator, running in the initrd."); - return EXIT_SUCCESS; + return 0; } if (detect_container() > 0) { log_debug("Skipping generator, running in a container."); - return EXIT_SUCCESS; + return 0; } if (!is_efi_boot()) { log_debug("Skipping generator, not an EFI boot."); - return EXIT_SUCCESS; + return 0; } if (access(EFIVAR_PATH(EFI_LOADER_VARIABLE(LoaderBootCountPath)), F_OK) < 0) { if (errno == ENOENT) { log_debug_errno(errno, "Skipping generator, not booted with boot counting in effect."); - return EXIT_SUCCESS; + return 0; } - log_error_errno(errno, "Failed to check if LoaderBootCountPath EFI variable exists: %m"); - return EXIT_FAILURE; + return log_error_errno(errno, "Failed to check if LoaderBootCountPath EFI variable exists: %m"); } - /* We pull this in from basic.target so that it ends up in all "regular" boot ups, but not in rescue.target or - * even emergency.target. */ - p = strjoina(arg_dest, "/" SPECIAL_BASIC_TARGET ".wants/systemd-bless-boot.service"); + /* We pull this in from basic.target so that it ends up in all "regular" boot ups, but not in + * rescue.target or even emergency.target. */ + const char *p = strjoina(dest_early, "/" SPECIAL_BASIC_TARGET ".wants/systemd-bless-boot.service"); (void) mkdir_parents(p, 0755); - if (symlink(SYSTEM_DATA_UNIT_DIR "/systemd-bless-boot.service", p) < 0) { - log_error_errno(errno, "Failed to create symlink '%s': %m", p); - return EXIT_FAILURE; - } + if (symlink(SYSTEM_DATA_UNIT_DIR "/systemd-bless-boot.service", p) < 0) + return log_error_errno(errno, "Failed to create symlink '%s': %m", p); - return EXIT_SUCCESS; + return 0; } + +DEFINE_MAIN_GENERATOR_FUNCTION(run); From 17021368fc5b9b3800125414362e82daf43a785d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 13 Jul 2022 18:19:04 +0200 Subject: [PATCH 06/15] generators: accept one or three args, do not write to /tmp Since the general generator logic was established in the rewrite in 07719a21b6425d378b36bb8d7f47ad5ec5296d28, generators would always write to /tmp by default. I think this not a good default at all, because generators write a bunch of files and would create a mess in /tmp. And for debugging, one generally needs to remove all the files in the output directory, because generators will complain in the output paths are already present. Thus the approach of disabling console logging and writing many files to /tmp when invoked with no arguments is not nice, so let's disallow operation with no args. But when debugging, one generally does not care about the separate output dirs (most generators use only one). Thus the general pattern I use is something like: rm -rf /tmp/x && mkdir /tmp/x && build/some-generator /tmp/{x,x,x} This commit allows only one directory to be specified and simplifies this to: rm -rf /tmp/x && mkdir /tmp/x && build/some-generator /tmp/x --- man/systemd.generator.xml | 21 +++++++++++---------- src/shared/generator.h | 11 ++++++----- 2 files changed, 17 insertions(+), 15 deletions(-) diff --git a/man/systemd.generator.xml b/man/systemd.generator.xml index 1f916ac65e..d837afb6f9 100644 --- a/man/systemd.generator.xml +++ b/man/systemd.generator.xml @@ -26,8 +26,8 @@ /path/to/generator normal-dir - early-dir - late-dir + early-dir + late-dir @@ -56,13 +56,13 @@ that they can extend the unit file hierarchy the service manager subsequently loads and operates on. - Each generator is called with three directory paths that are to be used for generator output. In - these three directories, generators may dynamically generate unit files (regular ones, instances, as well - as templates), unit file .d/ drop-ins, and create symbolic links to unit files to - add additional dependencies, create aliases, or instantiate existing templates. Those directories are - included in the unit load path of - systemd1, allowing - generated configuration to extend or override existing definitions. + systemd will call each generator with three directory paths that are to be used + for generator output. In these three directories, generators may dynamically generate unit files (regular + ones, instances, as well as templates), unit file .d/ drop-ins, and create symbolic + links to unit files to add additional dependencies, create aliases, or instantiate existing templates. + Those directories are included in the unit load path, allowing generated configuration to extend or + override existing definitions. For tests, generators may be called with just one argument; the generator + should assume that all three paths are the same in that case. Directory paths for generator output differ by priority: …/generator.early has priority higher than the admin configuration in /etc/, while @@ -96,7 +96,8 @@ Generators are invoked with three arguments: paths to directories where generators can place their generated unit files or symlinks. By default those paths are runtime directories that are included in the search path of systemd, but a generator may be called with different paths for - debugging purposes. + debugging purposes. If only one argument is provided, the generator should use the same directory as the + the three output paths. diff --git a/src/shared/generator.h b/src/shared/generator.h index bd768daf49..1b4f36ac53 100644 --- a/src/shared/generator.h +++ b/src/shared/generator.h @@ -3,6 +3,7 @@ #include +#include "macro.h" #include "main-func.h" int generator_open_unit_file( @@ -86,11 +87,11 @@ void log_setup_generator(void); _DEFINE_MAIN_FUNCTION( \ ({ \ log_setup_generator(); \ - if (argc > 1 && argc != 4) \ + if (!IN_SET(argc, 2, 4)) \ return log_error_errno(SYNTHETIC_ERRNO(EINVAL), \ - "This program takes zero or three arguments."); \ + "This program takes one or three arguments."); \ }), \ - impl(argc > 1 ? argv[1] : "/tmp", \ - argc > 1 ? argv[2] : "/tmp", \ - argc > 1 ? argv[3] : "/tmp"), \ + impl(argv[1], \ + argv[argc == 4 ? 2 : 1], \ + argv[argc == 4 ? 3 : 1]), \ r < 0 ? EXIT_FAILURE : EXIT_SUCCESS) From 494f4ee9c770e401ea3d570a9caf4b3058cf7758 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 15 Jul 2022 11:38:01 +0200 Subject: [PATCH 07/15] basic/log: split out invoked_by_systemd() utility function --- src/basic/log.c | 22 ++-------------------- src/basic/process-util.c | 24 ++++++++++++++++++++++++ src/basic/process-util.h | 2 ++ 3 files changed, 28 insertions(+), 20 deletions(-) diff --git a/src/basic/log.c b/src/basic/log.c index 02fc2c6f92..cac17a76f4 100644 --- a/src/basic/log.c +++ b/src/basic/log.c @@ -1153,30 +1153,12 @@ static int parse_proc_cmdline_item(const char *key, const char *value, void *dat } static bool should_parse_proc_cmdline(void) { - const char *e; - pid_t p; - /* PID1 always reads the kernel command line. */ if (getpid_cached() == 1) return true; - /* If the process is directly executed by PID1 (e.g. ExecStart= or generator), systemd-importd, - * or systemd-homed, then $SYSTEMD_EXEC_PID= is set, and read the command line. */ - e = getenv("SYSTEMD_EXEC_PID"); - if (!e) - return false; - - if (streq(e, "*")) - /* For testing. */ - return true; - - if (parse_pid(e, &p) < 0) { - /* We know that systemd sets the variable correctly. Something else must have set it. */ - log_debug("Failed to parse \"$SYSTEMD_EXEC_PID=%s\". Ignoring.", e); - return false; - } - - return getpid_cached() == p; + /* Otherwise, parse the commandline if invoked directly by systemd. */ + return invoked_by_systemd(); } void log_parse_environment_variables(void) { diff --git a/src/basic/process-util.c b/src/basic/process-util.c index 6980e0c4f6..d885d92dfb 100644 --- a/src/basic/process-util.c +++ b/src/basic/process-util.c @@ -1579,6 +1579,30 @@ bool invoked_as(char *argv[], const char *token) { return strstr(last_path_component(argv[0]), token); } +bool invoked_by_systemd(void) { + int r; + + /* If the process is directly executed by PID1 (e.g. ExecStart= or generator), systemd-importd, + * or systemd-homed, then $SYSTEMD_EXEC_PID= is set, and read the command line. */ + const char *e = getenv("SYSTEMD_EXEC_PID"); + if (!e) + return false; + + if (streq(e, "*")) + /* For testing. */ + return true; + + pid_t p; + r = parse_pid(e, &p); + if (r < 0) { + /* We know that systemd sets the variable correctly. Something else must have set it. */ + log_debug_errno(r, "Failed to parse \"SYSTEMD_EXEC_PID=%s\", ignoring: %m", e); + return false; + } + + return getpid_cached() == p; +} + _noreturn_ void freeze(void) { log_close(); diff --git a/src/basic/process-util.h b/src/basic/process-util.h index c07575e580..f8c374a310 100644 --- a/src/basic/process-util.h +++ b/src/basic/process-util.h @@ -190,6 +190,8 @@ int setpriority_closest(int priority); bool invoked_as(char *argv[], const char *token); +bool invoked_by_systemd(void); + _noreturn_ void freeze(void); bool argv_looks_like_help(int argc, char **argv); From 911017f0526df9d1477588afb1f26afacdab9511 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 13 Jul 2022 18:43:05 +0200 Subject: [PATCH 08/15] generators: only redirect logging when invoked by systemd MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We would always print output to the kmsg or journal, but that is only needed and useful when invoked by systemd. So let's skip redirection unless we are invoked by systemd. Otherwise, let's log normally. This makes test invocations easier, and also helps when the generator is invoked by mistake. If redirection is necessary, the generator can be invoked with SYSTEMD_LOG_TARGET=… even during tests. --- src/shared/generator.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/shared/generator.c b/src/shared/generator.c index ca66673d84..12170d3642 100644 --- a/src/shared/generator.c +++ b/src/shared/generator.c @@ -15,6 +15,7 @@ #include "macro.h" #include "mkdir-label.h" #include "path-util.h" +#include "process-util.h" #include "special.h" #include "specifier.h" #include "string-util.h" @@ -737,11 +738,15 @@ int generator_write_veritysetup_service_section( } void log_setup_generator(void) { - /* Disable talking to syslog/journal (i.e. the two IPC-based loggers) if we run in system context. */ - if (cg_pid_get_owner_uid(0, NULL) == -ENXIO /* not running in a per-user slice */) - log_set_prohibit_ipc(true); + if (invoked_by_systemd()) { + /* Disable talking to syslog/journal (i.e. the two IPC-based loggers) if we run in system context. */ + if (cg_pid_get_owner_uid(0, NULL) == -ENXIO /* not running in a per-user slice */) + log_set_prohibit_ipc(true); + + /* This effectively means: journal for per-user generators, kmsg otherwise */ + log_set_target(LOG_TARGET_JOURNAL_OR_KMSG); + } - log_set_target(LOG_TARGET_JOURNAL_OR_KMSG); /* This effectively means: journal for per-user generators, kmsg otherwise */ log_parse_environment(); (void) log_open(); } From efc5fd3d72b51a7c32822f19515d0624b7a286e0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 13 Jul 2022 18:58:15 +0200 Subject: [PATCH 09/15] fstab-generator: use path_equal() to detect alternate spellings of /usr In the initrd we would treat e.g. "/usr/" as different from "/usr", which doesn't seem right. --- src/fstab-generator/fstab-generator.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/fstab-generator/fstab-generator.c b/src/fstab-generator/fstab-generator.c index 75523d0c07..046ca421ef 100644 --- a/src/fstab-generator/fstab-generator.c +++ b/src/fstab-generator/fstab-generator.c @@ -188,7 +188,7 @@ static bool mount_in_initrd(struct mntent *me) { assert(me); return fstab_test_option(me->mnt_opts, "x-initrd.mount\0") || - streq(me->mnt_dir, "/usr"); + path_equal(me->mnt_dir, "/usr"); } static int write_timeout( From 7772c177b811e534bd4351e103a56e320e5b699a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 14 Jul 2022 12:46:18 +0200 Subject: [PATCH 10/15] fstab-generator: properly report the source of data Mount information can come from /etc/fstab, /sysroot/etc/fstab, and /proc/cmdline. Even when we had the path to the right source handy, we would often write something inaccurate. In particular, in the initrd, we would generally write "/etc/fstab" instead of "/sysroot/etc/fstab" for no good reason. --- src/fstab-generator/fstab-generator.c | 45 ++++++++++++++------------- 1 file changed, 23 insertions(+), 22 deletions(-) diff --git a/src/fstab-generator/fstab-generator.c b/src/fstab-generator/fstab-generator.c index 046ca421ef..b4bcc89132 100644 --- a/src/fstab-generator/fstab-generator.c +++ b/src/fstab-generator/fstab-generator.c @@ -92,6 +92,7 @@ static int write_what(FILE *f, const char *what) { } static int add_swap( + const char *source, const char *what, struct mntent *me, MountPointFlags flags) { @@ -122,7 +123,7 @@ static int add_swap( if (r < 0) return log_error_errno(r, "Failed to generate unit name: %m"); - r = generator_open_unit_file(arg_dest, fstab_path(), name, &f); + r = generator_open_unit_file(arg_dest, source, name, &f); if (r < 0) return r; @@ -130,7 +131,7 @@ static int add_swap( "[Unit]\n" "Documentation=man:fstab(5) man:systemd-fstab-generator(8)\n" "SourcePath=%s\n", - fstab_path()); + source); r = generator_write_blockdev_dependency(f, what); if (r < 0) @@ -339,6 +340,7 @@ static int write_extra_dependencies(FILE *f, const char *opts) { } static int add_mount( + const char *source, const char *dest, const char *what, const char *where, @@ -347,8 +349,7 @@ static int add_mount( const char *opts, int passno, MountPointFlags flags, - const char *post, - const char *source) { + const char *post) { _cleanup_free_ char *name = NULL, @@ -406,7 +407,7 @@ static int add_mount( if (r < 0) return log_error_errno(r, "Failed to generate unit name: %m"); - r = generator_open_unit_file(dest, fstab_path(), name, &f); + r = generator_open_unit_file(dest, source, name, &f); if (r < 0) return r; @@ -536,7 +537,7 @@ static int add_mount( f = safe_fclose(f); - r = generator_open_unit_file(dest, fstab_path(), automount_name, &f); + r = generator_open_unit_file(dest, source, automount_name, &f); if (r < 0) return r; @@ -650,7 +651,7 @@ static int parse_fstab(bool initrd) { nofail * MOUNT_NOFAIL; if (streq(me->mnt_type, "swap")) - k = add_swap(what, me, flags); + k = add_swap(fstab, what, me, flags); else { bool rw_only, automount; const char *post; @@ -670,7 +671,8 @@ static int parse_fstab(bool initrd) { else post = SPECIAL_LOCAL_FS_TARGET; - k = add_mount(arg_dest, + k = add_mount(fstab, + arg_dest, what, canonical_where ?: where, canonical_where ? where: NULL, @@ -678,8 +680,7 @@ static int parse_fstab(bool initrd) { me->mnt_opts, me->mnt_passno, flags, - post, - fstab); + post); } if (r >= 0 && k < 0) @@ -818,7 +819,8 @@ static int add_sysroot_mount(void) { return r; } - return add_mount(arg_dest, + return add_mount("/proc/cmdline", + arg_dest, what, "/sysroot", NULL, @@ -826,8 +828,7 @@ static int add_sysroot_mount(void) { opts, is_device_path(what) ? 1 : 0, /* passno */ 0, /* makefs off, growfs off, noauto off, nofail off, automount off */ - SPECIAL_INITRD_ROOT_FS_TARGET, - "/proc/cmdline"); + SPECIAL_INITRD_ROOT_FS_TARGET); } static int add_sysroot_usr_mount(void) { @@ -898,7 +899,8 @@ static int add_sysroot_usr_mount(void) { log_debug("Found entry what=%s where=/sysusr/usr type=%s opts=%s", what, strna(arg_usr_fstype), strempty(opts)); - r = add_mount(arg_dest, + r = add_mount("/proc/cmdline", + arg_dest, what, "/sysusr/usr", NULL, @@ -906,14 +908,14 @@ static int add_sysroot_usr_mount(void) { opts, is_device_path(what) ? 1 : 0, /* passno */ 0, - SPECIAL_INITRD_USR_FS_TARGET, - "/proc/cmdline"); + SPECIAL_INITRD_USR_FS_TARGET); if (r < 0) return r; log_debug("Synthesizing entry what=/sysusr/usr where=/sysrootr/usr opts=bind"); - r = add_mount(arg_dest, + r = add_mount("/proc/cmdline", + arg_dest, "/sysusr/usr", "/sysroot/usr", NULL, @@ -921,8 +923,7 @@ static int add_sysroot_usr_mount(void) { "bind", 0, 0, - SPECIAL_INITRD_FS_TARGET, - "/proc/cmdline"); + SPECIAL_INITRD_FS_TARGET); if (r < 0) return r; @@ -966,7 +967,8 @@ static int add_volatile_var(void) { /* If requested, mount /var as tmpfs, but do so only if there's nothing else defined for this. */ - return add_mount(arg_dest_late, + return add_mount("/proc/cmdline", + arg_dest_late, "tmpfs", "/var", NULL, @@ -974,8 +976,7 @@ static int add_volatile_var(void) { "mode=0755" TMPFS_LIMITS_VAR, 0, 0, - SPECIAL_LOCAL_FS_TARGET, - "/proc/cmdline"); + SPECIAL_LOCAL_FS_TARGET); } static int parse_proc_cmdline_item(const char *key, const char *value, void *data) { From 99e3d4767932bce5febb45e8543162d729d17425 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 14 Jul 2022 13:29:06 +0200 Subject: [PATCH 11/15] fstab-generator: allow overriding path to /sysroot/etc/fstab too This adds $SYSTEMD_SYSROOT_FSTAB analoguous to $SYSTEMD_FSTAB. --- docs/ENVIRONMENT.md | 3 +++ man/systemd-fstab-generator.xml | 6 +++--- src/fstab-generator/fstab-generator.c | 10 +++++++++- 3 files changed, 15 insertions(+), 4 deletions(-) diff --git a/docs/ENVIRONMENT.md b/docs/ENVIRONMENT.md index 7caa951814..fc173289d9 100644 --- a/docs/ENVIRONMENT.md +++ b/docs/ENVIRONMENT.md @@ -51,6 +51,9 @@ All tools: * `$SYSTEMD_FSTAB` — if set, use this path instead of `/etc/fstab`. Only useful for debugging. +* `$SYSTEMD_SYSROOT_FSTAB` — if set, use this path instead of + `/sysroot/etc/fstab`. Only useful for debugging `systemd-fstab-generator`. + * `$SYSTEMD_CRYPTTAB` — if set, use this path instead of `/etc/crypttab`. Only useful for debugging. Currently only supported by `systemd-cryptsetup-generator`. diff --git a/man/systemd-fstab-generator.xml b/man/systemd-fstab-generator.xml index 21fa85da7d..21c3ea94a7 100644 --- a/man/systemd-fstab-generator.xml +++ b/man/systemd-fstab-generator.xml @@ -46,7 +46,7 @@ for more information about special /etc/fstab mount options this generator understands. - One special topic is handling of symbolic links. Historical init + One special topic is handling of symbolic links. Historical init implementations supported symlinks in /etc/fstab. Because mount units will refuse mounts where the target is a symbolic link, this generator will resolve any symlinks as far as possible when processing @@ -251,8 +251,8 @@ systemd.swap5, systemd-cryptsetup-generator8, systemd-gpt-auto-generator8, - kernel-command-line7 + kernel-command-line7, + Known Environment Variables - diff --git a/src/fstab-generator/fstab-generator.c b/src/fstab-generator/fstab-generator.c index b4bcc89132..84d7edb922 100644 --- a/src/fstab-generator/fstab-generator.c +++ b/src/fstab-generator/fstab-generator.c @@ -570,13 +570,21 @@ static int add_mount( return 0; } +static const char* sysroot_fstab_path(void) { + return getenv("SYSTEMD_SYSROOT_FSTAB") ?: "/sysroot/etc/fstab"; +} + static int parse_fstab(bool initrd) { _cleanup_endmntent_ FILE *f = NULL; const char *fstab; struct mntent *me; int r = 0; - fstab = initrd ? "/sysroot/etc/fstab" : fstab_path(); + if (initrd) + fstab = sysroot_fstab_path(); + else + fstab = fstab_path(); + log_debug("Parsing %s...", fstab); f = setmntent(fstab, "re"); From 4981427cd48a13a8245427458e49c5712c2db67e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 14 Jul 2022 14:35:37 +0200 Subject: [PATCH 12/15] fstab-generator: tweak comments --- src/fstab-generator/fstab-generator.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/fstab-generator/fstab-generator.c b/src/fstab-generator/fstab-generator.c index 84d7edb922..257e99fa13 100644 --- a/src/fstab-generator/fstab-generator.c +++ b/src/fstab-generator/fstab-generator.c @@ -635,7 +635,7 @@ static int parse_fstab(bool initrd) { k = chase_symlinks(where, initrd ? "/sysroot" : NULL, CHASE_PREFIX_ROOT | CHASE_NONEXISTENT, &canonical_where, NULL); - if (k < 0) /* If we can't canonicalize we continue on as if it wasn't a symlink */ + if (k < 0) /* If we can't canonicalize, continue as if it wasn't a symlink */ log_debug_errno(k, "Failed to read symlink target for %s, ignoring: %m", where); else if (streq(canonical_where, where)) /* If it was fully canonicalized, suppress the change */ canonical_where = mfree(canonical_where); @@ -761,7 +761,7 @@ static int add_sysroot_mount(void) { } if (streq(arg_root_what, "gpt-auto")) { - /* This is handled by the gpt-auto generator */ + /* This is handled by gpt-auto-generator */ log_debug("Skipping root directory handling, as gpt-auto was requested."); return 0; } From da69e8e430dccf4b257f2a6442b9c010dcfe2180 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 14 Jul 2022 16:16:25 +0200 Subject: [PATCH 13/15] fstab-generator: rename 'post' variable Originally (6b1dc2bd3cdb3bd932b0692be636ddd2879edb92) we had 'pre' and 'post' to refer to remote-fs-pre.target and remote-fs.target or local-fs-pre.target and local-fs.target. But 'pre' is long gone, and 'post' by itself doesn't make much sense. Rename it for clarity. --- src/fstab-generator/fstab-generator.c | 29 ++++++++++++--------------- 1 file changed, 13 insertions(+), 16 deletions(-) diff --git a/src/fstab-generator/fstab-generator.c b/src/fstab-generator/fstab-generator.c index 257e99fa13..8bd6022175 100644 --- a/src/fstab-generator/fstab-generator.c +++ b/src/fstab-generator/fstab-generator.c @@ -349,7 +349,7 @@ static int add_mount( const char *opts, int passno, MountPointFlags flags, - const char *post) { + const char *target_unit) { _cleanup_free_ char *name = NULL, @@ -363,7 +363,7 @@ static int add_mount( assert(what); assert(where); assert(opts); - assert(post); + assert(target_unit); assert(source); if (streq_ptr(fstype, "autofs")) @@ -435,10 +435,10 @@ static int add_mount( if (r < 0) return r; - /* Order the mount unit we generate relative to the post unit, so that DefaultDependencies= on the + /* Order the mount unit we generate relative to target_unit, so that DefaultDependencies= on the * target unit won't affect us. */ - if (post && !FLAGS_SET(flags, MOUNT_NOFAIL)) - fprintf(f, "Before=%s\n", post); + if (!FLAGS_SET(flags, MOUNT_NOFAIL)) + fprintf(f, "Before=%s\n", target_unit); if (passno != 0) { r = generator_write_fsck_deps(f, dest, what, where, fstype); @@ -506,14 +506,14 @@ static int add_mount( } if (flags & MOUNT_GROWFS) { - r = generator_hook_up_growfs(dest, where, post); + r = generator_hook_up_growfs(dest, where, target_unit); if (r < 0) return r; } if (!FLAGS_SET(flags, MOUNT_AUTOMOUNT)) { if (!FLAGS_SET(flags, MOUNT_NOAUTO) && strv_isempty(wanted_by) && strv_isempty(required_by)) { - r = generator_add_symlink(dest, post, + r = generator_add_symlink(dest, target_unit, (flags & MOUNT_NOFAIL) ? "wants" : "requires", name); if (r < 0) return r; @@ -561,7 +561,7 @@ static int add_mount( if (r < 0) return log_error_errno(r, "Failed to write unit file %s: %m", automount_name); - r = generator_add_symlink(dest, post, + r = generator_add_symlink(dest, target_unit, (flags & MOUNT_NOFAIL) ? "wants" : "requires", automount_name); if (r < 0) return r; @@ -662,7 +662,6 @@ static int parse_fstab(bool initrd) { k = add_swap(fstab, what, me, flags); else { bool rw_only, automount; - const char *post; rw_only = fstab_test_option(me->mnt_opts, "x-systemd.rw-only\0"); automount = fstab_test_option(me->mnt_opts, @@ -672,12 +671,10 @@ static int parse_fstab(bool initrd) { flags |= rw_only * MOUNT_RW_ONLY | automount * MOUNT_AUTOMOUNT; - if (initrd) - post = SPECIAL_INITRD_FS_TARGET; - else if (mount_is_network(me)) - post = SPECIAL_REMOTE_FS_TARGET; - else - post = SPECIAL_LOCAL_FS_TARGET; + const char *target_unit = + initrd ? SPECIAL_INITRD_FS_TARGET : + mount_is_network(me) ? SPECIAL_REMOTE_FS_TARGET : + SPECIAL_LOCAL_FS_TARGET; k = add_mount(fstab, arg_dest, @@ -688,7 +685,7 @@ static int parse_fstab(bool initrd) { me->mnt_opts, me->mnt_passno, flags, - post); + target_unit); } if (r >= 0 && k < 0) From 640dc2f3ad658587b2ce2b1d308a23c28b415a5b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 14 Jul 2022 16:46:24 +0200 Subject: [PATCH 14/15] fsck,sulogin: fix cargo-culted comment --- src/fsck/fsck.c | 2 +- src/sulogin-shell/sulogin-shell.c | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/fsck/fsck.c b/src/fsck/fsck.c index 46ccc9781f..b7fe729836 100644 --- a/src/fsck/fsck.c +++ b/src/fsck/fsck.c @@ -54,7 +54,7 @@ static void start_target(const char *target, const char *mode) { log_info("Running request %s/start/%s", target, mode); - /* Start these units only if we can replace base.target with it */ + /* Start this unit only if we can replace basic.target with it */ r = sd_bus_call_method(bus, "org.freedesktop.systemd1", "/org/freedesktop/systemd1", diff --git a/src/sulogin-shell/sulogin-shell.c b/src/sulogin-shell/sulogin-shell.c index 0207e85b7e..148e8acd73 100644 --- a/src/sulogin-shell/sulogin-shell.c +++ b/src/sulogin-shell/sulogin-shell.c @@ -49,9 +49,9 @@ static int start_default_target(sd_bus *bus) { _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; int r; - log_info("Starting default target"); + log_info("Starting "SPECIAL_DEFAULT_TARGET); - /* Start these units only if we can replace base.target with it */ + /* Start this unit only if we can replace basic.target with it */ r = sd_bus_call_method(bus, "org.freedesktop.systemd1", "/org/freedesktop/systemd1", @@ -62,7 +62,7 @@ static int start_default_target(sd_bus *bus) { "ss", SPECIAL_DEFAULT_TARGET, "isolate"); if (r < 0) - return log_error_errno(r, "Failed to start default target: %s", bus_error_message(&error, r)); + return log_error_errno(r, "Failed to start "SPECIAL_DEFAULT_TARGET": %s", bus_error_message(&error, r)); return 0; } From ff52ff2565a3034a0ad063454a58d0c5649adc00 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 14 Jul 2022 17:33:13 +0200 Subject: [PATCH 15/15] fstab-generator: do not skip /sysroot prefix if the mount point is missing When chase_symlinks() is called on something on a doesn't exist, it immediately returns an error. But we were relying on it to prepend "/sysroot/". If it fails, we need to do that ourselves. For example, with /sysroot/etc/fstab containing a line for /foo, if /sysroot/foo doesn't exist, we'd generate a mount point for /foo. --- src/fstab-generator/fstab-generator.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/src/fstab-generator/fstab-generator.c b/src/fstab-generator/fstab-generator.c index 8bd6022175..da7ea627f1 100644 --- a/src/fstab-generator/fstab-generator.c +++ b/src/fstab-generator/fstab-generator.c @@ -631,13 +631,25 @@ static int parse_fstab(bool initrd) { * mount units, but causes problems since it historically worked to have symlinks in e.g. * /etc/fstab. So we canonicalize here. Note that we use CHASE_NONEXISTENT to handle the case * where a symlink refers to another mount target; this works assuming the sub-mountpoint - * target is the final directory. */ + * target is the final directory. + * + * FIXME: when chase_symlinks() learns to chase non-existent paths, use this here and + * drop the prefixing with /sysroot on error below. + */ k = chase_symlinks(where, initrd ? "/sysroot" : NULL, CHASE_PREFIX_ROOT | CHASE_NONEXISTENT, &canonical_where, NULL); - if (k < 0) /* If we can't canonicalize, continue as if it wasn't a symlink */ - log_debug_errno(k, "Failed to read symlink target for %s, ignoring: %m", where); - else if (streq(canonical_where, where)) /* If it was fully canonicalized, suppress the change */ + if (k < 0) { + /* If we can't canonicalize, continue as if it wasn't a symlink */ + log_debug_errno(k, "Failed to read symlink target for %s, using as-is: %m", where); + + if (initrd) { + canonical_where = path_join("/sysroot", where); + if (!canonical_where) + return log_oom(); + } + + } else if (streq(canonical_where, where)) /* If it was fully canonicalized, suppress the change */ canonical_where = mfree(canonical_where); else log_debug("Canonicalized what=%s where=%s to %s", what, where, canonical_where);