From 3aa96361ed32b4084cdd59caaebca9cbdc66db0f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 9 Jun 2021 16:33:23 +0200 Subject: [PATCH 1/6] shared/install: ignore failures for auxiliary files If Also= fails, warn, but otherwise ignore the failure. Fixes #19407. --- src/shared/install.c | 23 ++++++++++++++++++----- src/shared/install.h | 1 + 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/src/shared/install.c b/src/shared/install.c index b6dc0c6e60b..a97a4ef5c5c 100644 --- a/src/shared/install.c +++ b/src/shared/install.c @@ -269,7 +269,6 @@ int unit_file_changes_add( _cleanup_free_ char *p = NULL, *s = NULL; UnitFileChange *c; - assert(path); assert(!changes == !n_changes); if (type_or_errno >= 0) @@ -285,11 +284,13 @@ int unit_file_changes_add( return -ENOMEM; *changes = c; - p = strdup(path); - if (!p) - return -ENOMEM; + if (path) { + p = strdup(path); + if (!p) + return -ENOMEM; - path_simplify(p); + path_simplify(p); + } if (source) { s = strdup(source); @@ -355,6 +356,10 @@ void unit_file_dump_changes(int r, const char *verb, const UnitFileChange *chang log_warning("Unit %s is added as a dependency to a non-existent unit %s.", changes[i].source, changes[i].path); break; + case UNIT_FILE_AUXILIARY_FAILED: + if (!quiet) + log_warning("Failed to enable auxiliary unit %s, ignoring.", changes[i].source); + break; case -EEXIST: if (changes[i].source) log_error_errno(changes[i].type_or_errno, @@ -2037,6 +2042,13 @@ static int install_context_apply( q = install_info_traverse(scope, c, paths, i, flags, NULL); if (q < 0) { + if (i->auxiliary) { + q = unit_file_changes_add(changes, n_changes, UNIT_FILE_AUXILIARY_FAILED, NULL, i->name); + if (q < 0) + return q; + continue; + } + unit_file_changes_add(changes, n_changes, q, i->name, NULL); return q; } @@ -3493,6 +3505,7 @@ static const char* const unit_file_change_type_table[_UNIT_FILE_CHANGE_TYPE_MAX] [UNIT_FILE_IS_MASKED] = "masked", [UNIT_FILE_IS_DANGLING] = "dangling", [UNIT_FILE_DESTINATION_NOT_PRESENT] = "destination not present", + [UNIT_FILE_AUXILIARY_FAILED] = "auxiliary unit failed", }; DEFINE_STRING_TABLE_LOOKUP(unit_file_change_type, int); diff --git a/src/shared/install.h b/src/shared/install.h index 7ab0a871e04..74a45db04c6 100644 --- a/src/shared/install.h +++ b/src/shared/install.h @@ -33,6 +33,7 @@ enum { UNIT_FILE_IS_MASKED, UNIT_FILE_IS_DANGLING, UNIT_FILE_DESTINATION_NOT_PRESENT, + UNIT_FILE_AUXILIARY_FAILED, _UNIT_FILE_CHANGE_TYPE_MAX, _UNIT_FILE_CHANGE_TYPE_INVALID = -EINVAL, }; From 4a203a5177b7d9aa499221c315bc0e327a23b5cf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 9 Jun 2021 16:34:20 +0200 Subject: [PATCH 2/6] shared/install: remove custom error handling in unit_file_preset_all() This had some purpose back in the day, but right now I cannot see what difference this makes. It's hard to keep the list of all possible errors up to date. So let's remove this, hopefully nothing breaks. --- src/shared/install.c | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/src/shared/install.c b/src/shared/install.c index a97a4ef5c5c..1cae42df6bd 100644 --- a/src/shared/install.c +++ b/src/shared/install.c @@ -3370,17 +3370,11 @@ int unit_file_preset_all( if (!IN_SET(de->d_type, DT_LNK, DT_REG)) continue; - /* we don't pass changes[] in, because we want to handle errors on our own */ - r = preset_prepare_one(scope, &plus, &minus, &paths, de->d_name, &presets, NULL, 0); - if (r == -ERFKILL) - r = unit_file_changes_add(changes, n_changes, - UNIT_FILE_IS_MASKED, de->d_name, NULL); - else if (r == -ENOLINK) - r = unit_file_changes_add(changes, n_changes, - UNIT_FILE_IS_DANGLING, de->d_name, NULL); - else if (r == -EADDRNOTAVAIL) /* Ignore generated/transient units when applying preset */ - continue; - if (r < 0) + r = preset_prepare_one(scope, &plus, &minus, &paths, de->d_name, &presets, changes, n_changes); + if (r < 0 && + !IN_SET(r, -EEXIST, -ERFKILL, -EADDRNOTAVAIL, -EIDRM, -EUCLEAN, -ELOOP, -ENOENT)) + /* Ignore generated/transient/missing/invalid units when applying preset, propagate other errors. + * Coordinate with unit_file_dump_changes() above. */ return r; } } From e1f2f7f194bf8687e19e74ac703923e4c107b46e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 9 Jun 2021 17:24:52 +0200 Subject: [PATCH 3/6] shared/install: improve message about template mismatch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit $ systemctl enable --root=/ serial-getty@.service Failed to enable unit, unit getty.target is a non-template unit. ↓ Failed to enable serial-getty@.service, destination unit getty.target is a non-template unit. --- src/shared/install.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/shared/install.c b/src/shared/install.c index 1cae42df6bd..c10c15b9ff8 100644 --- a/src/shared/install.c +++ b/src/shared/install.c @@ -382,8 +382,8 @@ void unit_file_dump_changes(int r, const char *verb, const UnitFileChange *chang logged = true; break; case -EIDRM: - log_error_errno(changes[i].type_or_errno, "Failed to %s unit, unit %s is a non-template unit.", - verb, changes[i].path); + log_error_errno(changes[i].type_or_errno, "Failed to %s %s, destination unit %s is a non-template unit.", + verb, changes[i].source, changes[i].path); logged = true; break; case -EUCLEAN: From 8331b221bac2f8bcabe06bcbfae286c084f3d276 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 9 Jun 2021 18:33:14 +0200 Subject: [PATCH 4/6] core/dbus: rename internal variable for clarity --- src/core/dbus-manager.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/core/dbus-manager.c b/src/core/dbus-manager.c index 68a108742a3..758dd8f1b8b 100644 --- a/src/core/dbus-manager.c +++ b/src/core/dbus-manager.c @@ -2272,7 +2272,7 @@ static int method_preset_unit_files_with_mode(sd_bus_message *message, void *use UnitFileChange *changes = NULL; size_t n_changes = 0; Manager *m = userdata; - UnitFilePresetMode mm; + UnitFilePresetMode preset_mode; int runtime, force, r; UnitFileFlags flags; const char *mode; @@ -2291,10 +2291,10 @@ static int method_preset_unit_files_with_mode(sd_bus_message *message, void *use flags = unit_file_bools_to_flags(runtime, force); if (isempty(mode)) - mm = UNIT_FILE_PRESET_FULL; + preset_mode = UNIT_FILE_PRESET_FULL; else { - mm = unit_file_preset_mode_from_string(mode); - if (mm < 0) + preset_mode = unit_file_preset_mode_from_string(mode); + if (preset_mode < 0) return -EINVAL; } @@ -2304,7 +2304,7 @@ static int method_preset_unit_files_with_mode(sd_bus_message *message, void *use if (r == 0) return 1; /* No authorization for now, but the async polkit stuff will call us again when it has it */ - r = unit_file_preset(m->unit_file_scope, flags, NULL, l, mm, &changes, &n_changes); + r = unit_file_preset(m->unit_file_scope, flags, NULL, l, preset_mode, &changes, &n_changes); if (r < 0) return install_error(error, r, changes, n_changes); @@ -2436,7 +2436,7 @@ static int method_preset_all_unit_files(sd_bus_message *message, void *userdata, UnitFileChange *changes = NULL; size_t n_changes = 0; Manager *m = userdata; - UnitFilePresetMode mm; + UnitFilePresetMode preset_mode; const char *mode; UnitFileFlags flags; int force, runtime, r; @@ -2455,10 +2455,10 @@ static int method_preset_all_unit_files(sd_bus_message *message, void *userdata, flags = unit_file_bools_to_flags(runtime, force); if (isempty(mode)) - mm = UNIT_FILE_PRESET_FULL; + preset_mode = UNIT_FILE_PRESET_FULL; else { - mm = unit_file_preset_mode_from_string(mode); - if (mm < 0) + preset_mode = unit_file_preset_mode_from_string(mode); + if (preset_mode < 0) return -EINVAL; } @@ -2468,7 +2468,7 @@ static int method_preset_all_unit_files(sd_bus_message *message, void *userdata, if (r == 0) return 1; /* No authorization for now, but the async polkit stuff will call us again when it has it */ - r = unit_file_preset_all(m->unit_file_scope, flags, NULL, mm, &changes, &n_changes); + r = unit_file_preset_all(m->unit_file_scope, flags, NULL, preset_mode, &changes, &n_changes); if (r < 0) return install_error(error, r, changes, n_changes); From 9b69770a495a170bd6efd5b0c7a89a3ad093a021 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 10 Jun 2021 10:00:16 +0200 Subject: [PATCH 5/6] shared/install: pass UnitFileFlags down into the call chain This just propagates the parameter down into leaf functions, without any functional change. --- src/shared/install.c | 42 +++++++++++++++++++++++------------------- 1 file changed, 23 insertions(+), 19 deletions(-) diff --git a/src/shared/install.c b/src/shared/install.c index c10c15b9ff8..f61d10532f5 100644 --- a/src/shared/install.c +++ b/src/shared/install.c @@ -1848,6 +1848,7 @@ static int install_info_symlink_alias( static int install_info_symlink_wants( UnitFileScope scope, + UnitFileFlags file_flags, UnitFileInstallInfo *i, const LookupPaths *paths, const char *config_path, @@ -1974,10 +1975,10 @@ static int install_info_symlink_link( static int install_info_apply( UnitFileScope scope, + UnitFileFlags file_flags, UnitFileInstallInfo *i, const LookupPaths *paths, const char *config_path, - bool force, UnitFileChange **changes, size_t *n_changes) { @@ -1990,13 +1991,15 @@ static int install_info_apply( if (i->type != UNIT_FILE_TYPE_REGULAR) return 0; + bool force = file_flags & UNIT_FILE_FORCE; + r = install_info_symlink_alias(i, paths, config_path, force, changes, n_changes); - q = install_info_symlink_wants(scope, i, paths, config_path, i->wanted_by, ".wants/", changes, n_changes); + q = install_info_symlink_wants(scope, file_flags, i, paths, config_path, i->wanted_by, ".wants/", changes, n_changes); if (r == 0) r = q; - q = install_info_symlink_wants(scope, i, paths, config_path, i->required_by, ".requires/", changes, n_changes); + q = install_info_symlink_wants(scope, file_flags, i, paths, config_path, i->required_by, ".requires/", changes, n_changes); if (r == 0) r = q; @@ -2010,10 +2013,10 @@ static int install_info_apply( static int install_context_apply( UnitFileScope scope, + UnitFileFlags file_flags, InstallContext *c, const LookupPaths *paths, const char *config_path, - bool force, SearchFlags flags, UnitFileChange **changes, size_t *n_changes) { @@ -2067,7 +2070,7 @@ static int install_context_apply( if (i->type != UNIT_FILE_TYPE_REGULAR) continue; - q = install_info_apply(scope, i, paths, config_path, force, changes, n_changes); + q = install_info_apply(scope, file_flags, i, paths, config_path, changes, n_changes); if (r >= 0) { if (q < 0) r = q; @@ -2540,7 +2543,7 @@ int unit_file_revert( int unit_file_add_dependency( UnitFileScope scope, - UnitFileFlags flags, + UnitFileFlags file_flags, const char *root_dir, char **files, const char *target, @@ -2569,7 +2572,7 @@ int unit_file_add_dependency( if (r < 0) return r; - config_path = (flags & UNIT_FILE_RUNTIME) ? paths.runtime_config : paths.persistent_config; + config_path = (file_flags & UNIT_FILE_RUNTIME) ? paths.runtime_config : paths.persistent_config; if (!config_path) return -ENXIO; @@ -2605,12 +2608,13 @@ int unit_file_add_dependency( return -ENOMEM; } - return install_context_apply(scope, &c, &paths, config_path, !!(flags & UNIT_FILE_FORCE), SEARCH_FOLLOW_CONFIG_SYMLINKS, changes, n_changes); + return install_context_apply(scope, file_flags, &c, &paths, config_path, + SEARCH_FOLLOW_CONFIG_SYMLINKS, changes, n_changes); } int unit_file_enable( UnitFileScope scope, - UnitFileFlags flags, + UnitFileFlags file_flags, const char *root_dir, char **files, UnitFileChange **changes, @@ -2630,7 +2634,7 @@ int unit_file_enable( if (r < 0) return r; - config_path = config_path_from_flags(&paths, flags); + config_path = config_path_from_flags(&paths, file_flags); if (!config_path) return -ENXIO; @@ -2648,7 +2652,7 @@ int unit_file_enable( is useful to determine whether the passed files had any installation data at all. */ - return install_context_apply(scope, &c, &paths, config_path, !!(flags & UNIT_FILE_FORCE), SEARCH_LOAD, changes, n_changes); + return install_context_apply(scope, file_flags, &c, &paths, config_path, SEARCH_LOAD, changes, n_changes); } int unit_file_disable( @@ -3178,13 +3182,13 @@ int unit_file_query_preset(UnitFileScope scope, const char *root_dir, const char static int execute_preset( UnitFileScope scope, + UnitFileFlags file_flags, InstallContext *plus, InstallContext *minus, const LookupPaths *paths, const char *config_path, char **files, UnitFilePresetMode mode, - bool force, UnitFileChange **changes, size_t *n_changes) { @@ -3210,7 +3214,7 @@ static int execute_preset( int q; /* Returns number of symlinks that where supposed to be installed. */ - q = install_context_apply(scope, plus, paths, config_path, force, SEARCH_LOAD, changes, n_changes); + q = install_context_apply(scope, file_flags, plus, paths, config_path, SEARCH_LOAD, changes, n_changes); if (r >= 0) { if (q < 0) r = q; @@ -3278,7 +3282,7 @@ static int preset_prepare_one( int unit_file_preset( UnitFileScope scope, - UnitFileFlags flags, + UnitFileFlags file_flags, const char *root_dir, char **files, UnitFilePresetMode mode, @@ -3300,7 +3304,7 @@ int unit_file_preset( if (r < 0) return r; - config_path = (flags & UNIT_FILE_RUNTIME) ? paths.runtime_config : paths.persistent_config; + config_path = (file_flags & UNIT_FILE_RUNTIME) ? paths.runtime_config : paths.persistent_config; if (!config_path) return -ENXIO; @@ -3314,12 +3318,12 @@ int unit_file_preset( return r; } - return execute_preset(scope, &plus, &minus, &paths, config_path, files, mode, !!(flags & UNIT_FILE_FORCE), changes, n_changes); + return execute_preset(scope, file_flags, &plus, &minus, &paths, config_path, files, mode, changes, n_changes); } int unit_file_preset_all( UnitFileScope scope, - UnitFileFlags flags, + UnitFileFlags file_flags, const char *root_dir, UnitFilePresetMode mode, UnitFileChange **changes, @@ -3340,7 +3344,7 @@ int unit_file_preset_all( if (r < 0) return r; - config_path = (flags & UNIT_FILE_RUNTIME) ? paths.runtime_config : paths.persistent_config; + config_path = (file_flags & UNIT_FILE_RUNTIME) ? paths.runtime_config : paths.persistent_config; if (!config_path) return -ENXIO; @@ -3379,7 +3383,7 @@ int unit_file_preset_all( } } - return execute_preset(scope, &plus, &minus, &paths, config_path, NULL, mode, !!(flags & UNIT_FILE_FORCE), changes, n_changes); + return execute_preset(scope, file_flags, &plus, &minus, &paths, config_path, NULL, mode, changes, n_changes); } static UnitFileList* unit_file_list_free_one(UnitFileList *f) { From ad5fdd391248432e0c105003a8a13f821bde0b8e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 9 Jun 2021 18:41:17 +0200 Subject: [PATCH 6/6] shared/install: ignore enablement of template units w/o instance when presetting When we have a unit which cannot be enabled: # foo@.service: ... [Install] WantedBy=foo.target # there is no instance, so we don't know what to enable we should throw an error when invoked directly with 'enable', but not when doing 'preset' or 'preset-all'. Fixes #19856. --- src/shared/install.c | 16 +++++++++++++--- src/shared/install.h | 9 +++++---- 2 files changed, 18 insertions(+), 7 deletions(-) diff --git a/src/shared/install.c b/src/shared/install.c index f61d10532f5..ba4fa5b136c 100644 --- a/src/shared/install.c +++ b/src/shared/install.c @@ -1916,8 +1916,16 @@ static int install_info_symlink_wants( return q; if (!unit_name_is_valid(dst, valid_dst_type)) { - /* Generate a proper error here: EUCLEAN if the name is generally bad, - * EIDRM if the template status doesn't match. */ + /* Generate a proper error here: EUCLEAN if the name is generally bad, EIDRM if the + * template status doesn't match. If we are doing presets don't bother reporting the + * error. This also covers cases like 'systemctl preset serial-getty@.service', which + * has no DefaultInstance, so there is nothing we can do. At the same time, + * 'systemctl enable serial-getty@.service' should fail, the user should specify an + * instance like in 'systemctl enable serial-getty@ttyS0.service'. + */ + if (file_flags & UNIT_FILE_IGNORE_AUXILIARY_FAILURE) + continue; + if (unit_name_is_valid(dst, UNIT_NAME_ANY)) { unit_file_changes_add(changes, n_changes, -EIDRM, dst, n); r = -EIDRM; @@ -3214,7 +3222,9 @@ static int execute_preset( int q; /* Returns number of symlinks that where supposed to be installed. */ - q = install_context_apply(scope, file_flags, plus, paths, config_path, SEARCH_LOAD, changes, n_changes); + q = install_context_apply(scope, + file_flags | UNIT_FILE_IGNORE_AUXILIARY_FAILURE, + plus, paths, config_path, SEARCH_LOAD, changes, n_changes); if (r >= 0) { if (q < 0) r = q; diff --git a/src/shared/install.h b/src/shared/install.h index 74a45db04c6..c3a0249f5f7 100644 --- a/src/shared/install.h +++ b/src/shared/install.h @@ -39,10 +39,11 @@ enum { }; enum UnitFileFlags { - UNIT_FILE_RUNTIME = 1 << 0, /* Public API via DBUS, do not change */ - UNIT_FILE_FORCE = 1 << 1, /* Public API via DBUS, do not change */ - UNIT_FILE_PORTABLE = 1 << 2, /* Public API via DBUS, do not change */ - UNIT_FILE_DRY_RUN = 1 << 3, + UNIT_FILE_RUNTIME = 1 << 0, /* Public API via DBUS, do not change */ + UNIT_FILE_FORCE = 1 << 1, /* Public API via DBUS, do not change */ + UNIT_FILE_PORTABLE = 1 << 2, /* Public API via DBUS, do not change */ + UNIT_FILE_DRY_RUN = 1 << 3, + UNIT_FILE_IGNORE_AUXILIARY_FAILURE = 1 << 4, _UNIT_FILE_FLAGS_MASK_PUBLIC = UNIT_FILE_RUNTIME|UNIT_FILE_PORTABLE|UNIT_FILE_FORCE, };