From b3b45ed9385341e72edfc1bae08819026d841d46 Mon Sep 17 00:00:00 2001

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

Date: Tue, 8 Mar 2022 12:08:00 +0100

Subject: [PATCH] shared/specifier: provide proper error messages when

 specifiers fail to read files

ENOENT is easily confused with the file that we're working on not being

present, e.g. when the file contains %o or something else that requires

os-release to be present. Let's use -EUNATCH instead to reduce that chances of

confusion if the context of the error is lost.

And once we have pinpointed the reason, let's provide a proper error message:

+ build/systemctl --root=/tmp/systemctl-test.TO7Mcb enable some-some-link6@.socket

/tmp/systemctl-test.TO7Mcb/etc/systemd/system/some-some-link6@.socket: Failed to resolve alias "target@A:%A.socket": Protocol driver not attached

Failed to enable unit, cannot resolve specifiers in "target@A:%A.socket".

(cherry picked from commit 6ec4c852c910b1aca649e87ba3143841334f01fa)

Related: #2082131

---

 src/shared/install.c          | 31 +++++++++++++-------

 src/shared/specifier.c        | 27 ++++++++++++-----

 src/test/test-specifier.c     |  2 +-

 test/test-systemctl-enable.sh | 55 ++++++++++++++++++++++++++---------

 4 files changed, 82 insertions(+), 33 deletions(-)

diff --git a/src/shared/install.c b/src/shared/install.c

index cbfe96b1e8..ea5bc36482 100644

--- a/src/shared/install.c

+++ b/src/shared/install.c

@@ -374,6 +374,7 @@ void unit_file_dump_changes(int r, const char *verb, const UnitFileChange *chang

                                         verb, changes[i].path);

                         logged = true;

                         break;

+

                 case -EADDRNOTAVAIL:

                         log_error_errno(changes[i].type_or_errno, "Failed to %s unit, unit %s is transient or generated.",

                                         verb, changes[i].path);

@@ -401,6 +402,12 @@ void unit_file_dump_changes(int r, const char *verb, const UnitFileChange *chang

                         logged = true;

                         break;

+                case -EUNATCH:

+                        log_error_errno(changes[i].type_or_errno, "Failed to %s unit, cannot resolve specifiers in \"%s\".",

+                                        verb, changes[i].path);

+                        logged = true;

+                        break;

+

                 default:

                         assert(changes[i].type_or_errno < 0);

                         log_error_errno(changes[i].type_or_errno, "Failed to %s unit, file \"%s\": %m",

@@ -1154,7 +1161,8 @@ static int config_parse_also(

                 r = install_name_printf(info, word, info->root, &printed);

                 if (r < 0)

-                        return r;

+                        return log_syntax(unit, LOG_WARNING, filename, line, r,

+                                          "Failed to resolve unit name in Also=\"%s\": %m", word);

                 r = install_info_add(c, printed, NULL, info->root, /* auxiliary= */ true, NULL);

                 if (r < 0)

@@ -1201,14 +1209,13 @@ static int config_parse_default_instance(

         r = install_name_printf(i, rvalue, i->root, &printed);

         if (r < 0)

-                return r;

+                return log_syntax(unit, LOG_WARNING, filename, line, r,

+                                  "Failed to resolve instance name in DefaultInstance=\"%s\": %m", rvalue);

-        if (isempty(printed)) {

-                i->default_instance = mfree(i->default_instance);

-                return 0;

-        }

+        if (isempty(printed))

+                printed = mfree(printed);

-        if (!unit_instance_is_valid(printed))

+        if (printed && !unit_instance_is_valid(printed))

                 return log_syntax(unit, LOG_WARNING, filename, line, SYNTHETIC_ERRNO(EINVAL),

                                   "Invalid DefaultInstance= value \"%s\".", printed);

@@ -1776,8 +1783,10 @@ static int install_info_symlink_alias(

                 _cleanup_free_ char *alias_path = NULL, *dst = NULL, *dst_updated = NULL;

                 q = install_name_printf(i, *s, i->root, &dst);

-                if (q < 0)

+                if (q < 0) {

+                        unit_file_changes_add(changes, n_changes, q, *s, NULL);

                         return q;

+                }

                 q = unit_file_verify_alias(i, dst, &dst_updated);

                 if (q < 0)

@@ -1861,8 +1870,10 @@ static int install_info_symlink_wants(

                 _cleanup_free_ char *path = NULL, *dst = NULL;

                 q = install_name_printf(i, *s, i->root, &dst);

-                if (q < 0)

+                if (q < 0) {

+                        unit_file_changes_add(changes, n_changes, q, *s, NULL);

                         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

@@ -3332,7 +3343,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, -EIDRM, -EUCLEAN, -ELOOP, -ENOENT))

+                            !IN_SET(r, -EEXIST, -ERFKILL, -EADDRNOTAVAIL, -EIDRM, -EUCLEAN, -ELOOP, -ENOENT, -EUNATCH))

                                 /* 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/specifier.c b/src/shared/specifier.c

index c26628975c..a917378427 100644

--- a/src/shared/specifier.c

+++ b/src/shared/specifier.c

@@ -131,7 +131,8 @@ int specifier_machine_id(char specifier, const void *data, const char *root, con

                 fd = chase_symlinks_and_open("/etc/machine-id", root, CHASE_PREFIX_ROOT, O_RDONLY|O_CLOEXEC|O_NOCTTY, NULL);

                 if (fd < 0)

-                        return fd;

+                        /* Translate error for missing os-release file to EUNATCH. */

+                        return fd == -ENOENT ? -EUNATCH : fd;

                 r = id128_read_fd(fd, ID128_PLAIN, &id;;

         } else

@@ -214,31 +215,41 @@ int specifier_architecture(char specifier, const void *data, const char *root, c

 /* Note: fields in /etc/os-release might quite possibly be missing, even if everything is entirely valid

  * otherwise. We'll return an empty value or NULL in that case from the functions below. But if the

- * os-release file is missing, we'll return -ENOENT. This means that something is seriously wrong with the

+ * os-release file is missing, we'll return -EUNATCH. This means that something is seriously wrong with the

  * installation. */

+static int parse_os_release_specifier(const char *root, const char *id, char **ret) {

+        int r;

+

+        assert(ret);

+

+        /* Translate error for missing os-release file to EUNATCH. */

+        r = parse_os_release(root, id, ret);

+        return r == -ENOENT ? -EUNATCH : r;

+}

+

 int specifier_os_id(char specifier, const void *data, const char *root, const void *userdata, char **ret) {

-        return parse_os_release(root, "ID", ret);

+        return parse_os_release_specifier(root, "ID", ret);

 }

 int specifier_os_version_id(char specifier, const void *data, const char *root, const void *userdata, char **ret) {

-        return parse_os_release(root, "VERSION_ID", ret);

+        return parse_os_release_specifier(root, "VERSION_ID", ret);

 }

 int specifier_os_build_id(char specifier, const void *data, const char *root, const void *userdata, char **ret) {

-        return parse_os_release(root, "BUILD_ID", ret);

+        return parse_os_release_specifier(root, "BUILD_ID", ret);

 }

 int specifier_os_variant_id(char specifier, const void *data, const char *root, const void *userdata, char **ret) {

-        return parse_os_release(root, "VARIANT_ID", ret);

+        return parse_os_release_specifier(root, "VARIANT_ID", ret);

 }

 int specifier_os_image_id(char specifier, const void *data, const char *root, const void *userdata, char **ret) {

-        return parse_os_release(root, "IMAGE_ID", ret);

+        return parse_os_release_specifier(root, "IMAGE_ID", ret);

 }

 int specifier_os_image_version(char specifier, const void *data, const char *root, const void *userdata, char **ret) {

-        return parse_os_release(root, "IMAGE_VERSION", ret);

+        return parse_os_release_specifier(root, "IMAGE_VERSION", ret);

 }

 int specifier_group_name(char specifier, const void *data, const char *root, const void *userdata, char **ret) {

diff --git a/src/test/test-specifier.c b/src/test/test-specifier.c

index 790f0252d7..a45d1bd0b9 100644

--- a/src/test/test-specifier.c

+++ b/src/test/test-specifier.c

@@ -104,7 +104,7 @@ TEST(specifiers_missing_data_ok) {

         assert_se(streq(resolved, "-----"));

         assert_se(setenv("SYSTEMD_OS_RELEASE", "/nosuchfileordirectory", 1) == 0);

-        assert_se(specifier_printf("%A-%B-%M-%o-%w-%W", SIZE_MAX, specifier_table, NULL, NULL, &resolved) == -ENOENT);

+        assert_se(specifier_printf("%A-%B-%M-%o-%w-%W", SIZE_MAX, specifier_table, NULL, NULL, &resolved) == -EUNATCH);

         assert_se(streq(resolved, "-----"));

         assert_se(unsetenv("SYSTEMD_OS_RELEASE") == 0);

diff --git a/test/test-systemctl-enable.sh b/test/test-systemctl-enable.sh

index 769341129c..43a2c0a0fb 100644

--- a/test/test-systemctl-enable.sh

+++ b/test/test-systemctl-enable.sh

@@ -445,12 +445,45 @@ EOF

 check_alias a "$(uname -m | tr '_' '-')"

-# FIXME: when os-release is not found, we fail we a cryptic error

-# Alias=target@%A.socket

+test ! -e "$root/etc/os-release"

+test ! -e "$root/usr/lib/os-release"

+

+check_alias A '' && { echo "Expected failure" >&2; exit 1; }

+check_alias B '' && { echo "Expected failure" >&2; exit 1; }

+check_alias M '' && { echo "Expected failure" >&2; exit 1; }

+check_alias o '' && { echo "Expected failure" >&2; exit 1; }

+check_alias w '' && { echo "Expected failure" >&2; exit 1; }

+check_alias W '' && { echo "Expected failure" >&2; exit 1; }

+

+cat >"$root/etc/os-release" <

+# empty

+EOF

-check_alias b "$(systemd-id128 boot-id)"

+check_alias A ''

+check_alias B ''

+check_alias M ''

+check_alias o ''

+check_alias w ''

+check_alias W ''

+

+cat >"$root/etc/os-release" <

+ID='the-id'

+VERSION_ID=39a

+BUILD_ID=build-id

+VARIANT_ID=wrong

+VARIANT_ID=right

+IMAGE_ID="foobar"

+IMAGE_VERSION='1-2-3'

+EOF

-# Alias=target@%B.socket

+check_alias A '1-2-3'

+check_alias B 'build-id'

+check_alias M 'foobar'

+check_alias o 'the-id'

+check_alias w '39a'

+check_alias W 'right'

+

+check_alias b "$(systemd-id128 boot-id)"

 # FIXME: Failed to enable: Invalid slot.

 # Alias=target@%C.socket

@@ -479,18 +512,15 @@ check_alias l "$(uname -n | sed 's/\..*//')"

 # FIXME: Failed to enable: Invalid slot.

 # Alias=target@%L.socket

-# FIXME: Failed to enable: No such file or directory.

-# Alias=target@%m.socket

+test ! -e "$root/etc/machine-id"

+check_alias m '' && { echo "Expected failure" >&2; exit 1; }

-# FIXME: Failed to enable: No such file or directory.

-# Alias=target@%M.socket

+systemd-id128 new >"$root/etc/machine-id"

+check_alias m "$(cat "$root/etc/machine-id")"

 check_alias n 'some-some-link6@.socket'

 check_alias N 'some-some-link6@'

-# FIXME: Failed to enable: No such file or directory.

-# Alias=target@%o.socket

-

 check_alias p 'some-some-link6'

 # FIXME: Failed to enable: Invalid slot.

@@ -509,9 +539,6 @@ check_alias v "$(uname -r)"

 # FIXME: Failed to enable: Invalid slot.

 # Alias=target@%V.socket

-# Alias=target@%w.socket

-# Alias=target@%W.socket

-

 check_alias % '%' && { echo "Expected failure because % is not legal in unit name" >&2; exit 1; }

 check_alias z 'z' && { echo "Expected failure because %z is not known" >&2; exit 1; }