From 64eb78135c49b7d84bfa183cae0dc3a56789b20c Mon Sep 17 00:00:00 2001 From: =?utf8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 29 Jan 2026 08:12:52 +0100 Subject: [PATCH] shared/install: rework error propagation again MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 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 508b9fc9345..277935bab20 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 7e3469efb06..dc27d53f098 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 aa2e7c7f59e..c4592fed358 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 b1fd9242092..bc1a1f00f69 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 72e222c2cb8..f6277b72871 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 bf681a6d36a..bcc001fab77 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 3a8e3969e20..c4faa31b4c1 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 { -- 2.47.3