From b3b45ed9385341e72edfc1bae08819026d841d46 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= 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" <"$root/etc/os-release" <&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; }