Skip to content

Commit 32c2e96

Browse files
yuwatabluca
yuwata
authored and
bluca
committed
core: introduce Unit.dependency_generation counter and restart loop when dependency is updated in the loop
When starting unit A, a dependent unit B may be loaded if it is not loaded yet, and the dependencies in unit A may be updated. As Hashmap does not allow a new entry to be added in a loop, we need to restart loop in such case. Fixes a bug introduced by cda6677. Fixes #36031. (cherry picked from commit b7777d0) (cherry picked from commit 4dc4fdcfe051b10aa4f7fe4d3ab220c27084eaf5) (cherry picked from commit 01f34bf5df4c42bdd0e3622d0823a54f91316f61) (cherry picked from commit 9316f1b)
1 parent 69cee4f commit 32c2e96

File tree

3 files changed

+45
-14
lines changed

3 files changed

+45
-14
lines changed

‎src/core/transaction.c‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -892,7 +892,7 @@ void transaction_add_propagate_reload_jobs(
892892
assert(tr);
893893
assert(unit);
894894

895-
UNIT_FOREACH_DEPENDENCY(dep, unit, UNIT_ATOM_PROPAGATES_RELOAD_TO) {
895+
UNIT_FOREACH_DEPENDENCY_SAFE(dep, unit, UNIT_ATOM_PROPAGATES_RELOAD_TO) {
896896
_cleanup_(sd_bus_error_free) sd_bus_error e = SD_BUS_ERROR_NULL;
897897

898898
nt = job_type_collapse(JOB_TRY_RELOAD, dep);
@@ -1037,7 +1037,7 @@ int transaction_add_job_and_dependencies(
10371037

10381038
/* Finally, recursively add in all dependencies. */
10391039
if (IN_SET(type, JOB_START, JOB_RESTART)) {
1040-
UNIT_FOREACH_DEPENDENCY(dep, job->unit, UNIT_ATOM_PULL_IN_START) {
1040+
UNIT_FOREACH_DEPENDENCY_SAFE(dep, job->unit, UNIT_ATOM_PULL_IN_START) {
10411041
r = transaction_add_job_and_dependencies(tr, JOB_START, dep, job, TRANSACTION_MATTERS | (flags & TRANSACTION_IGNORE_ORDER), e);
10421042
if (r < 0) {
10431043
if (r != -EBADR) /* job type not applicable */
@@ -1047,7 +1047,7 @@ int transaction_add_job_and_dependencies(
10471047
}
10481048
}
10491049

1050-
UNIT_FOREACH_DEPENDENCY(dep, job->unit, UNIT_ATOM_PULL_IN_START_IGNORED) {
1050+
UNIT_FOREACH_DEPENDENCY_SAFE(dep, job->unit, UNIT_ATOM_PULL_IN_START_IGNORED) {
10511051
r = transaction_add_job_and_dependencies(tr, JOB_START, dep, job, flags & TRANSACTION_IGNORE_ORDER, e);
10521052
if (r < 0) {
10531053
/* unit masked, job type not applicable and unit not found are not considered
@@ -1060,7 +1060,7 @@ int transaction_add_job_and_dependencies(
10601060
}
10611061
}
10621062

1063-
UNIT_FOREACH_DEPENDENCY(dep, job->unit, UNIT_ATOM_PULL_IN_VERIFY) {
1063+
UNIT_FOREACH_DEPENDENCY_SAFE(dep, job->unit, UNIT_ATOM_PULL_IN_VERIFY) {
10641064
r = transaction_add_job_and_dependencies(tr, JOB_VERIFY_ACTIVE, dep, job, TRANSACTION_MATTERS | (flags & TRANSACTION_IGNORE_ORDER), e);
10651065
if (r < 0) {
10661066
if (r != -EBADR) /* job type not applicable */
@@ -1070,7 +1070,7 @@ int transaction_add_job_and_dependencies(
10701070
}
10711071
}
10721072

1073-
UNIT_FOREACH_DEPENDENCY(dep, job->unit, UNIT_ATOM_PULL_IN_STOP) {
1073+
UNIT_FOREACH_DEPENDENCY_SAFE(dep, job->unit, UNIT_ATOM_PULL_IN_STOP) {
10741074
r = transaction_add_job_and_dependencies(tr, JOB_STOP, dep, job, TRANSACTION_MATTERS | TRANSACTION_CONFLICTS | (flags & TRANSACTION_IGNORE_ORDER), e);
10751075
if (r < 0) {
10761076
if (r != -EBADR) /* job type not applicable */
@@ -1080,7 +1080,7 @@ int transaction_add_job_and_dependencies(
10801080
}
10811081
}
10821082

1083-
UNIT_FOREACH_DEPENDENCY(dep, job->unit, UNIT_ATOM_PULL_IN_STOP_IGNORED) {
1083+
UNIT_FOREACH_DEPENDENCY_SAFE(dep, job->unit, UNIT_ATOM_PULL_IN_STOP_IGNORED) {
10841084
r = transaction_add_job_and_dependencies(tr, JOB_STOP, dep, job, flags & TRANSACTION_IGNORE_ORDER, e);
10851085
if (r < 0) {
10861086
log_unit_warning(dep,
@@ -1094,7 +1094,7 @@ int transaction_add_job_and_dependencies(
10941094
if (IN_SET(type, JOB_RESTART, JOB_STOP) || (type == JOB_START && FLAGS_SET(flags, TRANSACTION_PROPAGATE_START_AS_RESTART))) {
10951095
bool is_stop = type == JOB_STOP;
10961096

1097-
UNIT_FOREACH_DEPENDENCY(dep, job->unit, is_stop ? UNIT_ATOM_PROPAGATE_STOP : UNIT_ATOM_PROPAGATE_RESTART) {
1097+
UNIT_FOREACH_DEPENDENCY_SAFE(dep, job->unit, is_stop ? UNIT_ATOM_PROPAGATE_STOP : UNIT_ATOM_PROPAGATE_RESTART) {
10981098
/* We propagate RESTART only as TRY_RESTART, in order not to start dependencies that
10991099
* are not around. */
11001100
JobType nt;
@@ -1116,7 +1116,7 @@ int transaction_add_job_and_dependencies(
11161116
* all other dependencies are processed, i.e. we're the anchor job or already in the recursion
11171117
* that handles it. */
11181118
if (!by || FLAGS_SET(flags, TRANSACTION_PROCESS_PROPAGATE_STOP_GRACEFUL))
1119-
UNIT_FOREACH_DEPENDENCY(dep, job->unit, UNIT_ATOM_PROPAGATE_STOP_GRACEFUL) {
1119+
UNIT_FOREACH_DEPENDENCY_SAFE(dep, job->unit, UNIT_ATOM_PROPAGATE_STOP_GRACEFUL) {
11201120
JobType nt;
11211121
Job *j;
11221122

@@ -1214,7 +1214,7 @@ int transaction_add_triggering_jobs(Transaction *tr, Unit *u) {
12141214
assert(tr);
12151215
assert(u);
12161216

1217-
UNIT_FOREACH_DEPENDENCY(trigger, u, UNIT_ATOM_TRIGGERED_BY) {
1217+
UNIT_FOREACH_DEPENDENCY_SAFE(trigger, u, UNIT_ATOM_TRIGGERED_BY) {
12181218
_cleanup_(sd_bus_error_free) sd_bus_error e = SD_BUS_ERROR_NULL;
12191219

12201220
/* No need to stop inactive jobs */

‎src/core/unit.c‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -646,12 +646,14 @@ static void unit_clear_dependencies(Unit *u) {
646646
hashmap_remove(other_deps, u);
647647

648648
unit_add_to_gc_queue(other);
649+
other->dependency_generation++;
649650
}
650651

651652
hashmap_free(deps);
652653
}
653654

654655
u->dependencies = hashmap_free(u->dependencies);
656+
u->dependency_generation++;
655657
}
656658

657659
static void unit_remove_transient(Unit *u) {
@@ -1151,6 +1153,9 @@ static void unit_merge_dependencies(Unit *u, Unit *other) {
11511153
}
11521154

11531155
other->dependencies = hashmap_free(other->dependencies);
1156+
1157+
u->dependency_generation++;
1158+
other->dependency_generation++;
11541159
}
11551160

11561161
int unit_merge(Unit *u, Unit *other) {
@@ -3183,6 +3188,7 @@ static int unit_add_dependency_impl(
31833188
return r;
31843189

31853190
flags = NOTIFY_DEPENDENCY_UPDATE_FROM;
3191+
u->dependency_generation++;
31863192
}
31873193

31883194
if (other_info.data != other_info_old.data) {
@@ -3199,6 +3205,7 @@ static int unit_add_dependency_impl(
31993205
}
32003206

32013207
flags |= NOTIFY_DEPENDENCY_UPDATE_TO;
3208+
other->dependency_generation++;
32023209
}
32033210

32043211
return flags;
@@ -5451,6 +5458,9 @@ void unit_remove_dependencies(Unit *u, UnitDependencyMask mask) {
54515458
/* The unit 'other' may not be wanted by the unit 'u'. */
54525459
unit_submit_to_stop_when_unneeded_queue(other);
54535460

5461+
u->dependency_generation++;
5462+
other->dependency_generation++;
5463+
54545464
done = false;
54555465
break;
54565466
}

‎src/core/unit.h‎

Lines changed: 26 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -214,6 +214,7 @@ typedef struct Unit {
214214
* and whose value encodes why the dependency exists, using the UnitDependencyInfo type. i.e. a
215215
* Hashmap(UnitDependency → Hashmap(Unit* → UnitDependencyInfo)) */
216216
Hashmap *dependencies;
217+
uint64_t dependency_generation;
217218

218219
/* Similar, for RequiresMountsFor= path dependencies. The key is the path, the value the
219220
* UnitDependencyInfo type */
@@ -1178,27 +1179,44 @@ CollectMode collect_mode_from_string(const char *s) _pure_;
11781179
typedef struct UnitForEachDependencyData {
11791180
/* Stores state for the FOREACH macro below for iterating through all deps that have any of the
11801181
* specified dependency atom bits set */
1182+
const Unit *unit;
11811183
UnitDependencyAtom match_atom;
11821184
Hashmap *by_type, *by_unit;
11831185
void *current_type;
11841186
Iterator by_type_iterator, by_unit_iterator;
11851187
Unit **current_unit;
1188+
uint64_t generation;
1189+
unsigned n_restart;
1190+
bool restart_on_generation_change;
11861191
} UnitForEachDependencyData;
11871192

1193+
/* Let's not restart the loop infinitely. */
1194+
#define MAX_FOREACH_DEPENDENCY_RESTART 100000
1195+
11881196
/* Iterates through all dependencies that have a specific atom in the dependency type set. This tries to be
11891197
* smart: if the atom is unique, we'll directly go to right entry. Otherwise we'll iterate through the
11901198
* per-dependency type hashmap and match all dep that have the right atom set. */
1191-
#define _UNIT_FOREACH_DEPENDENCY(other, u, ma, data) \
1199+
#define _UNIT_FOREACH_DEPENDENCY(other, u, ma, restart, data) \
11921200
for (UnitForEachDependencyData data = { \
1201+
.unit = (u), \
11931202
.match_atom = (ma), \
1194-
.by_type = (u)->dependencies, \
1195-
.by_type_iterator = ITERATOR_FIRST, \
11961203
.current_unit = &(other), \
1204+
.restart_on_generation_change = (restart), \
11971205
}; \
11981206
({ \
11991207
UnitDependency _dt = _UNIT_DEPENDENCY_INVALID; \
12001208
bool _found; \
12011209
\
1210+
if (data.generation == 0 || \
1211+
(data.restart_on_generation_change && \
1212+
data.generation != data.unit->dependency_generation)) { \
1213+
data.generation = data.unit->dependency_generation; \
1214+
data.by_type = data.unit->dependencies; \
1215+
data.by_type_iterator = ITERATOR_FIRST; \
1216+
assert_se(data.n_restart++ < MAX_FOREACH_DEPENDENCY_RESTART); \
1217+
} else \
1218+
assert(data.generation == data.unit->dependency_generation); \
1219+
\
12021220
if (data.by_type && ITERATOR_IS_FIRST(data.by_type_iterator)) { \
12031221
_dt = unit_dependency_from_unique_atom(data.match_atom); \
12041222
if (_dt >= 0) { \
@@ -1211,20 +1229,23 @@ typedef struct UnitForEachDependencyData {
12111229
if (_dt < 0) \
12121230
_found = hashmap_iterate(data.by_type, \
12131231
&data.by_type_iterator, \
1214-
(void**)&(data.by_unit), \
1232+
(void**) &(data.by_unit), \
12151233
(const void**) &(data.current_type)); \
12161234
_found; \
12171235
}); ) \
12181236
if ((unit_dependency_to_atom(UNIT_DEPENDENCY_FROM_PTR(data.current_type)) & data.match_atom) != 0) \
12191237
for (data.by_unit_iterator = ITERATOR_FIRST; \
1238+
data.generation == data.unit->dependency_generation && \
12201239
hashmap_iterate(data.by_unit, \
12211240
&data.by_unit_iterator, \
12221241
NULL, \
12231242
(const void**) data.current_unit); )
12241243

12251244
/* Note: this matches deps that have *any* of the atoms specified in match_atom set */
12261245
#define UNIT_FOREACH_DEPENDENCY(other, u, match_atom) \
1227-
_UNIT_FOREACH_DEPENDENCY(other, u, match_atom, UNIQ_T(data, UNIQ))
1246+
_UNIT_FOREACH_DEPENDENCY(other, u, match_atom, false, UNIQ_T(data, UNIQ))
1247+
#define UNIT_FOREACH_DEPENDENCY_SAFE(other, u, match_atom) \
1248+
_UNIT_FOREACH_DEPENDENCY(other, u, match_atom, true, UNIQ_T(data, UNIQ))
12281249

12291250
#define _LOG_CONTEXT_PUSH_UNIT(unit, u, c) \
12301251
const Unit *u = (unit); \

0 commit comments

Comments
 (0)