From 35bcb96c1ac2db777db1649026f931ce77c9c7ba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 10 Mar 2022 11:03:41 +0100 Subject: [PATCH] shared/install: propagate errors about invalid aliases and such too If an invalid arg appears in [Install] Alias=, WantedBy=, RequiredBy=, we'd warn in the logs, but not propagate this information to the caller, and in particular not over dbus. But if we call "systemctl enable" on a unit, and the config if invalid, this information is quite important. (cherry picked from commit cbfdbffb618f1d75e668c59887a27c7a60950546) Related: #2082131 --- src/basic/unit-file.c | 44 +++++++++++++------------- src/basic/unit-file.h | 2 +- src/shared/install.c | 61 ++++++++++++++++++++++++++---------- src/shared/install.h | 7 ++++- src/test/test-install-root.c | 2 +- src/test/test-unit-file.c | 28 ++++++++--------- 6 files changed, 88 insertions(+), 56 deletions(-) diff --git a/src/basic/unit-file.c b/src/basic/unit-file.c index f7a10b22c6..105dacc1b2 100644 --- a/src/basic/unit-file.c +++ b/src/basic/unit-file.c @@ -69,7 +69,7 @@ int unit_symlink_name_compatible(const char *symlink, const char *target, bool i return 0; } -int unit_validate_alias_symlink_and_warn(const char *filename, const char *target) { +int unit_validate_alias_symlink_or_warn(int log_level, const char *filename, const char *target) { const char *src, *dst; _cleanup_free_ char *src_instance = NULL, *dst_instance = NULL; UnitType src_unit_type, dst_unit_type; @@ -92,51 +92,51 @@ int unit_validate_alias_symlink_and_warn(const char *filename, const char *targe src_name_type = unit_name_to_instance(src, &src_instance); if (src_name_type < 0) - return log_notice_errno(src_name_type, - "%s: not a valid unit name \"%s\": %m", filename, src); + return log_full_errno(log_level, src_name_type, + "%s: not a valid unit name \"%s\": %m", filename, src); src_unit_type = unit_name_to_type(src); assert(src_unit_type >= 0); /* unit_name_to_instance() checked the suffix already */ if (!unit_type_may_alias(src_unit_type)) - return log_notice_errno(SYNTHETIC_ERRNO(EINVAL), - "%s: symlinks are not allowed for units of this type, rejecting.", - filename); + return log_full_errno(log_level, SYNTHETIC_ERRNO(EINVAL), + "%s: symlinks are not allowed for units of this type, rejecting.", + filename); if (src_name_type != UNIT_NAME_PLAIN && !unit_type_may_template(src_unit_type)) - return log_notice_errno(SYNTHETIC_ERRNO(EINVAL), - "%s: templates not allowed for %s units, rejecting.", - filename, unit_type_to_string(src_unit_type)); + return log_full_errno(log_level, SYNTHETIC_ERRNO(EINVAL), + "%s: templates not allowed for %s units, rejecting.", + filename, unit_type_to_string(src_unit_type)); /* dst checks */ dst_name_type = unit_name_to_instance(dst, &dst_instance); if (dst_name_type < 0) - return log_notice_errno(dst_name_type == -EINVAL ? SYNTHETIC_ERRNO(EXDEV) : dst_name_type, - "%s points to \"%s\" which is not a valid unit name: %m", - filename, dst); + return log_full_errno(log_level, dst_name_type == -EINVAL ? SYNTHETIC_ERRNO(EXDEV) : dst_name_type, + "%s points to \"%s\" which is not a valid unit name: %m", + filename, dst); if (!(dst_name_type == src_name_type || (src_name_type == UNIT_NAME_INSTANCE && dst_name_type == UNIT_NAME_TEMPLATE))) - return log_notice_errno(SYNTHETIC_ERRNO(EXDEV), - "%s: symlink target name type \"%s\" does not match source, rejecting.", - filename, dst); + return log_full_errno(log_level, SYNTHETIC_ERRNO(EXDEV), + "%s: symlink target name type \"%s\" does not match source, rejecting.", + filename, dst); if (dst_name_type == UNIT_NAME_INSTANCE) { assert(src_instance); assert(dst_instance); if (!streq(src_instance, dst_instance)) - return log_notice_errno(SYNTHETIC_ERRNO(EXDEV), - "%s: unit symlink target \"%s\" instance name doesn't match, rejecting.", - filename, dst); + return log_full_errno(log_level, SYNTHETIC_ERRNO(EXDEV), + "%s: unit symlink target \"%s\" instance name doesn't match, rejecting.", + filename, dst); } dst_unit_type = unit_name_to_type(dst); if (dst_unit_type != src_unit_type) - return log_notice_errno(SYNTHETIC_ERRNO(EXDEV), - "%s: symlink target \"%s\" has incompatible suffix, rejecting.", - filename, dst); + return log_full_errno(log_level, SYNTHETIC_ERRNO(EXDEV), + "%s: symlink target \"%s\" has incompatible suffix, rejecting.", + filename, dst); return 0; } @@ -355,7 +355,7 @@ int unit_file_resolve_symlink( "Suspicious symlink %s/%sā†’%s, treating as alias.", dir, filename, simplified); - r = unit_validate_alias_symlink_and_warn(filename, simplified); + r = unit_validate_alias_symlink_or_warn(LOG_NOTICE, filename, simplified); if (r < 0) return r; diff --git a/src/basic/unit-file.h b/src/basic/unit-file.h index e29e878cfd..b7c03e9c2c 100644 --- a/src/basic/unit-file.h +++ b/src/basic/unit-file.h @@ -41,7 +41,7 @@ bool unit_type_may_alias(UnitType type) _const_; bool unit_type_may_template(UnitType type) _const_; int unit_symlink_name_compatible(const char *symlink, const char *target, bool instance_propagation); -int unit_validate_alias_symlink_and_warn(const char *filename, const char *target); +int unit_validate_alias_symlink_or_warn(int log_level, const char *filename, const char *target); bool lookup_paths_timestamp_hash_same(const LookupPaths *lp, uint64_t timestamp_hash, uint64_t *ret_new); diff --git a/src/shared/install.c b/src/shared/install.c index 80863b448b..6da9ba6b0c 100644 --- a/src/shared/install.c +++ b/src/shared/install.c @@ -394,6 +394,14 @@ void unit_file_dump_changes(int r, const char *verb, const UnitFileChange *chang err = log_error_errno(changes[i].type_or_errno, "Failed to %s unit, refusing to operate on linked unit file %s.", verb, changes[i].path); break; + case -EXDEV: + if (changes[i].source) + err = log_error_errno(changes[i].type_or_errno, "Failed to %s unit, cannot alias %s as %s.", + verb, changes[i].source, changes[i].path); + else + err = log_error_errno(changes[i].type_or_errno, "Failed to %s unit, invalid unit reference \"%s\".", + verb, changes[i].path); + break; case -ENOENT: err = log_error_errno(changes[i].type_or_errno, "Failed to %s unit, unit %s does not exist.", verb, changes[i].path); @@ -1678,7 +1686,13 @@ static int install_info_discover_and_check( return install_info_may_process(ret ? *ret : NULL, lp, changes, n_changes); } -int unit_file_verify_alias(const UnitFileInstallInfo *info, const char *dst, char **ret_dst) { +int unit_file_verify_alias( + const UnitFileInstallInfo *info, + const char *dst, + char **ret_dst, + UnitFileChange **changes, + size_t *n_changes) { + _cleanup_free_ char *dst_updated = NULL; int r; @@ -1705,15 +1719,19 @@ int unit_file_verify_alias(const UnitFileInstallInfo *info, const char *dst, cha p = endswith(dir, ".wants"); if (!p) p = endswith(dir, ".requires"); - if (!p) - return log_warning_errno(SYNTHETIC_ERRNO(EXDEV), - "Invalid path \"%s\" in alias.", dir); + if (!p) { + unit_file_changes_add(changes, n_changes, -EXDEV, dst, NULL); + return log_debug_errno(SYNTHETIC_ERRNO(EXDEV), "Invalid path \"%s\" in alias.", dir); + } + *p = '\0'; /* dir should now be a unit name */ UnitNameFlags type = unit_name_classify(dir); - if (type < 0) - return log_warning_errno(SYNTHETIC_ERRNO(EXDEV), - "Invalid unit name component \"%s\" in alias.", dir); + if (type < 0) { + unit_file_changes_add(changes, n_changes, -EXDEV, dst, NULL); + return log_debug_errno(SYNTHETIC_ERRNO(EXDEV), + "Invalid unit name component \"%s\" in alias.", dir); + } const bool instance_propagation = type == UNIT_NAME_TEMPLATE; @@ -1721,10 +1739,12 @@ int unit_file_verify_alias(const UnitFileInstallInfo *info, const char *dst, cha r = unit_symlink_name_compatible(path_alias, info->name, instance_propagation); if (r < 0) return log_error_errno(r, "Failed to verify alias validity: %m"); - if (r == 0) - return log_warning_errno(SYNTHETIC_ERRNO(EXDEV), - "Invalid unit \"%s\" symlink \"%s\".", - info->name, dst); + if (r == 0) { + unit_file_changes_add(changes, n_changes, -EXDEV, dst, info->name); + return log_debug_errno(SYNTHETIC_ERRNO(EXDEV), + "Invalid unit \"%s\" symlink \"%s\".", + info->name, dst); + } } else { /* If the symlink target has an instance set and the symlink source doesn't, we "propagate @@ -1733,8 +1753,10 @@ int unit_file_verify_alias(const UnitFileInstallInfo *info, const char *dst, cha _cleanup_free_ char *inst = NULL; UnitNameFlags type = unit_name_to_instance(info->name, &inst); - if (type < 0) - return log_error_errno(type, "Failed to extract instance name from \"%s\": %m", info->name); + if (type < 0) { + unit_file_changes_add(changes, n_changes, -EUCLEAN, info->name, NULL); + return log_debug_errno(type, "Failed to extract instance name from \"%s\": %m", info->name); + } if (type == UNIT_NAME_INSTANCE) { r = unit_name_replace_instance(dst, inst, &dst_updated); @@ -1744,9 +1766,14 @@ int unit_file_verify_alias(const UnitFileInstallInfo *info, const char *dst, cha } } - r = unit_validate_alias_symlink_and_warn(dst_updated ?: dst, info->name); - if (r < 0) + r = unit_validate_alias_symlink_or_warn(LOG_DEBUG, dst_updated ?: dst, info->name); + if (r < 0) { + unit_file_changes_add(changes, n_changes, + r == -EINVAL ? -EXDEV : r, + dst_updated ?: dst, + info->name); return r; + } } *ret_dst = TAKE_PTR(dst_updated); @@ -1778,7 +1805,7 @@ static int install_info_symlink_alias( return q; } - q = unit_file_verify_alias(info, dst, &dst_updated); + q = unit_file_verify_alias(info, dst, &dst_updated, changes, n_changes); if (q < 0) continue; @@ -3332,7 +3359,7 @@ int unit_file_preset_all( r = preset_prepare_one(scope, &plus, &minus, &lp, de->d_name, &presets, changes, n_changes); if (r < 0 && - !IN_SET(r, -EEXIST, -ERFKILL, -EADDRNOTAVAIL, -EBADSLT, -EIDRM, -EUCLEAN, -ELOOP, -ENOENT, -EUNATCH)) + !IN_SET(r, -EEXIST, -ERFKILL, -EADDRNOTAVAIL, -EBADSLT, -EIDRM, -EUCLEAN, -ELOOP, -ENOENT, -EUNATCH, -EXDEV)) /* Ignore generated/transient/missing/invalid units when applying preset, propagate other errors. * Coordinate with unit_file_dump_changes() above. */ return r; diff --git a/src/shared/install.h b/src/shared/install.h index cdc5435035..d21e2aaa45 100644 --- a/src/shared/install.h +++ b/src/shared/install.h @@ -193,7 +193,12 @@ int unit_file_changes_add(UnitFileChange **changes, size_t *n_changes, int type, void unit_file_changes_free(UnitFileChange *changes, size_t n_changes); void unit_file_dump_changes(int r, const char *verb, const UnitFileChange *changes, size_t n_changes, bool quiet); -int unit_file_verify_alias(const UnitFileInstallInfo *i, const char *dst, char **ret_dst); +int unit_file_verify_alias( + const UnitFileInstallInfo *info, + const char *dst, + char **ret_dst, + UnitFileChange **changes, + size_t *n_changes); typedef struct UnitFilePresetRule UnitFilePresetRule; diff --git a/src/test/test-install-root.c b/src/test/test-install-root.c index f718689c3a..4f66c12655 100644 --- a/src/test/test-install-root.c +++ b/src/test/test-install-root.c @@ -1091,7 +1091,7 @@ static void verify_one( if (i != last_info) log_info("-- %s --", (last_info = i)->name); - r = unit_file_verify_alias(i, alias, &alias2); + r = unit_file_verify_alias(i, alias, &alias2, NULL, NULL); log_info_errno(r, "alias %s ā† %s: %d/%m (expected %d)%s%s%s", i->name, alias, r, expected, alias2 ? " [" : "", strempty(alias2), diff --git a/src/test/test-unit-file.c b/src/test/test-unit-file.c index cc08a4ae4b..8ed56ad3b8 100644 --- a/src/test/test-unit-file.c +++ b/src/test/test-unit-file.c @@ -8,20 +8,20 @@ #include "unit-file.h" TEST(unit_validate_alias_symlink_and_warn) { - assert_se(unit_validate_alias_symlink_and_warn("/path/a.service", "/other/b.service") == 0); - assert_se(unit_validate_alias_symlink_and_warn("/path/a.service", "/other/b.socket") == -EXDEV); - assert_se(unit_validate_alias_symlink_and_warn("/path/a.service", "/other/b.foobar") == -EXDEV); - assert_se(unit_validate_alias_symlink_and_warn("/path/a@.service", "/other/b@.service") == 0); - assert_se(unit_validate_alias_symlink_and_warn("/path/a@.service", "/other/b@.socket") == -EXDEV); - assert_se(unit_validate_alias_symlink_and_warn("/path/a@XXX.service", "/other/b@YYY.service") == -EXDEV); - assert_se(unit_validate_alias_symlink_and_warn("/path/a@XXX.service", "/other/b@YYY.socket") == -EXDEV); - assert_se(unit_validate_alias_symlink_and_warn("/path/a@.service", "/other/b@YYY.service") == -EXDEV); - assert_se(unit_validate_alias_symlink_and_warn("/path/a@XXX.service", "/other/b@XXX.service") == 0); - assert_se(unit_validate_alias_symlink_and_warn("/path/a@XXX.service", "/other/b@.service") == 0); - assert_se(unit_validate_alias_symlink_and_warn("/path/a@.service", "/other/b.service") == -EXDEV); - assert_se(unit_validate_alias_symlink_and_warn("/path/a.service", "/other/b@.service") == -EXDEV); - assert_se(unit_validate_alias_symlink_and_warn("/path/a@.slice", "/other/b.slice") == -EINVAL); - assert_se(unit_validate_alias_symlink_and_warn("/path/a.slice", "/other/b.slice") == -EINVAL); + assert_se(unit_validate_alias_symlink_or_warn(LOG_INFO, "/path/a.service", "/other/b.service") == 0); + assert_se(unit_validate_alias_symlink_or_warn(LOG_INFO, "/path/a.service", "/other/b.socket") == -EXDEV); + assert_se(unit_validate_alias_symlink_or_warn(LOG_INFO, "/path/a.service", "/other/b.foobar") == -EXDEV); + assert_se(unit_validate_alias_symlink_or_warn(LOG_INFO, "/path/a@.service", "/other/b@.service") == 0); + assert_se(unit_validate_alias_symlink_or_warn(LOG_INFO, "/path/a@.service", "/other/b@.socket") == -EXDEV); + assert_se(unit_validate_alias_symlink_or_warn(LOG_INFO, "/path/a@XXX.service", "/other/b@YYY.service") == -EXDEV); + assert_se(unit_validate_alias_symlink_or_warn(LOG_INFO, "/path/a@XXX.service", "/other/b@YYY.socket") == -EXDEV); + assert_se(unit_validate_alias_symlink_or_warn(LOG_INFO, "/path/a@.service", "/other/b@YYY.service") == -EXDEV); + assert_se(unit_validate_alias_symlink_or_warn(LOG_INFO, "/path/a@XXX.service", "/other/b@XXX.service") == 0); + assert_se(unit_validate_alias_symlink_or_warn(LOG_INFO, "/path/a@XXX.service", "/other/b@.service") == 0); + assert_se(unit_validate_alias_symlink_or_warn(LOG_INFO, "/path/a@.service", "/other/b.service") == -EXDEV); + assert_se(unit_validate_alias_symlink_or_warn(LOG_INFO, "/path/a.service", "/other/b@.service") == -EXDEV); + assert_se(unit_validate_alias_symlink_or_warn(LOG_INFO, "/path/a@.slice", "/other/b.slice") == -EINVAL); + assert_se(unit_validate_alias_symlink_or_warn(LOG_INFO, "/path/a.slice", "/other/b.slice") == -EINVAL); } TEST(unit_file_build_name_map) {