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" <<EOF
+# 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" <<EOF
+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; }