From 5ad58fea7303adebf588f8baf34fa84bbb4f036a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 20 Oct 2022 10:32:46 +0200 Subject: [PATCH 1/6] Make comment about coordinating offline and online installation symmetric https://github.com/systemd/systemd/pull/24728#issuecomment-1260966910 --- src/core/dbus-manager.c | 5 ++--- src/shared/install.c | 2 ++ 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/core/dbus-manager.c b/src/core/dbus-manager.c index a6b3c7b36b..bdbaa9b58c 100644 --- a/src/core/dbus-manager.c +++ b/src/core/dbus-manager.c @@ -2133,8 +2133,6 @@ static int send_unit_files_changed(sd_bus *bus, void *userdata) { /* Create an error reply, using the error information from changes[] * if possible, and fall back to generating an error from error code c. * The error message only describes the first error. - * - * Coordinate with install_changes_dump() in install.c. */ static int install_error( sd_bus_error *error, @@ -2146,8 +2144,9 @@ static int install_error( for (size_t i = 0; i < n_changes; i++) - switch (changes[i].type) { + /* When making changes here, make sure to also change install_changes_dump() in install.c. */ + switch (changes[i].type) { case 0 ... _INSTALL_CHANGE_TYPE_MAX: /* not errors */ break; diff --git a/src/shared/install.c b/src/shared/install.c index 53b5aefbe6..4cc0b67b8b 100644 --- a/src/shared/install.c +++ b/src/shared/install.c @@ -332,6 +332,8 @@ void install_changes_dump(int r, const char *verb, const InstallChange *changes, for (size_t i = 0; i < n_changes; i++) { assert(verb || changes[i].type >= 0); + /* When making changes here, make sure to also change install_error() in dbus-manager.c. */ + switch (changes[i].type) { case INSTALL_CHANGE_SYMLINK: if (!quiet) From 94e7298d309fef7710174def820e9d38e512a086 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 20 Oct 2022 11:41:53 +0200 Subject: [PATCH 2/6] shared/install: make install_changes_add propagate passed-in errno value The function was written to only return an error from internal allocation failures, because when using it to create a bus message, we want to distinguish a failed operation from an allocation error when sending the reply. But it turns out that the only caller that makes this distinction checks that the passed-in errno value ('type') is not negative beforehand. So we can make the function pass 'type' value through, which makes most of the callers nicer. No functional change. --- src/shared/install.c | 80 +++++++++++++++++--------------------------- src/shared/install.h | 2 +- 2 files changed, 32 insertions(+), 50 deletions(-) diff --git a/src/shared/install.c b/src/shared/install.c index 4cc0b67b8b..01d4c2ac96 100644 --- a/src/shared/install.c +++ b/src/shared/install.c @@ -265,7 +265,7 @@ static const char* config_path_from_flags(const LookupPaths *lp, UnitFileFlags f return FLAGS_SET(flags, UNIT_FILE_RUNTIME) ? lp->runtime_config : lp->persistent_config; } -int install_changes_add( +InstallChangeType install_changes_add( InstallChange **changes, size_t *n_changes, InstallChangeType type, /* INSTALL_CHANGE_SYMLINK, _UNLINK, _IS_MASKED, _IS_DANGLING, … if positive or errno if negative */ @@ -278,8 +278,11 @@ int install_changes_add( assert(!changes == !n_changes); assert(INSTALL_CHANGE_TYPE_VALID(type)); + /* Register a change or error. Note that the return value may be the error + * that was passed in, or -ENOMEM generated internally. */ + if (!changes) - return 0; + return type; c = reallocarray(*changes, *n_changes + 1, sizeof(InstallChange)); if (!c) @@ -308,7 +311,7 @@ int install_changes_add( .source = TAKE_PTR(s), }; - return 0; + return type; } void install_changes_free(InstallChange *changes, size_t n_changes) { @@ -515,10 +518,8 @@ static int create_symlink( return 1; } - if (errno != EEXIST) { - install_changes_add(changes, n_changes, -errno, new_path, NULL); - return -errno; - } + if (errno != EEXIST) + return install_changes_add(changes, n_changes, -errno, new_path, NULL); r = readlink_malloc(new_path, &dest); if (r < 0) { @@ -526,8 +527,7 @@ static int create_symlink( if (r == -EINVAL) r = -EEXIST; - install_changes_add(changes, n_changes, r, new_path, NULL); - return r; + return install_changes_add(changes, n_changes, r, new_path, NULL); } if (chroot_unit_symlinks_equivalent(lp, new_path, dest, old_path)) { @@ -536,16 +536,12 @@ static int create_symlink( return 1; } - if (!force) { - install_changes_add(changes, n_changes, -EEXIST, new_path, dest); - return -EEXIST; - } + if (!force) + return install_changes_add(changes, n_changes, -EEXIST, new_path, dest); r = symlink_atomic(old_path, new_path); - if (r < 0) { - install_changes_add(changes, n_changes, r, new_path, NULL); - return r; - } + if (r < 0) + return install_changes_add(changes, n_changes, r, new_path, NULL); install_changes_add(changes, n_changes, INSTALL_CHANGE_UNLINK, new_path, NULL); install_changes_add(changes, n_changes, INSTALL_CHANGE_SYMLINK, new_path, old_path); @@ -1093,15 +1089,11 @@ static int install_info_may_process( /* Checks whether the loaded unit file is one we should process, or is masked, * transient or generated and thus not subject to enable/disable operations. */ - if (i->install_mode == INSTALL_MODE_MASKED) { - install_changes_add(changes, n_changes, -ERFKILL, i->path, NULL); - return -ERFKILL; - } + if (i->install_mode == INSTALL_MODE_MASKED) + return install_changes_add(changes, n_changes, -ERFKILL, i->path, NULL); if (path_is_generator(lp, i->path) || - path_is_transient(lp, i->path)) { - install_changes_add(changes, n_changes, -EADDRNOTAVAIL, i->path, NULL); - return -EADDRNOTAVAIL; - } + path_is_transient(lp, i->path)) + return install_changes_add(changes, n_changes, -EADDRNOTAVAIL, i->path, NULL); return 0; } @@ -1854,13 +1846,11 @@ int unit_file_verify_alias( r = unit_validate_alias_symlink_or_warn(LOG_DEBUG, dst_updated ?: dst, info->name); if (r == -ELOOP) /* -ELOOP means self-alias, which we (quietly) ignore */ return r; - if (r < 0) { - install_changes_add(changes, n_changes, - r == -EINVAL ? -EXDEV : r, - dst_updated ?: dst, - info->name); - return r; - } + if (r < 0) + return install_changes_add(changes, n_changes, + r == -EINVAL ? -EXDEV : r, + dst_updated ?: dst, + info->name); } *ret_dst = TAKE_PTR(dst_updated); @@ -1952,10 +1942,8 @@ static int install_info_symlink_wants( if (r < 0) return r; - if (instance.install_mode == INSTALL_MODE_MASKED) { - install_changes_add(changes, n_changes, -ERFKILL, instance.path, NULL); - return -ERFKILL; - } + if (instance.install_mode == INSTALL_MODE_MASKED) + return install_changes_add(changes, n_changes, -ERFKILL, instance.path, NULL); n = instance.name; @@ -1971,10 +1959,8 @@ static int install_info_symlink_wants( _cleanup_free_ char *path = NULL, *dst = NULL; q = install_name_printf(scope, info, *s, &dst); - if (q < 0) { - install_changes_add(changes, n_changes, q, *s, NULL); - return q; - } + if (q < 0) + return install_changes_add(changes, n_changes, q, *s, NULL); if (!unit_name_is_valid(dst, valid_dst_type)) { /* Generate a proper error here: EUCLEAN if the name is generally bad, EIDRM if the @@ -1987,13 +1973,10 @@ static int install_info_symlink_wants( if (file_flags & UNIT_FILE_IGNORE_AUXILIARY_FAILURE) continue; - if (unit_name_is_valid(dst, UNIT_NAME_ANY)) { - install_changes_add(changes, n_changes, -EIDRM, dst, n); - r = -EIDRM; - } else { - install_changes_add(changes, n_changes, -EUCLEAN, dst, NULL); - r = -EUCLEAN; - } + if (unit_name_is_valid(dst, UNIT_NAME_ANY)) + return install_changes_add(changes, n_changes, -EIDRM, dst, n); + else + return install_changes_add(changes, n_changes, -EUCLEAN, dst, NULL); continue; } @@ -2122,8 +2105,7 @@ static int install_context_apply( continue; } - install_changes_add(changes, n_changes, q, i->name, NULL); - return q; + return install_changes_add(changes, n_changes, q, i->name, NULL); } /* We can attempt to process a masked unit when a different unit diff --git a/src/shared/install.h b/src/shared/install.h index d13b143e4e..9bb412ba06 100644 --- a/src/shared/install.h +++ b/src/shared/install.h @@ -197,7 +197,7 @@ int unit_file_exists(LookupScope scope, const LookupPaths *paths, const char *na int unit_file_get_list(LookupScope scope, const char *root_dir, Hashmap *h, char **states, char **patterns); Hashmap* unit_file_list_free(Hashmap *h); -int install_changes_add(InstallChange **changes, size_t *n_changes, int type, const char *path, const char *source); +InstallChangeType install_changes_add(InstallChange **changes, size_t *n_changes, int type, const char *path, const char *source); void install_changes_free(InstallChange *changes, size_t n_changes); void install_changes_dump(int r, const char *verb, const InstallChange *changes, size_t n_changes, bool quiet); From 81de6962737a7d2817faad1cc2c50511a5972119 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 20 Oct 2022 11:31:41 +0200 Subject: [PATCH 3/6] shared/install: add forgotten calls to install_changes_add() The machinery to report a good error message only works if the error was registered with install_changes_add() and a file name. Otherwise we only get a generic "Op failed: %m" message. In some places -EINVAL is replaced by -EUCLEAN, so that we get the proper error message. --- src/shared/install.c | 49 ++++++++++++++++++++++---------------------- 1 file changed, 24 insertions(+), 25 deletions(-) diff --git a/src/shared/install.c b/src/shared/install.c index 01d4c2ac96..71010cff37 100644 --- a/src/shared/install.c +++ b/src/shared/install.c @@ -2386,26 +2386,27 @@ int unit_file_link( char *fn; if (!path_is_absolute(*file)) - return -EINVAL; + return install_changes_add(changes, n_changes, -EINVAL, *file, NULL); fn = basename(*file); if (!unit_name_is_valid(fn, UNIT_NAME_ANY)) - return -EINVAL; + return install_changes_add(changes, n_changes, -EUCLEAN, *file, NULL); full = path_join(lp.root_dir, *file); if (!full) return -ENOMEM; if (lstat(full, &st) < 0) - return -errno; + return install_changes_add(changes, n_changes, -errno, *file, NULL); + r = stat_verify_regular(&st); if (r < 0) - return r; + return install_changes_add(changes, n_changes, r, *file, NULL); - q = in_search_path(&lp, *file); - if (q < 0) - return q; - if (q > 0) + r = in_search_path(&lp, *file); + if (r < 0) + return install_changes_add(changes, n_changes, r, *file, NULL); + if (r > 0) continue; if (!GREEDY_REALLOC0(todo, n_todo + 2)) @@ -2498,15 +2499,15 @@ int unit_file_revert( if (!path) return -ENOMEM; - r = lstat(path, &st); + r = RET_NERRNO(lstat(path, &st)); if (r < 0) { - if (errno != ENOENT) - return -errno; + if (r != -ENOENT) + return install_changes_add(changes, n_changes, r, path, NULL); } else if (S_ISREG(st.st_mode)) { /* Check if there's a vendor version */ r = path_is_vendor_or_generator(&lp, path); if (r < 0) - return r; + return install_changes_add(changes, n_changes, r, path, NULL); if (r > 0) has_vendor = true; } @@ -2515,15 +2516,15 @@ int unit_file_revert( if (!dropin) return -ENOMEM; - r = lstat(dropin, &st); + r = RET_NERRNO(lstat(dropin, &st)); if (r < 0) { - if (errno != ENOENT) - return -errno; + if (r != -ENOENT) + return install_changes_add(changes, n_changes, r, dropin, NULL); } else if (S_ISDIR(st.st_mode)) { /* Remove the drop-ins */ r = path_shall_revert(&lp, dropin); if (r < 0) - return r; + return install_changes_add(changes, n_changes, r, dropin, NULL); if (r > 0) { if (!GREEDY_REALLOC0(todo, n_todo + 2)) return -ENOMEM; @@ -2545,14 +2546,14 @@ int unit_file_revert( if (!path) return -ENOMEM; - r = lstat(path, &st); + r = RET_NERRNO(lstat(path, &st)); if (r < 0) { - if (errno != ENOENT) - return -errno; + if (r != -ENOENT) + return install_changes_add(changes, n_changes, r, path, NULL); } else if (S_ISREG(st.st_mode) || S_ISLNK(st.st_mode)) { r = path_is_config(&lp, path, true); if (r < 0) - return r; + return install_changes_add(changes, n_changes, r, path, NULL); if (r > 0) { if (!GREEDY_REALLOC0(todo, n_todo + 2)) return -ENOMEM; @@ -2626,12 +2627,10 @@ int unit_file_add_dependency( assert(scope >= 0); assert(scope < _LOOKUP_SCOPE_MAX); assert(target); - - if (!IN_SET(dep, UNIT_WANTS, UNIT_REQUIRES)) - return -EINVAL; + assert(IN_SET(dep, UNIT_WANTS, UNIT_REQUIRES)); if (!unit_name_is_valid(target, UNIT_NAME_ANY)) - return -EINVAL; + return install_changes_add(changes, n_changes, -EUCLEAN, target, NULL); r = lookup_paths_init(&lp, scope, 0, root_dir); if (r < 0) @@ -2750,7 +2749,7 @@ static int do_unit_file_disable( STRV_FOREACH(name, names) { if (!unit_name_is_valid(*name, UNIT_NAME_ANY)) - return -EINVAL; + return install_changes_add(changes, n_changes, -EUCLEAN, *name, NULL); r = install_info_add(&ctx, *name, NULL, lp->root_dir, /* auxiliary= */ false, NULL); if (r < 0) From f31f10a6207efc9ae9e0b1f73975b5b610914017 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 20 Oct 2022 12:07:10 +0200 Subject: [PATCH 4/6] shared/install: check that install_changes_add() didn't fail on success This adds a check for an allocation error for the calls to install_changes_add() where we're plannig to return success from the call. In cases where we're returning failure, it doesn't matter as much: the operation will fail anyway, and if the allocation fails, we'll just get a less descriptive error message. --- src/shared/install.c | 60 ++++++++++++++++++++++++++++++-------------- 1 file changed, 41 insertions(+), 19 deletions(-) diff --git a/src/shared/install.c b/src/shared/install.c index 71010cff37..cca8b0b0c8 100644 --- a/src/shared/install.c +++ b/src/shared/install.c @@ -514,7 +514,9 @@ static int create_symlink( (void) mkdir_parents_label(new_path, 0755); if (symlink(old_path, new_path) >= 0) { - install_changes_add(changes, n_changes, INSTALL_CHANGE_SYMLINK, new_path, old_path); + r = install_changes_add(changes, n_changes, INSTALL_CHANGE_SYMLINK, new_path, old_path); + if (r < 0) + return r; return 1; } @@ -543,8 +545,12 @@ static int create_symlink( if (r < 0) return install_changes_add(changes, n_changes, r, new_path, NULL); - install_changes_add(changes, n_changes, INSTALL_CHANGE_UNLINK, new_path, NULL); - install_changes_add(changes, n_changes, INSTALL_CHANGE_SYMLINK, new_path, old_path); + r = install_changes_add(changes, n_changes, INSTALL_CHANGE_UNLINK, new_path, NULL); + if (r < 0) + return r; + r = install_changes_add(changes, n_changes, INSTALL_CHANGE_SYMLINK, new_path, old_path); + if (r < 0) + return r; return 1; } @@ -697,7 +703,9 @@ static int remove_marked_symlinks_fd( (void) rmdir_parents(p, config_path); } - install_changes_add(changes, n_changes, INSTALL_CHANGE_UNLINK, p, NULL); + q = install_changes_add(changes, n_changes, INSTALL_CHANGE_UNLINK, p, NULL); + if (q < 0) + return q; /* Now, remember the full path (but with the root prefix removed) of * the symlink we just removed, and remove any symlinks to it, too. */ @@ -1989,8 +1997,11 @@ static int install_info_symlink_wants( if (r == 0) r = q; - if (unit_file_exists(scope, lp, dst) == 0) - install_changes_add(changes, n_changes, INSTALL_CHANGE_DESTINATION_NOT_PRESENT, dst, info->path); + if (unit_file_exists(scope, lp, dst) == 0) { + q = install_changes_add(changes, n_changes, INSTALL_CHANGE_DESTINATION_NOT_PRESENT, dst, info->path); + if (q < 0) + return q; + } } return r; @@ -2111,7 +2122,9 @@ static int install_context_apply( /* We can attempt to process a masked unit when a different unit * that we were processing specifies it in Also=. */ if (i->install_mode == INSTALL_MODE_MASKED) { - install_changes_add(changes, n_changes, INSTALL_CHANGE_IS_MASKED, i->path, NULL); + q = install_changes_add(changes, n_changes, INSTALL_CHANGE_IS_MASKED, i->path, NULL); + if (q < 0) + return q; if (r >= 0) /* Assume that something *could* have been enabled here, * avoid "empty [Install] section" warning. */ @@ -2167,16 +2180,18 @@ static int install_context_mark_for_removal( r = install_info_traverse(ctx, lp, i, SEARCH_LOAD|SEARCH_FOLLOW_CONFIG_SYMLINKS, NULL); if (r == -ENOLINK) { log_debug_errno(r, "Name %s leads to a dangling symlink, removing name.", i->name); - install_changes_add(changes, n_changes, INSTALL_CHANGE_IS_DANGLING, i->path ?: i->name, NULL); + r = install_changes_add(changes, n_changes, INSTALL_CHANGE_IS_DANGLING, i->path ?: i->name, NULL); + if (r < 0) + return r; } else if (r == -ENOENT) { - if (i->auxiliary) /* some unit specified in Also= or similar is missing */ log_debug_errno(r, "Auxiliary unit of %s not found, removing name.", i->name); else { log_debug_errno(r, "Unit %s not found, removing name.", i->name); - install_changes_add(changes, n_changes, r, i->path ?: i->name, NULL); + r = install_changes_add(changes, n_changes, r, i->path ?: i->name, NULL); + if (r < 0) + return r; } - } else if (r < 0) { log_debug_errno(r, "Failed to find unit %s, removing name: %m", i->name); install_changes_add(changes, n_changes, r, i->path ?: i->name, NULL); @@ -2287,11 +2302,12 @@ int unit_file_unmask( if (r < 0) { if (r != -ENOENT) log_debug_errno(r, "Failed to look up unit %s, ignoring: %m", info.name); - } else { - if (info.install_mode == INSTALL_MODE_MASKED && - path_is_generator(&lp, info.path)) - install_changes_add(changes, n_changes, - INSTALL_CHANGE_IS_MASKED_GENERATOR, info.name, info.path); + } else if (info.install_mode == INSTALL_MODE_MASKED && + path_is_generator(&lp, info.path)) { + r = install_changes_add(changes, n_changes, + INSTALL_CHANGE_IS_MASKED_GENERATOR, info.name, info.path); + if (r < 0) + return r; } TAKE_PTR(info.name); /* … and give it back here */ @@ -2340,7 +2356,9 @@ int unit_file_unmask( continue; } - install_changes_add(changes, n_changes, INSTALL_CHANGE_UNLINK, path, NULL); + q = install_changes_add(changes, n_changes, INSTALL_CHANGE_UNLINK, path, NULL); + if (q < 0) + return q; rp = skip_root(lp.root_dir, path); q = mark_symlink_for_removal(&remove_symlinks_to, rp ?: path); @@ -2586,10 +2604,14 @@ int unit_file_revert( if (!t) return -ENOMEM; - install_changes_add(changes, n_changes, INSTALL_CHANGE_UNLINK, t, NULL); + q = install_changes_add(changes, n_changes, INSTALL_CHANGE_UNLINK, t, NULL); + if (q < 0) + return q; } - install_changes_add(changes, n_changes, INSTALL_CHANGE_UNLINK, *i, NULL); + q = install_changes_add(changes, n_changes, INSTALL_CHANGE_UNLINK, *i, NULL); + if (q < 0) + return q; rp = skip_root(lp.root_dir, *i); q = mark_symlink_for_removal(&remove_symlinks_to, rp ?: *i); From a6f318a554d357d66329c869f7ca0440f704b8c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 20 Oct 2022 11:32:25 +0200 Subject: [PATCH 5/6] systemctl,manager: refuse linking unit files underneath the search paths We treat symlinks to unit files outside of the search path differently from symlinks to unit files *in* the search path. The former are "linked" unit files, while the latter are enablement symlinks and such and will be removed when disabling the unit. The history of the check for in_search_path() is interesting: this condition was added already in the first version of the code in 830964834f330836b9d33752e83de09d4f38da87. Since the beginning, matching arguments would simply be ignored. I think this is dubious. The man page says: > Link a unit file that is *not* in the unit file search paths > into the unit file search path But for backwards-compat, let's continue to silently do nothing for files *in* the search path. The case of symlinks to unit files underneath the search path, but in some subdirectory, is less clear. We didn't check for this case, so it was implicitly allowed. But that's just an oversight, we don't want to allow people to create additional subhierarchies under our hierarchy. Let's check for this case and refuse. Closes #24605. --- src/core/dbus-manager.c | 5 +++++ src/libsystemd/sd-bus/bus-common-errors.h | 1 + src/shared/install.c | 23 ++++++++++++++++++++--- 3 files changed, 26 insertions(+), 3 deletions(-) diff --git a/src/core/dbus-manager.c b/src/core/dbus-manager.c index bdbaa9b58c..633873da27 100644 --- a/src/core/dbus-manager.c +++ b/src/core/dbus-manager.c @@ -2171,6 +2171,11 @@ static int install_error( "Unit %s is transient or generated.", changes[i].path); goto found; + case -ETXTBSY: + r = sd_bus_error_setf(error, BUS_ERROR_UNIT_BAD_PATH, + "File %s is under the systemd unit hierarchy already.", changes[i].path); + goto found; + case -EUCLEAN: r = sd_bus_error_setf(error, BUS_ERROR_BAD_UNIT_SETTING, "\"%s\" is not a valid unit name.", diff --git a/src/libsystemd/sd-bus/bus-common-errors.h b/src/libsystemd/sd-bus/bus-common-errors.h index c3c25d69c3..d4a1fb689e 100644 --- a/src/libsystemd/sd-bus/bus-common-errors.h +++ b/src/libsystemd/sd-bus/bus-common-errors.h @@ -20,6 +20,7 @@ #define BUS_ERROR_UNIT_MASKED "org.freedesktop.systemd1.UnitMasked" #define BUS_ERROR_UNIT_GENERATED "org.freedesktop.systemd1.UnitGenerated" #define BUS_ERROR_UNIT_LINKED "org.freedesktop.systemd1.UnitLinked" +#define BUS_ERROR_UNIT_BAD_PATH "org.freedesktop.systemd1.UnitBadPath" #define BUS_ERROR_JOB_TYPE_NOT_APPLICABLE "org.freedesktop.systemd1.JobTypeNotApplicable" #define BUS_ERROR_NO_ISOLATION "org.freedesktop.systemd1.NoIsolation" #define BUS_ERROR_SHUTTING_DOWN "org.freedesktop.systemd1.ShuttingDown" diff --git a/src/shared/install.c b/src/shared/install.c index cca8b0b0c8..6d91a02f11 100644 --- a/src/shared/install.c +++ b/src/shared/install.c @@ -104,13 +104,19 @@ static int in_search_path(const LookupPaths *lp, const char *path) { _cleanup_free_ char *parent = NULL; int r; - assert(path); + /* Check if 'path' is in lp->search_path. */ - r = path_extract_directory(path, &parent); + r = path_extract_directory(ASSERT_PTR(path), &parent); if (r < 0) return r; - return path_strv_contains(lp->search_path, parent); + return path_strv_contains(ASSERT_PTR(lp)->search_path, parent); +} + +static int underneath_search_path(const LookupPaths *lp, const char *path) { + /* Check if 'path' is underneath lp->search_path. */ + + return !!path_startswith_strv(ASSERT_PTR(path), ASSERT_PTR(lp)->search_path); } static const char* skip_root(const char *root_dir, const char *path) { @@ -390,6 +396,10 @@ void install_changes_dump(int r, const char *verb, const InstallChange *changes, err = log_error_errno(changes[i].type, "Failed to %s unit, unit %s is transient or generated.", verb, changes[i].path); break; + case -ETXTBSY: + err = log_error_errno(changes[i].type, "Failed to %s unit, file %s is under the systemd unit hierarchy already.", + verb, changes[i].path); + break; case -EBADSLT: err = log_error_errno(changes[i].type, "Failed to %s unit, invalid specifier in \"%s\".", verb, changes[i].path); @@ -2425,8 +2435,15 @@ int unit_file_link( if (r < 0) return install_changes_add(changes, n_changes, r, *file, NULL); if (r > 0) + /* A silent noop if the file is already in the search path. */ continue; + r = underneath_search_path(&lp, *file); + if (r > 0) + r = -ETXTBSY; + if (r < 0) + return install_changes_add(changes, n_changes, r, *file, NULL); + if (!GREEDY_REALLOC0(todo, n_todo + 2)) return -ENOMEM; From 32d2e70ae48e11f80a4f9f59dbf23e124342fd6a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 20 Oct 2022 12:27:29 +0200 Subject: [PATCH 6/6] man: fix count mismatch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We said "search path" and "search paths" in the same sentence… --- man/systemctl.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/man/systemctl.xml b/man/systemctl.xml index 6dbf5f566d..4d4f6c3992 100644 --- a/man/systemctl.xml +++ b/man/systemctl.xml @@ -986,7 +986,7 @@ Jan 12 10:46:45 example.com bluetoothd[8900]: gatt-time-server: Input/output err link PATH… - Link a unit file that is not in the unit file search paths into the unit file search path. This + Link a unit file that is not in the unit file search path into the unit file search path. This command expects an absolute path to a unit file. The effect of this may be undone with disable. The effect of this command is that a unit file is made available for commands such as start, even though it is not installed directly in the unit search path. The