diff --git a/src/core/transaction.c b/src/core/transaction.c index b451c906da..699d60a865 100644 --- a/src/core/transaction.c +++ b/src/core/transaction.c @@ -880,7 +880,7 @@ void transaction_add_propagate_reload_jobs( assert(tr); assert(unit); - UNIT_FOREACH_DEPENDENCY(dep, unit, UNIT_ATOM_PROPAGATES_RELOAD_TO) { + UNIT_FOREACH_DEPENDENCY_SAFE(dep, unit, UNIT_ATOM_PROPAGATES_RELOAD_TO) { _cleanup_(sd_bus_error_free) sd_bus_error e = SD_BUS_ERROR_NULL; nt = job_type_collapse(JOB_TRY_RELOAD, dep); @@ -935,7 +935,7 @@ int transaction_add_job_and_dependencies( sd_bus_error *e) { bool is_new; - Job *ret; + Job *job; int r; assert(tr); @@ -944,8 +944,8 @@ int transaction_add_job_and_dependencies( assert(type < _JOB_TYPE_MAX_IN_TRANSACTION); assert(unit); - /* Before adding jobs for this unit, let's ensure that its state has been loaded This matters when - * jobs are spawned as part of coldplugging itself (see e. g. path_coldplug()). This way, we + /* Before adding jobs for this unit, let's ensure that its state has been loaded. This matters when + * jobs are spawned as part of coldplugging itself (see e. g. path_coldplug()). This way, we * "recursively" coldplug units, ensuring that we do not look at state of not-yet-coldplugged * units. */ if (MANAGER_IS_RELOADING(unit->manager)) @@ -960,7 +960,6 @@ int transaction_add_job_and_dependencies( return sd_bus_error_setf(e, BUS_ERROR_LOAD_FAILED, "Unit %s is not loaded properly.", unit->id); if (type != JOB_STOP) { - r = bus_unit_validate_load_state(unit, e); /* The time-based cache allows new units to be started without daemon-reload, but if they are * already referenced (because of dependencies or ordering) then we have to force a load of * the fragment. As an optimization, check first if anything in the usual paths was modified @@ -971,14 +970,15 @@ int transaction_add_job_and_dependencies( * * Given building up the transaction is a synchronous operation, attempt * to load the unit immediately. */ - if (r < 0 && manager_unit_cache_should_retry_load(unit)) { - sd_bus_error_free(e); + if (manager_unit_cache_should_retry_load(unit)) { + assert(unit->load_state == UNIT_NOT_FOUND); unit->load_state = UNIT_STUB; - r = unit_load(unit); - if (r < 0 || unit->load_state == UNIT_STUB) - unit->load_state = UNIT_NOT_FOUND; - r = bus_unit_validate_load_state(unit, e); + unit->load_error = 0; + (void) unit_load(unit); + assert(unit->load_state != UNIT_STUB); } + + r = bus_unit_validate_load_state(unit, e); if (r < 0) return r; } @@ -999,24 +999,24 @@ int transaction_add_job_and_dependencies( } /* First add the job. */ - ret = transaction_add_one_job(tr, type, unit, &is_new); - if (!ret) + job = transaction_add_one_job(tr, type, unit, &is_new); + if (!job) return -ENOMEM; if (FLAGS_SET(flags, TRANSACTION_IGNORE_ORDER)) - ret->ignore_order = true; + job->ignore_order = true; /* Then, add a link to the job. */ if (by) { - if (!job_dependency_new(by, ret, FLAGS_SET(flags, TRANSACTION_MATTERS), FLAGS_SET(flags, TRANSACTION_CONFLICTS))) + if (!job_dependency_new(by, job, FLAGS_SET(flags, TRANSACTION_MATTERS), FLAGS_SET(flags, TRANSACTION_CONFLICTS))) return -ENOMEM; } else { /* If the job has no parent job, it is the anchor job. */ assert(!tr->anchor_job); - tr->anchor_job = ret; + tr->anchor_job = job; if (FLAGS_SET(flags, TRANSACTION_REENQUEUE_ANCHOR)) - ret->refuse_late_merge = true; + job->refuse_late_merge = true; } if (!is_new || FLAGS_SET(flags, TRANSACTION_IGNORE_REQUIREMENTS) || type == JOB_NOP) @@ -1026,9 +1026,9 @@ int transaction_add_job_and_dependencies( Unit *dep; /* If we are following some other unit, make sure we add all dependencies of everybody following. */ - if (unit_following_set(ret->unit, &following) > 0) + if (unit_following_set(job->unit, &following) > 0) SET_FOREACH(dep, following) { - r = transaction_add_job_and_dependencies(tr, type, dep, ret, flags & TRANSACTION_IGNORE_ORDER, e); + r = transaction_add_job_and_dependencies(tr, type, dep, job, flags & TRANSACTION_IGNORE_ORDER, e); if (r < 0) { log_unit_full_errno(dep, r == -ERFKILL ? LOG_INFO : LOG_WARNING, r, "Cannot add dependency job, ignoring: %s", @@ -1039,8 +1039,8 @@ int transaction_add_job_and_dependencies( /* Finally, recursively add in all dependencies. */ if (IN_SET(type, JOB_START, JOB_RESTART)) { - UNIT_FOREACH_DEPENDENCY(dep, ret->unit, UNIT_ATOM_PULL_IN_START) { - r = transaction_add_job_and_dependencies(tr, JOB_START, dep, ret, TRANSACTION_MATTERS | (flags & TRANSACTION_IGNORE_ORDER), e); + UNIT_FOREACH_DEPENDENCY_SAFE(dep, job->unit, UNIT_ATOM_PULL_IN_START) { + r = transaction_add_job_and_dependencies(tr, JOB_START, dep, job, TRANSACTION_MATTERS | (flags & TRANSACTION_IGNORE_ORDER), e); if (r < 0) { if (r != -EBADR) /* job type not applicable */ goto fail; @@ -1049,8 +1049,8 @@ int transaction_add_job_and_dependencies( } } - UNIT_FOREACH_DEPENDENCY(dep, ret->unit, UNIT_ATOM_PULL_IN_START_IGNORED) { - r = transaction_add_job_and_dependencies(tr, JOB_START, dep, ret, flags & TRANSACTION_IGNORE_ORDER, e); + UNIT_FOREACH_DEPENDENCY_SAFE(dep, job->unit, UNIT_ATOM_PULL_IN_START_IGNORED) { + r = transaction_add_job_and_dependencies(tr, JOB_START, dep, job, flags & TRANSACTION_IGNORE_ORDER, e); if (r < 0) { /* unit masked, job type not applicable and unit not found are not considered * as errors. */ @@ -1062,8 +1062,8 @@ int transaction_add_job_and_dependencies( } } - UNIT_FOREACH_DEPENDENCY(dep, ret->unit, UNIT_ATOM_PULL_IN_VERIFY) { - r = transaction_add_job_and_dependencies(tr, JOB_VERIFY_ACTIVE, dep, ret, TRANSACTION_MATTERS | (flags & TRANSACTION_IGNORE_ORDER), e); + UNIT_FOREACH_DEPENDENCY_SAFE(dep, job->unit, UNIT_ATOM_PULL_IN_VERIFY) { + r = transaction_add_job_and_dependencies(tr, JOB_VERIFY_ACTIVE, dep, job, TRANSACTION_MATTERS | (flags & TRANSACTION_IGNORE_ORDER), e); if (r < 0) { if (r != -EBADR) /* job type not applicable */ goto fail; @@ -1072,8 +1072,8 @@ int transaction_add_job_and_dependencies( } } - UNIT_FOREACH_DEPENDENCY(dep, ret->unit, UNIT_ATOM_PULL_IN_STOP) { - r = transaction_add_job_and_dependencies(tr, JOB_STOP, dep, ret, TRANSACTION_MATTERS | TRANSACTION_CONFLICTS | (flags & TRANSACTION_IGNORE_ORDER), e); + UNIT_FOREACH_DEPENDENCY_SAFE(dep, job->unit, UNIT_ATOM_PULL_IN_STOP) { + r = transaction_add_job_and_dependencies(tr, JOB_STOP, dep, job, TRANSACTION_MATTERS | TRANSACTION_CONFLICTS | (flags & TRANSACTION_IGNORE_ORDER), e); if (r < 0) { if (r != -EBADR) /* job type not applicable */ goto fail; @@ -1082,8 +1082,8 @@ int transaction_add_job_and_dependencies( } } - UNIT_FOREACH_DEPENDENCY(dep, ret->unit, UNIT_ATOM_PULL_IN_STOP_IGNORED) { - r = transaction_add_job_and_dependencies(tr, JOB_STOP, dep, ret, flags & TRANSACTION_IGNORE_ORDER, e); + UNIT_FOREACH_DEPENDENCY_SAFE(dep, job->unit, UNIT_ATOM_PULL_IN_STOP_IGNORED) { + r = transaction_add_job_and_dependencies(tr, JOB_STOP, dep, job, flags & TRANSACTION_IGNORE_ORDER, e); if (r < 0) { log_unit_warning(dep, "Cannot add dependency job, ignoring: %s", @@ -1096,7 +1096,7 @@ int transaction_add_job_and_dependencies( if (IN_SET(type, JOB_RESTART, JOB_STOP) || (type == JOB_START && FLAGS_SET(flags, TRANSACTION_PROPAGATE_START_AS_RESTART))) { bool is_stop = type == JOB_STOP; - UNIT_FOREACH_DEPENDENCY(dep, ret->unit, UNIT_ATOM_PROPAGATE_STOP) { + UNIT_FOREACH_DEPENDENCY_SAFE(dep, job->unit, UNIT_ATOM_PROPAGATE_STOP) { /* We propagate RESTART only as TRY_RESTART, in order not to start dependencies that * are not around. */ JobType nt; @@ -1105,7 +1105,7 @@ int transaction_add_job_and_dependencies( if (nt == JOB_NOP) continue; - r = transaction_add_job_and_dependencies(tr, nt, dep, ret, TRANSACTION_MATTERS | (flags & TRANSACTION_IGNORE_ORDER), e); + r = transaction_add_job_and_dependencies(tr, nt, dep, job, TRANSACTION_MATTERS | (flags & TRANSACTION_IGNORE_ORDER), e); if (r < 0) { if (r != -EBADR) /* job type not applicable */ return r; @@ -1118,7 +1118,7 @@ int transaction_add_job_and_dependencies( * all other dependencies are processed, i.e. we're the anchor job or already in the recursion * that handles it. */ if (!by || FLAGS_SET(flags, TRANSACTION_PROCESS_PROPAGATE_STOP_GRACEFUL)) - UNIT_FOREACH_DEPENDENCY(dep, ret->unit, UNIT_ATOM_PROPAGATE_STOP_GRACEFUL) { + UNIT_FOREACH_DEPENDENCY_SAFE(dep, job->unit, UNIT_ATOM_PROPAGATE_STOP_GRACEFUL) { JobType nt; Job *j; @@ -1128,7 +1128,7 @@ int transaction_add_job_and_dependencies( if (nt == JOB_NOP) continue; - r = transaction_add_job_and_dependencies(tr, nt, dep, ret, TRANSACTION_MATTERS | (flags & TRANSACTION_IGNORE_ORDER) | TRANSACTION_PROCESS_PROPAGATE_STOP_GRACEFUL, e); + r = transaction_add_job_and_dependencies(tr, nt, dep, job, TRANSACTION_MATTERS | (flags & TRANSACTION_IGNORE_ORDER) | TRANSACTION_PROCESS_PROPAGATE_STOP_GRACEFUL, e); if (r < 0) { if (r != -EBADR) /* job type not applicable */ return r; @@ -1139,7 +1139,7 @@ int transaction_add_job_and_dependencies( } if (type == JOB_RELOAD) - transaction_add_propagate_reload_jobs(tr, ret->unit, ret, flags & TRANSACTION_IGNORE_ORDER); + transaction_add_propagate_reload_jobs(tr, job->unit, job, flags & TRANSACTION_IGNORE_ORDER); /* JOB_VERIFY_ACTIVE requires no dependency handling */ @@ -1150,7 +1150,7 @@ fail: log_unit_debug_errno(unit, r, "Cannot add dependency job to transaction, deleting job %s/%s again: %s", unit->id, job_type_to_string(type), bus_error_message(e, r)); - transaction_delete_job(tr, ret, /* delete_dependencies= */ false); + transaction_delete_job(tr, job, /* delete_dependencies= */ false); return r; } @@ -1216,7 +1216,7 @@ int transaction_add_triggering_jobs(Transaction *tr, Unit *u) { assert(tr); assert(u); - UNIT_FOREACH_DEPENDENCY(trigger, u, UNIT_ATOM_TRIGGERED_BY) { + UNIT_FOREACH_DEPENDENCY_SAFE(trigger, u, UNIT_ATOM_TRIGGERED_BY) { _cleanup_(sd_bus_error_free) sd_bus_error e = SD_BUS_ERROR_NULL; /* No need to stop inactive jobs */ diff --git a/src/core/unit.c b/src/core/unit.c index f7ff0061fa..d617624ded 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -631,12 +631,14 @@ static void unit_clear_dependencies(Unit *u) { hashmap_remove(other_deps, u); unit_add_to_gc_queue(other); + other->dependency_generation++; } hashmap_free(deps); } u->dependencies = hashmap_free(u->dependencies); + u->dependency_generation++; } static void unit_remove_transient(Unit *u) { @@ -1094,6 +1096,9 @@ static void unit_merge_dependencies(Unit *u, Unit *other) { } other->dependencies = hashmap_free(other->dependencies); + + u->dependency_generation++; + other->dependency_generation++; } int unit_merge(Unit *u, Unit *other) { @@ -3088,6 +3093,7 @@ static int unit_add_dependency_impl( return r; flags = NOTIFY_DEPENDENCY_UPDATE_FROM; + u->dependency_generation++; } if (other_info.data != other_info_old.data) { @@ -3104,6 +3110,7 @@ static int unit_add_dependency_impl( } flags |= NOTIFY_DEPENDENCY_UPDATE_TO; + other->dependency_generation++; } return flags; @@ -5618,6 +5625,9 @@ void unit_remove_dependencies(Unit *u, UnitDependencyMask mask) { /* The unit 'other' may not be wanted by the unit 'u'. */ unit_submit_to_stop_when_unneeded_queue(other); + u->dependency_generation++; + other->dependency_generation++; + done = false; break; } diff --git a/src/core/unit.h b/src/core/unit.h index f3a44fd1a1..d508865748 100644 --- a/src/core/unit.h +++ b/src/core/unit.h @@ -238,6 +238,7 @@ typedef struct Unit { * and whose value encodes why the dependency exists, using the UnitDependencyInfo type. i.e. a * Hashmap(UnitDependency → Hashmap(Unit* → UnitDependencyInfo)) */ Hashmap *dependencies; + uint64_t dependency_generation; /* Similar, for RequiresMountsFor= and WantsMountsFor= path dependencies. The key is the path, the * value the UnitDependencyInfo type */ @@ -1152,27 +1153,44 @@ CollectMode collect_mode_from_string(const char *s) _pure_; typedef struct UnitForEachDependencyData { /* Stores state for the FOREACH macro below for iterating through all deps that have any of the * specified dependency atom bits set */ + const Unit *unit; UnitDependencyAtom match_atom; Hashmap *by_type, *by_unit; void *current_type; Iterator by_type_iterator, by_unit_iterator; Unit **current_unit; + uint64_t generation; + unsigned n_restart; + bool restart_on_generation_change; } UnitForEachDependencyData; +/* Let's not restart the loop infinitely. */ +#define MAX_FOREACH_DEPENDENCY_RESTART 100000 + /* Iterates through all dependencies that have a specific atom in the dependency type set. This tries to be * smart: if the atom is unique, we'll directly go to right entry. Otherwise we'll iterate through the * per-dependency type hashmap and match all dep that have the right atom set. */ -#define _UNIT_FOREACH_DEPENDENCY(other, u, ma, data) \ +#define _UNIT_FOREACH_DEPENDENCY(other, u, ma, restart, data) \ for (UnitForEachDependencyData data = { \ + .unit = (u), \ .match_atom = (ma), \ - .by_type = (u)->dependencies, \ - .by_type_iterator = ITERATOR_FIRST, \ .current_unit = &(other), \ + .restart_on_generation_change = (restart), \ }; \ ({ \ UnitDependency _dt = _UNIT_DEPENDENCY_INVALID; \ bool _found; \ \ + if (data.generation == 0 || \ + (data.restart_on_generation_change && \ + data.generation != data.unit->dependency_generation)) { \ + data.generation = data.unit->dependency_generation; \ + data.by_type = data.unit->dependencies; \ + data.by_type_iterator = ITERATOR_FIRST; \ + assert_se(data.n_restart++ < MAX_FOREACH_DEPENDENCY_RESTART); \ + } else \ + assert(data.generation == data.unit->dependency_generation); \ + \ if (data.by_type && ITERATOR_IS_FIRST(data.by_type_iterator)) { \ _dt = unit_dependency_from_unique_atom(data.match_atom); \ if (_dt >= 0) { \ @@ -1185,12 +1203,13 @@ typedef struct UnitForEachDependencyData { if (_dt < 0) \ _found = hashmap_iterate(data.by_type, \ &data.by_type_iterator, \ - (void**)&(data.by_unit), \ + (void**) &(data.by_unit), \ (const void**) &(data.current_type)); \ _found; \ }); ) \ if ((unit_dependency_to_atom(UNIT_DEPENDENCY_FROM_PTR(data.current_type)) & data.match_atom) != 0) \ for (data.by_unit_iterator = ITERATOR_FIRST; \ + data.generation == data.unit->dependency_generation && \ hashmap_iterate(data.by_unit, \ &data.by_unit_iterator, \ NULL, \ @@ -1198,7 +1217,9 @@ typedef struct UnitForEachDependencyData { /* Note: this matches deps that have *any* of the atoms specified in match_atom set */ #define UNIT_FOREACH_DEPENDENCY(other, u, match_atom) \ - _UNIT_FOREACH_DEPENDENCY(other, u, match_atom, UNIQ_T(data, UNIQ)) + _UNIT_FOREACH_DEPENDENCY(other, u, match_atom, false, UNIQ_T(data, UNIQ)) +#define UNIT_FOREACH_DEPENDENCY_SAFE(other, u, match_atom) \ + _UNIT_FOREACH_DEPENDENCY(other, u, match_atom, true, UNIQ_T(data, UNIQ)) #define _LOG_CONTEXT_PUSH_UNIT(unit, u, c) \ const Unit *u = (unit); \ diff --git a/test/units/TEST-23-UNIT-FILE.start-stop-no-reload.sh b/test/units/TEST-23-UNIT-FILE.start-stop-no-reload.sh index 61a6592cb6..0d29c2393a 100755 --- a/test/units/TEST-23-UNIT-FILE.start-stop-no-reload.sh +++ b/test/units/TEST-23-UNIT-FILE.start-stop-no-reload.sh @@ -10,7 +10,14 @@ set -o pipefail at_exit() { set +e - rm -f /run/systemd/system/TEST-23-UNIT-FILE-no-reload.{service,target} + rm -f /run/systemd/system/TEST-23-UNIT-FILE-no-reload.target + rm -f /run/systemd/system/TEST-23-UNIT-FILE-no-reload.service + rm -f /run/systemd/system/TEST-23-UNIT-FILE-no-reload-2.service + rm -f /run/systemd/system/TEST-23-UNIT-FILE-no-reload-3.service + systemctl stop TEST-23-UNIT-FILE-no-reload.target + systemctl stop TEST-23-UNIT-FILE-no-reload.service + systemctl stop TEST-23-UNIT-FILE-no-reload-2.service + systemctl stop TEST-23-UNIT-FILE-no-reload-3.service } trap at_exit EXIT @@ -91,3 +98,58 @@ EOF systemctl restart TEST-23-UNIT-FILE-no-reload.target systemctl is-active TEST-23-UNIT-FILE-no-reload.service + +# Stop and remove, and try again to exercise https://github.com/systemd/systemd/issues/36031 +systemctl stop TEST-23-UNIT-FILE-no-reload.service TEST-23-UNIT-FILE-no-reload.target +rm -f /run/systemd/system/TEST-23-UNIT-FILE-no-reload.service /run/systemd/system/TEST-23-UNIT-FILE-no-reload.target +systemctl daemon-reload + +sleep 3.1 + +cat >/run/systemd/system/TEST-23-UNIT-FILE-no-reload.target </run/systemd/system/TEST-23-UNIT-FILE-no-reload.service </run/systemd/system/TEST-23-UNIT-FILE-no-reload-2.service </run/systemd/system/TEST-23-UNIT-FILE-no-reload-3.service <