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".

diff --git a/src/shared/install.c b/src/shared/install.c
index e2fcf6cf62d26eb6a03bf0a70a24846b905d2969..72be477ee99324a06fb8bdc89b10d774d8343c67 100644 (file)
--- 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",
@@ -1151,7 +1158,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)
@@ -1198,14 +1206,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);
 
@@ -1768,8 +1775,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)
@@ -1852,8 +1861,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
@@ -3307,7 +3318,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 df52f8cf8183b5263b755ed58bb56222dfd91d89..e994884196d2355457f4b8b35f984ba46b02c11f 100644 (file)
--- a/src/shared/specifier.c
+++ b/src/shared/specifier.c
@@ -162,7 +162,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
@@ -270,37 +271,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. */
 
-int specifier_os_id(char specifier, const void *data, const char *root, const void *userdata, char **ret) {
+static int parse_os_release_specifier(const char *root, const char *id, char **ret) {
+        int r;
+
         assert(ret);
-        return parse_os_release(root, "ID", 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_specifier(root, "ID", ret);
 }
 
 int specifier_os_version_id(char specifier, const void *data, const char *root, const void *userdata, char **ret) {
-        assert(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) {
-        assert(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) {
-        assert(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) {
-        assert(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) {
-        assert(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 5ece520e9b4ce48191bc2c8cbab841253327c887..bb1c651c2abd284bab4d27c6bb342f3b1ee83d8c 100644 (file)
--- a/src/test/test-specifier.c
+++ b/src/test/test-specifier.c
@@ -146,7 +146,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 769341129ca15be9130449306ddc18c6987282cc..43a2c0a0fba475c90c15174427b643d494f30a7c 100644 (file)
--- 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; }