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

3 files changed, 41 insertions, 25 deletions

diff --git a/src/shared/install.c b/src/shared/install.c
index e2fcf6cf62..72be477ee9 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",
@@ -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 df52f8cf81..e994884196 100644
--- 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 5ece520e9b..bb1c651c2a 100644
--- 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);