From c67d64add6f5bad6f65c7b61b2dc0f138d5370bd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 29 Jan 2026 07:45:55 +0100 Subject: [PATCH 1/2] shared/install: fix bogus error handling This partially reverts a4f0e0da3573a10bc5404142be8799418760b1d1. The intent was good, we gather the errors, but we have no mechanism to propagate the result, so the gathered result was ignored. In 'changes' we can only report errors for specific units. If reading of the directory fails, we might just as well report the error immediately. This isn't great, but it's better then ignoring the errors. In practice, failing halfway in this manner is unlikely, since it'd mean that the fs is corrupted or something like that. We might as well return immediately on such catastrophic errors. --- src/shared/install.c | 51 ++++++++++++++++++++++++-------------------- 1 file changed, 28 insertions(+), 23 deletions(-) diff --git a/src/shared/install.c b/src/shared/install.c index d85ac2df62..7e3469efb0 100644 --- a/src/shared/install.c +++ b/src/shared/install.c @@ -370,6 +370,24 @@ static void install_change_dump_success(const InstallChange *change) { } } +/* Generated/transient/missing/invalid units when applying presets. + * Coordinate with install_change_dump_error() below. */ +static bool ERRNO_IS_NEG_UNIT_ISSUE(intmax_t r) { + return IN_SET(r, + -EEXIST, + -ERFKILL, + -EADDRNOTAVAIL, + -ETXTBSY, + -EBADSLT, + -EIDRM, + -EUCLEAN, + -ELOOP, + -EXDEV, + -ENOENT, + -ENOLINK, + -EUNATCH); +} + int install_change_dump_error(const InstallChange *change, char **ret_errmsg, const char **ret_bus_error) { char *m; const char *bus_error; @@ -3609,6 +3627,7 @@ static int preset_prepare_one( &info, changes, n_changes); if (r < 0) return r; + if (!streq(name, info->name)) { log_debug("Skipping %s because it is an alias for %s.", name, info->name); return 0; @@ -3710,19 +3729,19 @@ int unit_file_preset_all( if (r < 0) return r; - r = 0; STRV_FOREACH(i, lp.search_path) { _cleanup_closedir_ DIR *d = NULL; d = opendir(*i); if (!d) { - if (errno != ENOENT) - RET_GATHER(r, -errno); - continue; + if (errno == ENOENT) + continue; + + return log_debug_errno(errno, "Failed to opendir %s: %m", *i); } - FOREACH_DIRENT(de, d, RET_GATHER(r, -errno)) { - int k; + FOREACH_DIRENT(de, d, + return log_debug_errno(errno, "Failed to read directory %s: %m", *i)) { if (!unit_name_is_valid(de->d_name, UNIT_NAME_ANY)) continue; @@ -3730,23 +3749,9 @@ int unit_file_preset_all( if (!IN_SET(de->d_type, DT_LNK, DT_REG)) continue; - k = preset_prepare_one(scope, &plus, &minus, &lp, de->d_name, &presets, changes, n_changes); - if (k < 0 && - !IN_SET(k, -EEXIST, - -ERFKILL, - -EADDRNOTAVAIL, - -ETXTBSY, - -EBADSLT, - -EIDRM, - -EUCLEAN, - -ELOOP, - -EXDEV, - -ENOENT, - -ENOLINK, - -EUNATCH)) - /* Ignore generated/transient/missing/invalid units when applying preset, propagate other errors. - * Coordinate with install_change_dump_error() above. */ - RET_GATHER(r, k); + r = preset_prepare_one(scope, &plus, &minus, &lp, de->d_name, &presets, changes, n_changes); + if (r < 0 && !ERRNO_IS_NEG_UNIT_ISSUE(r)) + return r; } } From 64eb78135c49b7d84bfa183cae0dc3a56789b20c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 29 Jan 2026 08:12:52 +0100 Subject: [PATCH 2/2] shared/install: rework error propagation again MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The immediate impulse for this change is the fedora scriptlet which called: /usr/lib/systemd/systemd-update-helper install-system-units cryptsetup-pre.target cryptsetup.target getty@.service ... system-systemd\x2dcryptsetup.slice system-systemd\x2dveritysetup.slice ... which called systemctl preset cryptsetup-pre.target cryptsetup.target getty@.service ... system-systemd\x2dcryptsetup.slice system-systemd\x2dveritysetup.slice ... which threw an error that system-systemdx2dcryptsetup.slice does not exist and did nothing at all. (The backslash is consumed by the shell.) The obvious fix here is to figure out more levels of escaping… But we should do something more robust in such cases. If we fail in processing of a single unit, let preset all continue processing units, report the failure through 'changes'. At the end, return failure. In general, for operations which operate on a list of units specified by the user, fail the whole operation if any of the individual operations failed. The only operation where we don't do this is 'preset-all'. $ SYSTEMD_LOG_LEVEL=debug build/systemctl --root=/ preset asdf1.servie asdf2.path; echo $? Cannot find unit asdf1.servie.service. Cannot find unit asdf2.path. Failed to preset unit: Unit asdf1.servie.service does not exist Failed to preset unit: Unit asdf2.path does not exist 1 While at it, fix double logging in the manager: dump_unit_changes() already logs about errors, so the manager should only log on success. --- src/core/manager.c | 7 ++----- src/shared/install.c | 18 ++++++++++++------ src/shared/install.h | 2 +- src/systemctl/systemctl-add-dependency.c | 2 +- src/systemctl/systemctl-enable.c | 2 +- src/systemctl/systemctl-preset-all.c | 3 ++- src/systemctl/systemctl-set-default.c | 2 +- 7 files changed, 20 insertions(+), 16 deletions(-) diff --git a/src/core/manager.c b/src/core/manager.c index 508b9fc934..277935bab2 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -1946,11 +1946,8 @@ static void manager_preset_all(Manager *m) { log_info("Applying preset policy."); r = unit_file_preset_all(RUNTIME_SCOPE_SYSTEM, /* file_flags= */ 0, /* root_dir= */ NULL, mode, &changes, &n_changes); - install_changes_dump(r, "preset", changes, n_changes, /* quiet= */ false); - if (r < 0) - log_full_errno(r == -EEXIST ? LOG_NOTICE : LOG_WARNING, r, - "Failed to populate /etc with preset unit settings, ignoring: %m"); - else + r = install_changes_dump(r, "preset all", changes, n_changes, /* quiet= */ false); + if (r >= 0) log_info("Populated /etc with preset unit settings."); } diff --git a/src/shared/install.c b/src/shared/install.c index 7e3469efb0..dc27d53f09 100644 --- a/src/shared/install.c +++ b/src/shared/install.c @@ -480,7 +480,7 @@ int install_change_dump_error(const InstallChange *change, char **ret_errmsg, co return 0; } -void install_changes_dump( +int install_changes_dump( int error, const char *verb, const InstallChange *changes, @@ -494,6 +494,8 @@ void install_changes_dump( assert(verb || error >= 0); assert(changes || n_changes == 0); + /* An error is returned if 'error' contains an error or if any of the changes failed. */ + FOREACH_ARRAY(i, changes, n_changes) if (i->type >= 0) { if (!quiet) @@ -505,17 +507,21 @@ void install_changes_dump( r = install_change_dump_error(i, &err_message, /* ret_bus_error= */ NULL); if (r == -ENOMEM) - return (void) log_oom(); + return log_oom(); if (r < 0) - log_error_errno(r, "Failed to %s unit %s: %m", verb, i->path); + RET_GATHER(error, + log_error_errno(r, "Failed to %s unit %s: %m", verb, i->path)); else - log_error_errno(i->type, "Failed to %s unit: %s", verb, err_message); + RET_GATHER(error, + log_error_errno(i->type, "Failed to %s unit: %s", verb, err_message)); err_logged = true; } if (error < 0 && !err_logged) - log_error_errno(error, "Failed to %s unit: %m.", verb); + log_error_errno(error, "Failed to %s units: %m.", verb); + + return error; } /** @@ -3692,7 +3698,7 @@ int unit_file_preset( STRV_FOREACH(name, names) { r = preset_prepare_one(scope, &plus, &minus, &lp, *name, &presets, changes, n_changes); - if (r < 0) + if (r < 0 && !ERRNO_IS_NEG_UNIT_ISSUE(r)) return r; } diff --git a/src/shared/install.h b/src/shared/install.h index aa2e7c7f59..c4592fed35 100644 --- a/src/shared/install.h +++ b/src/shared/install.h @@ -192,7 +192,7 @@ InstallChangeType install_changes_add(InstallChange **changes, size_t *n_changes void install_changes_free(InstallChange *changes, size_t n_changes); int install_change_dump_error(const InstallChange *change, char **ret_errmsg, const char **ret_bus_error); -void install_changes_dump( +int install_changes_dump( int error, const char *verb, const InstallChange *changes, diff --git a/src/systemctl/systemctl-add-dependency.c b/src/systemctl/systemctl-add-dependency.c index b1fd924209..bc1a1f00f6 100644 --- a/src/systemctl/systemctl-add-dependency.c +++ b/src/systemctl/systemctl-add-dependency.c @@ -51,7 +51,7 @@ int verb_add_dependency(int argc, char *argv[], void *userdata) { CLEANUP_ARRAY(changes, n_changes, install_changes_free); r = unit_file_add_dependency(arg_runtime_scope, unit_file_flags_from_args(), arg_root, names, target, dep, &changes, &n_changes); - install_changes_dump(r, "add dependency on", changes, n_changes, arg_quiet); + r = install_changes_dump(r, "add dependency on", changes, n_changes, arg_quiet); if (r < 0) return r; } else { diff --git a/src/systemctl/systemctl-enable.c b/src/systemctl/systemctl-enable.c index 72e222c2cb..f6277b7287 100644 --- a/src/systemctl/systemctl-enable.c +++ b/src/systemctl/systemctl-enable.c @@ -258,7 +258,7 @@ int verb_enable(int argc, char *argv[], void *userdata) { else assert_not_reached(); - install_changes_dump(r, verb, changes, n_changes, arg_quiet); + r = install_changes_dump(r, verb, changes, n_changes, arg_quiet); if (r < 0) return r; } diff --git a/src/systemctl/systemctl-preset-all.c b/src/systemctl/systemctl-preset-all.c index bf681a6d36..bcc001fab7 100644 --- a/src/systemctl/systemctl-preset-all.c +++ b/src/systemctl/systemctl-preset-all.c @@ -26,7 +26,8 @@ int verb_preset_all(int argc, char *argv[], void *userdata) { CLEANUP_ARRAY(changes, n_changes, install_changes_free); r = unit_file_preset_all(arg_runtime_scope, unit_file_flags_from_args(), arg_root, arg_preset_mode, &changes, &n_changes); - install_changes_dump(r, "preset", changes, n_changes, arg_quiet); + /* We do not treat propagate failure of individual units here. */ + (void) install_changes_dump(r, "preset all", changes, n_changes, arg_quiet); if (r < 0) return r; } else { diff --git a/src/systemctl/systemctl-set-default.c b/src/systemctl/systemctl-set-default.c index 3a8e3969e2..c4faa31b4c 100644 --- a/src/systemctl/systemctl-set-default.c +++ b/src/systemctl/systemctl-set-default.c @@ -123,7 +123,7 @@ int verb_set_default(int argc, char *argv[], void *userdata) { CLEANUP_ARRAY(changes, n_changes, install_changes_free); r = unit_file_set_default(arg_runtime_scope, UNIT_FILE_FORCE, arg_root, unit, &changes, &n_changes); - install_changes_dump(r, "set default", changes, n_changes, arg_quiet); + r = install_changes_dump(r, "set default", changes, n_changes, arg_quiet); if (r < 0) return r; } else {