From 35bcb96c1ac2db777db1649026f931ce77c9c7ba Mon Sep 17 00:00:00 2001

From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= <zbyszek@in.waw.pl>

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) {