Merge pull request #19453 from keszybz/consider-instance-aliases

Consider instance aliases

src/basic/unit-file.c

@@ -284,7 +284,7 @@ int unit_file_build_name_map(
                         continue;
                 }
 
-                FOREACH_DIRENT_ALL(de, d, log_warning_errno(errno, "Failed to read \"%s\", ignoring: %m", *dir)) {
+                FOREACH_DIRENT(de, d, log_warning_errno(errno, "Failed to read \"%s\", ignoring: %m", *dir)) {
                         char *filename;
                         _cleanup_free_ char *_filename_free = NULL, *simplified = NULL;
                         const char *suffix, *dst = NULL;
@@ -403,6 +403,7 @@ int unit_file_build_name_map(
         /* Let's also put the names in the reverse db. */
         const char *dummy, *src;
         HASHMAP_FOREACH_KEY(dummy, src, ids) {
+                _cleanup_free_ char *inst = NULL, *dst_inst = NULL;
                 const char *dst;
 
                 r = unit_ids_map_get(ids, src, &dst);
@@ -412,15 +413,31 @@ int unit_file_build_name_map(
                 if (null_or_empty_path(dst) != 0)
                         continue;
 
-                /* Do not treat instance symlinks that point to the template as aliases */
+                dst = basename(dst);
-                if (unit_name_is_valid(basename(dst), UNIT_NAME_TEMPLATE) &&
-                    unit_name_is_valid(src, UNIT_NAME_INSTANCE))
-                        continue;
 
-                r = string_strv_hashmap_put(&names, basename(dst), src);
+                /* If we have an symlink from an instance name to a template name, it is an alias just for
+                 * this specific instance, foo@id.service ↔ template@id.service. */
+                if (unit_name_is_valid(dst, UNIT_NAME_TEMPLATE)) {
+                        UnitNameFlags t = unit_name_to_instance(src, &inst);
+                        if (t < 0)
+                                return log_error_errno(t, "Failed to extract instance part from %s: %m", src);
+                        if (t == UNIT_NAME_INSTANCE) {
+                                r = unit_name_replace_instance(dst, inst, &dst_inst);
+                                if (r < 0) {
+                                        /* This might happen e.g. if the combined length is too large.
+                                         * Let's not make too much of a fuss. */
+                                        log_debug_errno(r, "Failed to build alias name (%s + %s), ignoring: %m",
+                                                        dst, inst);
+                                        continue;
+                                }
+
+                                dst = dst_inst;
+                        }
+                }
+
+                r = string_strv_hashmap_put(&names, dst, src);
                 if (r < 0)
-                        return log_warning_errno(r, "Failed to add entry to hashmap (%s→%s): %m",
+                        return log_warning_errno(r, "Failed to add entry to hashmap (%s→%s): %m", dst, src);
-                                                 basename(dst), src);
         }
 
         if (cache_timestamp_hash)
@@ -434,6 +451,88 @@ int unit_file_build_name_map(
         return 1;
 }
 
+static int add_name(
+                const char *unit_name,
+                Set **names,
+                const char *name) {
+        int r;
+
+        assert(names);
+        assert(name);
+
+        r = set_put_strdup(names, name);
+        if (r < 0)
+                return r;
+        if (r > 0 && !streq(unit_name, name))
+                log_debug("Unit %s has alias %s.", unit_name, name);
+        return r;
+}
+
+static int add_names(
+                Hashmap *unit_ids_map,
+                Hashmap *unit_name_map,
+                const char *unit_name,
+                const char *fragment_basename,  /* Only set when adding additional names based on fragment path */
+                UnitNameFlags name_type,
+                const char *instance,
+                Set **names,
+                const char *name) {
+
+        char **aliases, **alias;
+        int r;
+
+        assert(name_type == UNIT_NAME_PLAIN || instance);
+
+        /* The unit has its own name if it's not a template. If we're looking at a fragment, the fragment
+         * name (possibly with instance inserted), is also always one of the unit names. */
+        if (name_type != UNIT_NAME_TEMPLATE) {
+                r = add_name(unit_name, names, name);
+                if (r < 0)
+                        return r;
+        }
+
+        /* Add any aliases of the name to the set of names.
+         *
+         * We don't even need to know which fragment we will use. The unit_name_map should return the same
+         * set of names for any of the aliases. */
+        aliases = hashmap_get(unit_name_map, name);
+        STRV_FOREACH(alias, aliases) {
+                if (name_type == UNIT_NAME_INSTANCE && unit_name_is_valid(*alias, UNIT_NAME_TEMPLATE)) {
+                        _cleanup_free_ char *inst = NULL;
+                        const char *inst_fragment = NULL;
+
+                        r = unit_name_replace_instance(*alias, instance, &inst);
+                        if (r < 0)
+                                return log_debug_errno(r, "Cannot build instance name %s + %s: %m",
+                                                       *alias, instance);
+
+                        /* Exclude any aliases that point in some other direction.
+                         *
+                         * See https://github.com/systemd/systemd/pull/13119#discussion_r308145418. */
+                        r = unit_ids_map_get(unit_ids_map, inst, &inst_fragment);
+                        if (r < 0 && !IN_SET(r, -ENOENT, -ENXIO))
+                                return log_debug_errno(r, "Cannot find instance fragment %s: %m", inst);
+
+                        if (inst_fragment &&
+                            !streq(basename(inst_fragment), fragment_basename)) {
+                                log_debug("Instance %s has fragment %s and is not an alias of %s.",
+                                          inst, inst_fragment, unit_name);
+                                continue;
+                        }
+
+                        r = set_consume(*names, TAKE_PTR(inst));
+                        if (r > 0)
+                                log_debug("Unit %s has alias %s.", unit_name, inst);
+                } else
+                        r = add_name(unit_name, names, *alias);
+
+                if (r < 0)
+                        return r;
+        }
+
+        return 0;
+}
+
 int unit_file_find_fragment(
                 Hashmap *unit_ids_map,
                 Hashmap *unit_name_map,
@@ -443,9 +542,8 @@ int unit_file_find_fragment(
 
         const char *fragment = NULL;
         _cleanup_free_ char *template = NULL, *instance = NULL;
-        _cleanup_set_free_free_ Set *names = NULL;
+        _cleanup_set_free_ Set *names = NULL;
-        char **t, **nnn;
+        int r;
-        int r, name_type;
 
         /* Finds a fragment path, and returns the set of names:
          * if we have …/foo.service and …/foo-alias.service→foo.service,
@@ -461,53 +559,19 @@ int unit_file_find_fragment(
          * foo-alias@inst.service → …/foo@inst.service, {foo@inst.service, foo-alias@inst.service}.
          */
 
-        name_type = unit_name_to_instance(unit_name, &instance);
+        UnitNameFlags name_type = unit_name_to_instance(unit_name, &instance);
         if (name_type < 0)
                 return name_type;
 
-        names = set_new(&string_hash_ops);
+        r = add_names(unit_ids_map, unit_name_map, unit_name, NULL, name_type, instance, &names, unit_name);
-        if (!names)
+        if (r < 0)
-                return -ENOMEM;
+                return r;
-
-        /* The unit always has its own name if it's not a template. */
-        if (IN_SET(name_type, UNIT_NAME_PLAIN, UNIT_NAME_INSTANCE)) {
-                r = set_put_strdup(&names, unit_name);
-                if (r < 0)
-                        return r;
-        }
 
         /* First try to load fragment under the original name */
         r = unit_ids_map_get(unit_ids_map, unit_name, &fragment);
         if (r < 0 && !IN_SET(r, -ENOENT, -ENXIO))
                 return log_debug_errno(r, "Cannot load unit %s: %m", unit_name);
 
-        if (fragment) {
-                /* Add any aliases of the original name to the set of names */
-                nnn = hashmap_get(unit_name_map, basename(fragment));
-                STRV_FOREACH(t, nnn) {
-                        if (name_type == UNIT_NAME_INSTANCE && unit_name_is_valid(*t, UNIT_NAME_TEMPLATE)) {
-                                char *inst;
-
-                                r = unit_name_replace_instance(*t, instance, &inst);
-                                if (r < 0)
-                                        return log_debug_errno(r, "Cannot build instance name %s+%s: %m", *t, instance);
-
-                                if (!streq(unit_name, inst))
-                                        log_debug("%s: %s has alias %s", __func__, unit_name, inst);
-
-                                log_info("%s: %s+%s → %s", __func__, *t, instance, inst);
-                                r = set_consume(names, inst);
-                        } else {
-                                if (!streq(unit_name, *t))
-                                        log_debug("%s: %s has alias %s", __func__, unit_name, *t);
-
-                                r = set_put_strdup(&names, *t);
-                        }
-                        if (r < 0)
-                                return r;
-                }
-        }
-
         if (!fragment && name_type == UNIT_NAME_INSTANCE) {
                 /* Look for a fragment under the template name */
 
@@ -518,43 +582,22 @@ int unit_file_find_fragment(
                 r = unit_ids_map_get(unit_ids_map, template, &fragment);
                 if (r < 0 && !IN_SET(r, -ENOENT, -ENXIO))
                         return log_debug_errno(r, "Cannot load template %s: %m", template);
+        }
 
-                if (fragment) {
+        if (fragment) {
-                        /* Add any aliases of the original name to the set of names */
+                const char *fragment_basename = basename(fragment);
-                        nnn = hashmap_get(unit_name_map, basename(fragment));
-                        STRV_FOREACH(t, nnn) {
-                                _cleanup_free_ char *inst = NULL;
-                                const char *inst_fragment = NULL;
 
-                                r = unit_name_replace_instance(*t, instance, &inst);
+                if (!streq(fragment_basename, unit_name)) {
-                                if (r < 0)
+                        /* Add names based on the fragment name to the set of names */
-                                        return log_debug_errno(r, "Cannot build instance name %s+%s: %m", template, instance);
+                        r = add_names(unit_ids_map, unit_name_map, unit_name, fragment_basename, name_type, instance, &names, fragment_basename);
-
+                        if (r < 0)
-                                /* Exclude any aliases that point in some other direction. */
+                                return r;
-                                r = unit_ids_map_get(unit_ids_map, inst, &inst_fragment);
-                                if (r < 0 && !IN_SET(r, -ENOENT, -ENXIO))
-                                        return log_debug_errno(r, "Cannot find instance fragment %s: %m", inst);
-
-                                if (inst_fragment &&
-                                    !streq(basename(inst_fragment), basename(fragment))) {
-                                        log_debug("Instance %s has fragment %s and is not an alias of %s.",
-                                                  inst, inst_fragment, unit_name);
-                                        continue;
-                                }
-
-                                if (!streq(unit_name, inst))
-                                        log_debug("%s: %s has alias %s", __func__, unit_name, inst);
-                                r = set_consume(names, TAKE_PTR(inst));
-                                if (r < 0)
-                                        return r;
-                        }
                 }
         }
 
         *ret_fragment_path = fragment;
         *ret_names = TAKE_PTR(names);
 
-        // FIXME: if instance, consider any unit names with different template name
         return 0;
 }
 

src/core/manager.c

@@ -688,7 +688,8 @@ static int manager_setup_prefix(Manager *m) {
         for (ExecDirectoryType i = 0; i < _EXEC_DIRECTORY_TYPE_MAX; i++) {
                 r = sd_path_lookup(p[i].type, p[i].suffix, &m->prefix[i]);
                 if (r < 0)
-                        return r;
+                        return log_warning_errno(r, "Failed to lookup %s path: %m",
+                                                 exec_directory_type_to_string(i));
         }
 
         return 0;

src/test/test-unit-file.c

@@ -92,6 +92,7 @@ static void test_runlevel_to_target(void) {
 }
 
 int main(int argc, char **argv) {
+        log_show_color(true);
         test_setup_logging(LOG_DEBUG);
 
         test_unit_validate_alias_symlink_and_warn();

test/test-functions

@@ -1134,8 +1134,7 @@ check_result_common() {
     local ret
 
     if [ -s "$workspace/failed" ]; then
-        # …/failed only counts if non-empty
+        # Non-empty …/failed has highest priority
-        ls -l "$workspace/failed"
         cp -a "$workspace/failed" "${TESTDIR:?}/"
         ret=1
     elif [ -e "$workspace/testok" ]; then
@@ -1147,24 +1146,28 @@ check_result_common() {
         cat "$workspace/skipped"
         ret=0
     elif [ -n "$TIMED_OUT" ]; then
-        echo "${TESTNAME:?} timed out!"
+        echo "(timeout)" >"${TESTDIR:?}/failed"
         ret=2
     else
-        echo "${TESTNAME:?} did not report a result!"
+        echo "(failed; see logs)" >"${TESTDIR:?}/failed"
+        ret=3
     fi
 
     save_journal "$workspace/var/log/journal"
 
-    check_asan_reports "$workspace" || ret=3
+    check_asan_reports "$workspace" || ret=4
 
     if [ -d "${ARTIFACT_DIRECTORY}" ] && [ -f "$workspace/strace.out" ]; then
         cp "$workspace/strace.out" "${ARTIFACT_DIRECTORY}/"
     fi
 
-    [ -f "$TESTDIR/failed" ] && cat "$TESTDIR/failed"
+    if [ ${ret:?} != 0 ] && [ -f "$TESTDIR/failed" ]; then
+        echo -n "${TESTNAME:?}: "
+        cat "$TESTDIR/failed"
+    fi
     echo "${JOURNAL_LIST:-"No journals were saved"}"
 
-    return $ret
+    return ${ret:?}
 }
 
 check_result_nspawn() {

test/units/testsuite-15.sh

@@ -32,12 +32,9 @@ create_service () {
 Description=$SERVICE_NAME unit
 
 [Service]
-ExecStart=/bin/sleep 100000
+ExecStart=sleep 100000
 EOF
-    mkdir -p /{etc,run,usr/lib}/systemd/system/"$SERVICE_NAME".service.d
+    mkdir -p /{etc,run,usr/lib}/systemd/system/"$SERVICE_NAME".service.{d,wants,requires}
-    mkdir -p /etc/systemd/system/"$SERVICE_NAME".service.{wants,requires}
-    mkdir -p /run/systemd/system/"$SERVICE_NAME".service.{wants,requires}
-    mkdir -p /usr/lib/systemd/system/"$SERVICE_NAME".service.{wants,requires}
 }
 
 create_services () {
@@ -47,12 +44,10 @@ create_services () {
 }
 
 check_ok () {
-    [ $# -eq 3 ] || return
+    x="$(systemctl show --value -p "${2:?}" "${1:?}")"
-
-    x="$(systemctl show --value -p "$2" "$1")"
     case "$x" in
-        *$3*) return 0 ;;
+        *${3:?}*) return 0 ;;
-        *)    return 1 ;;
+        *)        return 1 ;;
     esac
 }
 
@@ -123,7 +118,7 @@ EOF
     check_ok test15-b ExecCondition "/bin/echo test15-b"
     rm -rf /usr/lib/systemd/system/service.d
 
-    clear_services test15-a test15-b test15-c
+    clear_services test15-a test15-b test15-c test15-c1
 }
 
 test_linked_units () {
@@ -153,6 +148,32 @@ test_linked_units () {
     clear_services test15-a test15-b
 }
 
+test_template_alias() {
+    echo "Testing instance alias..."
+    echo "*** forward"
+
+    create_service test15-a@
+    ln -s test15-a@inst.service /etc/systemd/system/test15-b@inst.service  # alias
+
+    check_ok test15-a@inst Names test15-a@inst.service
+    check_ok test15-a@inst Names test15-b@inst.service
+
+    check_ok test15-a@other Names test15-a@other.service
+    check_ko test15-a@other Names test15-b@other.service
+
+    echo "*** reverse"
+
+    systemctl daemon-reload
+
+    check_ok test15-b@inst Names test15-a@inst.service
+    check_ok test15-b@inst Names test15-b@inst.service
+
+    check_ko test15-b@other Names test15-a@other.service
+    check_ok test15-b@other Names test15-b@other.service
+
+    clear_services test15-a@ test15-b@
+}
+
 test_hierarchical_dropins () {
     echo "Testing hierarchical dropins..."
     echo "*** test service.d/ top level drop-in"
@@ -495,6 +516,7 @@ test_invalid_dropins () {
 
 test_basic_dropins
 test_linked_units
+test_template_alias
 test_hierarchical_dropins
 test_template_dropins
 test_alias_dropins