From e82d4edd20eb6fbde2a0dcf515c7eca8cad65edb Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Fri, 6 Dec 2024 17:46:03 +0100 Subject: [PATCH 1/7] shared/generator: trivial coding style cleanups --- src/shared/generator.c | 15 +++---------- src/shared/generator.h | 50 ++++++++++++++++-------------------------- 2 files changed, 22 insertions(+), 43 deletions(-) diff --git a/src/shared/generator.c b/src/shared/generator.c index b3e57770aa..ceb26c570d 100644 --- a/src/shared/generator.c +++ b/src/shared/generator.c @@ -787,10 +787,7 @@ int generator_enable_remount_fs_service(const char *dir) { SYSTEM_DATA_UNIT_DIR "/" SPECIAL_REMOUNT_FS_SERVICE); } -int generator_write_blockdev_dependency( - FILE *f, - const char *what) { - +int generator_write_blockdev_dependency(FILE *f, const char *what) { _cleanup_free_ char *escaped = NULL; int r; @@ -811,10 +808,7 @@ int generator_write_blockdev_dependency( return 0; } -int generator_write_cryptsetup_unit_section( - FILE *f, - const char *source) { - +int generator_write_cryptsetup_unit_section(FILE *f, const char *source) { assert(f); fprintf(f, @@ -886,10 +880,7 @@ int generator_write_cryptsetup_service_section( return 0; } -int generator_write_veritysetup_unit_section( - FILE *f, - const char *source) { - +int generator_write_veritysetup_unit_section(FILE *f, const char *source) { assert(f); fprintf(f, diff --git a/src/shared/generator.h b/src/shared/generator.h index f07d7d6b3a..e3f1197205 100644 --- a/src/shared/generator.h +++ b/src/shared/generator.h @@ -7,13 +7,11 @@ #include "main-func.h" int generator_open_unit_file_full(const char *dest, const char *source, const char *name, FILE **ret_file, char **ret_final_path, char **ret_temp_path); - static inline int generator_open_unit_file(const char *dest, const char *source, const char *name, FILE **ret_file) { return generator_open_unit_file_full(dest, source, name, ret_file, NULL, NULL); } int generator_add_symlink_full(const char *dir, const char *dst, const char *dep_type, const char *src, const char *instance); - static inline int generator_add_symlink(const char *dir, const char *dst, const char *dep_type, const char *src) { return generator_add_symlink_full(dir, dst, dep_type, src, NULL); } @@ -32,32 +30,7 @@ int generator_write_timeouts( const char *opts, char **filtered); -int generator_write_blockdev_dependency( - FILE *f, - const char *what); - -int generator_write_cryptsetup_unit_section( - FILE *f, - const char *source); - -int generator_write_cryptsetup_service_section( - FILE *f, - const char *name, - const char *what, - const char *password, - const char *options); - -int generator_write_veritysetup_unit_section( - FILE *f, - const char *source); - -int generator_write_veritysetup_service_section( - FILE *f, - const char *name, - const char *data_what, - const char *hash_what, - const char *roothash, - const char *options); +int generator_write_blockdev_dependency(FILE *f, const char *what); int generator_write_device_deps( const char *dir, @@ -65,9 +38,7 @@ int generator_write_device_deps( const char *where, const char *opts); -int generator_write_initrd_root_device_deps( - const char *dir, - const char *what); +int generator_write_initrd_root_device_deps(const char *dir, const char *what); int generator_hook_up_mkswap( const char *dir, @@ -96,6 +67,23 @@ int generator_hook_up_quotaon( const char *where, const char *target); +int generator_write_cryptsetup_unit_section(FILE *f, const char *source); +int generator_write_cryptsetup_service_section( + FILE *f, + const char *name, + const char *what, + const char *password, + const char *options); + +int generator_write_veritysetup_unit_section(FILE *f, const char *source); +int generator_write_veritysetup_service_section( + FILE *f, + const char *name, + const char *data_what, + const char *hash_what, + const char *roothash, + const char *options); + int generator_enable_remount_fs_service(const char *dir); void log_setup_generator(void); From 1d8ce41318710e1b1f513b00bdc7357015b8735c Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Fri, 6 Dec 2024 17:49:44 +0100 Subject: [PATCH 2/7] shared/generator: rename generator_write_device_deps() to _network_device_deps The previous name was awfully vague. Let's be specific that this is about _netdev only. --- src/fstab-generator/fstab-generator.c | 2 +- src/shared/generator.c | 9 ++++++--- src/shared/generator.h | 2 +- 3 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/fstab-generator/fstab-generator.c b/src/fstab-generator/fstab-generator.c index 6b7445b201..f3f435c685 100644 --- a/src/fstab-generator/fstab-generator.c +++ b/src/fstab-generator/fstab-generator.c @@ -674,7 +674,7 @@ static int add_mount( if (r < 0) return r; - r = generator_write_device_deps(dest, what, where, opts); + r = generator_write_network_device_deps(dest, what, where, opts); if (r < 0) return r; diff --git a/src/shared/generator.c b/src/shared/generator.c index ceb26c570d..0564e2e9e3 100644 --- a/src/shared/generator.c +++ b/src/shared/generator.c @@ -421,7 +421,7 @@ int generator_write_timeouts( timeout); } -int generator_write_device_deps( +int generator_write_network_device_deps( const char *dir, const char *what, const char *where, @@ -435,6 +435,10 @@ int generator_write_device_deps( _cleanup_free_ char *node = NULL, *unit = NULL; int r; + assert(dir); + assert(what); + assert(where); + if (fstab_is_extrinsic(where, opts)) return 0; @@ -451,8 +455,7 @@ int generator_write_device_deps( r = unit_name_from_path(node, ".device", &unit); if (r < 0) - return log_error_errno(r, "Failed to make unit name from path \"%s\": %m", - node); + return log_error_errno(r, "Failed to make unit name from path \"%s\": %m", node); /* See mount_add_default_dependencies for explanation why we create such * dependencies. */ diff --git a/src/shared/generator.h b/src/shared/generator.h index e3f1197205..675af99f3c 100644 --- a/src/shared/generator.h +++ b/src/shared/generator.h @@ -32,7 +32,7 @@ int generator_write_timeouts( int generator_write_blockdev_dependency(FILE *f, const char *what); -int generator_write_device_deps( +int generator_write_network_device_deps( const char *dir, const char *what, const char *where, From 9a097eced649816669579184a4943291e4286157 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Fri, 6 Dec 2024 18:20:06 +0100 Subject: [PATCH 3/7] shared/generator: clean up generator_write_device_timeout() a bit - Rephrase a few log messages - Remove unused where parameter - Return 1 if we set something No functional change, preparation for later commits. --- src/cryptsetup/cryptsetup-generator.c | 2 +- src/fstab-generator/fstab-generator.c | 4 +-- .../hibernate-resume-generator.c | 6 +--- src/shared/generator.c | 32 +++++++++++-------- src/shared/generator.h | 3 +- 5 files changed, 24 insertions(+), 23 deletions(-) diff --git a/src/cryptsetup/cryptsetup-generator.c b/src/cryptsetup/cryptsetup-generator.c index 6a3ff961fc..12b6044d8c 100644 --- a/src/cryptsetup/cryptsetup-generator.c +++ b/src/cryptsetup/cryptsetup-generator.c @@ -518,7 +518,7 @@ static int create_disk( "After=modprobe@loop.service\n", u_escaped); - r = generator_write_timeouts(arg_dest, device, name, options, &filtered); + r = generator_write_device_timeout(arg_dest, device, options, &filtered); if (r < 0) log_warning_errno(r, "Failed to write device timeout drop-in: %m"); diff --git a/src/fstab-generator/fstab-generator.c b/src/fstab-generator/fstab-generator.c index f3f435c685..6a7cf57ba3 100644 --- a/src/fstab-generator/fstab-generator.c +++ b/src/fstab-generator/fstab-generator.c @@ -289,7 +289,7 @@ static int add_swap( return log_error_errno(r, "Failed to write unit file %s: %m", name); /* use what as where, to have a nicer error message */ - r = generator_write_timeouts(arg_dest, what, what, options, NULL); + r = generator_write_device_timeout(arg_dest, what, options, NULL); if (r < 0) return r; @@ -670,7 +670,7 @@ static int add_mount( fprintf(f, "Type=%s\n", t); } - r = generator_write_timeouts(dest, what, where, opts, &filtered); + r = generator_write_device_timeout(dest, what, opts, &filtered); if (r < 0) return r; diff --git a/src/hibernate-resume/hibernate-resume-generator.c b/src/hibernate-resume/hibernate-resume-generator.c index 2b175342c1..b6495a2581 100644 --- a/src/hibernate-resume/hibernate-resume-generator.c +++ b/src/hibernate-resume/hibernate-resume-generator.c @@ -75,11 +75,7 @@ static int process_resume(const HibernateInfo *info) { if (r < 0) log_warning_errno(r, "Failed to write device timeout drop-in, ignoring: %m"); - r = generator_write_timeouts(arg_dest, - info->device, - info->device, - arg_resume_options ?: arg_root_options, - NULL); + r = generator_write_device_timeout(arg_dest, info->device, arg_resume_options ?: arg_root_options, NULL); if (r < 0) log_warning_errno(r, "Failed to write device timeout drop-in, ignoring: %m"); diff --git a/src/shared/generator.c b/src/shared/generator.c index 0564e2e9e3..5397ad1569 100644 --- a/src/shared/generator.c +++ b/src/shared/generator.c @@ -368,10 +368,9 @@ int generator_write_fsck_deps( return 0; } -int generator_write_timeouts( +int generator_write_device_timeout( const char *dir, const char *what, - const char *where, const char *opts, char **filtered) { @@ -383,6 +382,9 @@ int generator_write_timeouts( usec_t u; int r; + assert(dir); + assert(what); + r = fstab_filter_options(opts, "comment=systemd.device-timeout\0" "x-systemd.device-timeout\0", NULL, &timeout, NULL, filtered); @@ -395,7 +397,7 @@ int generator_write_timeouts( r = parse_sec_fix_0(timeout, &u); if (r < 0) { - log_warning("Failed to parse timeout for %s, ignoring: %s", where, timeout); + log_warning("Failed to parse timeout for device '%s', ignoring: %s", what, timeout); return 0; } @@ -403,22 +405,26 @@ int generator_write_timeouts( if (!node) return log_oom(); if (!is_device_path(node)) { - log_warning("x-systemd.device-timeout ignored for %s", what); + log_warning("'%s' is not a device path, ignoring x-systemd.device-timeout= option.", node); return 0; } r = unit_name_from_path(node, ".device", &unit); if (r < 0) - return log_error_errno(r, "Failed to make unit name from path: %m"); + return log_error_errno(r, "Failed to make unit name from device path '%s': %m", node); - return write_drop_in_format(dir, unit, 50, "device-timeout", - "# Automatically generated by %s\n" - "# from supplied options \"%s\"\n\n" - "[Unit]\n" - "JobRunningTimeoutSec=%s", - program_invocation_short_name, - opts, - timeout); + r = write_drop_in_format(dir, unit, 50, "device-timeout", + "# Automatically generated by %s\n" + "# from supplied options \"%s\"\n\n" + "[Unit]\n" + "JobRunningTimeoutSec=%s", + program_invocation_short_name, + opts, + timeout); + if (r < 0) + return r; + + return 1; } int generator_write_network_device_deps( diff --git a/src/shared/generator.h b/src/shared/generator.h index 675af99f3c..ff8e8d1c6b 100644 --- a/src/shared/generator.h +++ b/src/shared/generator.h @@ -23,10 +23,9 @@ int generator_write_fsck_deps( const char *where, const char *type); -int generator_write_timeouts( +int generator_write_device_timeout( const char *dir, const char *what, - const char *where, const char *opts, char **filtered); From a46f36de79a5191c619b6d66248c911f244241e7 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Sun, 8 Dec 2024 20:39:23 +0100 Subject: [PATCH 4/7] hibernate-resume: always respect user-defined timeout Follow-up for 6cfce71b5034eb304eea82a31c400ff511cfa69e Let's not override resumeflags=x-systemd.device-timeout= specified by user, especially in the case of EFI if the user actually wants to extend the timeout. --- .../hibernate-resume-generator.c | 23 ++++++++++--------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/src/hibernate-resume/hibernate-resume-generator.c b/src/hibernate-resume/hibernate-resume-generator.c index b6495a2581..666cd52e47 100644 --- a/src/hibernate-resume/hibernate-resume-generator.c +++ b/src/hibernate-resume/hibernate-resume-generator.c @@ -64,20 +64,21 @@ static int process_resume(const HibernateInfo *info) { if (r < 0) return log_error_errno(r, "Failed to generate device unit name from path '%s': %m", info->device); - /* If hibernate info is acquired from EFI variable, don't wait forever by default. Otherwise, if - * swap device is not present and HibernateLocation was not correctly cleared, we end up blocking - * the boot process infinitely. */ - r = write_drop_in_format(arg_dest, device_unit, 40, "device-timeout", - "# Automatically generated by systemd-hibernate-resume-generator\n\n" - "[Unit]\n" - "JobTimeoutSec=%s\n", - info->cmdline ? "infinity" : "2min"); - if (r < 0) - log_warning_errno(r, "Failed to write device timeout drop-in, ignoring: %m"); - r = generator_write_device_timeout(arg_dest, info->device, arg_resume_options ?: arg_root_options, NULL); if (r < 0) log_warning_errno(r, "Failed to write device timeout drop-in, ignoring: %m"); + if (r <= 0) { + /* No timeout explicitly defined? Wait infinitely if resume= is specified, 2min if from EFI + * HibernateLocation variable. In the latter case, we avoid blocking the boot process forever + * if a stale var is detected while the swap device is not present. */ + r = write_drop_in_format(arg_dest, device_unit, 40, "device-timeout", + "# Automatically generated by systemd-hibernate-resume-generator\n\n" + "[Unit]\n" + "JobTimeoutSec=%s\n", + info->cmdline ? "infinity" : "2min"); + if (r < 0) + log_warning_errno(r, "Failed to write fallback device timeout drop-in, ignoring: %m"); + } r = write_drop_in_format(arg_dest, SPECIAL_HIBERNATE_RESUME_SERVICE, 90, "device-dependency", "# Automatically generated by systemd-hibernate-resume-generator\n\n" From 149e05c7e98738c5c29b4a0db71b42d56283e861 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Sun, 8 Dec 2024 20:47:41 +0100 Subject: [PATCH 5/7] fstab-generator: move write_(mount_)timeout() to shared/generator --- src/fstab-generator/fstab-generator.c | 44 ++------------------------- src/shared/generator.c | 33 ++++++++++++++++++++ src/shared/generator.h | 11 +++++++ 3 files changed, 47 insertions(+), 41 deletions(-) diff --git a/src/fstab-generator/fstab-generator.c b/src/fstab-generator/fstab-generator.c index 6a7cf57ba3..4dfe9fea89 100644 --- a/src/fstab-generator/fstab-generator.c +++ b/src/fstab-generator/fstab-generator.c @@ -325,47 +325,9 @@ static bool mount_in_initrd(const char *where, const char *options, bool accept_ (where && PATH_IN_SET(where, "/usr", accept_root ? "/" : NULL)); } -static int write_timeout( - FILE *f, - const char *where, - const char *opts, - const char *filter, - const char *unit_setting) { - - _cleanup_free_ char *timeout = NULL; - usec_t u; - int r; - - assert(f); - assert(where); - assert(filter); - assert(unit_setting); - - r = fstab_filter_options(opts, filter, NULL, &timeout, NULL, NULL); - if (r < 0) - return log_error_errno(r, "Failed to parse options for '%s': %m", where); - if (r == 0) - return 0; - - r = parse_sec_fix_0(timeout, &u); - if (r < 0) { - log_warning_errno(r, "Failed to parse timeout '%s' for '%s', ignoring: %m", timeout, where); - return 0; - } - - fprintf(f, "%s=%s\n", unit_setting, FORMAT_TIMESPAN(u, 0)); - - return 0; -} - static int write_idle_timeout(FILE *f, const char *where, const char *opts) { - return write_timeout(f, where, opts, - "x-systemd.idle-timeout\0", "TimeoutIdleSec"); -} - -static int write_mount_timeout(FILE *f, const char *where, const char *opts) { - return write_timeout(f, where, opts, - "x-systemd.mount-timeout\0", "TimeoutSec"); + return generator_write_unit_timeout(f, where, opts, + "x-systemd.idle-timeout\0", "TimeoutIdleSec"); } static int write_dependency( @@ -684,7 +646,7 @@ static int add_mount( return r; } - r = write_mount_timeout(f, where, opts); + r = generator_write_mount_timeout(f, where, opts); if (r < 0) return r; diff --git a/src/shared/generator.c b/src/shared/generator.c index 5397ad1569..738f189279 100644 --- a/src/shared/generator.c +++ b/src/shared/generator.c @@ -427,6 +427,39 @@ int generator_write_device_timeout( return 1; } +int generator_write_unit_timeout( + FILE *f, + const char *where, + const char *opts, + const char *filter, + const char *unit_setting) { + + _cleanup_free_ char *timeout = NULL; + usec_t u; + int r; + + assert(f); + assert(where); + assert(filter); + assert(unit_setting); + + r = fstab_filter_options(opts, filter, NULL, &timeout, NULL, NULL); + if (r < 0) + return log_error_errno(r, "Failed to parse options for '%s': %m", where); + if (r == 0) + return 0; + + r = parse_sec_fix_0(timeout, &u); + if (r < 0) { + log_warning_errno(r, "Failed to parse timeout '%s' for '%s', ignoring: %m", timeout, where); + return 0; + } + + fprintf(f, "%s=%s\n", unit_setting, FORMAT_TIMESPAN(u, 0)); + + return 0; +} + int generator_write_network_device_deps( const char *dir, const char *what, diff --git a/src/shared/generator.h b/src/shared/generator.h index ff8e8d1c6b..5879e34ea2 100644 --- a/src/shared/generator.h +++ b/src/shared/generator.h @@ -29,6 +29,17 @@ int generator_write_device_timeout( const char *opts, char **filtered); +int generator_write_unit_timeout( + FILE *f, + const char *where, + const char *opts, + const char *filter, + const char *unit_setting); +static inline int generator_write_mount_timeout(FILE *f, const char *where, const char *opts) { + return generator_write_unit_timeout(f, where, opts, + "x-systemd.mount-timeout\0", "TimeoutSec"); +} + int generator_write_blockdev_dependency(FILE *f, const char *what); int generator_write_network_device_deps( From 756b6749a26d1b31289e197c02e05c7e5954c8ae Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Sun, 8 Dec 2024 20:50:27 +0100 Subject: [PATCH 6/7] gpt-auto: define loader partition timeout as macro --- src/gpt-auto-generator/gpt-auto-generator.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/gpt-auto-generator/gpt-auto-generator.c b/src/gpt-auto-generator/gpt-auto-generator.c index 8bb32afb27..150417626c 100644 --- a/src/gpt-auto-generator/gpt-auto-generator.c +++ b/src/gpt-auto-generator/gpt-auto-generator.c @@ -51,6 +51,8 @@ STATIC_DESTRUCTOR_REGISTER(arg_image_policy, image_policy_freep); STATIC_DESTRUCTOR_REGISTER(arg_root_fstype, freep); STATIC_DESTRUCTOR_REGISTER(arg_root_options, freep); +#define LOADER_PARTITION_IDLE_USEC (120 * USEC_PER_SEC) + static int add_cryptsetup( const char *id, const char *what, @@ -499,7 +501,7 @@ static int add_partition_xbootldr(DissectedPartition *p) { /* growfs= */ false, options, "Boot Loader Partition", - 120 * USEC_PER_SEC); + LOADER_PARTITION_IDLE_USEC); } #if ENABLE_EFI @@ -566,7 +568,7 @@ static int add_partition_esp(DissectedPartition *p, bool has_xbootldr) { /* growfs= */ false, options, "EFI System Partition Automount", - 120 * USEC_PER_SEC); + LOADER_PARTITION_IDLE_USEC); } #else static int add_partition_esp(DissectedPartition *p, bool has_xbootldr) { From f4a65d5f2d96069c6b7753023e7333d2afb1a0d5 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Sun, 8 Dec 2024 21:30:12 +0100 Subject: [PATCH 7/7] gpt-auto: take timeout opts in rootflags= into account We document that rootflags= is honored for gpt-auto, but only fstab-generator actually carried the logic for x-systemd.device-timeout= and .mount-timeout=. Apply that to gpt-auto too. --- src/gpt-auto-generator/gpt-auto-generator.c | 27 ++++++++++++++++----- 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/src/gpt-auto-generator/gpt-auto-generator.c b/src/gpt-auto-generator/gpt-auto-generator.c index 150417626c..dbcef3b0a9 100644 --- a/src/gpt-auto-generator/gpt-auto-generator.c +++ b/src/gpt-auto-generator/gpt-auto-generator.c @@ -56,6 +56,7 @@ STATIC_DESTRUCTOR_REGISTER(arg_root_options, freep); static int add_cryptsetup( const char *id, const char *what, + const char *mount_opts, bool rw, bool require, bool measure, @@ -128,6 +129,10 @@ static int add_cryptsetup( if (r < 0) return log_error_errno(r, "Failed to write file %s: %m", n); + r = generator_write_device_timeout(arg_dest, what, mount_opts, /* filtered = */ NULL); + if (r < 0) + return r; + r = generator_add_symlink(arg_dest, d, "wants", n); if (r < 0) return r; @@ -180,7 +185,7 @@ static int add_mount( const char *description, const char *post) { - _cleanup_free_ char *unit = NULL, *crypto_what = NULL; + _cleanup_free_ char *unit = NULL, *crypto_what = NULL, *opts_filtered = NULL; _cleanup_fclose_ FILE *f = NULL; int r; @@ -196,7 +201,9 @@ static int add_mount( log_debug("Adding %s: %s fstype=%s", where, what, fstype ?: "(any)"); if (streq_ptr(fstype, "crypto_LUKS")) { - r = add_cryptsetup(id, what, rw, /* require= */ true, measure, &crypto_what); + /* Mount options passed are determined by partition_pick_mount_options(), whose result + * is known to not contain timeout options. */ + r = add_cryptsetup(id, what, /* mount_opts = */ NULL, rw, /* require= */ true, measure, &crypto_what); if (r < 0) return r; @@ -213,6 +220,10 @@ static int add_mount( fstype, where); } + r = generator_write_device_timeout(arg_dest, what, options, &opts_filtered); + if (r < 0) + return r; + r = unit_name_from_path(where, ".mount", &unit); if (r < 0) return log_error_errno(r, "Failed to generate unit name: %m"); @@ -248,8 +259,12 @@ static int add_mount( if (fstype) fprintf(f, "Type=%s\n", fstype); - if (options) - fprintf(f, "Options=%s\n", options); + if (opts_filtered) + fprintf(f, "Options=%s\n", opts_filtered); + + r = generator_write_mount_timeout(f, where, opts_filtered); + if (r < 0) + return r; r = fflush_and_check(f); if (r < 0) @@ -368,7 +383,7 @@ static int add_partition_swap(DissectedPartition *p) { } if (streq_ptr(p->fstype, "crypto_LUKS")) { - r = add_cryptsetup("swap", p->node, /* rw= */ true, /* require= */ true, /* measure= */ false, &crypto_what); + r = add_cryptsetup("swap", p->node, /* mount_opts = */ NULL, /* rw= */ true, /* require= */ true, /* measure= */ false, &crypto_what); if (r < 0) return r; what = crypto_what; @@ -646,7 +661,7 @@ static int add_root_cryptsetup(void) { /* If a device /dev/gpt-auto-root-luks appears, then make it pull in systemd-cryptsetup-root.service, which * sets it up, and causes /dev/gpt-auto-root to appear which is all we are looking for. */ - return add_cryptsetup("root", "/dev/gpt-auto-root-luks", /* rw= */ true, /* require= */ false, /* measure= */ true, NULL); + return add_cryptsetup("root", "/dev/gpt-auto-root-luks", arg_root_options, /* rw= */ true, /* require= */ false, /* measure= */ true, NULL); #else return 0; #endif