From 641d3761d45c53000195ed4c6fff7d0d08cdc912 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 13 Apr 2021 17:20:58 +0200 Subject: [PATCH 01/18] hashmap: add helper to test if iterator is still at beginning --- src/basic/hashmap.h | 1 + 1 file changed, 1 insertion(+) diff --git a/src/basic/hashmap.h b/src/basic/hashmap.h index c855f39b1d..f7ade2e774 100644 --- a/src/basic/hashmap.h +++ b/src/basic/hashmap.h @@ -51,6 +51,7 @@ typedef struct { #define _IDX_ITERATOR_FIRST (UINT_MAX - 1) #define ITERATOR_FIRST ((Iterator) { .idx = _IDX_ITERATOR_FIRST, .next_key = NULL }) +#define ITERATOR_IS_FIRST(i) ((i).idx == _IDX_ITERATOR_FIRST) /* Macros for type checking */ #define PTR_COMPATIBLE_WITH_HASHMAP_BASE(h) \ From 15ed3c3a188cf7fa5a60ae508fc7a3ed048d2220 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 13 Apr 2021 17:25:42 +0200 Subject: [PATCH 02/18] core: split dependency types into atoms --- src/core/cgroup.c | 27 +- src/core/dbus-unit.c | 56 +-- src/core/device.c | 2 +- src/core/job.c | 84 ++-- src/core/job.h | 4 +- src/core/manager.c | 34 +- src/core/meson.build | 2 + src/core/service.c | 3 +- src/core/slice.c | 7 +- src/core/socket.c | 6 +- src/core/target.c | 37 +- src/core/transaction.c | 91 ++--- src/core/unit-dependency-atom.c | 196 ++++++++++ src/core/unit-dependency-atom.h | 77 ++++ src/core/unit-serialize.c | 2 +- src/core/unit.c | 661 ++++++++++++++++++-------------- src/core/unit.h | 80 +++- src/test/test-engine.c | 32 +- 18 files changed, 909 insertions(+), 492 deletions(-) create mode 100644 src/core/unit-dependency-atom.c create mode 100644 src/core/unit-dependency-atom.h diff --git a/src/core/cgroup.c b/src/core/cgroup.c index 5453b5ae96..094d1becd3 100644 --- a/src/core/cgroup.c +++ b/src/core/cgroup.c @@ -1700,12 +1700,14 @@ CGroupMask unit_get_members_mask(Unit *u) { u->cgroup_members_mask = 0; if (u->type == UNIT_SLICE) { - void *v; Unit *member; - HASHMAP_FOREACH_KEY(v, member, u->dependencies[UNIT_BEFORE]) - if (UNIT_DEREF(member->slice) == u) - u->cgroup_members_mask |= unit_get_subtree_mask(member); /* note that this calls ourselves again, for the children */ + UNIT_FOREACH_DEPENDENCY(member, u, UNIT_ATOM_BEFORE) { + if (UNIT_DEREF(member->slice) != u) + continue; + + u->cgroup_members_mask |= unit_get_subtree_mask(member); /* note that this calls ourselves again, for the children */ + } } u->cgroup_members_mask_valid = true; @@ -2343,14 +2345,13 @@ static int unit_realize_cgroup_now_enable(Unit *u, ManagerState state) { * hierarchy upwards to the unit in question. */ static int unit_realize_cgroup_now_disable(Unit *u, ManagerState state) { Unit *m; - void *v; assert(u); if (u->type != UNIT_SLICE) return 0; - HASHMAP_FOREACH_KEY(v, m, u->dependencies[UNIT_BEFORE]) { + UNIT_FOREACH_DEPENDENCY(m, u, UNIT_ATOM_BEFORE) { CGroupMask target_mask, enable_mask, new_target_mask, new_enable_mask; int r; @@ -2515,12 +2516,12 @@ void unit_add_family_to_cgroup_realize_queue(Unit *u) { do { Unit *m; - void *v; /* Children of u likely changed when we're called */ u->cgroup_members_mask_valid = false; - HASHMAP_FOREACH_KEY(v, m, u->dependencies[UNIT_BEFORE]) { + UNIT_FOREACH_DEPENDENCY(m, u, UNIT_ATOM_BEFORE) { + /* Skip units that have a dependency on the slice but aren't actually in it. */ if (UNIT_DEREF(m->slice) != u) continue; @@ -3782,11 +3783,13 @@ void unit_invalidate_cgroup_bpf(Unit *u) { * list of our children includes our own. */ if (u->type == UNIT_SLICE) { Unit *member; - void *v; - HASHMAP_FOREACH_KEY(v, member, u->dependencies[UNIT_BEFORE]) - if (UNIT_DEREF(member->slice) == u) - unit_invalidate_cgroup_bpf(member); + UNIT_FOREACH_DEPENDENCY(member, u, UNIT_ATOM_BEFORE) { + if (UNIT_DEREF(member->slice) != u) + continue; + + unit_invalidate_cgroup_bpf(member); + } } } diff --git a/src/core/dbus-unit.c b/src/core/dbus-unit.c index a83648d968..e8818c919c 100644 --- a/src/core/dbus-unit.c +++ b/src/core/dbus-unit.c @@ -155,21 +155,27 @@ static int property_get_dependencies( void *userdata, sd_bus_error *error) { - Hashmap **h = userdata; - Unit *u; + Unit *u = userdata, *other; + UnitDependency d; + Hashmap *deps; void *v; int r; assert(bus); assert(reply); - assert(h); + assert(u); + + d = unit_dependency_from_string(property); + assert_se(d >= 0); + + deps = unit_get_dependencies(u, d); r = sd_bus_message_open_container(reply, 'a', "s"); if (r < 0) return r; - HASHMAP_FOREACH_KEY(v, u, *h) { - r = sd_bus_message_append(reply, "s", u->id); + HASHMAP_FOREACH_KEY(v, other, deps) { + r = sd_bus_message_append(reply, "s", other->id); if (r < 0) return r; } @@ -844,26 +850,26 @@ const sd_bus_vtable bus_unit_vtable[] = { SD_BUS_PROPERTY("Id", "s", NULL, offsetof(Unit, id), SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("Names", "as", property_get_names, 0, SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("Following", "s", property_get_following, 0, 0), - SD_BUS_PROPERTY("Requires", "as", property_get_dependencies, offsetof(Unit, dependencies[UNIT_REQUIRES]), SD_BUS_VTABLE_PROPERTY_CONST), - SD_BUS_PROPERTY("Requisite", "as", property_get_dependencies, offsetof(Unit, dependencies[UNIT_REQUISITE]), SD_BUS_VTABLE_PROPERTY_CONST), - SD_BUS_PROPERTY("Wants", "as", property_get_dependencies, offsetof(Unit, dependencies[UNIT_WANTS]), SD_BUS_VTABLE_PROPERTY_CONST), - SD_BUS_PROPERTY("BindsTo", "as", property_get_dependencies, offsetof(Unit, dependencies[UNIT_BINDS_TO]), SD_BUS_VTABLE_PROPERTY_CONST), - SD_BUS_PROPERTY("PartOf", "as", property_get_dependencies, offsetof(Unit, dependencies[UNIT_PART_OF]), SD_BUS_VTABLE_PROPERTY_CONST), - SD_BUS_PROPERTY("RequiredBy", "as", property_get_dependencies, offsetof(Unit, dependencies[UNIT_REQUIRED_BY]), SD_BUS_VTABLE_PROPERTY_CONST), - SD_BUS_PROPERTY("RequisiteOf", "as", property_get_dependencies, offsetof(Unit, dependencies[UNIT_REQUISITE_OF]), SD_BUS_VTABLE_PROPERTY_CONST), - SD_BUS_PROPERTY("WantedBy", "as", property_get_dependencies, offsetof(Unit, dependencies[UNIT_WANTED_BY]), SD_BUS_VTABLE_PROPERTY_CONST), - SD_BUS_PROPERTY("BoundBy", "as", property_get_dependencies, offsetof(Unit, dependencies[UNIT_BOUND_BY]), SD_BUS_VTABLE_PROPERTY_CONST), - SD_BUS_PROPERTY("ConsistsOf", "as", property_get_dependencies, offsetof(Unit, dependencies[UNIT_CONSISTS_OF]), SD_BUS_VTABLE_PROPERTY_CONST), - SD_BUS_PROPERTY("Conflicts", "as", property_get_dependencies, offsetof(Unit, dependencies[UNIT_CONFLICTS]), SD_BUS_VTABLE_PROPERTY_CONST), - SD_BUS_PROPERTY("ConflictedBy", "as", property_get_dependencies, offsetof(Unit, dependencies[UNIT_CONFLICTED_BY]), SD_BUS_VTABLE_PROPERTY_CONST), - SD_BUS_PROPERTY("Before", "as", property_get_dependencies, offsetof(Unit, dependencies[UNIT_BEFORE]), SD_BUS_VTABLE_PROPERTY_CONST), - SD_BUS_PROPERTY("After", "as", property_get_dependencies, offsetof(Unit, dependencies[UNIT_AFTER]), SD_BUS_VTABLE_PROPERTY_CONST), - SD_BUS_PROPERTY("OnFailure", "as", property_get_dependencies, offsetof(Unit, dependencies[UNIT_ON_FAILURE]), SD_BUS_VTABLE_PROPERTY_CONST), - SD_BUS_PROPERTY("Triggers", "as", property_get_dependencies, offsetof(Unit, dependencies[UNIT_TRIGGERS]), SD_BUS_VTABLE_PROPERTY_CONST), - SD_BUS_PROPERTY("TriggeredBy", "as", property_get_dependencies, offsetof(Unit, dependencies[UNIT_TRIGGERED_BY]), SD_BUS_VTABLE_PROPERTY_CONST), - SD_BUS_PROPERTY("PropagatesReloadTo", "as", property_get_dependencies, offsetof(Unit, dependencies[UNIT_PROPAGATES_RELOAD_TO]), SD_BUS_VTABLE_PROPERTY_CONST), - SD_BUS_PROPERTY("ReloadPropagatedFrom", "as", property_get_dependencies, offsetof(Unit, dependencies[UNIT_RELOAD_PROPAGATED_FROM]), SD_BUS_VTABLE_PROPERTY_CONST), - SD_BUS_PROPERTY("JoinsNamespaceOf", "as", property_get_dependencies, offsetof(Unit, dependencies[UNIT_JOINS_NAMESPACE_OF]), SD_BUS_VTABLE_PROPERTY_CONST), + SD_BUS_PROPERTY("Requires", "as", property_get_dependencies, 0, SD_BUS_VTABLE_PROPERTY_CONST), + SD_BUS_PROPERTY("Requisite", "as", property_get_dependencies, 0, SD_BUS_VTABLE_PROPERTY_CONST), + SD_BUS_PROPERTY("Wants", "as", property_get_dependencies, 0, SD_BUS_VTABLE_PROPERTY_CONST), + SD_BUS_PROPERTY("BindsTo", "as", property_get_dependencies, 0, SD_BUS_VTABLE_PROPERTY_CONST), + SD_BUS_PROPERTY("PartOf", "as", property_get_dependencies, 0, SD_BUS_VTABLE_PROPERTY_CONST), + SD_BUS_PROPERTY("RequiredBy", "as", property_get_dependencies, 0, SD_BUS_VTABLE_PROPERTY_CONST), + SD_BUS_PROPERTY("RequisiteOf", "as", property_get_dependencies, 0, SD_BUS_VTABLE_PROPERTY_CONST), + SD_BUS_PROPERTY("WantedBy", "as", property_get_dependencies, 0, SD_BUS_VTABLE_PROPERTY_CONST), + SD_BUS_PROPERTY("BoundBy", "as", property_get_dependencies, 0, SD_BUS_VTABLE_PROPERTY_CONST), + SD_BUS_PROPERTY("ConsistsOf", "as", property_get_dependencies, 0, SD_BUS_VTABLE_PROPERTY_CONST), + SD_BUS_PROPERTY("Conflicts", "as", property_get_dependencies, 0, SD_BUS_VTABLE_PROPERTY_CONST), + SD_BUS_PROPERTY("ConflictedBy", "as", property_get_dependencies, 0, SD_BUS_VTABLE_PROPERTY_CONST), + SD_BUS_PROPERTY("Before", "as", property_get_dependencies, 0, SD_BUS_VTABLE_PROPERTY_CONST), + SD_BUS_PROPERTY("After", "as", property_get_dependencies, 0, SD_BUS_VTABLE_PROPERTY_CONST), + SD_BUS_PROPERTY("OnFailure", "as", property_get_dependencies, 0, SD_BUS_VTABLE_PROPERTY_CONST), + SD_BUS_PROPERTY("Triggers", "as", property_get_dependencies, 0, SD_BUS_VTABLE_PROPERTY_CONST), + SD_BUS_PROPERTY("TriggeredBy", "as", property_get_dependencies, 0, SD_BUS_VTABLE_PROPERTY_CONST), + SD_BUS_PROPERTY("PropagatesReloadTo", "as", property_get_dependencies, 0, SD_BUS_VTABLE_PROPERTY_CONST), + SD_BUS_PROPERTY("ReloadPropagatedFrom", "as", property_get_dependencies, 0, SD_BUS_VTABLE_PROPERTY_CONST), + SD_BUS_PROPERTY("JoinsNamespaceOf", "as", property_get_dependencies, 0, SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("RequiresMountsFor", "as", property_get_requires_mounts_for, offsetof(Unit, requires_mounts_for), SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("Documentation", "as", NULL, offsetof(Unit, documentation), SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("Description", "s", property_get_description, 0, SD_BUS_VTABLE_PROPERTY_CONST), diff --git a/src/core/device.c b/src/core/device.c index 356c389c55..d34d48d0ff 100644 --- a/src/core/device.c +++ b/src/core/device.c @@ -468,7 +468,7 @@ static void device_upgrade_mount_deps(Unit *u) { /* Let's upgrade Requires= to BindsTo= on us. (Used when SYSTEMD_MOUNT_DEVICE_BOUND is set) */ - HASHMAP_FOREACH_KEY(v, other, u->dependencies[UNIT_REQUIRED_BY]) { + HASHMAP_FOREACH_KEY(v, other, unit_get_dependencies(u, UNIT_REQUIRED_BY)) { if (other->type != UNIT_MOUNT) continue; diff --git a/src/core/job.c b/src/core/job.c index c626ddca91..ae54956b69 100644 --- a/src/core/job.c +++ b/src/core/job.c @@ -455,7 +455,6 @@ int job_type_merge_and_collapse(JobType *a, JobType b, Unit *u) { static bool job_is_runnable(Job *j) { Unit *other; - void *v; assert(j); assert(j->installed); @@ -477,16 +476,16 @@ static bool job_is_runnable(Job *j) { if (j->type == JOB_NOP) return true; - HASHMAP_FOREACH_KEY(v, other, j->unit->dependencies[UNIT_AFTER]) - if (other->job && job_compare(j, other->job, UNIT_AFTER) > 0) { + UNIT_FOREACH_DEPENDENCY(other, j->unit, UNIT_ATOM_AFTER) + if (other->job && job_compare(j, other->job, UNIT_ATOM_AFTER) > 0) { log_unit_debug(j->unit, "starting held back, waiting for: %s", other->id); return false; } - HASHMAP_FOREACH_KEY(v, other, j->unit->dependencies[UNIT_BEFORE]) - if (other->job && job_compare(j, other->job, UNIT_BEFORE) > 0) { + UNIT_FOREACH_DEPENDENCY(other, j->unit, UNIT_ATOM_BEFORE) + if (other->job && job_compare(j, other->job, UNIT_ATOM_BEFORE) > 0) { log_unit_debug(j->unit, "stopping held back, waiting for: %s", other->id); @@ -951,13 +950,12 @@ static void job_emit_done_status_message(Unit *u, uint32_t job_id, JobType t, Jo job_print_done_status_message(u, t, result); } -static void job_fail_dependencies(Unit *u, UnitDependency d) { +static void job_fail_dependencies(Unit *u, UnitDependencyAtom match_atom) { Unit *other; - void *v; assert(u); - HASHMAP_FOREACH_KEY(v, other, u->dependencies[d]) { + UNIT_FOREACH_DEPENDENCY(other, u, match_atom) { Job *j = other->job; if (!j) @@ -970,10 +968,8 @@ static void job_fail_dependencies(Unit *u, UnitDependency d) { } int job_finish_and_invalidate(Job *j, JobResult result, bool recursive, bool already) { - Unit *u; - Unit *other; + Unit *u, *other; JobType t; - void *v; assert(j); assert(j->installed); @@ -1012,12 +1008,10 @@ int job_finish_and_invalidate(Job *j, JobResult result, bool recursive, bool alr /* Fail depending jobs on failure */ if (result != JOB_DONE && recursive) { - if (IN_SET(t, JOB_START, JOB_VERIFY_ACTIVE)) { - job_fail_dependencies(u, UNIT_REQUIRED_BY); - job_fail_dependencies(u, UNIT_REQUISITE_OF); - job_fail_dependencies(u, UNIT_BOUND_BY); - } else if (t == JOB_STOP) - job_fail_dependencies(u, UNIT_CONFLICTED_BY); + if (IN_SET(t, JOB_START, JOB_VERIFY_ACTIVE)) + job_fail_dependencies(u, UNIT_ATOM_PROPAGATE_START_FAILURE); + else if (t == JOB_STOP) + job_fail_dependencies(u, UNIT_ATOM_PROPAGATE_STOP_FAILURE); } /* A special check to make sure we take down anything RequisiteOf if we @@ -1041,14 +1035,13 @@ int job_finish_and_invalidate(Job *j, JobResult result, bool recursive, bool alr * not happen (the verify-active job may trigger after it finishes), so * you get undeterministic results without this check. */ - if (result == JOB_DONE && recursive && !UNIT_IS_ACTIVE_OR_RELOADING(unit_active_state(u))) { - if (IN_SET(t, JOB_START, JOB_RELOAD)) - job_fail_dependencies(u, UNIT_REQUISITE_OF); - } - /* Trigger OnFailure dependencies that are not generated by - * the unit itself. We don't treat JOB_CANCELED as failure in - * this context. And JOB_FAILURE is already handled by the - * unit itself. */ + if (result == JOB_DONE && recursive && + IN_SET(t, JOB_START, JOB_RELOAD) && + !UNIT_IS_ACTIVE_OR_RELOADING(unit_active_state(u))) + job_fail_dependencies(u, UNIT_ATOM_PROPAGATE_INACTIVE_START_AS_FAILURE); + + /* Trigger OnFailure= dependencies that are not generated by the unit itself. We don't treat + * JOB_CANCELED as failure in this context. And JOB_FAILURE is already handled by the unit itself. */ if (IN_SET(result, JOB_TIMEOUT, JOB_DEPENDENCY)) { log_unit_struct(u, LOG_NOTICE, "JOB_TYPE=%s", job_type_to_string(t), @@ -1065,12 +1058,12 @@ int job_finish_and_invalidate(Job *j, JobResult result, bool recursive, bool alr finish: /* Try to start the next jobs that can be started */ - HASHMAP_FOREACH_KEY(v, other, u->dependencies[UNIT_AFTER]) + UNIT_FOREACH_DEPENDENCY(other, u, UNIT_ATOM_AFTER) if (other->job) { job_add_to_run_queue(other->job); job_add_to_gc_queue(other->job); } - HASHMAP_FOREACH_KEY(v, other, u->dependencies[UNIT_BEFORE]) + UNIT_FOREACH_DEPENDENCY(other, u, UNIT_ATOM_BEFORE) if (other->job) { job_add_to_run_queue(other->job); job_add_to_gc_queue(other->job); @@ -1420,7 +1413,6 @@ int job_get_timeout(Job *j, usec_t *timeout) { bool job_may_gc(Job *j) { Unit *other; - void *v; assert(j); @@ -1449,12 +1441,12 @@ bool job_may_gc(Job *j) { return false; /* The logic is inverse to job_is_runnable, we cannot GC as long as we block any job. */ - HASHMAP_FOREACH_KEY(v, other, j->unit->dependencies[UNIT_BEFORE]) - if (other->job && job_compare(j, other->job, UNIT_BEFORE) < 0) + UNIT_FOREACH_DEPENDENCY(other, j->unit, UNIT_ATOM_BEFORE) + if (other->job && job_compare(j, other->job, UNIT_ATOM_BEFORE) < 0) return false; - HASHMAP_FOREACH_KEY(v, other, j->unit->dependencies[UNIT_AFTER]) - if (other->job && job_compare(j, other->job, UNIT_AFTER) < 0) + UNIT_FOREACH_DEPENDENCY(other, j->unit, UNIT_ATOM_AFTER) + if (other->job && job_compare(j, other->job, UNIT_ATOM_AFTER) < 0) return false; return true; @@ -1500,7 +1492,6 @@ int job_get_before(Job *j, Job*** ret) { _cleanup_free_ Job** list = NULL; Unit *other = NULL; size_t n = 0; - void *v; /* Returns a list of all pending jobs that need to finish before this job may be started. */ @@ -1512,10 +1503,10 @@ int job_get_before(Job *j, Job*** ret) { return 0; } - HASHMAP_FOREACH_KEY(v, other, j->unit->dependencies[UNIT_AFTER]) { + UNIT_FOREACH_DEPENDENCY(other, j->unit, UNIT_ATOM_AFTER) { if (!other->job) continue; - if (job_compare(j, other->job, UNIT_AFTER) <= 0) + if (job_compare(j, other->job, UNIT_ATOM_AFTER) <= 0) continue; if (!GREEDY_REALLOC(list, n+1)) @@ -1523,10 +1514,10 @@ int job_get_before(Job *j, Job*** ret) { list[n++] = other->job; } - HASHMAP_FOREACH_KEY(v, other, j->unit->dependencies[UNIT_BEFORE]) { + UNIT_FOREACH_DEPENDENCY(other, j->unit, UNIT_ATOM_BEFORE) { if (!other->job) continue; - if (job_compare(j, other->job, UNIT_BEFORE) <= 0) + if (job_compare(j, other->job, UNIT_ATOM_BEFORE) <= 0) continue; if (!GREEDY_REALLOC(list, n+1)) @@ -1545,21 +1536,20 @@ int job_get_after(Job *j, Job*** ret) { _cleanup_free_ Job** list = NULL; Unit *other = NULL; size_t n = 0; - void *v; assert(j); assert(ret); /* Returns a list of all pending jobs that are waiting for this job to finish. */ - HASHMAP_FOREACH_KEY(v, other, j->unit->dependencies[UNIT_BEFORE]) { + UNIT_FOREACH_DEPENDENCY(other, j->unit, UNIT_ATOM_BEFORE) { if (!other->job) continue; if (other->job->ignore_order) continue; - if (job_compare(j, other->job, UNIT_BEFORE) >= 0) + if (job_compare(j, other->job, UNIT_ATOM_BEFORE) >= 0) continue; if (!GREEDY_REALLOC(list, n+1)) @@ -1567,14 +1557,14 @@ int job_get_after(Job *j, Job*** ret) { list[n++] = other->job; } - HASHMAP_FOREACH_KEY(v, other, j->unit->dependencies[UNIT_AFTER]) { + UNIT_FOREACH_DEPENDENCY(other, j->unit, UNIT_ATOM_AFTER) { if (!other->job) continue; if (other->job->ignore_order) continue; - if (job_compare(j, other->job, UNIT_AFTER) >= 0) + if (job_compare(j, other->job, UNIT_ATOM_AFTER) >= 0) continue; if (!GREEDY_REALLOC(list, n+1)) @@ -1669,10 +1659,12 @@ const char* job_type_to_access_method(JobType t) { * This has the side effect that restarts are properly * synchronized too. */ -int job_compare(Job *a, Job *b, UnitDependency assume_dep) { +int job_compare(Job *a, Job *b, UnitDependencyAtom assume_dep) { + assert(a); + assert(b); assert(a->type < _JOB_TYPE_MAX_IN_TRANSACTION); assert(b->type < _JOB_TYPE_MAX_IN_TRANSACTION); - assert(IN_SET(assume_dep, UNIT_AFTER, UNIT_BEFORE)); + assert(IN_SET(assume_dep, UNIT_ATOM_AFTER, UNIT_ATOM_BEFORE)); /* Trivial cases first */ if (a->type == JOB_NOP || b->type == JOB_NOP) @@ -1681,8 +1673,8 @@ int job_compare(Job *a, Job *b, UnitDependency assume_dep) { if (a->ignore_order || b->ignore_order) return 0; - if (assume_dep == UNIT_AFTER) - return -job_compare(b, a, UNIT_BEFORE); + if (assume_dep == UNIT_ATOM_AFTER) + return -job_compare(b, a, UNIT_ATOM_BEFORE); /* Let's make it simple, JOB_STOP goes always first (in case both ua and ub stop, * then ub's stop goes first anyway). diff --git a/src/core/job.h b/src/core/job.h index 64ea847ff9..c033c8a4fa 100644 --- a/src/core/job.h +++ b/src/core/job.h @@ -6,7 +6,9 @@ #include "sd-event.h" #include "list.h" +#include "unit-dependency-atom.h" #include "unit-name.h" +#include "unit.h" typedef struct Job Job; typedef struct JobDependency JobDependency; @@ -240,4 +242,4 @@ JobResult job_result_from_string(const char *s) _pure_; const char* job_type_to_access_method(JobType t); -int job_compare(Job *a, Job *b, UnitDependency assume_dep); +int job_compare(Job *a, Job *b, UnitDependencyAtom assume_dep); diff --git a/src/core/manager.c b/src/core/manager.c index 30aadb0944..a9b51bb25d 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -1141,12 +1141,11 @@ enum { static void unit_gc_mark_good(Unit *u, unsigned gc_marker) { Unit *other; - void *v; u->gc_marker = gc_marker + GC_OFFSET_GOOD; /* Recursively mark referenced units as GOOD as well */ - HASHMAP_FOREACH_KEY(v, other, u->dependencies[UNIT_REFERENCES]) + UNIT_FOREACH_DEPENDENCY(other, u, UNIT_ATOM_REFERENCES) if (other->gc_marker == gc_marker + GC_OFFSET_UNSURE) unit_gc_mark_good(other, gc_marker); } @@ -1154,7 +1153,6 @@ static void unit_gc_mark_good(Unit *u, unsigned gc_marker) { static void unit_gc_sweep(Unit *u, unsigned gc_marker) { Unit *other; bool is_bad; - void *v; assert(u); @@ -1172,7 +1170,7 @@ static void unit_gc_sweep(Unit *u, unsigned gc_marker) { is_bad = true; - HASHMAP_FOREACH_KEY(v, other, u->dependencies[UNIT_REFERENCED_BY]) { + UNIT_FOREACH_DEPENDENCY(other, u, UNIT_ATOM_REFERENCED_BY) { unit_gc_sweep(other, gc_marker); if (other->gc_marker == gc_marker + GC_OFFSET_GOOD) @@ -1870,30 +1868,28 @@ static int manager_dispatch_target_deps_queue(Manager *m) { Unit *u; int r = 0; - static const UnitDependency deps[] = { - UNIT_REQUIRED_BY, - UNIT_REQUISITE_OF, - UNIT_WANTED_BY, - UNIT_BOUND_BY - }; - assert(m); while ((u = m->target_deps_queue)) { + _cleanup_free_ Unit **targets = NULL; + int n_targets; + assert(u->in_target_deps_queue); LIST_REMOVE(target_deps_queue, u->manager->target_deps_queue, u); u->in_target_deps_queue = false; - for (size_t k = 0; k < ELEMENTSOF(deps); k++) { - Unit *target; - void *v; + /* Take an "atomic" snapshot of dependencies here, as the call below will likely modify the + * dependencies, and we can't have it that hash tables we iterate through are modified while + * we are iterating through them. */ + n_targets = unit_get_dependency_array(u, UNIT_ATOM_DEFAULT_TARGET_DEPENDENCIES, &targets); + if (n_targets < 0) + return n_targets; - HASHMAP_FOREACH_KEY(v, target, u->dependencies[deps[k]]) { - r = unit_add_default_target_dependency(u, target); - if (r < 0) - return r; - } + for (int i = 0; i < n_targets; i++) { + r = unit_add_default_target_dependency(u, targets[i]); + if (r < 0) + return r; } } diff --git a/src/core/meson.build b/src/core/meson.build index e93d17a43a..46c69c6138 100644 --- a/src/core/meson.build +++ b/src/core/meson.build @@ -117,6 +117,8 @@ libcore_sources = ''' timer.h transaction.c transaction.h + unit-dependency-atom.c + unit-dependency-atom.h unit-printf.c unit-printf.h unit-serialize.c diff --git a/src/core/service.c b/src/core/service.c index ab3a476bcd..baad85e8b6 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -1245,12 +1245,11 @@ static int service_collect_fds( rn_socket_fds = 1; } else { - void *v; Unit *u; /* Pass all our configured sockets for singleton services */ - HASHMAP_FOREACH_KEY(v, u, UNIT(s)->dependencies[UNIT_TRIGGERED_BY]) { + UNIT_FOREACH_DEPENDENCY(u, UNIT(s), UNIT_ATOM_TRIGGERED_BY) { _cleanup_free_ int *cfds = NULL; Socket *sock; int cn_fds; diff --git a/src/core/slice.c b/src/core/slice.c index 94eb56e1ca..cb94c97b24 100644 --- a/src/core/slice.c +++ b/src/core/slice.c @@ -346,11 +346,10 @@ static void slice_enumerate_perpetual(Manager *m) { static bool slice_freezer_action_supported_by_children(Unit *s) { Unit *member; - void *v; assert(s); - HASHMAP_FOREACH_KEY(v, member, s->dependencies[UNIT_BEFORE]) { + UNIT_FOREACH_DEPENDENCY(member, s, UNIT_ATOM_BEFORE) { int r; if (UNIT_DEREF(member->slice) != s) @@ -371,7 +370,6 @@ static bool slice_freezer_action_supported_by_children(Unit *s) { static int slice_freezer_action(Unit *s, FreezerAction action) { Unit *member; - void *v; int r; assert(s); @@ -382,7 +380,7 @@ static int slice_freezer_action(Unit *s, FreezerAction action) { return 0; } - HASHMAP_FOREACH_KEY(v, member, s->dependencies[UNIT_BEFORE]) { + UNIT_FOREACH_DEPENDENCY(member, s, UNIT_ATOM_BEFORE) { if (UNIT_DEREF(member->slice) != s) continue; @@ -390,7 +388,6 @@ static int slice_freezer_action(Unit *s, FreezerAction action) { r = UNIT_VTABLE(member)->freeze(member); else r = UNIT_VTABLE(member)->thaw(member); - if (r < 0) return r; } diff --git a/src/core/socket.c b/src/core/socket.c index 016986401b..8be01aa8e1 100644 --- a/src/core/socket.c +++ b/src/core/socket.c @@ -2340,11 +2340,9 @@ static void socket_enter_running(Socket *s, int cfd_in) { if (cfd < 0) { bool pending = false; Unit *other; - void *v; - /* If there's already a start pending don't bother to - * do anything */ - HASHMAP_FOREACH_KEY(v, other, UNIT(s)->dependencies[UNIT_TRIGGERS]) + /* If there's already a start pending don't bother to do anything */ + UNIT_FOREACH_DEPENDENCY(other, UNIT(s), UNIT_ATOM_TRIGGERS) if (unit_active_or_pending(other)) { pending = true; break; diff --git a/src/core/target.c b/src/core/target.c index 5755f26615..99a9374e1f 100644 --- a/src/core/target.c +++ b/src/core/target.c @@ -35,34 +35,31 @@ static void target_set_state(Target *t, TargetState state) { } static int target_add_default_dependencies(Target *t) { - - static const UnitDependency deps[] = { - UNIT_REQUIRES, - UNIT_REQUISITE, - UNIT_WANTS, - UNIT_BINDS_TO, - UNIT_PART_OF - }; - - int r; + _cleanup_free_ Unit **others = NULL; + int r, n_others; assert(t); if (!UNIT(t)->default_dependencies) return 0; - /* Imply ordering for requirement dependencies on target units. Note that when the user created a contradicting - * ordering manually we won't add anything in here to make sure we don't create a loop. */ + /* Imply ordering for requirement dependencies on target units. Note that when the user created a + * contradicting ordering manually we won't add anything in here to make sure we don't create a + * loop. + * + * Note that quite likely iterating through these dependencies will add new dependencies, which + * conflicts with the hashmap-based iteration logic. Hence, instead of iterating through the + * dependencies and acting on them as we go, first take an "atomic snapshot" of sorts and iterate + * through that. */ - for (size_t k = 0; k < ELEMENTSOF(deps); k++) { - Unit *other; - void *v; + n_others = unit_get_dependency_array(UNIT(t), UNIT_ATOM_ADD_DEFAULT_TARGET_DEPENDENCY_QUEUE, &others); + if (n_others < 0) + return n_others; - HASHMAP_FOREACH_KEY(v, other, UNIT(t)->dependencies[deps[k]]) { - r = unit_add_default_target_dependency(other, UNIT(t)); - if (r < 0) - return r; - } + for (int i = 0; i < n_others; i++) { + r = unit_add_default_target_dependency(others[i], UNIT(t)); + if (r < 0) + return r; } if (unit_has_name(UNIT(t), SPECIAL_SHUTDOWN_TARGET)) diff --git a/src/core/transaction.c b/src/core/transaction.c index 4a55c075b4..8e1809302f 100644 --- a/src/core/transaction.c +++ b/src/core/transaction.c @@ -347,14 +347,13 @@ static char* merge_unit_ids(const char* unit_log_field, char **pairs) { } static int transaction_verify_order_one(Transaction *tr, Job *j, Job *from, unsigned generation, sd_bus_error *e) { - Unit *u; - void *v; - int r; - static const UnitDependency directions[] = { - UNIT_BEFORE, - UNIT_AFTER, + + static const UnitDependencyAtom directions[] = { + UNIT_ATOM_BEFORE, + UNIT_ATOM_AFTER, }; - size_t d; + + int r; assert(tr); assert(j); @@ -449,8 +448,10 @@ static int transaction_verify_order_one(Transaction *tr, Job *j, Job *from, unsi * the graph over 'before' edges in the actual job execution order. We traverse over both unit * ordering dependencies and we test with job_compare() whether it is the 'before' edge in the job * execution ordering. */ - for (d = 0; d < ELEMENTSOF(directions); d++) { - HASHMAP_FOREACH_KEY(v, u, j->unit->dependencies[directions[d]]) { + for (size_t d = 0; d < ELEMENTSOF(directions); d++) { + Unit *u; + + UNIT_FOREACH_DEPENDENCY(u, j->unit, directions[d]) { Job *o; /* Is there a job for this unit? */ @@ -879,13 +880,12 @@ static void transaction_unlink_job(Transaction *tr, Job *j, bool delete_dependen void transaction_add_propagate_reload_jobs(Transaction *tr, Unit *unit, Job *by, bool ignore_order, sd_bus_error *e) { JobType nt; Unit *dep; - void *v; int r; assert(tr); assert(unit); - HASHMAP_FOREACH_KEY(v, dep, unit->dependencies[UNIT_PROPAGATES_RELOAD_TO]) { + UNIT_FOREACH_DEPENDENCY(dep, unit, UNIT_ATOM_PROPAGATES_RELOAD_TO) { nt = job_type_collapse(JOB_TRY_RELOAD, dep); if (nt == JOB_NOP) continue; @@ -914,7 +914,6 @@ int transaction_add_job_and_dependencies( bool is_new; Unit *dep; Job *ret; - void *v; int r; assert(tr); @@ -1006,7 +1005,7 @@ int transaction_add_job_and_dependencies( /* Finally, recursively add in all dependencies. */ if (IN_SET(type, JOB_START, JOB_RESTART)) { - HASHMAP_FOREACH_KEY(v, dep, ret->unit->dependencies[UNIT_REQUIRES]) { + UNIT_FOREACH_DEPENDENCY(dep, ret->unit, UNIT_ATOM_PULL_IN_START) { r = transaction_add_job_and_dependencies(tr, JOB_START, dep, ret, true, false, false, ignore_order, e); if (r < 0) { if (r != -EBADR) /* job type not applicable */ @@ -1016,17 +1015,7 @@ int transaction_add_job_and_dependencies( } } - HASHMAP_FOREACH_KEY(v, dep, ret->unit->dependencies[UNIT_BINDS_TO]) { - r = transaction_add_job_and_dependencies(tr, JOB_START, dep, ret, true, false, false, ignore_order, e); - if (r < 0) { - if (r != -EBADR) /* job type not applicable */ - goto fail; - - sd_bus_error_free(e); - } - } - - HASHMAP_FOREACH_KEY(v, dep, ret->unit->dependencies[UNIT_WANTS]) { + UNIT_FOREACH_DEPENDENCY(dep, ret->unit, UNIT_ATOM_PULL_IN_START_IGNORED) { r = transaction_add_job_and_dependencies(tr, JOB_START, dep, ret, false, false, false, ignore_order, e); if (r < 0) { /* unit masked, job type not applicable and unit not found are not considered as errors. */ @@ -1038,7 +1027,7 @@ int transaction_add_job_and_dependencies( } } - HASHMAP_FOREACH_KEY(v, dep, ret->unit->dependencies[UNIT_REQUISITE]) { + UNIT_FOREACH_DEPENDENCY(dep, ret->unit, UNIT_ATOM_PULL_IN_VERIFY) { r = transaction_add_job_and_dependencies(tr, JOB_VERIFY_ACTIVE, dep, ret, true, false, false, ignore_order, e); if (r < 0) { if (r != -EBADR) /* job type not applicable */ @@ -1048,7 +1037,7 @@ int transaction_add_job_and_dependencies( } } - HASHMAP_FOREACH_KEY(v, dep, ret->unit->dependencies[UNIT_CONFLICTS]) { + UNIT_FOREACH_DEPENDENCY(dep, ret->unit, UNIT_ATOM_PULL_IN_STOP) { r = transaction_add_job_and_dependencies(tr, JOB_STOP, dep, ret, true, true, false, ignore_order, e); if (r < 0) { if (r != -EBADR) /* job type not applicable */ @@ -1058,7 +1047,7 @@ int transaction_add_job_and_dependencies( } } - HASHMAP_FOREACH_KEY(v, dep, ret->unit->dependencies[UNIT_CONFLICTED_BY]) { + UNIT_FOREACH_DEPENDENCY(dep, ret->unit, UNIT_ATOM_PULL_IN_STOP_IGNORED) { r = transaction_add_job_and_dependencies(tr, JOB_STOP, dep, ret, false, false, false, ignore_order, e); if (r < 0) { log_unit_warning(dep, @@ -1067,41 +1056,37 @@ int transaction_add_job_and_dependencies( sd_bus_error_free(e); } } - } if (IN_SET(type, JOB_STOP, JOB_RESTART)) { - static const UnitDependency propagate_deps[] = { - UNIT_REQUIRED_BY, - UNIT_REQUISITE_OF, - UNIT_BOUND_BY, - UNIT_CONSISTS_OF, - }; - + UnitDependencyAtom atom; JobType ptype; - unsigned j; - /* We propagate STOP as STOP, but RESTART only - * as TRY_RESTART, in order not to start + /* We propagate STOP as STOP, but RESTART only as TRY_RESTART, in order not to start * dependencies that are not around. */ - ptype = type == JOB_RESTART ? JOB_TRY_RESTART : type; + if (type == JOB_RESTART) { + atom = UNIT_ATOM_PROPAGATE_RESTART; + ptype = JOB_TRY_RESTART; + } else { + ptype = JOB_STOP; + atom = UNIT_ATOM_PROPAGATE_STOP; + } - for (j = 0; j < ELEMENTSOF(propagate_deps); j++) - HASHMAP_FOREACH_KEY(v, dep, ret->unit->dependencies[propagate_deps[j]]) { - JobType nt; + UNIT_FOREACH_DEPENDENCY(dep, ret->unit, atom) { + JobType nt; - nt = job_type_collapse(ptype, dep); - if (nt == JOB_NOP) - continue; + nt = job_type_collapse(ptype, dep); + if (nt == JOB_NOP) + continue; - r = transaction_add_job_and_dependencies(tr, nt, dep, ret, true, false, false, ignore_order, e); - if (r < 0) { - if (r != -EBADR) /* job type not applicable */ - goto fail; + r = transaction_add_job_and_dependencies(tr, nt, dep, ret, true, false, false, ignore_order, e); + if (r < 0) { + if (r != -EBADR) /* job type not applicable */ + goto fail; - sd_bus_error_free(e); - } + sd_bus_error_free(e); } + } } if (type == JOB_RELOAD) @@ -1150,14 +1135,14 @@ int transaction_add_isolate_jobs(Transaction *tr, Manager *m) { } int transaction_add_triggering_jobs(Transaction *tr, Unit *u) { - void *v; Unit *trigger; int r; assert(tr); assert(u); - HASHMAP_FOREACH_KEY(v, trigger, u->dependencies[UNIT_TRIGGERED_BY]) { + UNIT_FOREACH_DEPENDENCY(trigger, u, UNIT_ATOM_TRIGGERED_BY) { + /* No need to stop inactive jobs */ if (UNIT_IS_INACTIVE_OR_FAILED(unit_active_state(trigger)) && !trigger->job) continue; diff --git a/src/core/unit-dependency-atom.c b/src/core/unit-dependency-atom.c new file mode 100644 index 0000000000..4b012ed265 --- /dev/null +++ b/src/core/unit-dependency-atom.c @@ -0,0 +1,196 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ + +#include "unit-dependency-atom.h" + +static const UnitDependencyAtom atom_map[_UNIT_DEPENDENCY_MAX] = { + /* A table that maps high-level dependency types to low-level dependency "atoms". The latter actually + * describe specific facets of dependency behaviour. The former combine them into one user-facing + * concept. Atoms are a bit mask, though a bunch of dependency types have only a single bit set. + * + * Typically when the user configures a dependency they go via dependency type, but when we act on + * them we go by atom. + * + * NB: when you add a new dependency type here, make sure to also add one to the (best-effort) + * reverse table in unit_dependency_from_unique_atom() further down. */ + + [UNIT_REQUIRES] = UNIT_ATOM_PULL_IN_START | + UNIT_ATOM_RETROACTIVE_START_REPLACE | + UNIT_ATOM_ADD_STOP_WHEN_UNNEEDED_QUEUE | + UNIT_ATOM_ADD_DEFAULT_TARGET_DEPENDENCY_QUEUE, + + [UNIT_REQUISITE] = UNIT_ATOM_PULL_IN_VERIFY | + UNIT_ATOM_ADD_STOP_WHEN_UNNEEDED_QUEUE | + UNIT_ATOM_ADD_DEFAULT_TARGET_DEPENDENCY_QUEUE, + + [UNIT_WANTS] = UNIT_ATOM_PULL_IN_START_IGNORED | + UNIT_ATOM_RETROACTIVE_START_FAIL | + UNIT_ATOM_ADD_STOP_WHEN_UNNEEDED_QUEUE | + UNIT_ATOM_ADD_DEFAULT_TARGET_DEPENDENCY_QUEUE, + + [UNIT_BINDS_TO] = UNIT_ATOM_PULL_IN_START | + UNIT_ATOM_RETROACTIVE_START_REPLACE | + UNIT_ATOM_CANNOT_BE_ACTIVE_WITHOUT | + UNIT_ATOM_ADD_STOP_WHEN_UNNEEDED_QUEUE | + UNIT_ATOM_ADD_DEFAULT_TARGET_DEPENDENCY_QUEUE, + + [UNIT_PART_OF] = UNIT_ATOM_ADD_DEFAULT_TARGET_DEPENDENCY_QUEUE, + + [UNIT_REQUIRED_BY] = UNIT_ATOM_PROPAGATE_STOP | + UNIT_ATOM_PROPAGATE_RESTART | + UNIT_ATOM_PROPAGATE_START_FAILURE | + UNIT_ATOM_PINS_STOP_WHEN_UNNEEDED | + UNIT_ATOM_DEFAULT_TARGET_DEPENDENCIES, + + [UNIT_REQUISITE_OF] = UNIT_ATOM_PROPAGATE_STOP | + UNIT_ATOM_PROPAGATE_RESTART | + UNIT_ATOM_PROPAGATE_START_FAILURE | + UNIT_ATOM_PROPAGATE_INACTIVE_START_AS_FAILURE | + UNIT_ATOM_PINS_STOP_WHEN_UNNEEDED | + UNIT_ATOM_DEFAULT_TARGET_DEPENDENCIES, + + [UNIT_WANTED_BY] = UNIT_ATOM_DEFAULT_TARGET_DEPENDENCIES | + UNIT_ATOM_PINS_STOP_WHEN_UNNEEDED, + + [UNIT_BOUND_BY] = UNIT_ATOM_RETROACTIVE_STOP_ON_STOP | + UNIT_ATOM_PROPAGATE_STOP | + UNIT_ATOM_PROPAGATE_RESTART | + UNIT_ATOM_PROPAGATE_START_FAILURE | + UNIT_ATOM_PINS_STOP_WHEN_UNNEEDED | + UNIT_ATOM_DEFAULT_TARGET_DEPENDENCIES, + + [UNIT_CONSISTS_OF] = UNIT_ATOM_PROPAGATE_STOP | + UNIT_ATOM_PROPAGATE_RESTART, + + [UNIT_CONFLICTS] = UNIT_ATOM_PULL_IN_STOP | + UNIT_ATOM_RETROACTIVE_STOP_ON_START, + + [UNIT_CONFLICTED_BY] = UNIT_ATOM_PULL_IN_STOP_IGNORED | + UNIT_ATOM_RETROACTIVE_STOP_ON_START | + UNIT_ATOM_PROPAGATE_STOP_FAILURE, + + /* These are simple dependency types: they consist of a single atom only */ + [UNIT_BEFORE] = UNIT_ATOM_BEFORE, + [UNIT_AFTER] = UNIT_ATOM_AFTER, + [UNIT_ON_FAILURE] = UNIT_ATOM_ON_FAILURE, + [UNIT_TRIGGERS] = UNIT_ATOM_TRIGGERS, + [UNIT_TRIGGERED_BY] = UNIT_ATOM_TRIGGERED_BY, + [UNIT_PROPAGATES_RELOAD_TO] = UNIT_ATOM_PROPAGATES_RELOAD_TO, + [UNIT_JOINS_NAMESPACE_OF] = UNIT_ATOM_JOINS_NAMESPACE_OF, + [UNIT_REFERENCES] = UNIT_ATOM_REFERENCES, + [UNIT_REFERENCED_BY] = UNIT_ATOM_REFERENCED_BY, + + /* These are dependency types without effect on our state engine. We maintain them only to make + * things discoverable/debuggable as they are the inverse dependencies to some of the above. As they + * have no effect of their own, they all map to no atoms at all, i.e. the value 0. */ + [UNIT_RELOAD_PROPAGATED_FROM] = 0, +}; + +UnitDependencyAtom unit_dependency_to_atom(UnitDependency d) { + if (d < 0) + return _UNIT_DEPENDENCY_ATOM_INVALID; + + assert(d < _UNIT_DEPENDENCY_MAX); + + return atom_map[d]; +} + +UnitDependency unit_dependency_from_unique_atom(UnitDependencyAtom atom) { + + /* This is a "best-effort" function that maps the specified 'atom' mask to a dependency type that is + * is equal to or has a superset of bits set if that's uniquely possible. The idea is that this + * function is used when iterating through deps that have a specific atom: if there's exactly one + * dependency type of the specific atom we don't need iterate through all deps a unit has, but can + * pinpoint things directly. + * + * This function will return _UNIT_DEPENDENCY_INVALID in case the specified value is not known or not + * uniquely defined, i.e. there are multiple dependencies with the atom or the combination set. */ + + switch ((int64_t) atom) { + + /* Note that we can't list UNIT_REQUIRES here since it's a true subset of UNIT_BINDS_TO, and + * hence its atom bits not uniquely mappable. */ + + case UNIT_ATOM_PULL_IN_VERIFY | + UNIT_ATOM_ADD_STOP_WHEN_UNNEEDED_QUEUE | + UNIT_ATOM_ADD_DEFAULT_TARGET_DEPENDENCY_QUEUE: + case UNIT_ATOM_PULL_IN_VERIFY: /* a single dep type uses this atom */ + return UNIT_REQUISITE; + + case UNIT_ATOM_PULL_IN_START_IGNORED | + UNIT_ATOM_RETROACTIVE_START_FAIL | + UNIT_ATOM_ADD_STOP_WHEN_UNNEEDED_QUEUE | + UNIT_ATOM_ADD_DEFAULT_TARGET_DEPENDENCY_QUEUE: + case UNIT_ATOM_RETROACTIVE_START_FAIL: + return UNIT_WANTS; + + case UNIT_ATOM_PULL_IN_START | + UNIT_ATOM_RETROACTIVE_START_REPLACE | + UNIT_ATOM_CANNOT_BE_ACTIVE_WITHOUT | + UNIT_ATOM_ADD_STOP_WHEN_UNNEEDED_QUEUE | + UNIT_ATOM_ADD_DEFAULT_TARGET_DEPENDENCY_QUEUE: + case UNIT_ATOM_CANNOT_BE_ACTIVE_WITHOUT: + return UNIT_BINDS_TO; + + case UNIT_ATOM_PROPAGATE_STOP | + UNIT_ATOM_PROPAGATE_RESTART | + UNIT_ATOM_PROPAGATE_START_FAILURE | + UNIT_ATOM_PROPAGATE_INACTIVE_START_AS_FAILURE | + UNIT_ATOM_PINS_STOP_WHEN_UNNEEDED | + UNIT_ATOM_DEFAULT_TARGET_DEPENDENCIES: + case UNIT_ATOM_PROPAGATE_INACTIVE_START_AS_FAILURE: + return UNIT_REQUISITE_OF; + + case UNIT_ATOM_RETROACTIVE_STOP_ON_STOP | + UNIT_ATOM_PROPAGATE_STOP | + UNIT_ATOM_PROPAGATE_RESTART | + UNIT_ATOM_PROPAGATE_START_FAILURE | + UNIT_ATOM_PINS_STOP_WHEN_UNNEEDED | + UNIT_ATOM_DEFAULT_TARGET_DEPENDENCIES: + case UNIT_ATOM_RETROACTIVE_STOP_ON_STOP: + return UNIT_BOUND_BY; + + case UNIT_ATOM_PULL_IN_STOP | + UNIT_ATOM_RETROACTIVE_STOP_ON_START: + case UNIT_ATOM_PULL_IN_STOP: + return UNIT_CONFLICTS; + + case UNIT_ATOM_PULL_IN_STOP_IGNORED | + UNIT_ATOM_RETROACTIVE_STOP_ON_START | + UNIT_ATOM_PROPAGATE_STOP_FAILURE: + case UNIT_ATOM_PULL_IN_STOP_IGNORED: + case UNIT_ATOM_PROPAGATE_STOP_FAILURE: + return UNIT_CONFLICTED_BY; + + /* And now, the simple ones */ + + case UNIT_ATOM_BEFORE: + return UNIT_BEFORE; + + case UNIT_ATOM_AFTER: + return UNIT_AFTER; + + case UNIT_ATOM_ON_FAILURE: + return UNIT_ON_FAILURE; + + case UNIT_ATOM_TRIGGERS: + return UNIT_TRIGGERS; + + case UNIT_ATOM_TRIGGERED_BY: + return UNIT_TRIGGERED_BY; + + case UNIT_ATOM_PROPAGATES_RELOAD_TO: + return UNIT_PROPAGATES_RELOAD_TO; + + case UNIT_ATOM_JOINS_NAMESPACE_OF: + return UNIT_JOINS_NAMESPACE_OF; + + case UNIT_ATOM_REFERENCES: + return UNIT_REFERENCES; + + case UNIT_ATOM_REFERENCED_BY: + return UNIT_REFERENCED_BY; + + default: + return _UNIT_DEPENDENCY_INVALID; + } +} diff --git a/src/core/unit-dependency-atom.h b/src/core/unit-dependency-atom.h new file mode 100644 index 0000000000..d04d6134a0 --- /dev/null +++ b/src/core/unit-dependency-atom.h @@ -0,0 +1,77 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +#pragma once + +#include + +#include "unit-def.h" + +/* Flags that identify the various "atomic" behaviours a specific dependency type implies. Each dependency is + * a combination of one or more of these flags that define what they actually entail. */ +typedef enum UnitDependencyAtom { + + /* This unit pulls in the other unit as JOB_START job into the transaction, and if that doesn't work + * the transaction fails. */ + UNIT_ATOM_PULL_IN_START = UINT64_C(1) << 0, + /* Similar, but if it doesn't work, ignore. */ + UNIT_ATOM_PULL_IN_START_IGNORED = UINT64_C(1) << 1, + /* Pull in a JOB_VERIFY job into the transaction, i.e. pull in JOB_VERIFY rather than + * JOB_START. i.e. check the unit is started but don't pull it in. */ + UNIT_ATOM_PULL_IN_VERIFY = UINT64_C(1) << 2, + + /* Pull in a JOB_STOP job for the other job into transactions, and fail if that doesn't work. */ + UNIT_ATOM_PULL_IN_STOP = UINT64_C(1) << 3, + /* Same, but don't fail, ignore it. */ + UNIT_ATOM_PULL_IN_STOP_IGNORED = UINT64_C(1) << 4, + + /* If our enters inactive state, add the other unit to the StopWhenUneeded= queue */ + UNIT_ATOM_ADD_STOP_WHEN_UNNEEDED_QUEUE = UINT64_C(1) << 5, + /* Pin the other unit i.e. ensure StopWhenUneeded= won't trigger for the other unit as long as we are + * not in inactive state */ + UNIT_ATOM_PINS_STOP_WHEN_UNNEEDED = UINT64_C(1) << 6, + + /* Stop our unit if the other unit happens to inactive */ + UNIT_ATOM_CANNOT_BE_ACTIVE_WITHOUT = UINT64_C(1) << 7, + + /* If our unit unexpectedly becomes active, retroactively start the other unit too, in "replace" job + * mode */ + UNIT_ATOM_RETROACTIVE_START_REPLACE = UINT64_C(1) << 11, + /* Similar, but in "fail" job mode */ + UNIT_ATOM_RETROACTIVE_START_FAIL = UINT64_C(1) << 12, + /* If our unit unexpectedly becomes active, retroactively stop the other unit too */ + UNIT_ATOM_RETROACTIVE_STOP_ON_START = UINT64_C(1) << 13, + /* If our unit unexpectedly becomes inactive, retroactively stop the other unit too */ + UNIT_ATOM_RETROACTIVE_STOP_ON_STOP = UINT64_C(1) << 14, + + /* If a start job for this unit fails, propagate the failure to start job of other unit too */ + UNIT_ATOM_PROPAGATE_START_FAILURE = UINT64_C(1) << 15, + /* If a stop job for this unit fails, propagate the failure to any stop job of the other unit too */ + UNIT_ATOM_PROPAGATE_STOP_FAILURE = UINT64_C(1) << 16, + /* If our start job succeeded but the unit is inactive then (think: oneshot units), propagate this as + * failure to the other unit. */ + UNIT_ATOM_PROPAGATE_INACTIVE_START_AS_FAILURE = UINT64_C(1) << 17, + /* When putting together a transaction, propagate JOB_STOP from our unit to the other. */ + UNIT_ATOM_PROPAGATE_STOP = UINT64_C(1) << 18, + /* When putting together a transaction, propagate JOB_RESTART from our unit to the other. */ + UNIT_ATOM_PROPAGATE_RESTART = UINT64_C(1) << 19, + + /* Add the other unit to the default target dependency queue */ + UNIT_ATOM_ADD_DEFAULT_TARGET_DEPENDENCY_QUEUE = UINT64_C(1) << 20, + /* Recheck default target deps on other units (which are target units) */ + UNIT_ATOM_DEFAULT_TARGET_DEPENDENCIES = UINT64_C(1) << 21, + + /* The remaining atoms map 1:1 to the equally named high-level deps */ + UNIT_ATOM_BEFORE = UINT64_C(1) << 22, + UNIT_ATOM_AFTER = UINT64_C(1) << 23, + UNIT_ATOM_ON_FAILURE = UINT64_C(1) << 25, + UNIT_ATOM_TRIGGERS = UINT64_C(1) << 26, + UNIT_ATOM_TRIGGERED_BY = UINT64_C(1) << 27, + UNIT_ATOM_PROPAGATES_RELOAD_TO = UINT64_C(1) << 28, + UNIT_ATOM_JOINS_NAMESPACE_OF = UINT64_C(1) << 29, + UNIT_ATOM_REFERENCES = UINT64_C(1) << 30, + UNIT_ATOM_REFERENCED_BY = UINT64_C(1) << 31, + _UNIT_DEPENDENCY_ATOM_MAX = (UINT64_C(1) << 32) - 1, + _UNIT_DEPENDENCY_ATOM_INVALID = -EINVAL, +} UnitDependencyAtom; + +UnitDependencyAtom unit_dependency_to_atom(UnitDependency d); +UnitDependency unit_dependency_from_unique_atom(UnitDependencyAtom atom); diff --git a/src/core/unit-serialize.c b/src/core/unit-serialize.c index 509210ad5e..6384174a2b 100644 --- a/src/core/unit-serialize.c +++ b/src/core/unit-serialize.c @@ -735,7 +735,7 @@ void unit_dump(Unit *u, FILE *f, const char *prefix) { UnitDependencyInfo di; Unit *other; - HASHMAP_FOREACH_KEY(di.data, other, u->dependencies[d]) { + HASHMAP_FOREACH_KEY(di.data, other, unit_get_dependencies(u, d)) { bool space = false; fprintf(f, "%s\t%s: %s (", prefix, unit_dependency_to_string(d), other->id); diff --git a/src/core/unit.c b/src/core/unit.c index fc63f3bee5..e48aa5313b 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -85,8 +85,6 @@ const UnitVTable * const unit_vtable[_UNIT_TYPE_MAX] = { [UNIT_SCOPE] = &scope_vtable, }; -static void maybe_warn_about_dependency(Unit *u, const char *other, UnitDependency dependency); - Unit* unit_new(Manager *m, size_t size) { Unit *u; @@ -508,22 +506,26 @@ void unit_submit_to_stop_when_unneeded_queue(Unit *u) { u->in_stop_when_unneeded_queue = true; } -static void bidi_set_free(Unit *u, Hashmap *h) { - Unit *other; - void *v; - +static void unit_clear_dependencies(Unit *u) { assert(u); - /* Frees the hashmap and makes sure we are dropped from the inverse pointers */ + /* Removes all dependencies configured on u and their reverse dependencies. */ - HASHMAP_FOREACH_KEY(v, other, h) { - for (UnitDependency d = 0; d < _UNIT_DEPENDENCY_MAX; d++) - hashmap_remove(other->dependencies[d], u); + for (Hashmap *deps; (deps = hashmap_steal_first(u->dependencies));) { - unit_add_to_gc_queue(other); + for (Unit *other; (other = hashmap_steal_first_key(deps));) { + Hashmap *other_deps; + + HASHMAP_FOREACH(other_deps, other->dependencies) + hashmap_remove(other_deps, u); + + unit_add_to_gc_queue(other); + } + + hashmap_free(deps); } - hashmap_free(h); + u->dependencies = hashmap_free(u->dependencies); } static void unit_remove_transient(Unit *u) { @@ -656,8 +658,7 @@ Unit* unit_free(Unit *u) { job_free(j); } - for (UnitDependency d = 0; d < _UNIT_DEPENDENCY_MAX; d++) - bidi_set_free(u, u->dependencies[d]); + unit_clear_dependencies(u); /* A unit is being dropped from the tree, make sure our family is realized properly. Do this after we * detach the unit from slice tree in order to eliminate its effect on controller masks. */ @@ -809,22 +810,7 @@ const char* unit_sub_state_to_string(Unit *u) { return UNIT_VTABLE(u)->sub_state_to_string(u); } -static int hashmap_complete_move(Hashmap **s, Hashmap **other) { - assert(s); - assert(other); - - if (!*other) - return 0; - - if (*s) - return hashmap_move(*s, *other); - else - *s = TAKE_PTR(*other); - - return 0; -} - -static int merge_names(Unit *u, Unit *other) { +static int unit_merge_names(Unit *u, Unit *other) { char *name; int r; @@ -850,87 +836,252 @@ static int merge_names(Unit *u, Unit *other) { return 0; } -static int reserve_dependencies(Unit *u, Unit *other, UnitDependency d) { - unsigned n_reserve; - - assert(u); - assert(other); - assert(d < _UNIT_DEPENDENCY_MAX); - - /* - * If u does not have this dependency set allocated, there is no need - * to reserve anything. In that case other's set will be transferred - * as a whole to u by complete_move(). - */ - if (!u->dependencies[d]) - return 0; - - /* merge_dependencies() will skip a u-on-u dependency */ - n_reserve = hashmap_size(other->dependencies[d]) - !!hashmap_get(other->dependencies[d], u); - - return hashmap_reserve(u->dependencies[d], n_reserve); -} - -static void merge_dependencies(Unit *u, Unit *other, const char *other_id, UnitDependency d) { - Unit *back; - void *v; +static int unit_reserve_dependencies(Unit *u, Unit *other) { + size_t n_reserve; + Hashmap* deps; + void *d; int r; - /* Merges all dependencies of type 'd' of the unit 'other' into the deps of the unit 'u' */ + assert(u); + assert(other); + + /* Let's reserve some space in the dependency hashmaps so that later on merging the units cannot + * fail. + * + * First make some room in the per dependency type hashmaps. Using the summed size of both unit's + * hashmaps is an estimate that is likely too high since they probably use some of the same + * types. But it's never too low, and that's all we need. */ + + n_reserve = MIN(hashmap_size(other->dependencies), LESS_BY((size_t) _UNIT_DEPENDENCY_MAX, hashmap_size(u->dependencies))); + if (n_reserve > 0) { + r = hashmap_ensure_allocated(&u->dependencies, NULL); + if (r < 0) + return r; + + r = hashmap_reserve(u->dependencies, n_reserve); + if (r < 0) + return r; + } + + /* Now, enlarge our per dependency type hashmaps by the number of entries in the same hashmap of the + * other unit's dependencies. + * + * NB: If u does not have a dependency set allocated for some dependency type, there is no need to + * reserve anything for. In that case other's set will be transferred as a whole to u by + * complete_move(). */ + + HASHMAP_FOREACH_KEY(deps, d, u->dependencies) { + Hashmap *other_deps; + + other_deps = hashmap_get(other->dependencies, d); + + r = hashmap_reserve(deps, hashmap_size(other_deps)); + if (r < 0) + return r; + } + + return 0; +} + +static void unit_maybe_warn_about_dependency( + Unit *u, + const char *other_id, + UnitDependency dependency) { + + assert(u); + + /* Only warn about some unit types */ + if (!IN_SET(dependency, + UNIT_CONFLICTS, + UNIT_CONFLICTED_BY, + UNIT_BEFORE, + UNIT_AFTER, + UNIT_ON_FAILURE, + UNIT_TRIGGERS, + UNIT_TRIGGERED_BY)) + return; + + if (streq_ptr(u->id, other_id)) + log_unit_warning(u, "Dependency %s=%s dropped", unit_dependency_to_string(dependency), u->id); + else + log_unit_warning(u, "Dependency %s=%s dropped, merged into %s", unit_dependency_to_string(dependency), strna(other_id), u->id); +} + +static int unit_per_dependency_type_hashmap_update( + Hashmap *per_type, + Unit *other, + UnitDependencyMask origin_mask, + UnitDependencyMask destination_mask) { + + UnitDependencyInfo info; + int r; + + assert(other); + assert_cc(sizeof(void*) == sizeof(info)); + + /* Acquire the UnitDependencyInfo entry for the Unit* we are interested in, and update it if it + * exists, or insert it anew if not. */ + + info.data = hashmap_get(per_type, other); + if (info.data) { + /* Entry already exists. Add in our mask. */ + + if (FLAGS_SET(origin_mask, info.origin_mask) && + FLAGS_SET(destination_mask, info.destination_mask)) + return 0; /* NOP */ + + info.origin_mask |= origin_mask; + info.destination_mask |= destination_mask; + + r = hashmap_update(per_type, other, info.data); + } else { + info = (UnitDependencyInfo) { + .origin_mask = origin_mask, + .destination_mask = destination_mask, + }; + + r = hashmap_put(per_type, other, info.data); + } + if (r < 0) + return r; + + + return 1; +} + +static int unit_add_dependency_hashmap( + Hashmap **dependencies, + UnitDependency d, + Unit *other, + UnitDependencyMask origin_mask, + UnitDependencyMask destination_mask) { + + Hashmap *per_type; + int r; + + assert(dependencies); + assert(other); + assert(origin_mask < _UNIT_DEPENDENCY_MASK_FULL); + assert(destination_mask < _UNIT_DEPENDENCY_MASK_FULL); + assert(origin_mask > 0 || destination_mask > 0); + + /* Ensure the top-level dependency hashmap exists that maps UnitDependency → Hashmap(Unit* → + * UnitDependencyInfo) */ + r = hashmap_ensure_allocated(dependencies, NULL); + if (r < 0) + return r; + + /* Acquire the inner hashmap, that maps Unit* → UnitDependencyInfo, for the specified dependency + * type, and if it's missing allocate it and insert it. */ + per_type = hashmap_get(*dependencies, UNIT_DEPENDENCY_TO_PTR(d)); + if (!per_type) { + per_type = hashmap_new(NULL); + if (!per_type) + return -ENOMEM; + + r = hashmap_put(*dependencies, UNIT_DEPENDENCY_TO_PTR(d), per_type); + if (r < 0) { + hashmap_free(per_type); + return r; + } + } + + return unit_per_dependency_type_hashmap_update(per_type, other, origin_mask, destination_mask); +} + +static void unit_merge_dependencies( + Unit *u, + Unit *other) { + + int r; assert(u); assert(other); - assert(d < _UNIT_DEPENDENCY_MAX); - /* Fix backwards pointers. Let's iterate through all dependent units of the other unit. */ - HASHMAP_FOREACH_KEY(v, back, other->dependencies[d]) + if (u == other) + return; + + for (;;) { + _cleanup_(hashmap_freep) Hashmap *other_deps = NULL; + UnitDependencyInfo di_back; + Unit *back; + void *dt; /* Actually of type UnitDependency, except that we don't bother casting it here, + * since the hashmaps all want it as void pointer. */ + + /* Let's focus on one dependency type at a time, that 'other' has defined. */ + other_deps = hashmap_steal_first_key_and_value(other->dependencies, &dt); + if (!other_deps) + break; /* done! */ + + /* Now iterate through all dependencies of this dependency type, of 'other'. We refer to the + * referenced units as 'back'. */ + HASHMAP_FOREACH_KEY(di_back.data, back, other_deps) { + Hashmap *back_deps; + void *back_dt; - /* Let's now iterate through the dependencies of that dependencies of the other units, - * looking for pointers back, and let's fix them up, to instead point to 'u'. */ - for (UnitDependency k = 0; k < _UNIT_DEPENDENCY_MAX; k++) if (back == u) { - /* Do not add dependencies between u and itself. */ - if (hashmap_remove(back->dependencies[k], other)) - maybe_warn_about_dependency(u, other_id, k); - } else { - UnitDependencyInfo di_u, di_other; - - /* Let's drop this dependency between "back" and "other", and let's create it between - * "back" and "u" instead. Let's merge the bit masks of the dependency we are moving, - * and any such dependency which might already exist */ - - di_other.data = hashmap_get(back->dependencies[k], other); - if (!di_other.data) - continue; /* dependency isn't set, let's try the next one */ - - di_u.data = hashmap_get(back->dependencies[k], u); - - UnitDependencyInfo di_merged = { - .origin_mask = di_u.origin_mask | di_other.origin_mask, - .destination_mask = di_u.destination_mask | di_other.destination_mask, - }; - - r = hashmap_remove_and_replace(back->dependencies[k], other, u, di_merged.data); - if (r < 0) - log_warning_errno(r, "Failed to remove/replace: back=%s other=%s u=%s: %m", back->id, other_id, u->id); - assert(r >= 0); - - /* assert_se(hashmap_remove_and_replace(back->dependencies[k], other, u, di_merged.data) >= 0); */ + /* This is a dependency pointing back to the unit we want to merge with? + * Suppress it (but warn) */ + unit_maybe_warn_about_dependency(u, other->id, UNIT_DEPENDENCY_FROM_PTR(dt)); + continue; } - /* Also do not move dependencies on u to itself */ - back = hashmap_remove(other->dependencies[d], u); - if (back) - maybe_warn_about_dependency(u, other_id, d); + /* Now iterate through all deps of 'back', and fix the ones pointing to 'other' to + * point to 'u' instead. */ + HASHMAP_FOREACH_KEY(back_deps, back_dt, back->dependencies) { + UnitDependencyInfo di_move; - /* The move cannot fail. The caller must have performed a reservation. */ - assert_se(hashmap_complete_move(&u->dependencies[d], &other->dependencies[d]) == 0); + di_move.data = hashmap_remove(back_deps, other); + if (!di_move.data) + continue; - other->dependencies[d] = hashmap_free(other->dependencies[d]); + assert_se(unit_per_dependency_type_hashmap_update( + back_deps, + u, + di_move.origin_mask, + di_move.destination_mask) >= 0); + } + } + + /* Now all references towards 'other' of the current type 'dt' are corrected to point to + * 'u'. Lets's now move the deps of type 'dt' from 'other' to 'u'. First, let's try to move + * them per type wholesale. */ + r = hashmap_put(u->dependencies, dt, other_deps); + if (r == -EEXIST) { + Hashmap *deps; + + /* The target unit already has dependencies of this type, let's then merge this individually. */ + + assert_se(deps = hashmap_get(u->dependencies, dt)); + + for (;;) { + UnitDependencyInfo di_move; + + /* Get first dep */ + di_move.data = hashmap_steal_first_key_and_value(other_deps, (void**) &back); + if (!di_move.data) + break; /* done */ + if (back == u) { + /* Would point back to us, ignore */ + unit_maybe_warn_about_dependency(u, other->id, UNIT_DEPENDENCY_FROM_PTR(dt)); + continue; + } + + assert_se(unit_per_dependency_type_hashmap_update(deps, back, di_move.origin_mask, di_move.destination_mask) >= 0); + } + } else { + assert_se(r >= 0); + TAKE_PTR(other_deps); + + if (hashmap_remove(other_deps, u)) + unit_maybe_warn_about_dependency(u, other->id, UNIT_DEPENDENCY_FROM_PTR(dt)); + } + } + + other->dependencies = hashmap_free(other->dependencies); } int unit_merge(Unit *u, Unit *other) { - const char *other_id = NULL; int r; assert(u); @@ -964,22 +1115,14 @@ int unit_merge(Unit *u, Unit *other) { if (!UNIT_IS_INACTIVE_OR_FAILED(unit_active_state(other))) return -EEXIST; - if (other->id) - other_id = strdupa(other->id); - - /* Make reservations to ensure merge_dependencies() won't fail */ - for (UnitDependency d = 0; d < _UNIT_DEPENDENCY_MAX; d++) { - r = reserve_dependencies(u, other, d); - /* - * We don't rollback reservations if we fail. We don't have - * a way to undo reservations. A reservation is not a leak. - */ - if (r < 0) - return r; - } + /* Make reservations to ensure merge_dependencies() won't fail. We don't rollback reservations if we + * fail. We don't have a way to undo reservations. A reservation is not a leak. */ + r = unit_reserve_dependencies(u, other); + if (r < 0) + return r; /* Merge names */ - r = merge_names(u, other); + r = unit_merge_names(u, other); if (r < 0) return r; @@ -988,8 +1131,7 @@ int unit_merge(Unit *u, Unit *other) { unit_ref_set(other->refs_by_target, other->refs_by_target->source, u); /* Merge dependencies */ - for (UnitDependency d = 0; d < _UNIT_DEPENDENCY_MAX; d++) - merge_dependencies(u, other, other_id, d); + unit_merge_dependencies(u, other); other->load_state = UNIT_MERGED; other->merged_into = u; @@ -1248,7 +1390,7 @@ int unit_add_default_target_dependency(Unit *u, Unit *target) { return 0; /* Don't create loops */ - if (hashmap_get(target->dependencies[UNIT_BEFORE], u)) + if (unit_has_dependency(target, UNIT_ATOM_BEFORE, u)) return 0; return unit_add_dependency(target, UNIT_AFTER, u, true, UNIT_DEPENDENCY_DEFAULT); @@ -1416,10 +1558,18 @@ int unit_load(Unit *u) { if (r < 0) goto fail; - if (u->on_failure_job_mode == JOB_ISOLATE && hashmap_size(u->dependencies[UNIT_ON_FAILURE]) > 1) { - r = log_unit_error_errno(u, SYNTHETIC_ERRNO(ENOEXEC), - "More than one OnFailure= dependencies specified but OnFailureJobMode=isolate set. Refusing."); - goto fail; + if (u->on_failure_job_mode == JOB_ISOLATE) { + Unit *other, *found = NULL; + + UNIT_FOREACH_DEPENDENCY(other, u, UNIT_ATOM_ON_FAILURE) { + if (!found) + found = other; + else if (found != other) { + r = log_unit_error_errno(u, SYNTHETIC_ERRNO(ENOEXEC), + "More than one OnFailure= dependencies specified but OnFailureJobMode=isolate set. Refusing."); + goto fail; + } + } } if (u->job_running_timeout != USEC_INFINITY && u->job_running_timeout > u->job_timeout) @@ -1577,7 +1727,6 @@ bool unit_shall_confirm_spawn(Unit *u) { static bool unit_verify_deps(Unit *u) { Unit *other; - void *v; assert(u); @@ -1586,9 +1735,9 @@ static bool unit_verify_deps(Unit *u) { * processing, but do not have any effect afterwards. We don't check BindsTo= dependencies that are not used in * conjunction with After= as for them any such check would make things entirely racy. */ - HASHMAP_FOREACH_KEY(v, other, u->dependencies[UNIT_BINDS_TO]) { + UNIT_FOREACH_DEPENDENCY(other, u, UNIT_ATOM_CANNOT_BE_ACTIVE_WITHOUT) { - if (!hashmap_contains(u->dependencies[UNIT_AFTER], other)) + if (!unit_has_dependency(u, UNIT_ATOM_AFTER, other)) continue; if (!UNIT_IS_ACTIVE_OR_RELOADING(unit_active_state(other))) { @@ -1802,20 +1951,14 @@ bool unit_can_reload(Unit *u) { if (UNIT_VTABLE(u)->can_reload) return UNIT_VTABLE(u)->can_reload(u); - if (!hashmap_isempty(u->dependencies[UNIT_PROPAGATES_RELOAD_TO])) + if (unit_has_dependency(u, UNIT_ATOM_PROPAGATES_RELOAD_TO, NULL)) return true; return UNIT_VTABLE(u)->reload; } bool unit_is_unneeded(Unit *u) { - static const UnitDependency deps[] = { - UNIT_REQUIRED_BY, - UNIT_REQUISITE_OF, - UNIT_WANTED_BY, - UNIT_BOUND_BY, - }; - + Unit *other; assert(u); if (!u->stop_when_unneeded) @@ -1827,55 +1970,37 @@ bool unit_is_unneeded(Unit *u) { if (u->job) return false; - for (size_t j = 0; j < ELEMENTSOF(deps); j++) { - Unit *other; - void *v; - + UNIT_FOREACH_DEPENDENCY(other, u, UNIT_ATOM_PINS_STOP_WHEN_UNNEEDED) { /* If a dependent unit has a job queued, is active or transitioning, or is marked for * restart, then don't clean this one up. */ - HASHMAP_FOREACH_KEY(v, other, u->dependencies[deps[j]]) { - if (other->job) - return false; + if (other->job) + return false; - if (!UNIT_IS_INACTIVE_OR_FAILED(unit_active_state(other))) - return false; + if (!UNIT_IS_INACTIVE_OR_FAILED(unit_active_state(other))) + return false; - if (unit_will_restart(other)) - return false; - } + if (unit_will_restart(other)) + return false; } return true; } static void check_unneeded_dependencies(Unit *u) { - - static const UnitDependency deps[] = { - UNIT_REQUIRES, - UNIT_REQUISITE, - UNIT_WANTS, - UNIT_BINDS_TO, - }; - + Unit *other; assert(u); /* Add all units this unit depends on to the queue that processes StopWhenUnneeded= behaviour. */ - for (size_t j = 0; j < ELEMENTSOF(deps); j++) { - Unit *other; - void *v; - - HASHMAP_FOREACH_KEY(v, other, u->dependencies[deps[j]]) - unit_submit_to_stop_when_unneeded_queue(other); - } + UNIT_FOREACH_DEPENDENCY(other, u, UNIT_ATOM_ADD_STOP_WHEN_UNNEEDED_QUEUE) + unit_submit_to_stop_when_unneeded_queue(other); } static void unit_check_binds_to(Unit *u) { _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; bool stop = false; Unit *other; - void *v; int r; assert(u); @@ -1886,7 +2011,7 @@ static void unit_check_binds_to(Unit *u) { if (unit_active_state(u) != UNIT_ACTIVE) return; - HASHMAP_FOREACH_KEY(v, other, u->dependencies[UNIT_BINDS_TO]) { + UNIT_FOREACH_DEPENDENCY(other, u, UNIT_ATOM_CANNOT_BE_ACTIVE_WITHOUT) { if (other->job) continue; @@ -1923,63 +2048,52 @@ static void unit_check_binds_to(Unit *u) { static void retroactively_start_dependencies(Unit *u) { Unit *other; - void *v; assert(u); assert(UNIT_IS_ACTIVE_OR_ACTIVATING(unit_active_state(u))); - HASHMAP_FOREACH_KEY(v, other, u->dependencies[UNIT_REQUIRES]) - if (!hashmap_get(u->dependencies[UNIT_AFTER], other) && + UNIT_FOREACH_DEPENDENCY(other, u, UNIT_ATOM_RETROACTIVE_START_REPLACE) /* Requires= + BindsTo= */ + if (!unit_has_dependency(u, UNIT_ATOM_AFTER, other) && !UNIT_IS_ACTIVE_OR_ACTIVATING(unit_active_state(other))) manager_add_job(u->manager, JOB_START, other, JOB_REPLACE, NULL, NULL, NULL); - HASHMAP_FOREACH_KEY(v, other, u->dependencies[UNIT_BINDS_TO]) - if (!hashmap_get(u->dependencies[UNIT_AFTER], other) && - !UNIT_IS_ACTIVE_OR_ACTIVATING(unit_active_state(other))) - manager_add_job(u->manager, JOB_START, other, JOB_REPLACE, NULL, NULL, NULL); - - HASHMAP_FOREACH_KEY(v, other, u->dependencies[UNIT_WANTS]) - if (!hashmap_get(u->dependencies[UNIT_AFTER], other) && + UNIT_FOREACH_DEPENDENCY(other, u, UNIT_ATOM_RETROACTIVE_START_FAIL) /* Wants= */ + if (!unit_has_dependency(u, UNIT_ATOM_AFTER, other) && !UNIT_IS_ACTIVE_OR_ACTIVATING(unit_active_state(other))) manager_add_job(u->manager, JOB_START, other, JOB_FAIL, NULL, NULL, NULL); - HASHMAP_FOREACH_KEY(v, other, u->dependencies[UNIT_CONFLICTS]) - if (!UNIT_IS_INACTIVE_OR_DEACTIVATING(unit_active_state(other))) - manager_add_job(u->manager, JOB_STOP, other, JOB_REPLACE, NULL, NULL, NULL); - - HASHMAP_FOREACH_KEY(v, other, u->dependencies[UNIT_CONFLICTED_BY]) + UNIT_FOREACH_DEPENDENCY(other, u, UNIT_ATOM_RETROACTIVE_STOP_ON_START) /* Conflicts= (and inverse) */ if (!UNIT_IS_INACTIVE_OR_DEACTIVATING(unit_active_state(other))) manager_add_job(u->manager, JOB_STOP, other, JOB_REPLACE, NULL, NULL, NULL); } static void retroactively_stop_dependencies(Unit *u) { Unit *other; - void *v; assert(u); assert(UNIT_IS_INACTIVE_OR_DEACTIVATING(unit_active_state(u))); /* Pull down units which are bound to us recursively if enabled */ - HASHMAP_FOREACH_KEY(v, other, u->dependencies[UNIT_BOUND_BY]) + UNIT_FOREACH_DEPENDENCY(other, u, UNIT_ATOM_RETROACTIVE_STOP_ON_STOP) /* BoundBy= */ if (!UNIT_IS_INACTIVE_OR_DEACTIVATING(unit_active_state(other))) manager_add_job(u->manager, JOB_STOP, other, JOB_REPLACE, NULL, NULL, NULL); } void unit_start_on_failure(Unit *u) { + bool logged = false; Unit *other; - void *v; int r; assert(u); - if (hashmap_size(u->dependencies[UNIT_ON_FAILURE]) <= 0) - return; - - log_unit_info(u, "Triggering OnFailure= dependencies."); - - HASHMAP_FOREACH_KEY(v, other, u->dependencies[UNIT_ON_FAILURE]) { + UNIT_FOREACH_DEPENDENCY(other, u, UNIT_ATOM_ON_FAILURE) { _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; + if (!logged) { + log_unit_info(u, "Triggering OnFailure= dependencies."); + logged = true; + } + r = manager_add_job(u->manager, JOB_START, other, u->on_failure_job_mode, NULL, &error, NULL); if (r < 0) log_unit_warning_errno(u, r, "Failed to enqueue OnFailure= job, ignoring: %s", bus_error_message(&error, r)); @@ -1988,11 +2102,10 @@ void unit_start_on_failure(Unit *u) { void unit_trigger_notify(Unit *u) { Unit *other; - void *v; assert(u); - HASHMAP_FOREACH_KEY(v, other, u->dependencies[UNIT_TRIGGERED_BY]) + UNIT_FOREACH_DEPENDENCY(other, u, UNIT_ATOM_TRIGGERED_BY) if (UNIT_VTABLE(other)->trigger_notify) UNIT_VTABLE(other)->trigger_notify(other, u); } @@ -2733,66 +2846,6 @@ bool unit_job_is_applicable(Unit *u, JobType j) { } } -static void maybe_warn_about_dependency(Unit *u, const char *other, UnitDependency dependency) { - assert(u); - - /* Only warn about some unit types */ - if (!IN_SET(dependency, UNIT_CONFLICTS, UNIT_CONFLICTED_BY, UNIT_BEFORE, UNIT_AFTER, UNIT_ON_FAILURE, UNIT_TRIGGERS, UNIT_TRIGGERED_BY)) - return; - - if (streq_ptr(u->id, other)) - log_unit_warning(u, "Dependency %s=%s dropped", unit_dependency_to_string(dependency), u->id); - else - log_unit_warning(u, "Dependency %s=%s dropped, merged into %s", unit_dependency_to_string(dependency), strna(other), u->id); -} - -static int unit_add_dependency_hashmap( - Hashmap **h, - Unit *other, - UnitDependencyMask origin_mask, - UnitDependencyMask destination_mask) { - - UnitDependencyInfo info; - int r; - - assert(h); - assert(other); - assert(origin_mask < _UNIT_DEPENDENCY_MASK_FULL); - assert(destination_mask < _UNIT_DEPENDENCY_MASK_FULL); - assert(origin_mask > 0 || destination_mask > 0); - - r = hashmap_ensure_allocated(h, NULL); - if (r < 0) - return r; - - assert_cc(sizeof(void*) == sizeof(info)); - - info.data = hashmap_get(*h, other); - if (info.data) { - /* Entry already exists. Add in our mask. */ - - if (FLAGS_SET(origin_mask, info.origin_mask) && - FLAGS_SET(destination_mask, info.destination_mask)) - return 0; /* NOP */ - - info.origin_mask |= origin_mask; - info.destination_mask |= destination_mask; - - r = hashmap_update(*h, other, info.data); - } else { - info = (UnitDependencyInfo) { - .origin_mask = origin_mask, - .destination_mask = destination_mask, - }; - - r = hashmap_put(*h, other, info.data); - } - if (r < 0) - return r; - - return 1; -} - int unit_add_dependency( Unit *u, UnitDependency d, @@ -2825,10 +2878,12 @@ int unit_add_dependency( [UNIT_JOINS_NAMESPACE_OF] = UNIT_JOINS_NAMESPACE_OF, }; Unit *original_u = u, *original_other = other; + UnitDependencyAtom a; int r; - /* Helper to know whether sending a notification is necessary or not: - * if the dependency is already there, no need to notify! */ - bool noop = true; + + /* Helper to know whether sending a notification is necessary or not: if the dependency is already + * there, no need to notify! */ + bool noop; assert(u); assert(d >= 0 && d < _UNIT_DEPENDENCY_MAX); @@ -2836,63 +2891,64 @@ int unit_add_dependency( u = unit_follow_merge(u); other = unit_follow_merge(other); + a = unit_dependency_to_atom(d); + assert(a >= 0); - /* We won't allow dependencies on ourselves. We will not - * consider them an error however. */ + /* We won't allow dependencies on ourselves. We will not consider them an error however. */ if (u == other) { - maybe_warn_about_dependency(original_u, original_other->id, d); + unit_maybe_warn_about_dependency(original_u, original_other->id, d); return 0; } - /* Note that ordering a device unit after a unit is permitted since it - * allows to start its job running timeout at a specific time. */ - if (d == UNIT_BEFORE && other->type == UNIT_DEVICE) { + /* Note that ordering a device unit after a unit is permitted since it allows to start its job + * running timeout at a specific time. */ + if (FLAGS_SET(a, UNIT_ATOM_BEFORE) && other->type == UNIT_DEVICE) { log_unit_warning(u, "Dependency Before=%s ignored (.device units cannot be delayed)", other->id); return 0; } - if (d == UNIT_ON_FAILURE && !UNIT_VTABLE(u)->can_fail) { + if (FLAGS_SET(a, UNIT_ATOM_ON_FAILURE) && !UNIT_VTABLE(u)->can_fail) { log_unit_warning(u, "Requested dependency OnFailure=%s ignored (%s units cannot fail).", other->id, unit_type_to_string(u->type)); return 0; } - if (d == UNIT_TRIGGERS && !UNIT_VTABLE(u)->can_trigger) + if (FLAGS_SET(a, UNIT_ATOM_TRIGGERS) && !UNIT_VTABLE(u)->can_trigger) return log_unit_error_errno(u, SYNTHETIC_ERRNO(EINVAL), "Requested dependency Triggers=%s refused (%s units cannot trigger other units).", other->id, unit_type_to_string(u->type)); - if (d == UNIT_TRIGGERED_BY && !UNIT_VTABLE(other)->can_trigger) + if (FLAGS_SET(a, UNIT_ATOM_TRIGGERED_BY) && !UNIT_VTABLE(other)->can_trigger) return log_unit_error_errno(u, SYNTHETIC_ERRNO(EINVAL), "Requested dependency TriggeredBy=%s refused (%s units cannot trigger other units).", other->id, unit_type_to_string(other->type)); - r = unit_add_dependency_hashmap(u->dependencies + d, other, mask, 0); + r = unit_add_dependency_hashmap(&u->dependencies, d, other, mask, 0); if (r < 0) return r; - else if (r > 0) - noop = false; + noop = !r; if (inverse_table[d] != _UNIT_DEPENDENCY_INVALID && inverse_table[d] != d) { - r = unit_add_dependency_hashmap(other->dependencies + inverse_table[d], u, 0, mask); + r = unit_add_dependency_hashmap(&other->dependencies, inverse_table[d], u, 0, mask); if (r < 0) return r; - else if (r > 0) + if (r) noop = false; } if (add_reference) { - r = unit_add_dependency_hashmap(u->dependencies + UNIT_REFERENCES, other, mask, 0); + r = unit_add_dependency_hashmap(&u->dependencies, UNIT_REFERENCES, other, mask, 0); if (r < 0) return r; - else if (r > 0) + if (r) noop = false; - r = unit_add_dependency_hashmap(other->dependencies + UNIT_REFERENCED_BY, u, 0, mask); + r = unit_add_dependency_hashmap(&other->dependencies, UNIT_REFERENCED_BY, u, 0, mask); if (r < 0) return r; - else if (r > 0) + if (r) noop = false; } if (!noop) unit_add_to_dbus_queue(u); + return 0; } @@ -4443,7 +4499,6 @@ int unit_setup_exec_runtime(Unit *u) { ExecRuntime **rt; size_t offset; Unit *other; - void *v; int r; offset = UNIT_VTABLE(u)->exec_runtime_offset; @@ -4455,7 +4510,7 @@ int unit_setup_exec_runtime(Unit *u) { return 0; /* Try to get it from somebody else */ - HASHMAP_FOREACH_KEY(v, other, u->dependencies[UNIT_JOINS_NAMESPACE_OF]) { + UNIT_FOREACH_DEPENDENCY(other, u, UNIT_ATOM_JOINS_NAMESPACE_OF) { r = exec_runtime_acquire(u->manager, NULL, other->id, false, rt); if (r == 1) return 1; @@ -4830,22 +4885,20 @@ int unit_fork_and_watch_rm_rf(Unit *u, char **paths, pid_t *ret_pid) { return 0; } -static void unit_update_dependency_mask(Unit *u, UnitDependency d, Unit *other, UnitDependencyInfo di) { - assert(u); - assert(d >= 0); - assert(d < _UNIT_DEPENDENCY_MAX); +static void unit_update_dependency_mask(Hashmap *deps, Unit *other, UnitDependencyInfo di) { + assert(deps); assert(other); - if (di.origin_mask == 0 && di.destination_mask == 0) { + if (di.origin_mask == 0 && di.destination_mask == 0) /* No bit set anymore, let's drop the whole entry */ - assert_se(hashmap_remove(u->dependencies[d], other)); - log_unit_debug(u, "lost dependency %s=%s", unit_dependency_to_string(d), other->id); - } else + assert_se(hashmap_remove(deps, other)); + else /* Mask was reduced, let's update the entry */ - assert_se(hashmap_update(u->dependencies[d], other, di.data) == 0); + assert_se(hashmap_update(deps, other, di.data) == 0); } void unit_remove_dependencies(Unit *u, UnitDependencyMask mask) { + Hashmap *deps; assert(u); /* Removes all dependencies u has on other units marked for ownership by 'mask'. */ @@ -4853,7 +4906,7 @@ void unit_remove_dependencies(Unit *u, UnitDependencyMask mask) { if (mask == 0) return; - for (UnitDependency d = 0; d < _UNIT_DEPENDENCY_MAX; d++) { + HASHMAP_FOREACH(deps, u->dependencies) { bool done; do { @@ -4862,26 +4915,29 @@ void unit_remove_dependencies(Unit *u, UnitDependencyMask mask) { done = true; - HASHMAP_FOREACH_KEY(di.data, other, u->dependencies[d]) { + HASHMAP_FOREACH_KEY(di.data, other, deps) { + Hashmap *other_deps; + if (FLAGS_SET(~mask, di.origin_mask)) continue; + di.origin_mask &= ~mask; - unit_update_dependency_mask(u, d, other, di); + unit_update_dependency_mask(deps, other, di); /* We updated the dependency from our unit to the other unit now. But most dependencies * imply a reverse dependency. Hence, let's delete that one too. For that we go through * all dependency types on the other unit and delete all those which point to us and * have the right mask set. */ - for (UnitDependency q = 0; q < _UNIT_DEPENDENCY_MAX; q++) { + HASHMAP_FOREACH(other_deps, other->dependencies) { UnitDependencyInfo dj; - dj.data = hashmap_get(other->dependencies[q], u); + dj.data = hashmap_get(other_deps, u); if (FLAGS_SET(~mask, dj.destination_mask)) continue; - dj.destination_mask &= ~mask; - unit_update_dependency_mask(other, q, u, dj); + dj.destination_mask &= ~mask; + unit_update_dependency_mask(other_deps, u, dj); } unit_add_to_gc_queue(other); @@ -5589,3 +5645,44 @@ static const char* const collect_mode_table[_COLLECT_MODE_MAX] = { }; DEFINE_STRING_TABLE_LOOKUP(collect_mode, CollectMode); + +Unit* unit_has_dependency(const Unit *u, UnitDependencyAtom atom, Unit *other) { + Unit *i; + + assert(u); + + /* Checks if the unit has a dependency on 'other' with the specified dependency atom. If 'other' is + * NULL checks if the unit has *any* dependency of that atom. Returns 'other' if found (or if 'other' + * is NULL the first entry found), or NULL if not found. */ + + UNIT_FOREACH_DEPENDENCY(i, u, atom) + if (!other || other == i) + return i; + + return NULL; +} + +int unit_get_dependency_array(const Unit *u, UnitDependencyAtom atom, Unit ***ret_array) { + _cleanup_free_ Unit **array = NULL; + size_t n = 0; + Unit *other; + + assert(u); + assert(ret_array); + + /* Gets a list of units matching a specific atom as array. This is useful when iterating through + * dependencies while modifying them: the array is an "atomic snapshot" of sorts, that can be read + * while the dependency table is continously updated. */ + + UNIT_FOREACH_DEPENDENCY(other, u, atom) { + if (!GREEDY_REALLOC(array, n + 1)) + return -ENOMEM; + + array[n++] = other; + } + + *ret_array = TAKE_PTR(array); + + assert(n <= INT_MAX); + return (int) n; +} diff --git a/src/core/unit.h b/src/core/unit.h index abd8aecc1a..fec5e7a30d 100644 --- a/src/core/unit.h +++ b/src/core/unit.h @@ -102,6 +102,16 @@ typedef union UnitDependencyInfo { } _packed_; } UnitDependencyInfo; +/* Newer LLVM versions don't like implicit casts from large pointer types to smaller enums, hence let's add + * explicit type-safe helpers for that. */ +static inline UnitDependency UNIT_DEPENDENCY_FROM_PTR(const void *p) { + return PTR_TO_INT(p); +} + +static inline void* UNIT_DEPENDENCY_TO_PTR(UnitDependency d) { + return INT_TO_PTR(d); +} + #include "job.h" struct UnitRef { @@ -125,11 +135,13 @@ typedef struct Unit { Set *aliases; /* All the other names. */ - /* For each dependency type we maintain a Hashmap whose key is the Unit* object, and the value encodes why the - * dependency exists, using the UnitDependencyInfo type */ - Hashmap *dependencies[_UNIT_DEPENDENCY_MAX]; + /* For each dependency type we can look up another Hashmap with this, whose key is a Unit* object, + * and whose value encodes why the dependency exists, using the UnitDependencyInfo type. i.e. a + * Hashmap(UnitDependency → Hashmap(Unit* → UnitDependencyInfo)) */ + Hashmap *dependencies; - /* Similar, for RequiresMountsFor= path dependencies. The key is the path, the value the UnitDependencyInfo type */ + /* Similar, for RequiresMountsFor= path dependencies. The key is the path, the value the + * UnitDependencyInfo type */ Hashmap *requires_mounts_for; char *description; @@ -683,8 +695,15 @@ static inline const UnitVTable* UNIT_VTABLE(const Unit *u) { #define UNIT_HAS_CGROUP_CONTEXT(u) (UNIT_VTABLE(u)->cgroup_context_offset > 0) #define UNIT_HAS_KILL_CONTEXT(u) (UNIT_VTABLE(u)->kill_context_offset > 0) +Unit* unit_has_dependency(const Unit *u, UnitDependencyAtom atom, Unit *other); +int unit_get_dependency_array(const Unit *u, UnitDependencyAtom atom, Unit ***ret_array); + +static inline Hashmap* unit_get_dependencies(Unit *u, UnitDependency d) { + return hashmap_get(u->dependencies, UNIT_DEPENDENCY_TO_PTR(d)); +} + static inline Unit* UNIT_TRIGGER(Unit *u) { - return hashmap_first_key(u->dependencies[UNIT_TRIGGERS]); + return unit_has_dependency(u, UNIT_ATOM_TRIGGERS, NULL); } Unit* unit_new(Manager *m, size_t size); @@ -990,3 +1009,54 @@ int unit_thaw_vtable_common(Unit *u); const char* collect_mode_to_string(CollectMode m) _const_; 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 */ + UnitDependencyAtom match_atom; + Hashmap *by_type, *by_unit; + void *current_type; + Iterator by_type_iterator, by_unit_iterator; + Unit **current_unit; +} UnitForEachDependencyData; + +/* 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) \ + for (UnitForEachDependencyData data = { \ + .match_atom = (ma), \ + .by_type = (u)->dependencies, \ + .by_type_iterator = ITERATOR_FIRST, \ + .current_unit = &(other), \ + }; \ + ({ \ + UnitDependency _dt = _UNIT_DEPENDENCY_INVALID; \ + bool _found; \ + \ + if (data.by_type && ITERATOR_IS_FIRST(data.by_type_iterator)) { \ + _dt = unit_dependency_from_unique_atom(data.match_atom); \ + if (_dt >= 0) { \ + data.by_unit = hashmap_get(data.by_type, UNIT_DEPENDENCY_TO_PTR(_dt)); \ + data.current_type = UNIT_DEPENDENCY_TO_PTR(_dt); \ + data.by_type = NULL; \ + _found = !!data.by_unit; \ + } \ + } \ + if (_dt < 0) \ + _found = hashmap_iterate(data.by_type, \ + &data.by_type_iterator, \ + (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; \ + hashmap_iterate(data.by_unit, \ + &data.by_unit_iterator, \ + NULL, \ + (const void**) data.current_unit); ) + +/* 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)) diff --git a/src/test/test-engine.c b/src/test/test-engine.c index cd7cfd9f09..9b744618ac 100644 --- a/src/test/test-engine.c +++ b/src/test/test-engine.c @@ -122,32 +122,32 @@ int main(int argc, char *argv[]) { assert_se(manager_add_job(m, JOB_START, a_conj, JOB_REPLACE, NULL, NULL, &j) == -EDEADLK); manager_dump_jobs(m, stdout, "\t"); - assert_se(!hashmap_get(a->dependencies[UNIT_PROPAGATES_RELOAD_TO], b)); - assert_se(!hashmap_get(b->dependencies[UNIT_RELOAD_PROPAGATED_FROM], a)); - assert_se(!hashmap_get(a->dependencies[UNIT_PROPAGATES_RELOAD_TO], c)); - assert_se(!hashmap_get(c->dependencies[UNIT_RELOAD_PROPAGATED_FROM], a)); + assert_se(!hashmap_get(unit_get_dependencies(a, UNIT_PROPAGATES_RELOAD_TO), b)); + assert_se(!hashmap_get(unit_get_dependencies(b, UNIT_RELOAD_PROPAGATED_FROM), a)); + assert_se(!hashmap_get(unit_get_dependencies(a, UNIT_PROPAGATES_RELOAD_TO), c)); + assert_se(!hashmap_get(unit_get_dependencies(c, UNIT_RELOAD_PROPAGATED_FROM), a)); assert_se(unit_add_dependency(a, UNIT_PROPAGATES_RELOAD_TO, b, true, UNIT_DEPENDENCY_UDEV) == 0); assert_se(unit_add_dependency(a, UNIT_PROPAGATES_RELOAD_TO, c, true, UNIT_DEPENDENCY_PROC_SWAP) == 0); - assert_se(hashmap_get(a->dependencies[UNIT_PROPAGATES_RELOAD_TO], b)); - assert_se(hashmap_get(b->dependencies[UNIT_RELOAD_PROPAGATED_FROM], a)); - assert_se(hashmap_get(a->dependencies[UNIT_PROPAGATES_RELOAD_TO], c)); - assert_se(hashmap_get(c->dependencies[UNIT_RELOAD_PROPAGATED_FROM], a)); + assert_se(hashmap_get(unit_get_dependencies(a, UNIT_PROPAGATES_RELOAD_TO), b)); + assert_se(hashmap_get(unit_get_dependencies(b, UNIT_RELOAD_PROPAGATED_FROM), a)); + assert_se(hashmap_get(unit_get_dependencies(a, UNIT_PROPAGATES_RELOAD_TO), c)); + assert_se(hashmap_get(unit_get_dependencies(c, UNIT_RELOAD_PROPAGATED_FROM), a)); unit_remove_dependencies(a, UNIT_DEPENDENCY_UDEV); - assert_se(!hashmap_get(a->dependencies[UNIT_PROPAGATES_RELOAD_TO], b)); - assert_se(!hashmap_get(b->dependencies[UNIT_RELOAD_PROPAGATED_FROM], a)); - assert_se(hashmap_get(a->dependencies[UNIT_PROPAGATES_RELOAD_TO], c)); - assert_se(hashmap_get(c->dependencies[UNIT_RELOAD_PROPAGATED_FROM], a)); + assert_se(!hashmap_get(unit_get_dependencies(a, UNIT_PROPAGATES_RELOAD_TO), b)); + assert_se(!hashmap_get(unit_get_dependencies(b, UNIT_RELOAD_PROPAGATED_FROM), a)); + assert_se(hashmap_get(unit_get_dependencies(a, UNIT_PROPAGATES_RELOAD_TO), c)); + assert_se(hashmap_get(unit_get_dependencies(c, UNIT_RELOAD_PROPAGATED_FROM), a)); unit_remove_dependencies(a, UNIT_DEPENDENCY_PROC_SWAP); - assert_se(!hashmap_get(a->dependencies[UNIT_PROPAGATES_RELOAD_TO], b)); - assert_se(!hashmap_get(b->dependencies[UNIT_RELOAD_PROPAGATED_FROM], a)); - assert_se(!hashmap_get(a->dependencies[UNIT_PROPAGATES_RELOAD_TO], c)); - assert_se(!hashmap_get(c->dependencies[UNIT_RELOAD_PROPAGATED_FROM], a)); + assert_se(!hashmap_get(unit_get_dependencies(a, UNIT_PROPAGATES_RELOAD_TO), b)); + assert_se(!hashmap_get(unit_get_dependencies(b, UNIT_RELOAD_PROPAGATED_FROM), a)); + assert_se(!hashmap_get(unit_get_dependencies(a, UNIT_PROPAGATES_RELOAD_TO), c)); + assert_se(!hashmap_get(unit_get_dependencies(c, UNIT_RELOAD_PROPAGATED_FROM), a)); assert_se(manager_load_unit(m, "unit-with-multiple-dashes.service", NULL, NULL, &unit_with_multiple_dashes) >= 0); From defe63b0f3594b14b7784dd914f0f8f7eb2d82a4 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 13 Apr 2021 17:26:16 +0200 Subject: [PATCH 03/18] core: rebreak a few comments --- src/core/cgroup.c | 15 ++++++--------- src/core/job.c | 38 ++++++++++++++++---------------------- src/core/transaction.c | 23 +++++++++-------------- src/core/unit.c | 19 ++++++++++--------- 4 files changed, 41 insertions(+), 54 deletions(-) diff --git a/src/core/cgroup.c b/src/core/cgroup.c index 094d1becd3..92ac35af8e 100644 --- a/src/core/cgroup.c +++ b/src/core/cgroup.c @@ -2358,22 +2358,19 @@ static int unit_realize_cgroup_now_disable(Unit *u, ManagerState state) { if (UNIT_DEREF(m->slice) != u) continue; - /* The cgroup for this unit might not actually be fully - * realised yet, in which case it isn't holding any controllers - * open anyway. */ + /* The cgroup for this unit might not actually be fully realised yet, in which case it isn't + * holding any controllers open anyway. */ if (!m->cgroup_realized) continue; - /* We must disable those below us first in order to release the - * controller. */ + /* We must disable those below us first in order to release the controller. */ if (m->type == UNIT_SLICE) (void) unit_realize_cgroup_now_disable(m, state); target_mask = unit_get_target_mask(m); enable_mask = unit_get_enable_mask(m); - /* We can only disable in this direction, don't try to enable - * anything. */ + /* We can only disable in this direction, don't try to enable anything. */ if (unit_has_mask_disables_realized(m, target_mask, enable_mask)) continue; @@ -2535,8 +2532,8 @@ void unit_add_family_to_cgroup_realize_queue(Unit *u) { if (!m->cgroup_realized) continue; - /* If the unit doesn't need any new controllers and has current ones realized, it - * doesn't need any changes. */ + /* If the unit doesn't need any new controllers and has current ones + * realized, it doesn't need any changes. */ if (unit_has_mask_realized(m, unit_get_target_mask(m), unit_get_enable_mask(m))) diff --git a/src/core/job.c b/src/core/job.c index ae54956b69..45bb6bfd92 100644 --- a/src/core/job.c +++ b/src/core/job.c @@ -1014,26 +1014,22 @@ int job_finish_and_invalidate(Job *j, JobResult result, bool recursive, bool alr job_fail_dependencies(u, UNIT_ATOM_PROPAGATE_STOP_FAILURE); } - /* A special check to make sure we take down anything RequisiteOf if we - * aren't active. This is when the verify-active job merges with a - * satisfying job type, and then loses it's invalidation effect, as the - * result there is JOB_DONE for the start job we merged into, while we - * should be failing the depending job if the said unit isn't in fact - * active. Oneshots are an example of this, where going directly from - * activating to inactive is success. + /* A special check to make sure we take down anything RequisiteOf= if we aren't active. This is when + * the verify-active job merges with a satisfying job type, and then loses it's invalidation effect, + * as the result there is JOB_DONE for the start job we merged into, while we should be failing the + * depending job if the said unit isn't in fact active. Oneshots are an example of this, where going + * directly from activating to inactive is success. * - * This happens when you use ConditionXYZ= in a unit too, since in that - * case the job completes with the JOB_DONE result, but the unit never - * really becomes active. Note that such a case still involves merging: + * This happens when you use ConditionXYZ= in a unit too, since in that case the job completes with + * the JOB_DONE result, but the unit never really becomes active. Note that such a case still + * involves merging: * - * A start job waits for something else, and a verify-active comes in - * and merges in the installed job. Then, later, when it becomes - * runnable, it finishes with JOB_DONE result as execution on conditions - * not being met is skipped, breaking our dependency semantics. + * A start job waits for something else, and a verify-active comes in and merges in the installed + * job. Then, later, when it becomes runnable, it finishes with JOB_DONE result as execution on + * conditions not being met is skipped, breaking our dependency semantics. * - * Also, depending on if start job waits or not, the merging may or may - * not happen (the verify-active job may trigger after it finishes), so - * you get undeterministic results without this check. + * Also, depending on if start job waits or not, the merging may or may not happen (the verify-active + * job may trigger after it finishes), so you get undeterministic results without this check. */ if (result == JOB_DONE && recursive && IN_SET(t, JOB_START, JOB_RELOAD) && @@ -1656,8 +1652,7 @@ const char* job_type_to_access_method(JobType t) { * stop a + start b → 1st step stop a, 2nd step start b * stop a + stop b → 1st step stop b, 2nd step stop a * - * This has the side effect that restarts are properly - * synchronized too. + * This has the side effect that restarts are properly synchronized too. */ int job_compare(Job *a, Job *b, UnitDependencyAtom assume_dep) { assert(a); @@ -1676,9 +1671,8 @@ int job_compare(Job *a, Job *b, UnitDependencyAtom assume_dep) { if (assume_dep == UNIT_ATOM_AFTER) return -job_compare(b, a, UNIT_ATOM_BEFORE); - /* Let's make it simple, JOB_STOP goes always first (in case both ua and ub stop, - * then ub's stop goes first anyway). - * JOB_RESTART is JOB_STOP in disguise (before it is patched to JOB_START). */ + /* Let's make it simple, JOB_STOP goes always first (in case both ua and ub stop, then ub's stop goes + * first anyway). JOB_RESTART is JOB_STOP in disguise (before it is patched to JOB_START). */ if (IN_SET(b->type, JOB_STOP, JOB_RESTART)) return 1; else diff --git a/src/core/transaction.c b/src/core/transaction.c index 8e1809302f..51232e4ab1 100644 --- a/src/core/transaction.c +++ b/src/core/transaction.c @@ -359,8 +359,8 @@ static int transaction_verify_order_one(Transaction *tr, Job *j, Job *from, unsi assert(j); assert(!j->transaction_prev); - /* Does a recursive sweep through the ordering graph, looking - * for a cycle. If we find a cycle we try to break it. */ + /* Does a recursive sweep through the ordering graph, looking for a cycle. If we find a cycle we try + * to break it. */ /* Have we seen this before? */ if (j->generation == generation) { @@ -368,18 +368,14 @@ static int transaction_verify_order_one(Transaction *tr, Job *j, Job *from, unsi _cleanup_free_ char **array = NULL, *unit_ids = NULL; char **unit_id, **job_type; - /* If the marker is NULL we have been here already and - * decided the job was loop-free from here. Hence - * shortcut things and return right-away. */ + /* If the marker is NULL we have been here already and decided the job was loop-free from + * here. Hence shortcut things and return right-away. */ if (!j->marker) return 0; - /* So, the marker is not NULL and we already have been here. We have - * a cycle. Let's try to break it. We go backwards in our path and - * try to find a suitable job to remove. We use the marker to find - * our way back, since smart how we are we stored our way back in - * there. */ - + /* So, the marker is not NULL and we already have been here. We have a cycle. Let's try to + * break it. We go backwards in our path and try to find a suitable job to remove. We use the + * marker to find our way back, since smart how we are we stored our way back in there. */ for (k = from; k; k = ((k->generation == generation && k->marker != k) ? k->marker : NULL)) { /* For logging below */ @@ -457,9 +453,8 @@ static int transaction_verify_order_one(Transaction *tr, Job *j, Job *from, unsi /* Is there a job for this unit? */ o = hashmap_get(tr->jobs, u); if (!o) { - /* Ok, there is no job for this in the - * transaction, but maybe there is already one - * running? */ + /* Ok, there is no job for this in the transaction, but maybe there is + * already one running? */ o = u->job; if (!o) continue; diff --git a/src/core/unit.c b/src/core/unit.c index e48aa5313b..9b9c6d01b9 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -1730,10 +1730,11 @@ static bool unit_verify_deps(Unit *u) { assert(u); - /* Checks whether all BindsTo= dependencies of this unit are fulfilled — if they are also combined with - * After=. We do not check Requires= or Requisite= here as they only should have an effect on the job - * processing, but do not have any effect afterwards. We don't check BindsTo= dependencies that are not used in - * conjunction with After= as for them any such check would make things entirely racy. */ + /* Checks whether all BindsTo= dependencies of this unit are fulfilled — if they are also combined + * with After=. We do not check Requires= or Requisite= here as they only should have an effect on + * the job processing, but do not have any effect afterwards. We don't check BindsTo= dependencies + * that are not used in conjunction with After= as for them any such check would make things entirely + * racy. */ UNIT_FOREACH_DEPENDENCY(other, u, UNIT_ATOM_CANNOT_BE_ACTIVE_WITHOUT) { @@ -2875,7 +2876,7 @@ int unit_add_dependency( [UNIT_TRIGGERED_BY] = UNIT_TRIGGERS, [UNIT_PROPAGATES_RELOAD_TO] = UNIT_RELOAD_PROPAGATED_FROM, [UNIT_RELOAD_PROPAGATED_FROM] = UNIT_PROPAGATES_RELOAD_TO, - [UNIT_JOINS_NAMESPACE_OF] = UNIT_JOINS_NAMESPACE_OF, + [UNIT_JOINS_NAMESPACE_OF] = UNIT_JOINS_NAMESPACE_OF, /* symmetric! 👓 */ }; Unit *original_u = u, *original_other = other; UnitDependencyAtom a; @@ -4924,10 +4925,10 @@ void unit_remove_dependencies(Unit *u, UnitDependencyMask mask) { di.origin_mask &= ~mask; unit_update_dependency_mask(deps, other, di); - /* We updated the dependency from our unit to the other unit now. But most dependencies - * imply a reverse dependency. Hence, let's delete that one too. For that we go through - * all dependency types on the other unit and delete all those which point to us and - * have the right mask set. */ + /* We updated the dependency from our unit to the other unit now. But most + * dependencies imply a reverse dependency. Hence, let's delete that one + * too. For that we go through all dependency types on the other unit and + * delete all those which point to us and have the right mask set. */ HASHMAP_FOREACH(other_deps, other->dependencies) { UnitDependencyInfo dj; From 8ddba3f266a1deae3826a80e2d148909b7902020 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 13 Apr 2021 16:50:45 +0200 Subject: [PATCH 04/18] test-engine: extend engine test Let's verify that the dependency type to atom mapping is consistent. Let's also verify that dependency merging works correctly. --- src/test/test-engine.c | 92 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 90 insertions(+), 2 deletions(-) diff --git a/src/test/test-engine.c b/src/test/test-engine.c index 9b744618ac..8e196907ee 100644 --- a/src/test/test-engine.c +++ b/src/test/test-engine.c @@ -6,16 +6,58 @@ #include "bus-util.h" #include "manager.h" #include "rm-rf.h" +#include "service.h" +#include "special.h" #include "strv.h" #include "tests.h" -#include "service.h" +#include "unit-serialize.h" + +static void verify_dependency_atoms(void) { + UnitDependencyAtom combined = 0, multi_use_atoms = 0; + + /* Let's guarantee that our dependency type/atom translation tables are fully correct */ + + for (UnitDependency d = 0; d < _UNIT_DEPENDENCY_MAX; d++) { + UnitDependencyAtom a; + UnitDependency reverse; + bool has_superset = false; + + assert_se((a = unit_dependency_to_atom(d)) >= 0); + + for (UnitDependency t = 0; t < _UNIT_DEPENDENCY_MAX; t++) { + UnitDependencyAtom b; + + if (t == d) + continue; + + assert_se((b = unit_dependency_to_atom(t)) >= 0); + + if ((a & b) == a) { + has_superset = true; + break; + } + } + + reverse = unit_dependency_from_unique_atom(a); + assert_se(reverse == _UNIT_DEPENDENCY_INVALID || reverse >= 0); + + assert_se((reverse < 0) == has_superset); /* If one dependency type is a superset of another, + * then the reverse mapping is not unique, verify + * that. */ + + log_info("Verified dependency type: %s", unit_dependency_to_string(d)); + + multi_use_atoms |= combined & a; + combined |= a; + } +} int main(int argc, char *argv[]) { _cleanup_(rm_rf_physical_and_freep) char *runtime_dir = NULL; _cleanup_(sd_bus_error_free) sd_bus_error err = SD_BUS_ERROR_NULL; _cleanup_(manager_freep) Manager *m = NULL; Unit *a = NULL, *b = NULL, *c = NULL, *d = NULL, *e = NULL, *g = NULL, - *h = NULL, *i = NULL, *a_conj = NULL, *unit_with_multiple_dashes = NULL; + *h = NULL, *i = NULL, *a_conj = NULL, *unit_with_multiple_dashes = NULL, *stub = NULL; Job *j; int r; @@ -154,5 +196,51 @@ int main(int argc, char *argv[]) { assert_se(strv_equal(unit_with_multiple_dashes->documentation, STRV_MAKE("man:test", "man:override2", "man:override3"))); assert_se(streq_ptr(unit_with_multiple_dashes->description, "override4")); + /* Now merge a synthetic unit into the existing one */ + assert_se(unit_new_for_name(m, sizeof(Service), "merged.service", &stub) >= 0); + assert_se(unit_add_dependency_by_name(stub, UNIT_AFTER, SPECIAL_BASIC_TARGET, true, UNIT_DEPENDENCY_FILE) >= 0); + assert_se(unit_add_dependency_by_name(stub, UNIT_AFTER, "quux.target", true, UNIT_DEPENDENCY_FILE) >= 0); + assert_se(unit_add_dependency_by_name(stub, UNIT_AFTER, SPECIAL_ROOT_SLICE, true, UNIT_DEPENDENCY_FILE) >= 0); + assert_se(unit_add_dependency_by_name(stub, UNIT_REQUIRES, "non-existing.mount", true, UNIT_DEPENDENCY_FILE) >= 0); + assert_se(unit_add_dependency_by_name(stub, UNIT_ON_FAILURE, "non-existing-on-failure.target", true, UNIT_DEPENDENCY_FILE) >= 0); + + log_info("/* Merging a+stub, dumps before */"); + unit_dump(a, stderr, NULL); + unit_dump(stub, stderr, NULL); + assert_se(unit_merge(a, stub) >= 0); + log_info("/* Dump of merged a+stub */"); + unit_dump(a, stderr, NULL); + + assert_se(unit_has_dependency(a, UNIT_ATOM_AFTER, manager_get_unit(m, SPECIAL_BASIC_TARGET))); + assert_se(unit_has_dependency(a, UNIT_ATOM_AFTER, manager_get_unit(m, "quux.target"))); + assert_se(unit_has_dependency(a, UNIT_ATOM_AFTER, manager_get_unit(m, SPECIAL_ROOT_SLICE))); + assert_se(unit_has_dependency(a, UNIT_ATOM_PULL_IN_START, manager_get_unit(m, "non-existing.mount"))); + assert_se(unit_has_dependency(a, UNIT_ATOM_RETROACTIVE_START_REPLACE, manager_get_unit(m, "non-existing.mount"))); + assert_se(unit_has_dependency(a, UNIT_ATOM_ON_FAILURE, manager_get_unit(m, "non-existing-on-failure.target"))); + assert_se(!unit_has_dependency(a, UNIT_ATOM_ON_FAILURE, manager_get_unit(m, "basic.target"))); + assert_se(!unit_has_dependency(a, UNIT_ATOM_PROPAGATES_RELOAD_TO, manager_get_unit(m, "non-existing-on-failure.target"))); + + assert_se(unit_has_name(a, "a.service")); + assert_se(unit_has_name(a, "merged.service")); + + unsigned mm = 1; + Unit *other; + + UNIT_FOREACH_DEPENDENCY(other, a, UNIT_ATOM_AFTER) { + mm *= unit_has_name(other, SPECIAL_BASIC_TARGET) ? 3 : 1; + mm *= unit_has_name(other, "quux.target") ? 5 : 1; + mm *= unit_has_name(other, SPECIAL_ROOT_SLICE) ? 7 : 1; + } + + UNIT_FOREACH_DEPENDENCY(other, a, UNIT_ATOM_ON_FAILURE) + mm *= unit_has_name(other, "non-existing-on-failure.target") ? 11 : 1; + + UNIT_FOREACH_DEPENDENCY(other, a, UNIT_ATOM_PULL_IN_START) + mm *= unit_has_name(other, "non-existing.mount") ? 13 : 1; + + assert_se(mm == 3U*5U*7U*11U*13U); + + verify_dependency_atoms(); + return 0; } From 12f64221b0af5a9380a840ebeb897c2cd6cff955 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 26 Apr 2021 18:14:07 +0200 Subject: [PATCH 05/18] core: add UNIT_GET_SLICE() helper In a later commit we intend to move the slice logic to use proper dependencies instead of a "UnitRef" object. This preparatory commit drops direct use of the slice UnitRef object for a static inline function UNIT_GET_SLICE() that is both easier to grok, and allows us to easily replace its internal implementation later on. --- src/core/bpf-firewall.c | 4 +- src/core/cgroup.c | 58 +++++++++++++++++++---------- src/core/slice.c | 4 +- src/core/unit-printf.c | 9 ++--- src/core/unit.c | 24 +++++++----- src/core/unit.h | 5 +++ src/test/test-cgroup-mask.c | 14 +++---- src/test/test-cgroup-unit-default.c | 22 +++++------ 8 files changed, 83 insertions(+), 57 deletions(-) diff --git a/src/core/bpf-firewall.c b/src/core/bpf-firewall.c index 02e33399c3..2a41bffee6 100644 --- a/src/core/bpf-firewall.c +++ b/src/core/bpf-firewall.c @@ -422,7 +422,7 @@ static int bpf_firewall_prepare_access_maps( assert(ret_ipv6_map_fd); assert(ret_has_any); - for (p = u; p; p = UNIT_DEREF(p->slice)) { + for (p = u; p; p = UNIT_GET_SLICE(p)) { CGroupContext *cc; cc = unit_get_cgroup_context(p); @@ -463,7 +463,7 @@ static int bpf_firewall_prepare_access_maps( return ipv6_map_fd; } - for (p = u; p; p = UNIT_DEREF(p->slice)) { + for (p = u; p; p = UNIT_GET_SLICE(p)) { CGroupContext *cc; cc = unit_get_cgroup_context(p); diff --git a/src/core/cgroup.c b/src/core/cgroup.c index 92ac35af8e..97820eb264 100644 --- a/src/core/cgroup.c +++ b/src/core/cgroup.c @@ -680,7 +680,7 @@ int cgroup_add_bpf_foreign_program(CGroupContext *c, uint32_t attach_type, const if (c && c->entry##_set) \ return c->entry; \ \ - while ((u = UNIT_DEREF(u->slice))) { \ + while ((u = UNIT_GET_SLICE(u))) { \ c = unit_get_cgroup_context(u); \ if (c && c->default_##entry##_set) \ return c->default_##entry; \ @@ -1549,7 +1549,7 @@ static bool unit_get_needs_bpf_firewall(Unit *u) { return true; /* If any parent slice has an IP access list defined, it applies too */ - for (p = UNIT_DEREF(u->slice); p; p = UNIT_DEREF(p->slice)) { + for (p = UNIT_GET_SLICE(u); p; p = UNIT_GET_SLICE(p)) { c = unit_get_cgroup_context(p); if (!c) return false; @@ -1715,14 +1715,16 @@ CGroupMask unit_get_members_mask(Unit *u) { } CGroupMask unit_get_siblings_mask(Unit *u) { + Unit *slice; assert(u); /* Returns the mask of controllers all of the unit's siblings * require, i.e. the members mask of the unit's parent slice * if there is one. */ - if (UNIT_ISSET(u->slice)) - return unit_get_members_mask(UNIT_DEREF(u->slice)); + slice = UNIT_GET_SLICE(u); + if (slice) + return unit_get_members_mask(slice); return unit_get_subtree_mask(u); /* we are the top-level slice */ } @@ -1739,6 +1741,7 @@ static CGroupMask unit_get_disable_mask(Unit *u) { CGroupMask unit_get_ancestor_disable_mask(Unit *u) { CGroupMask mask; + Unit *slice; assert(u); mask = unit_get_disable_mask(u); @@ -1746,8 +1749,9 @@ CGroupMask unit_get_ancestor_disable_mask(Unit *u) { /* Returns the mask of controllers which are marked as forcibly * disabled in any ancestor unit or the unit in question. */ - if (UNIT_ISSET(u->slice)) - mask |= unit_get_ancestor_disable_mask(UNIT_DEREF(u->slice)); + slice = UNIT_GET_SLICE(u); + if (slice) + mask |= unit_get_ancestor_disable_mask(slice); return mask; } @@ -1789,13 +1793,16 @@ CGroupMask unit_get_enable_mask(Unit *u) { } void unit_invalidate_cgroup_members_masks(Unit *u) { + Unit *slice; + assert(u); /* Recurse invalidate the member masks cache all the way up the tree */ u->cgroup_members_mask_valid = false; - if (UNIT_ISSET(u->slice)) - unit_invalidate_cgroup_members_masks(UNIT_DEREF(u->slice)); + slice = UNIT_GET_SLICE(u); + if (slice) + unit_invalidate_cgroup_members_masks(slice); } const char *unit_get_realized_cgroup_path(Unit *u, CGroupMask mask) { @@ -1809,7 +1816,7 @@ const char *unit_get_realized_cgroup_path(Unit *u, CGroupMask mask) { FLAGS_SET(u->cgroup_realized_mask, mask)) return u->cgroup_path; - u = UNIT_DEREF(u->slice); + u = UNIT_GET_SLICE(u); } return NULL; @@ -1823,7 +1830,8 @@ static const char *migrate_callback(CGroupMask mask, void *userdata) { } char *unit_default_cgroup_path(const Unit *u) { - _cleanup_free_ char *escaped = NULL, *slice = NULL; + _cleanup_free_ char *escaped = NULL, *slice_path = NULL; + Unit *slice; int r; assert(u); @@ -1831,8 +1839,9 @@ char *unit_default_cgroup_path(const Unit *u) { if (unit_has_name(u, SPECIAL_ROOT_SLICE)) return strdup(u->manager->cgroup_root); - if (UNIT_ISSET(u->slice) && !unit_has_name(UNIT_DEREF(u->slice), SPECIAL_ROOT_SLICE)) { - r = cg_slice_to_path(UNIT_DEREF(u->slice)->id, &slice); + slice = UNIT_GET_SLICE(u); + if (slice && !unit_has_name(slice, SPECIAL_ROOT_SLICE)) { + r = cg_slice_to_path(slice->id, &slice_path); if (r < 0) return NULL; } @@ -1841,7 +1850,7 @@ char *unit_default_cgroup_path(const Unit *u) { if (!escaped) return NULL; - return path_join(empty_to_root(u->manager->cgroup_root), slice, escaped); + return path_join(empty_to_root(u->manager->cgroup_root), slice_path, escaped); } int unit_set_cgroup_path(Unit *u, const char *path) { @@ -2315,14 +2324,16 @@ static void unit_remove_from_cgroup_realize_queue(Unit *u) { * hierarchy downwards to the unit in question. */ static int unit_realize_cgroup_now_enable(Unit *u, ManagerState state) { CGroupMask target_mask, enable_mask, new_target_mask, new_enable_mask; + Unit *slice; int r; assert(u); /* First go deal with this unit's parent, or we won't be able to enable * any new controllers at this layer. */ - if (UNIT_ISSET(u->slice)) { - r = unit_realize_cgroup_now_enable(UNIT_DEREF(u->slice), state); + slice = UNIT_GET_SLICE(u); + if (slice) { + r = unit_realize_cgroup_now_enable(slice, state); if (r < 0) return r; } @@ -2431,6 +2442,7 @@ static int unit_realize_cgroup_now_disable(Unit *u, ManagerState state) { * Returns 0 on success and < 0 on failure. */ static int unit_realize_cgroup_now(Unit *u, ManagerState state) { CGroupMask target_mask, enable_mask; + Unit *slice; int r; assert(u); @@ -2449,8 +2461,9 @@ static int unit_realize_cgroup_now(Unit *u, ManagerState state) { return r; /* Enable controllers above us, if there are any */ - if (UNIT_ISSET(u->slice)) { - r = unit_realize_cgroup_now_enable(UNIT_DEREF(u->slice), state); + slice = UNIT_GET_SLICE(u); + if (slice) { + r = unit_realize_cgroup_now_enable(slice, state); if (r < 0) return r; } @@ -2544,10 +2557,14 @@ void unit_add_family_to_cgroup_realize_queue(Unit *u) { /* Parent comes after children */ unit_add_to_cgroup_realize_queue(u); - } while ((u = UNIT_DEREF(u->slice))); + + u = UNIT_GET_SLICE(u); + } while (u); } int unit_realize_cgroup(Unit *u) { + Unit *slice; + assert(u); if (!UNIT_HAS_CGROUP_CONTEXT(u)) @@ -2562,8 +2579,9 @@ int unit_realize_cgroup(Unit *u) { * This call will defer work on the siblings and derealized ancestors to the next event loop * iteration and synchronously creates the parent cgroups (unit_realize_cgroup_now). */ - if (UNIT_ISSET(u->slice)) - unit_add_family_to_cgroup_realize_queue(UNIT_DEREF(u->slice)); + slice = UNIT_GET_SLICE(u); + if (slice) + unit_add_family_to_cgroup_realize_queue(slice); /* And realize this one now (and apply the values) */ return unit_realize_cgroup_now(u, manager_state(u->manager)); diff --git a/src/core/slice.c b/src/core/slice.c index cb94c97b24..5d836a11e6 100644 --- a/src/core/slice.c +++ b/src/core/slice.c @@ -53,7 +53,7 @@ static int slice_add_parent_slice(Slice *s) { assert(s); - if (UNIT_ISSET(u->slice)) + if (UNIT_GET_SLICE(u)) return 0; r = slice_build_parent_slice(u->id, &a); @@ -101,7 +101,7 @@ static int slice_verify(Slice *s) { if (r < 0) return log_unit_error_errno(UNIT(s), r, "Failed to determine parent slice: %m"); - if (parent ? !unit_has_name(UNIT_DEREF(UNIT(s)->slice), parent) : UNIT_ISSET(UNIT(s)->slice)) + if (parent ? !unit_has_name(UNIT_GET_SLICE(UNIT(s)), parent) : !!UNIT_GET_SLICE(UNIT(s))) return log_unit_error_errno(UNIT(s), SYNTHETIC_ERRNO(ENOEXEC), "Located outside of parent slice. Refusing."); return 0; diff --git a/src/core/unit-printf.c b/src/core/unit-printf.c index ee8b0b3de8..113dd1cc8a 100644 --- a/src/core/unit-printf.c +++ b/src/core/unit-printf.c @@ -132,18 +132,15 @@ static int specifier_cgroup_root(char specifier, const void *data, const void *u } static int specifier_cgroup_slice(char specifier, const void *data, const void *userdata, char **ret) { - const Unit *u = userdata; + const Unit *u = userdata, *slice; char *n; assert(u); bad_specifier(u, specifier); - if (UNIT_ISSET(u->slice)) { - const Unit *slice; - - slice = UNIT_DEREF(u->slice); - + slice = UNIT_GET_SLICE(u); + if (slice) { if (slice->cgroup_path) n = strdup(slice->cgroup_path); else diff --git a/src/core/unit.c b/src/core/unit.c index 9b9c6d01b9..f3eaa59570 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -615,6 +615,7 @@ static void unit_done(Unit *u) { } Unit* unit_free(Unit *u) { + Unit *slice; char *t; if (!u) @@ -662,8 +663,9 @@ Unit* unit_free(Unit *u) { /* A unit is being dropped from the tree, make sure our family is realized properly. Do this after we * detach the unit from slice tree in order to eliminate its effect on controller masks. */ - if (UNIT_ISSET(u->slice)) - unit_add_family_to_cgroup_realize_queue(UNIT_DEREF(u->slice)); + slice = UNIT_GET_SLICE(u); + if (slice) + unit_add_family_to_cgroup_realize_queue(slice); if (u->on_console) manager_unref_console(u->manager); @@ -1397,6 +1399,7 @@ int unit_add_default_target_dependency(Unit *u, Unit *target) { } static int unit_add_slice_dependencies(Unit *u) { + Unit *slice; assert(u); if (!UNIT_HAS_CGROUP_CONTEXT(u)) @@ -1407,8 +1410,9 @@ static int unit_add_slice_dependencies(Unit *u) { relationship). */ UnitDependencyMask mask = u->type == UNIT_SLICE ? UNIT_DEPENDENCY_IMPLICIT : UNIT_DEPENDENCY_FILE; - if (UNIT_ISSET(u->slice)) - return unit_add_two_dependencies(u, UNIT_AFTER, UNIT_REQUIRES, UNIT_DEREF(u->slice), true, mask); + slice = UNIT_GET_SLICE(u); + if (slice) + return unit_add_two_dependencies(u, UNIT_AFTER, UNIT_REQUIRES, slice, true, mask); if (unit_has_name(u, SPECIAL_ROOT_SLICE)) return 0; @@ -3124,11 +3128,11 @@ int unit_set_slice(Unit *u, Unit *slice) { !unit_has_name(slice, SPECIAL_ROOT_SLICE)) return -EPERM; - if (UNIT_DEREF(u->slice) == slice) + if (UNIT_GET_SLICE(u) == slice) return 0; /* Disallow slice changes if @u is already bound to cgroups */ - if (UNIT_ISSET(u->slice) && u->cgroup_realized) + if (UNIT_GET_SLICE(u) && u->cgroup_realized) return -EBUSY; unit_ref_set(&u->slice, u, slice); @@ -3142,7 +3146,7 @@ int unit_set_default_slice(Unit *u) { assert(u); - if (UNIT_ISSET(u->slice)) + if (UNIT_GET_SLICE(u)) return 0; if (u->instance) { @@ -3185,12 +3189,14 @@ int unit_set_default_slice(Unit *u) { } const char *unit_slice_name(Unit *u) { + Unit *slice; assert(u); - if (!UNIT_ISSET(u->slice)) + slice = UNIT_GET_SLICE(u); + if (!slice) return NULL; - return UNIT_DEREF(u->slice)->id; + return slice->id; } int unit_load_related_unit(Unit *u, const char *type, Unit **_found) { diff --git a/src/core/unit.h b/src/core/unit.h index fec5e7a30d..cf110ebb55 100644 --- a/src/core/unit.h +++ b/src/core/unit.h @@ -706,6 +706,11 @@ static inline Unit* UNIT_TRIGGER(Unit *u) { return unit_has_dependency(u, UNIT_ATOM_TRIGGERS, NULL); } +static inline Unit* UNIT_GET_SLICE(const Unit *u) { + assert(u); + return u->slice.target; +} + Unit* unit_new(Manager *m, size_t size); Unit* unit_free(Unit *u); DEFINE_TRIVIAL_CLEANUP_FUNC(Unit *, unit_free); diff --git a/src/test/test-cgroup-mask.c b/src/test/test-cgroup-mask.c index 3edc2754e1..19e159b9ff 100644 --- a/src/test/test-cgroup-mask.c +++ b/src/test/test-cgroup-mask.c @@ -70,13 +70,13 @@ static int test_cgroup_mask(void) { assert_se(manager_load_startable_unit_or_warn(m, "parent-deep.slice", NULL, &parent_deep) >= 0); assert_se(manager_load_startable_unit_or_warn(m, "nomem.slice", NULL, &nomem_parent) >= 0); assert_se(manager_load_startable_unit_or_warn(m, "nomemleaf.service", NULL, &nomem_leaf) >= 0); - assert_se(UNIT_DEREF(son->slice) == parent); - assert_se(UNIT_DEREF(daughter->slice) == parent); - assert_se(UNIT_DEREF(parent_deep->slice) == parent); - assert_se(UNIT_DEREF(grandchild->slice) == parent_deep); - assert_se(UNIT_DEREF(nomem_leaf->slice) == nomem_parent); - root = UNIT_DEREF(parent->slice); - assert_se(UNIT_DEREF(nomem_parent->slice) == root); + assert_se(UNIT_GET_SLICE(son) == parent); + assert_se(UNIT_GET_SLICE(daughter) == parent); + assert_se(UNIT_GET_SLICE(parent_deep) == parent); + assert_se(UNIT_GET_SLICE(grandchild) == parent_deep); + assert_se(UNIT_GET_SLICE(nomem_leaf) == nomem_parent); + root = UNIT_GET_SLICE(parent); + assert_se(UNIT_GET_SLICE(nomem_parent) == root); /* Verify per-unit cgroups settings. */ ASSERT_CGROUP_MASK_JOINED(unit_get_own_mask(son), CGROUP_MASK_CPU); diff --git a/src/test/test-cgroup-unit-default.c b/src/test/test-cgroup-unit-default.c index b03f6ff12e..225d138e41 100644 --- a/src/test/test-cgroup-unit-default.c +++ b/src/test/test-cgroup-unit-default.c @@ -91,28 +91,28 @@ static int test_default_memory_low(void) { assert_se(manager_load_startable_unit_or_warn(m, "dml.slice", NULL, &dml) >= 0); assert_se(manager_load_startable_unit_or_warn(m, "dml-passthrough.slice", NULL, &dml_passthrough) >= 0); - assert_se(UNIT_DEREF(dml_passthrough->slice) == dml); + assert_se(UNIT_GET_SLICE(dml_passthrough) == dml); assert_se(manager_load_startable_unit_or_warn(m, "dml-passthrough-empty.service", NULL, &dml_passthrough_empty) >= 0); - assert_se(UNIT_DEREF(dml_passthrough_empty->slice) == dml_passthrough); + assert_se(UNIT_GET_SLICE(dml_passthrough_empty) == dml_passthrough); assert_se(manager_load_startable_unit_or_warn(m, "dml-passthrough-set-dml.service", NULL, &dml_passthrough_set_dml) >= 0); - assert_se(UNIT_DEREF(dml_passthrough_set_dml->slice) == dml_passthrough); + assert_se(UNIT_GET_SLICE(dml_passthrough_set_dml) == dml_passthrough); assert_se(manager_load_startable_unit_or_warn(m, "dml-passthrough-set-ml.service", NULL, &dml_passthrough_set_ml) >= 0); - assert_se(UNIT_DEREF(dml_passthrough_set_ml->slice) == dml_passthrough); + assert_se(UNIT_GET_SLICE(dml_passthrough_set_ml) == dml_passthrough); assert_se(manager_load_startable_unit_or_warn(m, "dml-override.slice", NULL, &dml_override) >= 0); - assert_se(UNIT_DEREF(dml_override->slice) == dml); + assert_se(UNIT_GET_SLICE(dml_override) == dml); assert_se(manager_load_startable_unit_or_warn(m, "dml-override-empty.service", NULL, &dml_override_empty) >= 0); - assert_se(UNIT_DEREF(dml_override_empty->slice) == dml_override); + assert_se(UNIT_GET_SLICE(dml_override_empty) == dml_override); assert_se(manager_load_startable_unit_or_warn(m, "dml-discard.slice", NULL, &dml_discard) >= 0); - assert_se(UNIT_DEREF(dml_discard->slice) == dml); + assert_se(UNIT_GET_SLICE(dml_discard) == dml); assert_se(manager_load_startable_unit_or_warn(m, "dml-discard-empty.service", NULL, &dml_discard_empty) >= 0); - assert_se(UNIT_DEREF(dml_discard_empty->slice) == dml_discard); + assert_se(UNIT_GET_SLICE(dml_discard_empty) == dml_discard); assert_se(manager_load_startable_unit_or_warn(m, "dml-discard-set-ml.service", NULL, &dml_discard_set_ml) >= 0); - assert_se(UNIT_DEREF(dml_discard_set_ml->slice) == dml_discard); + assert_se(UNIT_GET_SLICE(dml_discard_set_ml) == dml_discard); - root = UNIT_DEREF(dml->slice); - assert_se(!UNIT_ISSET(root->slice)); + assert_se(root = UNIT_GET_SLICE(dml)); + assert_se(!UNIT_GET_SLICE(root)); assert_se(unit_get_ancestor_memory_low(root) == CGROUP_LIMIT_MIN); From d219a2b07cc5dc8ffd5010f08561fab2780d8616 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 13 Apr 2021 18:37:25 +0200 Subject: [PATCH 06/18] core: convert Slice= into a proper dependency (and add a back dependency) The slice a unit is assigned to is currently a UnitRef reference. Let's turn it into a proper dependency, to simplify and clean up code a bit. Now that new dep types are cheaper, deps should generally be preferable over everything else, if the concept applies. This brings one major benefit: we often have to iterate through all unit a slice contains. So far we iterated through all Before= dependencies of the slice unit to achieve that, filtering out unrelated units, and taking benefit of the fact that slice units are implicitly ordered Before= the units they contain. By making Slice= a proper dependency, and having an accompanying SliceOf= dependency type, this is much simpler and nicer as we can directly enumerate the units a slice contains. The forward dependency is actually called InSlice internally, since we already used the UNIT_SLICE name as UnitType field. However, since we don't intend to expose the dependency to users as dep anyway (we already have the regular Slice D-Bus property for this) this shouldn't matter. The SliceOf= implicit dependency type (the erverse of Slice=/InSlice=) is exported over the bus, to make things a bit nicer to debug and discoverable. --- man/org.freedesktop.systemd1.xml | 6 +++++ src/basic/unit-def.c | 2 ++ src/basic/unit-def.h | 4 ++++ src/core/cgroup.c | 23 ++++-------------- src/core/dbus-unit.c | 3 ++- src/core/load-fragment.c | 2 +- src/core/slice.c | 21 ++++------------- src/core/unit-dependency-atom.c | 8 +++++++ src/core/unit-dependency-atom.h | 4 +++- src/core/unit.c | 40 +++++++++++++++++++++++--------- src/core/unit.h | 7 ++---- 11 files changed, 66 insertions(+), 54 deletions(-) diff --git a/man/org.freedesktop.systemd1.xml b/man/org.freedesktop.systemd1.xml index 3d6f1a39b3..de72412640 100644 --- a/man/org.freedesktop.systemd1.xml +++ b/man/org.freedesktop.systemd1.xml @@ -1640,6 +1640,8 @@ node /org/freedesktop/systemd1/unit/avahi_2ddaemon_2eservice { @org.freedesktop.DBus.Property.EmitsChangedSignal("const") readonly as JoinsNamespaceOf = ['...', ...]; @org.freedesktop.DBus.Property.EmitsChangedSignal("const") + readonly as SliceOf = ['...', ...]; + @org.freedesktop.DBus.Property.EmitsChangedSignal("const") readonly as RequiresMountsFor = ['...', ...]; @org.freedesktop.DBus.Property.EmitsChangedSignal("const") readonly as Documentation = ['...', ...]; @@ -1775,6 +1777,8 @@ node /org/freedesktop/systemd1/unit/avahi_2ddaemon_2eservice { + + @@ -1911,6 +1915,8 @@ node /org/freedesktop/systemd1/unit/avahi_2ddaemon_2eservice { + + diff --git a/src/basic/unit-def.c b/src/basic/unit-def.c index 5cfabca83f..5651620317 100644 --- a/src/basic/unit-def.c +++ b/src/basic/unit-def.c @@ -282,6 +282,8 @@ static const char* const unit_dependency_table[_UNIT_DEPENDENCY_MAX] = { [UNIT_JOINS_NAMESPACE_OF] = "JoinsNamespaceOf", [UNIT_REFERENCES] = "References", [UNIT_REFERENCED_BY] = "ReferencedBy", + [UNIT_IN_SLICE] = "InSlice", + [UNIT_SLICE_OF] = "SliceOf", }; DEFINE_STRING_TABLE_LOOKUP(unit_dependency, UnitDependency); diff --git a/src/basic/unit-def.h b/src/basic/unit-def.h index c30135f4d6..7d76576de1 100644 --- a/src/basic/unit-def.h +++ b/src/basic/unit-def.h @@ -245,6 +245,10 @@ typedef enum UnitDependency { UNIT_REFERENCES, /* Inverse of 'references' is 'referenced_by' */ UNIT_REFERENCED_BY, + /* Slice= */ + UNIT_IN_SLICE, + UNIT_SLICE_OF, + _UNIT_DEPENDENCY_MAX, _UNIT_DEPENDENCY_INVALID = -EINVAL, } UnitDependency; diff --git a/src/core/cgroup.c b/src/core/cgroup.c index 97820eb264..5b00faa6f6 100644 --- a/src/core/cgroup.c +++ b/src/core/cgroup.c @@ -1702,12 +1702,8 @@ CGroupMask unit_get_members_mask(Unit *u) { if (u->type == UNIT_SLICE) { Unit *member; - UNIT_FOREACH_DEPENDENCY(member, u, UNIT_ATOM_BEFORE) { - if (UNIT_DEREF(member->slice) != u) - continue; - + UNIT_FOREACH_DEPENDENCY(member, u, UNIT_ATOM_SLICE_OF) u->cgroup_members_mask |= unit_get_subtree_mask(member); /* note that this calls ourselves again, for the children */ - } } u->cgroup_members_mask_valid = true; @@ -2362,13 +2358,10 @@ static int unit_realize_cgroup_now_disable(Unit *u, ManagerState state) { if (u->type != UNIT_SLICE) return 0; - UNIT_FOREACH_DEPENDENCY(m, u, UNIT_ATOM_BEFORE) { + UNIT_FOREACH_DEPENDENCY(m, u, UNIT_ATOM_SLICE_OF) { CGroupMask target_mask, enable_mask, new_target_mask, new_enable_mask; int r; - if (UNIT_DEREF(m->slice) != u) - continue; - /* The cgroup for this unit might not actually be fully realised yet, in which case it isn't * holding any controllers open anyway. */ if (!m->cgroup_realized) @@ -2530,11 +2523,7 @@ void unit_add_family_to_cgroup_realize_queue(Unit *u) { /* Children of u likely changed when we're called */ u->cgroup_members_mask_valid = false; - UNIT_FOREACH_DEPENDENCY(m, u, UNIT_ATOM_BEFORE) { - - /* Skip units that have a dependency on the slice but aren't actually in it. */ - if (UNIT_DEREF(m->slice) != u) - continue; + UNIT_FOREACH_DEPENDENCY(m, u, UNIT_ATOM_SLICE_OF) { /* No point in doing cgroup application for units without active processes. */ if (UNIT_IS_INACTIVE_OR_FAILED(unit_active_state(m))) @@ -3799,12 +3788,8 @@ void unit_invalidate_cgroup_bpf(Unit *u) { if (u->type == UNIT_SLICE) { Unit *member; - UNIT_FOREACH_DEPENDENCY(member, u, UNIT_ATOM_BEFORE) { - if (UNIT_DEREF(member->slice) != u) - continue; - + UNIT_FOREACH_DEPENDENCY(member, u, UNIT_ATOM_SLICE_OF) unit_invalidate_cgroup_bpf(member); - } } } diff --git a/src/core/dbus-unit.c b/src/core/dbus-unit.c index e8818c919c..a1278dd6e9 100644 --- a/src/core/dbus-unit.c +++ b/src/core/dbus-unit.c @@ -870,6 +870,7 @@ const sd_bus_vtable bus_unit_vtable[] = { SD_BUS_PROPERTY("PropagatesReloadTo", "as", property_get_dependencies, 0, SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("ReloadPropagatedFrom", "as", property_get_dependencies, 0, SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("JoinsNamespaceOf", "as", property_get_dependencies, 0, SD_BUS_VTABLE_PROPERTY_CONST), + SD_BUS_PROPERTY("SliceOf", "as", property_get_dependencies, 0, SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("RequiresMountsFor", "as", property_get_requires_mounts_for, offsetof(Unit, requires_mounts_for), SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("Documentation", "as", NULL, offsetof(Unit, documentation), SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("Description", "s", property_get_description, 0, SD_BUS_VTABLE_PROPERTY_CONST), @@ -2237,7 +2238,7 @@ static int bus_unit_set_transient_property( return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "Unit name '%s' is not a slice", s); if (!UNIT_WRITE_FLAGS_NOOP(flags)) { - r = unit_set_slice(u, slice); + r = unit_set_slice(u, slice, UNIT_DEPENDENCY_FILE); if (r < 0) return r; diff --git a/src/core/load-fragment.c b/src/core/load-fragment.c index cd37314542..72378a4082 100644 --- a/src/core/load-fragment.c +++ b/src/core/load-fragment.c @@ -3574,7 +3574,7 @@ int config_parse_unit_slice( return 0; } - r = unit_set_slice(u, slice); + r = unit_set_slice(u, slice, UNIT_DEPENDENCY_FILE); if (r < 0) { log_syntax(unit, LOG_WARNING, filename, line, r, "Failed to assign slice %s to unit %s, ignoring: %m", slice->id, u->id); return 0; diff --git a/src/core/slice.c b/src/core/slice.c index 5d836a11e6..595f704a17 100644 --- a/src/core/slice.c +++ b/src/core/slice.c @@ -47,7 +47,7 @@ static void slice_set_state(Slice *t, SliceState state) { } static int slice_add_parent_slice(Slice *s) { - Unit *u = UNIT(s), *parent; + Unit *u = UNIT(s); _cleanup_free_ char *a = NULL; int r; @@ -60,12 +60,7 @@ static int slice_add_parent_slice(Slice *s) { if (r <= 0) /* 0 means root slice */ return r; - r = manager_load_unit(u->manager, a, NULL, NULL, &parent); - if (r < 0) - return r; - - unit_ref_set(&u->slice, u, parent); - return 0; + return unit_add_dependency_by_name(u, UNIT_IN_SLICE, a, true, UNIT_DEPENDENCY_IMPLICIT); } static int slice_add_default_dependencies(Slice *s) { @@ -346,14 +341,11 @@ static void slice_enumerate_perpetual(Manager *m) { static bool slice_freezer_action_supported_by_children(Unit *s) { Unit *member; + int r; assert(s); - UNIT_FOREACH_DEPENDENCY(member, s, UNIT_ATOM_BEFORE) { - int r; - - if (UNIT_DEREF(member->slice) != s) - continue; + UNIT_FOREACH_DEPENDENCY(member, s, UNIT_ATOM_SLICE_OF) { if (member->type == UNIT_SLICE) { r = slice_freezer_action_supported_by_children(member); @@ -380,10 +372,7 @@ static int slice_freezer_action(Unit *s, FreezerAction action) { return 0; } - UNIT_FOREACH_DEPENDENCY(member, s, UNIT_ATOM_BEFORE) { - if (UNIT_DEREF(member->slice) != s) - continue; - + UNIT_FOREACH_DEPENDENCY(member, s, UNIT_ATOM_SLICE_OF) { if (action == FREEZER_FREEZE) r = UNIT_VTABLE(member)->freeze(member); else diff --git a/src/core/unit-dependency-atom.c b/src/core/unit-dependency-atom.c index 4b012ed265..cfd5acd33b 100644 --- a/src/core/unit-dependency-atom.c +++ b/src/core/unit-dependency-atom.c @@ -78,6 +78,8 @@ static const UnitDependencyAtom atom_map[_UNIT_DEPENDENCY_MAX] = { [UNIT_JOINS_NAMESPACE_OF] = UNIT_ATOM_JOINS_NAMESPACE_OF, [UNIT_REFERENCES] = UNIT_ATOM_REFERENCES, [UNIT_REFERENCED_BY] = UNIT_ATOM_REFERENCED_BY, + [UNIT_IN_SLICE] = UNIT_ATOM_IN_SLICE, + [UNIT_SLICE_OF] = UNIT_ATOM_SLICE_OF, /* These are dependency types without effect on our state engine. We maintain them only to make * things discoverable/debuggable as they are the inverse dependencies to some of the above. As they @@ -190,6 +192,12 @@ UnitDependency unit_dependency_from_unique_atom(UnitDependencyAtom atom) { case UNIT_ATOM_REFERENCED_BY: return UNIT_REFERENCED_BY; + case UNIT_ATOM_IN_SLICE: + return UNIT_IN_SLICE; + + case UNIT_ATOM_SLICE_OF: + return UNIT_SLICE_OF; + default: return _UNIT_DEPENDENCY_INVALID; } diff --git a/src/core/unit-dependency-atom.h b/src/core/unit-dependency-atom.h index d04d6134a0..0148221511 100644 --- a/src/core/unit-dependency-atom.h +++ b/src/core/unit-dependency-atom.h @@ -69,7 +69,9 @@ typedef enum UnitDependencyAtom { UNIT_ATOM_JOINS_NAMESPACE_OF = UINT64_C(1) << 29, UNIT_ATOM_REFERENCES = UINT64_C(1) << 30, UNIT_ATOM_REFERENCED_BY = UINT64_C(1) << 31, - _UNIT_DEPENDENCY_ATOM_MAX = (UINT64_C(1) << 32) - 1, + UNIT_ATOM_IN_SLICE = UINT64_C(1) << 32, + UNIT_ATOM_SLICE_OF = UINT64_C(1) << 33, + _UNIT_DEPENDENCY_ATOM_MAX = (UINT64_C(1) << 34) - 1, _UNIT_DEPENDENCY_ATOM_INVALID = -EINVAL, } UnitDependencyAtom; diff --git a/src/core/unit.c b/src/core/unit.c index f3eaa59570..c94f4e7523 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -659,11 +659,10 @@ Unit* unit_free(Unit *u) { job_free(j); } - unit_clear_dependencies(u); - /* A unit is being dropped from the tree, make sure our family is realized properly. Do this after we * detach the unit from slice tree in order to eliminate its effect on controller masks. */ slice = UNIT_GET_SLICE(u); + unit_clear_dependencies(u); if (slice) unit_add_family_to_cgroup_realize_queue(slice); @@ -689,7 +688,6 @@ Unit* unit_free(Unit *u) { unit_unwatch_all_pids(u); - unit_ref_unset(&u->slice); while (u->refs_by_target) unit_ref_unset(u->refs_by_target); @@ -2881,6 +2879,8 @@ int unit_add_dependency( [UNIT_PROPAGATES_RELOAD_TO] = UNIT_RELOAD_PROPAGATED_FROM, [UNIT_RELOAD_PROPAGATED_FROM] = UNIT_PROPAGATES_RELOAD_TO, [UNIT_JOINS_NAMESPACE_OF] = UNIT_JOINS_NAMESPACE_OF, /* symmetric! 👓 */ + [UNIT_IN_SLICE] = UNIT_SLICE_OF, + [UNIT_SLICE_OF] = UNIT_IN_SLICE, }; Unit *original_u = u, *original_other = other; UnitDependencyAtom a; @@ -2924,6 +2924,21 @@ int unit_add_dependency( return log_unit_error_errno(u, SYNTHETIC_ERRNO(EINVAL), "Requested dependency TriggeredBy=%s refused (%s units cannot trigger other units).", other->id, unit_type_to_string(other->type)); + if (FLAGS_SET(a, UNIT_ATOM_IN_SLICE) && other->type != UNIT_SLICE) + return log_unit_error_errno(u, SYNTHETIC_ERRNO(EINVAL), + "Requested dependency Slice=%s refused (%s is not a slice unit).", other->id, other->id); + if (FLAGS_SET(a, UNIT_ATOM_SLICE_OF) && u->type != UNIT_SLICE) + return log_unit_error_errno(u, SYNTHETIC_ERRNO(EINVAL), + "Requested dependency SliceOf=%s refused (%s is not a slice unit).", other->id, u->id); + + if (FLAGS_SET(a, UNIT_ATOM_IN_SLICE) && !UNIT_HAS_CGROUP_CONTEXT(u)) + return log_unit_error_errno(u, SYNTHETIC_ERRNO(EINVAL), + "Requested dependency Slice=%s refused (%s is not a cgroup unit).", other->id, u->id); + + if (FLAGS_SET(a, UNIT_ATOM_SLICE_OF) && !UNIT_HAS_CGROUP_CONTEXT(other)) + return log_unit_error_errno(u, SYNTHETIC_ERRNO(EINVAL), + "Requested dependency SliceOf=%s refused (%s is not a cgroup unit).", other->id, other->id); + r = unit_add_dependency_hashmap(&u->dependencies, d, other, mask, 0); if (r < 0) return r; @@ -3102,15 +3117,15 @@ reset: return r; } -int unit_set_slice(Unit *u, Unit *slice) { +int unit_set_slice(Unit *u, Unit *slice, UnitDependencyMask mask) { + int r; + assert(u); assert(slice); - /* Sets the unit slice if it has not been set before. Is extra - * careful, to only allow this for units that actually have a - * cgroup context. Also, we don't allow to set this for slices - * (since the parent slice is derived from the name). Make - * sure the unit we set is actually a slice. */ + /* Sets the unit slice if it has not been set before. Is extra careful, to only allow this for units + * that actually have a cgroup context. Also, we don't allow to set this for slices (since the parent + * slice is derived from the name). Make sure the unit we set is actually a slice. */ if (!UNIT_HAS_CGROUP_CONTEXT(u)) return -EOPNOTSUPP; @@ -3135,7 +3150,10 @@ int unit_set_slice(Unit *u, Unit *slice) { if (UNIT_GET_SLICE(u) && u->cgroup_realized) return -EBUSY; - unit_ref_set(&u->slice, u, slice); + r = unit_add_dependency(u, UNIT_IN_SLICE, slice, true, mask); + if (r < 0) + return r; + return 1; } @@ -3185,7 +3203,7 @@ int unit_set_default_slice(Unit *u) { if (r < 0) return r; - return unit_set_slice(u, slice); + return unit_set_slice(u, slice, UNIT_DEPENDENCY_FILE); } const char *unit_slice_name(Unit *u) { diff --git a/src/core/unit.h b/src/core/unit.h index cf110ebb55..6b3f48596f 100644 --- a/src/core/unit.h +++ b/src/core/unit.h @@ -202,8 +202,6 @@ typedef struct Unit { dual_timestamp active_exit_timestamp; dual_timestamp inactive_enter_timestamp; - UnitRef slice; - /* Per type list */ LIST_FIELDS(Unit, units_by_type); @@ -707,8 +705,7 @@ static inline Unit* UNIT_TRIGGER(Unit *u) { } static inline Unit* UNIT_GET_SLICE(const Unit *u) { - assert(u); - return u->slice.target; + return unit_has_dependency(u, UNIT_ATOM_IN_SLICE, NULL); } Unit* unit_new(Manager *m, size_t size); @@ -751,7 +748,7 @@ Unit *unit_follow_merge(Unit *u) _pure_; int unit_load_fragment_and_dropin(Unit *u, bool fragment_required); int unit_load(Unit *unit); -int unit_set_slice(Unit *u, Unit *slice); +int unit_set_slice(Unit *u, Unit *slice, UnitDependencyMask mask); int unit_set_default_slice(Unit *u); const char *unit_description(Unit *u) _pure_; From 39628fedac5b35fe9d3c7a674c68a99f2cba4d28 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 13 Apr 2021 20:24:15 +0200 Subject: [PATCH 07/18] core: hide cgroup fields in unit_dump() for non-cgroup unit types A bunch of properties in the main Unit strcture only make sense for cgroup units. Let's hide them from unit types that have no relation to cgroups. --- src/core/unit-serialize.c | 74 +++++++++++++++++++++------------------ 1 file changed, 39 insertions(+), 35 deletions(-) diff --git a/src/core/unit-serialize.c b/src/core/unit-serialize.c index 6384174a2b..caee3a2823 100644 --- a/src/core/unit-serialize.c +++ b/src/core/unit-serialize.c @@ -604,10 +604,7 @@ void unit_dump(Unit *u, FILE *f, const char *prefix) { "%s\tNeed Daemon Reload: %s\n" "%s\tTransient: %s\n" "%s\tPerpetual: %s\n" - "%s\tGarbage Collection Mode: %s\n" - "%s\tSlice: %s\n" - "%s\tCGroup: %s\n" - "%s\tCGroup realized: %s\n", + "%s\tGarbage Collection Mode: %s\n", prefix, unit_description(u), prefix, strna(u->instance), prefix, unit_load_state_to_string(u->load_state), @@ -621,10 +618,7 @@ void unit_dump(Unit *u, FILE *f, const char *prefix) { prefix, yes_no(unit_need_daemon_reload(u)), prefix, yes_no(u->transient), prefix, yes_no(u->perpetual), - prefix, collect_mode_to_string(u->collect_mode), - prefix, strna(unit_slice_name(u)), - prefix, strna(u->cgroup_path), - prefix, yes_no(u->cgroup_realized)); + prefix, collect_mode_to_string(u->collect_mode)); if (u->markers != 0) { fprintf(f, "%s\tMarkers:", prefix); @@ -635,37 +629,47 @@ void unit_dump(Unit *u, FILE *f, const char *prefix) { fputs("\n", f); } - if (u->cgroup_realized_mask != 0) { - _cleanup_free_ char *s = NULL; - (void) cg_mask_to_string(u->cgroup_realized_mask, &s); - fprintf(f, "%s\tCGroup realized mask: %s\n", prefix, strnull(s)); - } + if (UNIT_HAS_CGROUP_CONTEXT(u)) { + fprintf(f, + "%s\tSlice: %s\n" + "%s\tCGroup: %s\n" + "%s\tCGroup realized: %s\n", + prefix, strna(unit_slice_name(u)), + prefix, strna(u->cgroup_path), + prefix, yes_no(u->cgroup_realized)); - if (u->cgroup_enabled_mask != 0) { - _cleanup_free_ char *s = NULL; - (void) cg_mask_to_string(u->cgroup_enabled_mask, &s); - fprintf(f, "%s\tCGroup enabled mask: %s\n", prefix, strnull(s)); - } + if (u->cgroup_realized_mask != 0) { + _cleanup_free_ char *s = NULL; + (void) cg_mask_to_string(u->cgroup_realized_mask, &s); + fprintf(f, "%s\tCGroup realized mask: %s\n", prefix, strnull(s)); + } - m = unit_get_own_mask(u); - if (m != 0) { - _cleanup_free_ char *s = NULL; - (void) cg_mask_to_string(m, &s); - fprintf(f, "%s\tCGroup own mask: %s\n", prefix, strnull(s)); - } + if (u->cgroup_enabled_mask != 0) { + _cleanup_free_ char *s = NULL; + (void) cg_mask_to_string(u->cgroup_enabled_mask, &s); + fprintf(f, "%s\tCGroup enabled mask: %s\n", prefix, strnull(s)); + } - m = unit_get_members_mask(u); - if (m != 0) { - _cleanup_free_ char *s = NULL; - (void) cg_mask_to_string(m, &s); - fprintf(f, "%s\tCGroup members mask: %s\n", prefix, strnull(s)); - } + m = unit_get_own_mask(u); + if (m != 0) { + _cleanup_free_ char *s = NULL; + (void) cg_mask_to_string(m, &s); + fprintf(f, "%s\tCGroup own mask: %s\n", prefix, strnull(s)); + } - m = unit_get_delegate_mask(u); - if (m != 0) { - _cleanup_free_ char *s = NULL; - (void) cg_mask_to_string(m, &s); - fprintf(f, "%s\tCGroup delegate mask: %s\n", prefix, strnull(s)); + m = unit_get_members_mask(u); + if (m != 0) { + _cleanup_free_ char *s = NULL; + (void) cg_mask_to_string(m, &s); + fprintf(f, "%s\tCGroup members mask: %s\n", prefix, strnull(s)); + } + + m = unit_get_delegate_mask(u); + if (m != 0) { + _cleanup_free_ char *s = NULL; + (void) cg_mask_to_string(m, &s); + fprintf(f, "%s\tCGroup delegate mask: %s\n", prefix, strnull(s)); + } } if (!sd_id128_is_null(u->invocation_id)) From 629b2a6f7be7b5b7ec2c35aceeeb82cedc08bef3 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 13 Apr 2021 20:50:21 +0200 Subject: [PATCH 08/18] core: add a reverse dep for OnFailure= Let's add an implicit reverse dep OnFailureOf=. This is exposed via the bus to make things more debuggable: you can now ask systemd for which units a specific unit is the failure handler. OnFailure= was the only dependency type that had no inverse, this fixes that. Now that deps are a bit cheaper, it should be OK to add deps that only serve debug purposes. --- man/org.freedesktop.systemd1.xml | 6 ++++++ src/basic/unit-def.c | 1 + src/basic/unit-def.h | 1 + src/core/dbus-unit.c | 1 + src/core/unit-dependency-atom.c | 1 + src/core/unit.c | 3 ++- 6 files changed, 12 insertions(+), 1 deletion(-) diff --git a/man/org.freedesktop.systemd1.xml b/man/org.freedesktop.systemd1.xml index de72412640..36bd7173d4 100644 --- a/man/org.freedesktop.systemd1.xml +++ b/man/org.freedesktop.systemd1.xml @@ -1630,6 +1630,8 @@ node /org/freedesktop/systemd1/unit/avahi_2ddaemon_2eservice { @org.freedesktop.DBus.Property.EmitsChangedSignal("const") readonly as OnFailure = ['...', ...]; @org.freedesktop.DBus.Property.EmitsChangedSignal("const") + readonly as OnFailureOf = ['...', ...]; + @org.freedesktop.DBus.Property.EmitsChangedSignal("const") readonly as Triggers = ['...', ...]; @org.freedesktop.DBus.Property.EmitsChangedSignal("const") readonly as TriggeredBy = ['...', ...]; @@ -1773,6 +1775,8 @@ node /org/freedesktop/systemd1/unit/avahi_2ddaemon_2eservice { + + @@ -1905,6 +1909,8 @@ node /org/freedesktop/systemd1/unit/avahi_2ddaemon_2eservice { + + diff --git a/src/basic/unit-def.c b/src/basic/unit-def.c index 5651620317..fd7615de88 100644 --- a/src/basic/unit-def.c +++ b/src/basic/unit-def.c @@ -275,6 +275,7 @@ static const char* const unit_dependency_table[_UNIT_DEPENDENCY_MAX] = { [UNIT_BEFORE] = "Before", [UNIT_AFTER] = "After", [UNIT_ON_FAILURE] = "OnFailure", + [UNIT_ON_FAILURE_OF] = "OnFailureOf", [UNIT_TRIGGERS] = "Triggers", [UNIT_TRIGGERED_BY] = "TriggeredBy", [UNIT_PROPAGATES_RELOAD_TO] = "PropagatesReloadTo", diff --git a/src/basic/unit-def.h b/src/basic/unit-def.h index 7d76576de1..ec40bab01f 100644 --- a/src/basic/unit-def.h +++ b/src/basic/unit-def.h @@ -229,6 +229,7 @@ typedef enum UnitDependency { /* On Failure */ UNIT_ON_FAILURE, + UNIT_ON_FAILURE_OF, /* Triggers (i.e. a socket triggers a service) */ UNIT_TRIGGERS, diff --git a/src/core/dbus-unit.c b/src/core/dbus-unit.c index a1278dd6e9..6ca16262ab 100644 --- a/src/core/dbus-unit.c +++ b/src/core/dbus-unit.c @@ -865,6 +865,7 @@ const sd_bus_vtable bus_unit_vtable[] = { SD_BUS_PROPERTY("Before", "as", property_get_dependencies, 0, SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("After", "as", property_get_dependencies, 0, SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("OnFailure", "as", property_get_dependencies, 0, SD_BUS_VTABLE_PROPERTY_CONST), + SD_BUS_PROPERTY("OnFailureOf", "as", property_get_dependencies, 0, SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("Triggers", "as", property_get_dependencies, 0, SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("TriggeredBy", "as", property_get_dependencies, 0, SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("PropagatesReloadTo", "as", property_get_dependencies, 0, SD_BUS_VTABLE_PROPERTY_CONST), diff --git a/src/core/unit-dependency-atom.c b/src/core/unit-dependency-atom.c index cfd5acd33b..c4fbfee015 100644 --- a/src/core/unit-dependency-atom.c +++ b/src/core/unit-dependency-atom.c @@ -85,6 +85,7 @@ static const UnitDependencyAtom atom_map[_UNIT_DEPENDENCY_MAX] = { * things discoverable/debuggable as they are the inverse dependencies to some of the above. As they * have no effect of their own, they all map to no atoms at all, i.e. the value 0. */ [UNIT_RELOAD_PROPAGATED_FROM] = 0, + [UNIT_ON_FAILURE_OF] = 0, }; UnitDependencyAtom unit_dependency_to_atom(UnitDependency d) { diff --git a/src/core/unit.c b/src/core/unit.c index c94f4e7523..708c725b49 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -2871,7 +2871,8 @@ int unit_add_dependency( [UNIT_CONFLICTED_BY] = UNIT_CONFLICTS, [UNIT_BEFORE] = UNIT_AFTER, [UNIT_AFTER] = UNIT_BEFORE, - [UNIT_ON_FAILURE] = _UNIT_DEPENDENCY_INVALID, + [UNIT_ON_FAILURE] = UNIT_ON_FAILURE_OF, + [UNIT_ON_FAILURE_OF] = UNIT_ON_FAILURE, [UNIT_REFERENCES] = UNIT_REFERENCED_BY, [UNIT_REFERENCED_BY] = UNIT_REFERENCES, [UNIT_TRIGGERS] = UNIT_TRIGGERED_BY, From ffec78c05bfc2e6458e05ee54256d0d766a36280 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 13 Apr 2021 22:30:31 +0200 Subject: [PATCH 09/18] core: add new PropagateStopTo= dependency (and inverse) This takes inspiration from PropagatesReloadTo=, but propagates stop jobs instead of restart jobs. This is defined based on exactly two atoms: UNIT_ATOM_PROPAGATE_STOP + UNIT_ATOM_RETROACTIVE_STOP_ON_STOP. The former ensures that when the unit the dependency is originating from is stopped based on user request, we'll propagate the stop job to the target unit, too. In addition, when the originating unit suddenly stops from external causes the stopping is propagated too. Note that this does *not* include the UNIT_ATOM_CANNOT_BE_ACTIVE_WITHOUT atom (which is used by BoundBy=), i.e. this dependency is purely about propagating "edges" and not "levels", i.e. it's about propagating specific events, instead of continious states. This is supposed to be useful for dependencies between .mount units and their backing .device units. So far we either placed a BindsTo= or Requires= dependency between them. The former gave a very clear binding of the to units together, however was problematic if users establish mounnts manually with different block device sources than our configuration defines, as we there might come to the conclusion that the backing device was absent and thus we need to umount again what the user mounted. By combining Requires= with the new StopPropagatedFrom= (i.e. the inverse PropagateStopTo=) we can get behaviour that matches BindsTo= in every single atom but one: UNIT_ATOM_CANNOT_BE_ACTIVE_WITHOUT is absent, and hence the level-triggered logic doesn't apply. Replaces: #11340 --- man/org.freedesktop.systemd1.xml | 12 ++++++++++++ man/systemd.unit.xml | 21 ++++++++++++++------- src/basic/unit-def.c | 2 ++ src/basic/unit-def.h | 4 ++++ src/core/dbus-unit.c | 4 ++++ src/core/load-fragment-gperf.gperf.in | 2 ++ src/core/unit-dependency-atom.c | 5 ++++- src/core/unit.c | 2 ++ test/fuzz/fuzz-unit-file/directives.service | 2 ++ 9 files changed, 46 insertions(+), 8 deletions(-) diff --git a/man/org.freedesktop.systemd1.xml b/man/org.freedesktop.systemd1.xml index 36bd7173d4..13322746fa 100644 --- a/man/org.freedesktop.systemd1.xml +++ b/man/org.freedesktop.systemd1.xml @@ -1640,6 +1640,10 @@ node /org/freedesktop/systemd1/unit/avahi_2ddaemon_2eservice { @org.freedesktop.DBus.Property.EmitsChangedSignal("const") readonly as ReloadPropagatedFrom = ['...', ...]; @org.freedesktop.DBus.Property.EmitsChangedSignal("const") + readonly as PropagatesStopTo = ['...', ...]; + @org.freedesktop.DBus.Property.EmitsChangedSignal("const") + readonly as StopPropagatedFrom = ['...', ...]; + @org.freedesktop.DBus.Property.EmitsChangedSignal("const") readonly as JoinsNamespaceOf = ['...', ...]; @org.freedesktop.DBus.Property.EmitsChangedSignal("const") readonly as SliceOf = ['...', ...]; @@ -1779,6 +1783,10 @@ node /org/freedesktop/systemd1/unit/avahi_2ddaemon_2eservice { + + + + @@ -1919,6 +1927,10 @@ node /org/freedesktop/systemd1/unit/avahi_2ddaemon_2eservice { + + + + diff --git a/man/systemd.unit.xml b/man/systemd.unit.xml index 913cad05b4..07ea21a6e7 100644 --- a/man/systemd.unit.xml +++ b/man/systemd.unit.xml @@ -787,13 +787,20 @@ PropagatesReloadTo= ReloadPropagatedFrom= - A space-separated list of one or more units - where reload requests on this unit will be propagated to, or - reload requests on the other unit will be propagated to this - unit, respectively. Issuing a reload request on a unit will - automatically also enqueue a reload request on all units that - the reload request shall be propagated to via these two - settings. + A space-separated list of one or more units to which reload requests from this unit + shall be propagated to, or units from which reload requests shall be propagated to this unit, + respectively. Issuing a reload request on a unit will automatically also enqueue reload requests on + all units that are linked to it using these two settings. + + + + PropagatesStopTo= + StopPropagatedFrom= + + A space-separated list of one or more units to which stop requests from this unit + shall be propagated to, or units from which stop requests shall be propagated to this unit, + respectively. Issuing a stop request on a unit will automatically also enqueue stop requests on all + units that are linked to it using these two settings. diff --git a/src/basic/unit-def.c b/src/basic/unit-def.c index fd7615de88..32be5219fc 100644 --- a/src/basic/unit-def.c +++ b/src/basic/unit-def.c @@ -280,6 +280,8 @@ static const char* const unit_dependency_table[_UNIT_DEPENDENCY_MAX] = { [UNIT_TRIGGERED_BY] = "TriggeredBy", [UNIT_PROPAGATES_RELOAD_TO] = "PropagatesReloadTo", [UNIT_RELOAD_PROPAGATED_FROM] = "ReloadPropagatedFrom", + [UNIT_PROPAGATES_STOP_TO] = "PropagatesStopTo", + [UNIT_STOP_PROPAGATED_FROM] = "StopPropagatedFrom", [UNIT_JOINS_NAMESPACE_OF] = "JoinsNamespaceOf", [UNIT_REFERENCES] = "References", [UNIT_REFERENCED_BY] = "ReferencedBy", diff --git a/src/basic/unit-def.h b/src/basic/unit-def.h index ec40bab01f..dc5aa5555d 100644 --- a/src/basic/unit-def.h +++ b/src/basic/unit-def.h @@ -239,6 +239,10 @@ typedef enum UnitDependency { UNIT_PROPAGATES_RELOAD_TO, UNIT_RELOAD_PROPAGATED_FROM, + /* Propagate stops */ + UNIT_PROPAGATES_STOP_TO, + UNIT_STOP_PROPAGATED_FROM, + /* Joins namespace of */ UNIT_JOINS_NAMESPACE_OF, diff --git a/src/core/dbus-unit.c b/src/core/dbus-unit.c index 6ca16262ab..5e5a0a33a1 100644 --- a/src/core/dbus-unit.c +++ b/src/core/dbus-unit.c @@ -870,6 +870,8 @@ const sd_bus_vtable bus_unit_vtable[] = { SD_BUS_PROPERTY("TriggeredBy", "as", property_get_dependencies, 0, SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("PropagatesReloadTo", "as", property_get_dependencies, 0, SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("ReloadPropagatedFrom", "as", property_get_dependencies, 0, SD_BUS_VTABLE_PROPERTY_CONST), + SD_BUS_PROPERTY("PropagatesStopTo", "as", property_get_dependencies, 0, SD_BUS_VTABLE_PROPERTY_CONST), + SD_BUS_PROPERTY("StopPropagatedFrom", "as", property_get_dependencies, 0, SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("JoinsNamespaceOf", "as", property_get_dependencies, 0, SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("SliceOf", "as", property_get_dependencies, 0, SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("RequiresMountsFor", "as", property_get_requires_mounts_for, offsetof(Unit, requires_mounts_for), SD_BUS_VTABLE_PROPERTY_CONST), @@ -2302,6 +2304,8 @@ static int bus_unit_set_transient_property( UNIT_ON_FAILURE, UNIT_PROPAGATES_RELOAD_TO, UNIT_RELOAD_PROPAGATED_FROM, + UNIT_PROPAGATES_STOP_TO, + UNIT_STOP_PROPAGATED_FROM, UNIT_JOINS_NAMESPACE_OF)) return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "Dependency type %s may not be created transiently.", unit_dependency_to_string(d)); diff --git a/src/core/load-fragment-gperf.gperf.in b/src/core/load-fragment-gperf.gperf.in index 98ece8054c..bb3c4453df 100644 --- a/src/core/load-fragment-gperf.gperf.in +++ b/src/core/load-fragment-gperf.gperf.in @@ -269,6 +269,8 @@ Unit.PropagatesReloadTo, config_parse_unit_deps, Unit.PropagateReloadTo, config_parse_unit_deps, UNIT_PROPAGATES_RELOAD_TO, 0 Unit.ReloadPropagatedFrom, config_parse_unit_deps, UNIT_RELOAD_PROPAGATED_FROM, 0 Unit.PropagateReloadFrom, config_parse_unit_deps, UNIT_RELOAD_PROPAGATED_FROM, 0 +Unit.PropagatesStopTo, config_parse_unit_deps, UNIT_PROPAGATES_STOP_TO, 0 +Unit.StopPropagatedFrom, config_parse_unit_deps, UNIT_STOP_PROPAGATED_FROM, 0 Unit.PartOf, config_parse_unit_deps, UNIT_PART_OF, 0 Unit.JoinsNamespaceOf, config_parse_unit_deps, UNIT_JOINS_NAMESPACE_OF, 0 Unit.RequiresOverridable, config_parse_obsolete_unit_deps, UNIT_REQUIRES, 0 diff --git a/src/core/unit-dependency-atom.c b/src/core/unit-dependency-atom.c index c4fbfee015..eaeb1d389e 100644 --- a/src/core/unit-dependency-atom.c +++ b/src/core/unit-dependency-atom.c @@ -68,6 +68,9 @@ static const UnitDependencyAtom atom_map[_UNIT_DEPENDENCY_MAX] = { UNIT_ATOM_RETROACTIVE_STOP_ON_START | UNIT_ATOM_PROPAGATE_STOP_FAILURE, + [UNIT_PROPAGATES_STOP_TO] = UNIT_ATOM_RETROACTIVE_STOP_ON_STOP | + UNIT_ATOM_PROPAGATE_STOP, + /* These are simple dependency types: they consist of a single atom only */ [UNIT_BEFORE] = UNIT_ATOM_BEFORE, [UNIT_AFTER] = UNIT_ATOM_AFTER, @@ -86,6 +89,7 @@ static const UnitDependencyAtom atom_map[_UNIT_DEPENDENCY_MAX] = { * have no effect of their own, they all map to no atoms at all, i.e. the value 0. */ [UNIT_RELOAD_PROPAGATED_FROM] = 0, [UNIT_ON_FAILURE_OF] = 0, + [UNIT_STOP_PROPAGATED_FROM] = 0, }; UnitDependencyAtom unit_dependency_to_atom(UnitDependency d) { @@ -149,7 +153,6 @@ UnitDependency unit_dependency_from_unique_atom(UnitDependencyAtom atom) { UNIT_ATOM_PROPAGATE_START_FAILURE | UNIT_ATOM_PINS_STOP_WHEN_UNNEEDED | UNIT_ATOM_DEFAULT_TARGET_DEPENDENCIES: - case UNIT_ATOM_RETROACTIVE_STOP_ON_STOP: return UNIT_BOUND_BY; case UNIT_ATOM_PULL_IN_STOP | diff --git a/src/core/unit.c b/src/core/unit.c index 708c725b49..ed6f87d960 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -2879,6 +2879,8 @@ int unit_add_dependency( [UNIT_TRIGGERED_BY] = UNIT_TRIGGERS, [UNIT_PROPAGATES_RELOAD_TO] = UNIT_RELOAD_PROPAGATED_FROM, [UNIT_RELOAD_PROPAGATED_FROM] = UNIT_PROPAGATES_RELOAD_TO, + [UNIT_PROPAGATES_STOP_TO] = UNIT_STOP_PROPAGATED_FROM, + [UNIT_STOP_PROPAGATED_FROM] = UNIT_PROPAGATES_STOP_TO, [UNIT_JOINS_NAMESPACE_OF] = UNIT_JOINS_NAMESPACE_OF, /* symmetric! 👓 */ [UNIT_IN_SLICE] = UNIT_SLICE_OF, [UNIT_SLICE_OF] = UNIT_IN_SLICE, diff --git a/test/fuzz/fuzz-unit-file/directives.service b/test/fuzz/fuzz-unit-file/directives.service index 73e1707c5c..95ab9a1c32 100644 --- a/test/fuzz/fuzz-unit-file/directives.service +++ b/test/fuzz/fuzz-unit-file/directives.service @@ -80,6 +80,7 @@ PartOf= PropagateReloadFrom= PropagateReloadTo= PropagatesReloadTo= +PropagatesStopTo= RebootArgument= RefuseManualStart= RefuseManualStop= @@ -97,6 +98,7 @@ StartLimitBurst= StartLimitInterval= StartLimitIntervalSec= StopWhenUnneeded= +StopPropagatedFrom= SuccessAction= SuccessActionExitStatus= Wants= From 47cd17ead4bd773691708f3e81a7d047b5692e51 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 14 Apr 2021 11:22:44 +0200 Subject: [PATCH 10/18] =?UTF-8?q?core:=20use=20StopPropagatedFrom=3D=20as?= =?UTF-8?q?=20default=20for=20.mount=20=E2=86=92=20.device=20unit=20depend?= =?UTF-8?q?encies?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Let's make use of the new dependency type for .mount/.device units, after all we added it for this purpose. Fixes: #9869 --- src/core/mount.c | 71 ++++++++++++++++++++++++++++++++---------------- 1 file changed, 47 insertions(+), 24 deletions(-) diff --git a/src/core/mount.c b/src/core/mount.c index ca5d0939a1..b496268164 100644 --- a/src/core/mount.c +++ b/src/core/mount.c @@ -66,6 +66,24 @@ static bool MOUNT_STATE_WITH_PROCESS(MountState state) { MOUNT_CLEANING); } +static MountParameters* get_mount_parameters_fragment(Mount *m) { + assert(m); + + if (m->from_fragment) + return &m->parameters_fragment; + + return NULL; +} + +static MountParameters* get_mount_parameters(Mount *m) { + assert(m); + + if (m->from_proc_self_mountinfo) + return &m->parameters_proc_self_mountinfo; + + return get_mount_parameters_fragment(m); +} + static bool mount_is_automount(const MountParameters *p) { assert(p); @@ -116,16 +134,33 @@ static bool mount_is_bind(const MountParameters *p) { return false; } -static bool mount_is_bound_to_device(const Mount *m) { +static bool mount_is_bound_to_device(Mount *m) { const MountParameters *p; - if (m->from_fragment) - return true; + assert(m); + + /* Determines whether to place a Requires= or BindsTo= dependency on the backing device unit. We do + * this by checking for the x-systemd.device-bound mount option. Iff it is set we use BindsTo=, + * otherwise Requires=. But note that we might combine the latter with StopPropagatedFrom=, see + * below. */ + + p = get_mount_parameters(m); + if (!p) + return false; - p = &m->parameters_proc_self_mountinfo; return fstab_test_option(p->options, "x-systemd.device-bound\0"); } +static bool mount_propagate_stop(Mount *m) { + assert(m); + + if (mount_is_bound_to_device(m)) /* If we are using BindsTo= the stop propagation is implicit, no need to bother */ + return false; + + return m->from_fragment; /* let's propagate stop whenever this is an explicitly configured unit, + * otherwise let's not bother. */ +} + static bool mount_needs_quota(const MountParameters *p) { assert(p); @@ -234,24 +269,6 @@ static void mount_done(Unit *u) { m->timer_event_source = sd_event_source_unref(m->timer_event_source); } -static MountParameters* get_mount_parameters_fragment(Mount *m) { - assert(m); - - if (m->from_fragment) - return &m->parameters_fragment; - - return NULL; -} - -static MountParameters* get_mount_parameters(Mount *m) { - assert(m); - - if (m->from_proc_self_mountinfo) - return &m->parameters_proc_self_mountinfo; - - return get_mount_parameters_fragment(m); -} - static int update_parameters_proc_self_mountinfo( Mount *m, const char *what, @@ -367,8 +384,9 @@ static int mount_add_device_dependencies(Mount *m) { return 0; /* Mount units from /proc/self/mountinfo are not bound to devices by default since they're subject to - * races when devices are unplugged. But the user can still force this dep with an appropriate option - * (or udev property) so the mount units are automatically stopped when the device disappears + * races when mounts are established by other tools with different backing devices than what we + * maintain. The user can still force this to be a BindsTo= dependency with an appropriate option (or + * udev property) so the mount units are automatically stopped when the device disappears * suddenly. */ dep = mount_is_bound_to_device(m) ? UNIT_BINDS_TO : UNIT_REQUIRES; @@ -378,6 +396,11 @@ static int mount_add_device_dependencies(Mount *m) { r = unit_add_node_dependency(UNIT(m), p->what, dep, mask); if (r < 0) return r; + if (mount_propagate_stop(m)) { + r = unit_add_node_dependency(UNIT(m), p->what, UNIT_STOP_PROPAGATED_FROM, mask); + if (r < 0) + return r; + } return unit_add_blockdev_dependency(UNIT(m), p->what, mask); } From 294446dcb98eaa1ced5839da674c2065b1367a3a Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 14 Apr 2021 14:36:15 +0200 Subject: [PATCH 11/18] core: add new OnSuccess= dependency type This is similar to OnFailure= but is activated whenever a unit returns into inactive state successfully. I was always afraid of adding this, since it effectively allows building loops and makes our engine Turing complete, but it pretty much already was it was just hidden. Given that we have per-unit ratelimits as well as an event loop global ratelimit I feel safe to add this finally, given it actually is useful. Fixes: #13386 --- man/org.freedesktop.systemd1.xml | 18 +++++ man/systemd.unit.xml | 15 ++-- src/basic/unit-def.c | 2 + src/basic/unit-def.h | 4 +- src/core/dbus-unit.c | 7 ++ src/core/job.c | 2 +- src/core/load-fragment-gperf.gperf.in | 2 + src/core/unit-dependency-atom.c | 5 ++ src/core/unit-dependency-atom.h | 1 + src/core/unit-serialize.c | 2 + src/core/unit.c | 77 ++++++++++++++++----- src/core/unit.h | 5 +- test/fuzz/fuzz-unit-file/directives.service | 2 + 13 files changed, 115 insertions(+), 27 deletions(-) diff --git a/man/org.freedesktop.systemd1.xml b/man/org.freedesktop.systemd1.xml index 13322746fa..d55f3c8f9b 100644 --- a/man/org.freedesktop.systemd1.xml +++ b/man/org.freedesktop.systemd1.xml @@ -1632,6 +1632,10 @@ node /org/freedesktop/systemd1/unit/avahi_2ddaemon_2eservice { @org.freedesktop.DBus.Property.EmitsChangedSignal("const") readonly as OnFailureOf = ['...', ...]; @org.freedesktop.DBus.Property.EmitsChangedSignal("const") + readonly as OnSuccess = ['...', ...]; + @org.freedesktop.DBus.Property.EmitsChangedSignal("const") + readonly as OnSuccessOf = ['...', ...]; + @org.freedesktop.DBus.Property.EmitsChangedSignal("const") readonly as Triggers = ['...', ...]; @org.freedesktop.DBus.Property.EmitsChangedSignal("const") readonly as TriggeredBy = ['...', ...]; @@ -1702,6 +1706,8 @@ node /org/freedesktop/systemd1/unit/avahi_2ddaemon_2eservice { @org.freedesktop.DBus.Property.EmitsChangedSignal("const") readonly b DefaultDependencies = ...; @org.freedesktop.DBus.Property.EmitsChangedSignal("const") + readonly s OnSuccesJobMode = '...'; + @org.freedesktop.DBus.Property.EmitsChangedSignal("const") readonly s OnFailureJobMode = '...'; @org.freedesktop.DBus.Property.EmitsChangedSignal("const") readonly b IgnoreOnIsolate = ...; @@ -1781,6 +1787,10 @@ node /org/freedesktop/systemd1/unit/avahi_2ddaemon_2eservice { + + + + @@ -1805,6 +1815,8 @@ node /org/freedesktop/systemd1/unit/avahi_2ddaemon_2eservice { + + @@ -1919,6 +1931,10 @@ node /org/freedesktop/systemd1/unit/avahi_2ddaemon_2eservice { + + + + @@ -2003,6 +2019,8 @@ node /org/freedesktop/systemd1/unit/avahi_2ddaemon_2eservice { + + diff --git a/man/systemd.unit.xml b/man/systemd.unit.xml index 07ea21a6e7..d1d7a9f6e0 100644 --- a/man/systemd.unit.xml +++ b/man/systemd.unit.xml @@ -776,11 +776,16 @@ OnFailure= - A space-separated list of one or more units - that are activated when this unit enters the - failed state. A service unit using - Restart= enters the failed state only after - the start limits are reached. + A space-separated list of one or more units that are activated when this unit enters + the failed state. A service unit using Restart= enters the + failed state only after the start limits are reached. + + + + OnSuccess= + + A space-separated list of one or more units that are activated when this unit enters + the inactive state. diff --git a/src/basic/unit-def.c b/src/basic/unit-def.c index 32be5219fc..f83e398a4b 100644 --- a/src/basic/unit-def.c +++ b/src/basic/unit-def.c @@ -274,6 +274,8 @@ static const char* const unit_dependency_table[_UNIT_DEPENDENCY_MAX] = { [UNIT_CONFLICTED_BY] = "ConflictedBy", [UNIT_BEFORE] = "Before", [UNIT_AFTER] = "After", + [UNIT_ON_SUCCESS] = "OnSuccess", + [UNIT_ON_SUCCESS_OF] = "OnSuccessOf", [UNIT_ON_FAILURE] = "OnFailure", [UNIT_ON_FAILURE_OF] = "OnFailureOf", [UNIT_TRIGGERS] = "Triggers", diff --git a/src/basic/unit-def.h b/src/basic/unit-def.h index dc5aa5555d..92b40e8a3d 100644 --- a/src/basic/unit-def.h +++ b/src/basic/unit-def.h @@ -227,7 +227,9 @@ typedef enum UnitDependency { UNIT_BEFORE, /* inverse of 'before' is 'after' and vice versa */ UNIT_AFTER, - /* On Failure */ + /* OnSuccess= + OnFailure= */ + UNIT_ON_SUCCESS, + UNIT_ON_SUCCESS_OF, UNIT_ON_FAILURE, UNIT_ON_FAILURE_OF, diff --git a/src/core/dbus-unit.c b/src/core/dbus-unit.c index 5e5a0a33a1..60f406a160 100644 --- a/src/core/dbus-unit.c +++ b/src/core/dbus-unit.c @@ -866,6 +866,8 @@ const sd_bus_vtable bus_unit_vtable[] = { SD_BUS_PROPERTY("After", "as", property_get_dependencies, 0, SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("OnFailure", "as", property_get_dependencies, 0, SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("OnFailureOf", "as", property_get_dependencies, 0, SD_BUS_VTABLE_PROPERTY_CONST), + SD_BUS_PROPERTY("OnSuccess", "as", property_get_dependencies, 0, SD_BUS_VTABLE_PROPERTY_CONST), + SD_BUS_PROPERTY("OnSuccessOf", "as", property_get_dependencies, 0, SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("Triggers", "as", property_get_dependencies, 0, SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("TriggeredBy", "as", property_get_dependencies, 0, SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("PropagatesReloadTo", "as", property_get_dependencies, 0, SD_BUS_VTABLE_PROPERTY_CONST), @@ -903,6 +905,7 @@ const sd_bus_vtable bus_unit_vtable[] = { SD_BUS_PROPERTY("RefuseManualStop", "b", bus_property_get_bool, offsetof(Unit, refuse_manual_stop), SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("AllowIsolate", "b", bus_property_get_bool, offsetof(Unit, allow_isolate), SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("DefaultDependencies", "b", bus_property_get_bool, offsetof(Unit, default_dependencies), SD_BUS_VTABLE_PROPERTY_CONST), + SD_BUS_PROPERTY("OnSuccesJobMode", "s", property_get_job_mode, offsetof(Unit, on_success_job_mode), SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("OnFailureJobMode", "s", property_get_job_mode, offsetof(Unit, on_failure_job_mode), SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("IgnoreOnIsolate", "b", bus_property_get_bool, offsetof(Unit, ignore_on_isolate), SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("NeedDaemonReload", "b", property_get_need_daemon_reload, 0, SD_BUS_VTABLE_PROPERTY_CONST), @@ -2126,6 +2129,9 @@ static int bus_unit_set_transient_property( if (streq(name, "DefaultDependencies")) return bus_set_transient_bool(u, name, &u->default_dependencies, message, flags, error); + if (streq(name, "OnSuccessJobMode")) + return bus_set_transient_job_mode(u, name, &u->on_success_job_mode, message, flags, error); + if (streq(name, "OnFailureJobMode")) return bus_set_transient_job_mode(u, name, &u->on_failure_job_mode, message, flags, error); @@ -2301,6 +2307,7 @@ static int bus_unit_set_transient_property( UNIT_CONFLICTS, UNIT_BEFORE, UNIT_AFTER, + UNIT_ON_SUCCESS, UNIT_ON_FAILURE, UNIT_PROPAGATES_RELOAD_TO, UNIT_RELOAD_PROPAGATED_FROM, diff --git a/src/core/job.c b/src/core/job.c index 45bb6bfd92..a273e4220e 100644 --- a/src/core/job.c +++ b/src/core/job.c @@ -1047,7 +1047,7 @@ int job_finish_and_invalidate(Job *j, JobResult result, bool recursive, bool alr job_type_to_string(t), job_result_to_string(result))); - unit_start_on_failure(u); + unit_start_on_failure(u, "OnFailure=", UNIT_ATOM_ON_FAILURE, u->on_failure_job_mode); } unit_trigger_notify(u); diff --git a/src/core/load-fragment-gperf.gperf.in b/src/core/load-fragment-gperf.gperf.in index bb3c4453df..25cd55e2f9 100644 --- a/src/core/load-fragment-gperf.gperf.in +++ b/src/core/load-fragment-gperf.gperf.in @@ -264,6 +264,7 @@ Unit.BindTo, config_parse_unit_deps, Unit.Conflicts, config_parse_unit_deps, UNIT_CONFLICTS, 0 Unit.Before, config_parse_unit_deps, UNIT_BEFORE, 0 Unit.After, config_parse_unit_deps, UNIT_AFTER, 0 +Unit.OnSuccess, config_parse_unit_deps, UNIT_ON_SUCCESS, 0 Unit.OnFailure, config_parse_unit_deps, UNIT_ON_FAILURE, 0 Unit.PropagatesReloadTo, config_parse_unit_deps, UNIT_PROPAGATES_RELOAD_TO, 0 Unit.PropagateReloadTo, config_parse_unit_deps, UNIT_PROPAGATES_RELOAD_TO, 0 @@ -281,6 +282,7 @@ Unit.RefuseManualStart, config_parse_bool, Unit.RefuseManualStop, config_parse_bool, 0, offsetof(Unit, refuse_manual_stop) Unit.AllowIsolate, config_parse_bool, 0, offsetof(Unit, allow_isolate) Unit.DefaultDependencies, config_parse_bool, 0, offsetof(Unit, default_dependencies) +Unit.OnSuccessJobMode, config_parse_job_mode, 0, offsetof(Unit, on_success_job_mode) Unit.OnFailureJobMode, config_parse_job_mode, 0, offsetof(Unit, on_failure_job_mode) {# The following is a legacy alias name for compatibility #} Unit.OnFailureIsolate, config_parse_job_mode_isolate, 0, offsetof(Unit, on_failure_job_mode) diff --git a/src/core/unit-dependency-atom.c b/src/core/unit-dependency-atom.c index eaeb1d389e..6bd961e92b 100644 --- a/src/core/unit-dependency-atom.c +++ b/src/core/unit-dependency-atom.c @@ -74,6 +74,7 @@ static const UnitDependencyAtom atom_map[_UNIT_DEPENDENCY_MAX] = { /* These are simple dependency types: they consist of a single atom only */ [UNIT_BEFORE] = UNIT_ATOM_BEFORE, [UNIT_AFTER] = UNIT_ATOM_AFTER, + [UNIT_ON_SUCCESS] = UNIT_ATOM_ON_SUCCESS, [UNIT_ON_FAILURE] = UNIT_ATOM_ON_FAILURE, [UNIT_TRIGGERS] = UNIT_ATOM_TRIGGERS, [UNIT_TRIGGERED_BY] = UNIT_ATOM_TRIGGERED_BY, @@ -88,6 +89,7 @@ static const UnitDependencyAtom atom_map[_UNIT_DEPENDENCY_MAX] = { * things discoverable/debuggable as they are the inverse dependencies to some of the above. As they * have no effect of their own, they all map to no atoms at all, i.e. the value 0. */ [UNIT_RELOAD_PROPAGATED_FROM] = 0, + [UNIT_ON_SUCCESS_OF] = 0, [UNIT_ON_FAILURE_OF] = 0, [UNIT_STOP_PROPAGATED_FROM] = 0, }; @@ -175,6 +177,9 @@ UnitDependency unit_dependency_from_unique_atom(UnitDependencyAtom atom) { case UNIT_ATOM_AFTER: return UNIT_AFTER; + case UNIT_ATOM_ON_SUCCESS: + return UNIT_ON_SUCCESS; + case UNIT_ATOM_ON_FAILURE: return UNIT_ON_FAILURE; diff --git a/src/core/unit-dependency-atom.h b/src/core/unit-dependency-atom.h index 0148221511..daa296f601 100644 --- a/src/core/unit-dependency-atom.h +++ b/src/core/unit-dependency-atom.h @@ -62,6 +62,7 @@ typedef enum UnitDependencyAtom { /* The remaining atoms map 1:1 to the equally named high-level deps */ UNIT_ATOM_BEFORE = UINT64_C(1) << 22, UNIT_ATOM_AFTER = UINT64_C(1) << 23, + UNIT_ATOM_ON_SUCCESS = UINT64_C(1) << 24, UNIT_ATOM_ON_FAILURE = UINT64_C(1) << 25, UNIT_ATOM_TRIGGERS = UINT64_C(1) << 26, UNIT_ATOM_TRIGGERED_BY = UINT64_C(1) << 27, diff --git a/src/core/unit-serialize.c b/src/core/unit-serialize.c index caee3a2823..7da0d8a3db 100644 --- a/src/core/unit-serialize.c +++ b/src/core/unit-serialize.c @@ -774,12 +774,14 @@ void unit_dump(Unit *u, FILE *f, const char *prefix) { "%s\tRefuseManualStart: %s\n" "%s\tRefuseManualStop: %s\n" "%s\tDefaultDependencies: %s\n" + "%s\tOnSuccessJobMode: %s\n" "%s\tOnFailureJobMode: %s\n" "%s\tIgnoreOnIsolate: %s\n", prefix, yes_no(u->stop_when_unneeded), prefix, yes_no(u->refuse_manual_start), prefix, yes_no(u->refuse_manual_stop), prefix, yes_no(u->default_dependencies), + prefix, job_mode_to_string(u->on_success_job_mode), prefix, job_mode_to_string(u->on_failure_job_mode), prefix, yes_no(u->ignore_on_isolate)); diff --git a/src/core/unit.c b/src/core/unit.c index ed6f87d960..6892930d7b 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -101,6 +101,7 @@ Unit* unit_new(Manager *m, size_t size) { u->unit_file_state = _UNIT_FILE_STATE_INVALID; u->unit_file_preset = -1; u->on_failure_job_mode = JOB_REPLACE; + u->on_success_job_mode = JOB_FAIL; u->cgroup_control_inotify_wd = -1; u->cgroup_memory_inotify_wd = -1; u->job_timeout = USEC_INFINITY; @@ -896,6 +897,7 @@ static void unit_maybe_warn_about_dependency( UNIT_CONFLICTED_BY, UNIT_BEFORE, UNIT_AFTER, + UNIT_ON_SUCCESS, UNIT_ON_FAILURE, UNIT_TRIGGERS, UNIT_TRIGGERED_BY)) @@ -1507,6 +1509,31 @@ static int unit_add_startup_units(Unit *u) { return set_ensure_put(&u->manager->startup_units, NULL, u); } +static int unit_validate_on_failure_job_mode( + Unit *u, + const char *job_mode_setting, + JobMode job_mode, + const char *dependency_name, + UnitDependencyAtom atom) { + + Unit *other, *found = NULL; + + if (job_mode != JOB_ISOLATE) + return 0; + + UNIT_FOREACH_DEPENDENCY(other, u, atom) { + if (!found) + found = other; + else if (found != other) + return log_unit_error_errno( + u, SYNTHETIC_ERRNO(ENOEXEC), + "More than one %s dependencies specified but %sisolate set. Refusing.", + dependency_name, job_mode_setting); + } + + return 0; +} + int unit_load(Unit *u) { int r; @@ -1560,19 +1587,13 @@ int unit_load(Unit *u) { if (r < 0) goto fail; - if (u->on_failure_job_mode == JOB_ISOLATE) { - Unit *other, *found = NULL; + r = unit_validate_on_failure_job_mode(u, "OnSuccessJobMode=", u->on_success_job_mode, "OnSuccess=", UNIT_ATOM_ON_SUCCESS); + if (r < 0) + goto fail; - UNIT_FOREACH_DEPENDENCY(other, u, UNIT_ATOM_ON_FAILURE) { - if (!found) - found = other; - else if (found != other) { - r = log_unit_error_errno(u, SYNTHETIC_ERRNO(ENOEXEC), - "More than one OnFailure= dependencies specified but OnFailureJobMode=isolate set. Refusing."); - goto fail; - } - } - } + r = unit_validate_on_failure_job_mode(u, "OnFailureJobMode=", u->on_failure_job_mode, "OnFailure=", UNIT_ATOM_ON_FAILURE); + if (r < 0) + goto fail; if (u->job_running_timeout != USEC_INFINITY && u->job_running_timeout > u->job_timeout) log_unit_warning(u, "JobRunningTimeoutSec= is greater than JobTimeoutSec=, it has no effect."); @@ -2082,25 +2103,39 @@ static void retroactively_stop_dependencies(Unit *u) { manager_add_job(u->manager, JOB_STOP, other, JOB_REPLACE, NULL, NULL, NULL); } -void unit_start_on_failure(Unit *u) { +void unit_start_on_failure( + Unit *u, + const char *dependency_name, + UnitDependencyAtom atom, + JobMode job_mode) { + bool logged = false; Unit *other; int r; assert(u); + assert(dependency_name); + assert(IN_SET(atom, UNIT_ATOM_ON_SUCCESS, UNIT_ATOM_ON_FAILURE)); - UNIT_FOREACH_DEPENDENCY(other, u, UNIT_ATOM_ON_FAILURE) { + /* Act on OnFailure= and OnSuccess= dependencies */ + + UNIT_FOREACH_DEPENDENCY(other, u, atom) { _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; if (!logged) { - log_unit_info(u, "Triggering OnFailure= dependencies."); + log_unit_info(u, "Triggering %s dependencies.", dependency_name); logged = true; } - r = manager_add_job(u->manager, JOB_START, other, u->on_failure_job_mode, NULL, &error, NULL); + r = manager_add_job(u->manager, JOB_START, other, job_mode, NULL, &error, NULL); if (r < 0) - log_unit_warning_errno(u, r, "Failed to enqueue OnFailure= job, ignoring: %s", bus_error_message(&error, r)); + log_unit_warning_errno( + u, r, "Failed to enqueue %s job, ignoring: %s", + dependency_name, bus_error_message(&error, r)); } + + if (logged) + log_unit_debug(u, "Triggering %s dependencies done.", dependency_name); } void unit_trigger_notify(Unit *u) { @@ -2568,7 +2603,7 @@ void unit_notify(Unit *u, UnitActiveState os, UnitActiveState ns, UnitNotifyFlag log_unit_debug(u, "Unit entered failed state."); if (!(flags & UNIT_NOTIFY_WILL_AUTO_RESTART)) - unit_start_on_failure(u); + unit_start_on_failure(u, "OnFailure=", UNIT_ATOM_ON_FAILURE, u->on_failure_job_mode); } if (UNIT_IS_ACTIVE_OR_RELOADING(ns) && !UNIT_IS_ACTIVE_OR_RELOADING(os)) { @@ -2584,6 +2619,10 @@ void unit_notify(Unit *u, UnitActiveState os, UnitActiveState ns, UnitNotifyFlag unit_emit_audit_stop(u, ns); unit_log_resources(u); } + + if (ns == UNIT_INACTIVE && !IN_SET(os, UNIT_FAILED, UNIT_INACTIVE, UNIT_MAINTENANCE) && + !(flags & UNIT_NOTIFY_WILL_AUTO_RESTART)) + unit_start_on_failure(u, "OnSuccess=", UNIT_ATOM_ON_SUCCESS, u->on_success_job_mode); } manager_recheck_journal(m); @@ -2871,6 +2910,8 @@ int unit_add_dependency( [UNIT_CONFLICTED_BY] = UNIT_CONFLICTS, [UNIT_BEFORE] = UNIT_AFTER, [UNIT_AFTER] = UNIT_BEFORE, + [UNIT_ON_SUCCESS] = UNIT_ON_SUCCESS_OF, + [UNIT_ON_SUCCESS_OF] = UNIT_ON_SUCCESS, [UNIT_ON_FAILURE] = UNIT_ON_FAILURE_OF, [UNIT_ON_FAILURE_OF] = UNIT_ON_FAILURE, [UNIT_REFERENCES] = UNIT_REFERENCED_BY, diff --git a/src/core/unit.h b/src/core/unit.h index 6b3f48596f..1a3aaa5020 100644 --- a/src/core/unit.h +++ b/src/core/unit.h @@ -334,7 +334,8 @@ typedef struct Unit { * ones which might have appeared. */ sd_event_source *rewatch_pids_event_source; - /* How to start OnFailure units */ + /* How to start OnSuccess=/OnFailure= units */ + JobMode on_success_job_mode; JobMode on_failure_job_mode; /* Tweaking the GC logic */ @@ -828,7 +829,7 @@ bool unit_will_restart(Unit *u); int unit_add_default_target_dependency(Unit *u, Unit *target); -void unit_start_on_failure(Unit *u); +void unit_start_on_failure(Unit *u, const char *dependency_name, UnitDependencyAtom atom, JobMode job_mode); void unit_trigger_notify(Unit *u); UnitFileState unit_get_unit_file_state(Unit *u); diff --git a/test/fuzz/fuzz-unit-file/directives.service b/test/fuzz/fuzz-unit-file/directives.service index 95ab9a1c32..50b4b4b7f6 100644 --- a/test/fuzz/fuzz-unit-file/directives.service +++ b/test/fuzz/fuzz-unit-file/directives.service @@ -76,6 +76,8 @@ JoinsNamespaceOf= OnFailure= OnFailureIsolate= OnFailureJobMode= +OnSuccess= +OnSuccessJobMode= PartOf= PropagateReloadFrom= PropagateReloadTo= From 0bc488c99ab2ed3464237607e381f4d72cd321d5 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 14 Apr 2021 15:24:08 +0200 Subject: [PATCH 12/18] core: implement Uphold= dependency type This is like a really strong version of Wants=, that keeps starting the specified unit if it is ever found inactive. This is an alternative to Restart= inside a unit, acknowledging the fact that whether to keep restarting the unit is sometimes not a property of the unit itself but the state of the system. This implements a part of what #4263 requests. i.e. there's no distinction between "always" and "opportunistic". We just dumbly implement "always" and become active whenever we see no job queued for an inactive unit that is supposed to be upheld. --- man/systemd.unit.xml | 18 +++++ src/basic/unit-def.c | 2 + src/basic/unit-def.h | 2 + src/core/dbus-unit.c | 1 + src/core/load-fragment-gperf.gperf.in | 1 + src/core/manager.c | 44 +++++++++++- src/core/manager.h | 3 + src/core/unit-dependency-atom.c | 24 +++++++ src/core/unit-dependency-atom.h | 5 ++ src/core/unit.c | 74 ++++++++++++++++++++- src/core/unit.h | 12 +++- test/fuzz/fuzz-unit-file/directives.service | 1 + 12 files changed, 180 insertions(+), 7 deletions(-) diff --git a/man/systemd.unit.xml b/man/systemd.unit.xml index d1d7a9f6e0..0b5e869321 100644 --- a/man/systemd.unit.xml +++ b/man/systemd.unit.xml @@ -708,6 +708,24 @@ + + Upholds= + + Configures dependencies similar to Wants=, but as long a this unit + is up, all units listed in Upholds= are started whenever found to be inactive or + failed, and no job is queued for them. While a Wants= dependency on another unit + has a one-time effect when this units started, a Upholds= dependency on it has a + continuous effect, constantly restarting the unit if necessary. This is an alternative to the + Restart= setting of service units, to ensure they are kept running whatever + happens. + + When Upholds=b.service is used on a.service, this + dependency will show as UpheldBy=a.service in the property listing of + b.service. The UpheldBy= dependency cannot be specified + directly. + + + Conflicts= diff --git a/src/basic/unit-def.c b/src/basic/unit-def.c index f83e398a4b..2667e61dc4 100644 --- a/src/basic/unit-def.c +++ b/src/basic/unit-def.c @@ -265,10 +265,12 @@ static const char* const unit_dependency_table[_UNIT_DEPENDENCY_MAX] = { [UNIT_WANTS] = "Wants", [UNIT_BINDS_TO] = "BindsTo", [UNIT_PART_OF] = "PartOf", + [UNIT_UPHOLDS] = "Upholds", [UNIT_REQUIRED_BY] = "RequiredBy", [UNIT_REQUISITE_OF] = "RequisiteOf", [UNIT_WANTED_BY] = "WantedBy", [UNIT_BOUND_BY] = "BoundBy", + [UNIT_UPHELD_BY] = "UpheldBy", [UNIT_CONSISTS_OF] = "ConsistsOf", [UNIT_CONFLICTS] = "Conflicts", [UNIT_CONFLICTED_BY] = "ConflictedBy", diff --git a/src/basic/unit-def.h b/src/basic/unit-def.h index 92b40e8a3d..08651efa57 100644 --- a/src/basic/unit-def.h +++ b/src/basic/unit-def.h @@ -211,6 +211,7 @@ typedef enum UnitDependency { UNIT_WANTS, UNIT_BINDS_TO, UNIT_PART_OF, + UNIT_UPHOLDS, /* Inverse of the above */ UNIT_REQUIRED_BY, /* inverse of 'requires' is 'required_by' */ @@ -218,6 +219,7 @@ typedef enum UnitDependency { UNIT_WANTED_BY, /* inverse of 'wants' */ UNIT_BOUND_BY, /* inverse of 'binds_to' */ UNIT_CONSISTS_OF, /* inverse of 'part_of' */ + UNIT_UPHELD_BY, /* inverse of 'uphold' */ /* Negative dependencies */ UNIT_CONFLICTS, /* inverse of 'conflicts' is 'conflicted_by' */ diff --git a/src/core/dbus-unit.c b/src/core/dbus-unit.c index 60f406a160..8875ba082d 100644 --- a/src/core/dbus-unit.c +++ b/src/core/dbus-unit.c @@ -2304,6 +2304,7 @@ static int bus_unit_set_transient_property( UNIT_WANTS, UNIT_BINDS_TO, UNIT_PART_OF, + UNIT_UPHOLDS, UNIT_CONFLICTS, UNIT_BEFORE, UNIT_AFTER, diff --git a/src/core/load-fragment-gperf.gperf.in b/src/core/load-fragment-gperf.gperf.in index 25cd55e2f9..17f2bea5b8 100644 --- a/src/core/load-fragment-gperf.gperf.in +++ b/src/core/load-fragment-gperf.gperf.in @@ -261,6 +261,7 @@ Unit.Requisite, config_parse_unit_deps, Unit.Wants, config_parse_unit_deps, UNIT_WANTS, 0 Unit.BindsTo, config_parse_unit_deps, UNIT_BINDS_TO, 0 Unit.BindTo, config_parse_unit_deps, UNIT_BINDS_TO, 0 +Unit.Upholds, config_parse_unit_deps, UNIT_UPHOLDS, 0 Unit.Conflicts, config_parse_unit_deps, UNIT_CONFLICTS, 0 Unit.Before, config_parse_unit_deps, UNIT_BEFORE, 0 Unit.After, config_parse_unit_deps, UNIT_AFTER, 0 diff --git a/src/core/manager.c b/src/core/manager.c index a9b51bb25d..e5ec84ac5e 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -1295,7 +1295,7 @@ static unsigned manager_dispatch_stop_when_unneeded_queue(Manager *m) { /* If stopping a unit fails continuously we might enter a stop loop here, hence stop acting on the * service being unnecessary after a while. */ - if (!ratelimit_below(&u->auto_stop_ratelimit)) { + if (!ratelimit_below(&u->auto_start_stop_ratelimit)) { log_unit_warning(u, "Unit not needed anymore, but not stopping since we tried this too often recently."); continue; } @@ -1309,6 +1309,44 @@ static unsigned manager_dispatch_stop_when_unneeded_queue(Manager *m) { return n; } +static unsigned manager_dispatch_start_when_upheld_queue(Manager *m) { + unsigned n = 0; + Unit *u; + int r; + + assert(m); + + while ((u = m->start_when_upheld_queue)) { + _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; + Unit *culprit = NULL; + + assert(u->in_start_when_upheld_queue); + LIST_REMOVE(start_when_upheld_queue, m->start_when_upheld_queue, u); + u->in_start_when_upheld_queue = false; + + n++; + + if (!unit_is_upheld_by_active(u, &culprit)) + continue; + + log_unit_debug(u, "Unit is started because upheld by active unit %s.", culprit->id); + + /* If stopping a unit fails continuously we might enter a stop loop here, hence stop acting on the + * service being unnecessary after a while. */ + + if (!ratelimit_below(&u->auto_start_stop_ratelimit)) { + log_unit_warning(u, "Unit needs to be started because active unit %s upholds it, but not starting since we tried this too often recently.", culprit->id); + continue; + } + + r = manager_add_job(u->manager, JOB_START, u, JOB_FAIL, NULL, &error, NULL); + if (r < 0) + log_unit_warning_errno(u, r, "Failed to enqueue start job, ignoring: %s", bus_error_message(&error, r)); + } + + return n; +} + static void manager_clear_jobs_and_units(Manager *m) { Unit *u; @@ -1327,6 +1365,7 @@ static void manager_clear_jobs_and_units(Manager *m) { assert(!m->gc_unit_queue); assert(!m->gc_job_queue); assert(!m->stop_when_unneeded_queue); + assert(!m->start_when_upheld_queue); assert(hashmap_isempty(m->jobs)); assert(hashmap_isempty(m->units)); @@ -2954,6 +2993,9 @@ int manager_loop(Manager *m) { if (manager_dispatch_cgroup_realize_queue(m) > 0) continue; + if (manager_dispatch_start_when_upheld_queue(m) > 0) + continue; + if (manager_dispatch_stop_when_unneeded_queue(m) > 0) continue; diff --git a/src/core/manager.h b/src/core/manager.h index f58982a364..bfb8b628e7 100644 --- a/src/core/manager.h +++ b/src/core/manager.h @@ -187,6 +187,9 @@ struct Manager { /* Units that might be subject to StopWhenUnneeded= clean-up */ LIST_HEAD(Unit, stop_when_unneeded_queue); + /* Units which are upheld by another other which we might need to act on */ + LIST_HEAD(Unit, start_when_upheld_queue); + sd_event *event; /* This maps PIDs we care about to units that are interested in. We allow multiple units to he interested in diff --git a/src/core/unit-dependency-atom.c b/src/core/unit-dependency-atom.c index 6bd961e92b..60939ce956 100644 --- a/src/core/unit-dependency-atom.c +++ b/src/core/unit-dependency-atom.c @@ -35,6 +35,12 @@ static const UnitDependencyAtom atom_map[_UNIT_DEPENDENCY_MAX] = { [UNIT_PART_OF] = UNIT_ATOM_ADD_DEFAULT_TARGET_DEPENDENCY_QUEUE, + [UNIT_UPHOLDS] = UNIT_ATOM_PULL_IN_START_IGNORED | + UNIT_ATOM_RETROACTIVE_START_REPLACE | + UNIT_ATOM_ADD_START_WHEN_UPHELD_QUEUE | + UNIT_ATOM_ADD_STOP_WHEN_UNNEEDED_QUEUE | + UNIT_ATOM_ADD_DEFAULT_TARGET_DEPENDENCY_QUEUE, + [UNIT_REQUIRED_BY] = UNIT_ATOM_PROPAGATE_STOP | UNIT_ATOM_PROPAGATE_RESTART | UNIT_ATOM_PROPAGATE_START_FAILURE | @@ -58,6 +64,10 @@ static const UnitDependencyAtom atom_map[_UNIT_DEPENDENCY_MAX] = { UNIT_ATOM_PINS_STOP_WHEN_UNNEEDED | UNIT_ATOM_DEFAULT_TARGET_DEPENDENCIES, + [UNIT_UPHELD_BY] = UNIT_ATOM_START_STEADILY | + UNIT_ATOM_DEFAULT_TARGET_DEPENDENCIES | + UNIT_ATOM_PINS_STOP_WHEN_UNNEEDED, + [UNIT_CONSISTS_OF] = UNIT_ATOM_PROPAGATE_STOP | UNIT_ATOM_PROPAGATE_RESTART, @@ -140,6 +150,14 @@ UnitDependency unit_dependency_from_unique_atom(UnitDependencyAtom atom) { case UNIT_ATOM_CANNOT_BE_ACTIVE_WITHOUT: return UNIT_BINDS_TO; + case UNIT_ATOM_PULL_IN_START_IGNORED | + UNIT_ATOM_RETROACTIVE_START_REPLACE | + UNIT_ATOM_ADD_START_WHEN_UPHELD_QUEUE | + UNIT_ATOM_ADD_STOP_WHEN_UNNEEDED_QUEUE | + UNIT_ATOM_ADD_DEFAULT_TARGET_DEPENDENCY_QUEUE: + case UNIT_ATOM_ADD_START_WHEN_UPHELD_QUEUE: + return UNIT_UPHOLDS; + case UNIT_ATOM_PROPAGATE_STOP | UNIT_ATOM_PROPAGATE_RESTART | UNIT_ATOM_PROPAGATE_START_FAILURE | @@ -157,6 +175,12 @@ UnitDependency unit_dependency_from_unique_atom(UnitDependencyAtom atom) { UNIT_ATOM_DEFAULT_TARGET_DEPENDENCIES: return UNIT_BOUND_BY; + case UNIT_ATOM_START_STEADILY | + UNIT_ATOM_DEFAULT_TARGET_DEPENDENCIES | + UNIT_ATOM_PINS_STOP_WHEN_UNNEEDED: + case UNIT_ATOM_START_STEADILY: + return UNIT_UPHELD_BY; + case UNIT_ATOM_PULL_IN_STOP | UNIT_ATOM_RETROACTIVE_STOP_ON_START: case UNIT_ATOM_PULL_IN_STOP: diff --git a/src/core/unit-dependency-atom.h b/src/core/unit-dependency-atom.h index daa296f601..3e684f01c4 100644 --- a/src/core/unit-dependency-atom.h +++ b/src/core/unit-dependency-atom.h @@ -32,6 +32,11 @@ typedef enum UnitDependencyAtom { /* Stop our unit if the other unit happens to inactive */ UNIT_ATOM_CANNOT_BE_ACTIVE_WITHOUT = UINT64_C(1) << 7, + /* Start this unit whenever we find it inactive and the other unit active */ + UNIT_ATOM_START_STEADILY = UINT64_C(1) << 9, + /* Whenever our unit becomes active, add other unit to start_when_upheld_queue */ + UNIT_ATOM_ADD_START_WHEN_UPHELD_QUEUE = UINT64_C(1) << 10, + /* If our unit unexpectedly becomes active, retroactively start the other unit too, in "replace" job * mode */ UNIT_ATOM_RETROACTIVE_START_REPLACE = UINT64_C(1) << 11, diff --git a/src/core/unit.c b/src/core/unit.c index 6892930d7b..f401a1457c 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -122,7 +122,7 @@ Unit* unit_new(Manager *m, size_t size) { u->last_section_private = -1; u->start_ratelimit = (RateLimit) { m->default_start_limit_interval, m->default_start_limit_burst }; - u->auto_stop_ratelimit = (RateLimit) { 10 * USEC_PER_SEC, 16 }; + u->auto_start_stop_ratelimit = (RateLimit) { 10 * USEC_PER_SEC, 16 }; for (CGroupIOAccountingMetric i = 0; i < _CGROUP_IO_ACCOUNTING_METRIC_MAX; i++) u->io_accounting_last[i] = UINT64_MAX; @@ -507,6 +507,22 @@ void unit_submit_to_stop_when_unneeded_queue(Unit *u) { u->in_stop_when_unneeded_queue = true; } +void unit_submit_to_start_when_upheld_queue(Unit *u) { + assert(u); + + if (u->in_start_when_upheld_queue) + return; + + if (!UNIT_IS_INACTIVE_OR_FAILED(unit_active_state(u))) + return; + + if (!unit_has_dependency(u, UNIT_ATOM_START_STEADILY, NULL)) + return; + + LIST_PREPEND(start_when_upheld_queue, u->manager->start_when_upheld_queue, u); + u->in_start_when_upheld_queue = true; +} + static void unit_clear_dependencies(Unit *u) { assert(u); @@ -719,6 +735,9 @@ Unit* unit_free(Unit *u) { if (u->in_stop_when_unneeded_queue) LIST_REMOVE(stop_when_unneeded_queue, u->manager->stop_when_unneeded_queue, u); + if (u->in_start_when_upheld_queue) + LIST_REMOVE(start_when_upheld_queue, u->manager->start_when_upheld_queue, u); + safe_close(u->ip_accounting_ingress_map_fd); safe_close(u->ip_accounting_egress_map_fd); @@ -2011,6 +2030,36 @@ bool unit_is_unneeded(Unit *u) { return true; } +bool unit_is_upheld_by_active(Unit *u, Unit **ret_culprit) { + Unit *other; + + assert(u); + + /* Checks if the unit needs to be started because it currently is not running, but some other unit + * that is active declared an Uphold= dependencies on it */ + + if (!UNIT_IS_INACTIVE_OR_FAILED(unit_active_state(u)) || u->job) { + if (ret_culprit) + *ret_culprit = NULL; + return false; + } + + UNIT_FOREACH_DEPENDENCY(other, u, UNIT_ATOM_START_STEADILY) { + if (other->job) + continue; + + if (UNIT_IS_ACTIVE_OR_RELOADING(unit_active_state(other))) { + if (ret_culprit) + *ret_culprit = other; + return true; + } + } + + if (ret_culprit) + *ret_culprit = NULL; + return false; +} + static void check_unneeded_dependencies(Unit *u) { Unit *other; assert(u); @@ -2021,6 +2070,16 @@ static void check_unneeded_dependencies(Unit *u) { unit_submit_to_stop_when_unneeded_queue(other); } +static void check_uphold_dependencies(Unit *u) { + Unit *other; + assert(u); + + /* Add all units this unit depends on to the queue that processes Uphold= behaviour. */ + + UNIT_FOREACH_DEPENDENCY(other, u, UNIT_ATOM_ADD_START_WHEN_UPHELD_QUEUE) + unit_submit_to_start_when_upheld_queue(other); +} + static void unit_check_binds_to(Unit *u) { _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; bool stop = false; @@ -2056,7 +2115,7 @@ static void unit_check_binds_to(Unit *u) { /* If stopping a unit fails continuously we might enter a stop * loop here, hence stop acting on the service being * unnecessary after a while. */ - if (!ratelimit_below(&u->auto_stop_ratelimit)) { + if (!ratelimit_below(&u->auto_start_stop_ratelimit)) { log_unit_warning(u, "Unit is bound to inactive unit %s, but not stopping since we tried this too often recently.", other->id); return; } @@ -2595,10 +2654,14 @@ void unit_notify(Unit *u, UnitActiveState os, UnitActiveState ns, UnitNotifyFlag retroactively_stop_dependencies(u); } - /* stop unneeded units regardless if going down was expected or not */ + /* Stop unneeded units regardless if going down was expected or not */ if (UNIT_IS_INACTIVE_OR_FAILED(ns)) check_unneeded_dependencies(u); + /* Start uphold units regardless if going up was expected or not */ + if (UNIT_IS_ACTIVE_OR_RELOADING(ns)) + check_uphold_dependencies(u); + if (ns != os && ns == UNIT_FAILED) { log_unit_debug(u, "Unit entered failed state."); @@ -2634,6 +2697,9 @@ void unit_notify(Unit *u, UnitActiveState os, UnitActiveState ns, UnitNotifyFlag /* Maybe we finished startup and are now ready for being stopped because unneeded? */ unit_submit_to_stop_when_unneeded_queue(u); + /* Maybe someone wants us to remain up? */ + unit_submit_to_start_when_upheld_queue(u); + /* Maybe we finished startup, but something we needed has vanished? Let's die then. (This happens when * something BindsTo= to a Type=oneshot unit, as these units go directly from starting to inactive, * without ever entering started.) */ @@ -2901,11 +2967,13 @@ int unit_add_dependency( [UNIT_REQUISITE] = UNIT_REQUISITE_OF, [UNIT_BINDS_TO] = UNIT_BOUND_BY, [UNIT_PART_OF] = UNIT_CONSISTS_OF, + [UNIT_UPHOLDS] = UNIT_UPHELD_BY, [UNIT_REQUIRED_BY] = UNIT_REQUIRES, [UNIT_REQUISITE_OF] = UNIT_REQUISITE, [UNIT_WANTED_BY] = UNIT_WANTS, [UNIT_BOUND_BY] = UNIT_BINDS_TO, [UNIT_CONSISTS_OF] = UNIT_PART_OF, + [UNIT_UPHELD_BY] = UNIT_UPHOLDS, [UNIT_CONFLICTS] = UNIT_CONFLICTED_BY, [UNIT_CONFLICTED_BY] = UNIT_CONFLICTS, [UNIT_BEFORE] = UNIT_AFTER, diff --git a/src/core/unit.h b/src/core/unit.h index 1a3aaa5020..67367d5e4c 100644 --- a/src/core/unit.h +++ b/src/core/unit.h @@ -229,9 +229,12 @@ typedef struct Unit { /* Target dependencies queue */ LIST_FIELDS(Unit, target_deps_queue); - /* Queue of units with StopWhenUnneeded set that shell be checked for clean-up. */ + /* Queue of units with StopWhenUnneeded= set that shall be checked for clean-up. */ LIST_FIELDS(Unit, stop_when_unneeded_queue); + /* Queue of units that have an Uphold= dependency from some other unit, and should be checked for starting */ + LIST_FIELDS(Unit, start_when_upheld_queue); + /* PIDs we keep an eye on. Note that a unit might have many * more, but these are the ones we care enough about to * process SIGCHLD for */ @@ -260,8 +263,8 @@ typedef struct Unit { int success_action_exit_status, failure_action_exit_status; char *reboot_arg; - /* Make sure we never enter endless loops with the check unneeded logic, or the BindsTo= logic */ - RateLimit auto_stop_ratelimit; + /* Make sure we never enter endless loops with the StopWhenUnneeded=, BindsTo=, Uphold= logic */ + RateLimit auto_start_stop_ratelimit; /* Reference to a specific UID/GID */ uid_t ref_uid; @@ -383,6 +386,7 @@ typedef struct Unit { bool in_cgroup_oom_queue:1; bool in_target_deps_queue:1; bool in_stop_when_unneeded_queue:1; + bool in_start_when_upheld_queue:1; bool sent_dbus_new_signal:1; @@ -740,6 +744,7 @@ void unit_add_to_cleanup_queue(Unit *u); void unit_add_to_gc_queue(Unit *u); void unit_add_to_target_deps_queue(Unit *u); void unit_submit_to_stop_when_unneeded_queue(Unit *u); +void unit_submit_to_start_when_upheld_queue(Unit *u); int unit_merge(Unit *u, Unit *other); int unit_merge_by_name(Unit *u, const char *other); @@ -869,6 +874,7 @@ bool unit_type_supported(UnitType t); bool unit_is_pristine(Unit *u); bool unit_is_unneeded(Unit *u); +bool unit_is_upheld_by_active(Unit *u, Unit **ret_culprit); pid_t unit_control_pid(Unit *u); pid_t unit_main_pid(Unit *u); diff --git a/test/fuzz/fuzz-unit-file/directives.service b/test/fuzz/fuzz-unit-file/directives.service index 50b4b4b7f6..1ce6967c65 100644 --- a/test/fuzz/fuzz-unit-file/directives.service +++ b/test/fuzz/fuzz-unit-file/directives.service @@ -103,6 +103,7 @@ StopWhenUnneeded= StopPropagatedFrom= SuccessAction= SuccessActionExitStatus= +Upholds= Wants= [Install] Alias= From 7e9212bf1a0fdcebc08876033f95f64973a048b3 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 14 Apr 2021 18:38:42 +0200 Subject: [PATCH 13/18] core: order reverse dep table in same way as enum --- src/core/unit.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/core/unit.c b/src/core/unit.c index f401a1457c..8f744da2c4 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -2963,8 +2963,8 @@ int unit_add_dependency( static const UnitDependency inverse_table[_UNIT_DEPENDENCY_MAX] = { [UNIT_REQUIRES] = UNIT_REQUIRED_BY, - [UNIT_WANTS] = UNIT_WANTED_BY, [UNIT_REQUISITE] = UNIT_REQUISITE_OF, + [UNIT_WANTS] = UNIT_WANTED_BY, [UNIT_BINDS_TO] = UNIT_BOUND_BY, [UNIT_PART_OF] = UNIT_CONSISTS_OF, [UNIT_UPHOLDS] = UNIT_UPHELD_BY, @@ -2982,8 +2982,6 @@ int unit_add_dependency( [UNIT_ON_SUCCESS_OF] = UNIT_ON_SUCCESS, [UNIT_ON_FAILURE] = UNIT_ON_FAILURE_OF, [UNIT_ON_FAILURE_OF] = UNIT_ON_FAILURE, - [UNIT_REFERENCES] = UNIT_REFERENCED_BY, - [UNIT_REFERENCED_BY] = UNIT_REFERENCES, [UNIT_TRIGGERS] = UNIT_TRIGGERED_BY, [UNIT_TRIGGERED_BY] = UNIT_TRIGGERS, [UNIT_PROPAGATES_RELOAD_TO] = UNIT_RELOAD_PROPAGATED_FROM, @@ -2991,6 +2989,8 @@ int unit_add_dependency( [UNIT_PROPAGATES_STOP_TO] = UNIT_STOP_PROPAGATED_FROM, [UNIT_STOP_PROPAGATED_FROM] = UNIT_PROPAGATES_STOP_TO, [UNIT_JOINS_NAMESPACE_OF] = UNIT_JOINS_NAMESPACE_OF, /* symmetric! 👓 */ + [UNIT_REFERENCES] = UNIT_REFERENCED_BY, + [UNIT_REFERENCED_BY] = UNIT_REFERENCES, [UNIT_IN_SLICE] = UNIT_SLICE_OF, [UNIT_SLICE_OF] = UNIT_IN_SLICE, }; From 116654d2cf2817785a48d83453509465c82e3750 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 26 Apr 2021 21:02:31 +0200 Subject: [PATCH 14/18] core: make unneeded check a bit tighter Let's not consider a unit unneeded while it is reloading. Uneeded should be a pretty weak concept: if there's any doubt that something bit be needed, then assume it is. --- src/core/unit.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/unit.c b/src/core/unit.c index 8f744da2c4..32fbda9884 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -2008,7 +2008,7 @@ bool unit_is_unneeded(Unit *u) { return false; /* Don't clean up while the unit is transitioning or is even inactive. */ - if (!UNIT_IS_ACTIVE_OR_RELOADING(unit_active_state(u))) + if (unit_active_state(u) != UNIT_ACTIVE) return false; if (u->job) return false; From 56c595920220adcc2cedacfada8f9a1000a3c878 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 8 Apr 2021 22:05:15 +0200 Subject: [PATCH 15/18] core: change BoundBy= dependency handling to be processed by a deferred work queue MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit So far StopWhenUnneeded= handling and UpheldBy= handling was already processed by a queue that is dispatched in a deferred mode of operation instead of instantly. This changes BoundBy= handling to be processed the same way. This should ensure that all *event*-to-job propagation is done directly from unit_notify(), while all *state*-to-job propagation is done from a deferred work queue, quite systematically. The work queue is submitted to by unit_notify() too. Key really is the difference between event and state: some jobs shall be queued one-time on events (think: OnFailure= + OnSuccess= and similar), others shall be queued continuously when a specific state is in effect (think: UpheldBy=). The latter cases are usually effect of the combination of states of a few units (e.g. StopWhenUnneeded= checks wether any of the Wants=/Requires=/… deps are still up before acting), and hence it makes sense to trigger them to be run after an individual unit's state changed, but process them on a queue that runs whenever there's nothing else to do that ensures the decision on them is only taken after all jobs/queued IO events are dispatched, and things settled, so that it makes sense to come to a combined conclusion. If we'd dispatch this work immediately inside of unit_notify() we'd always act instantly, even though another event from another unit that is already queued might make the work unnecessary or invalid. This is mostly a commit to make things philosophically clean. It does not add features, but it should make corner cases more robust. --- src/core/manager.c | 42 +++++++++++++ src/core/manager.h | 3 + src/core/unit-dependency-atom.c | 3 + src/core/unit-dependency-atom.h | 2 + src/core/unit.c | 106 ++++++++++++++++++-------------- src/core/unit.h | 6 ++ 6 files changed, 116 insertions(+), 46 deletions(-) diff --git a/src/core/manager.c b/src/core/manager.c index e5ec84ac5e..419bab5b9d 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -1347,6 +1347,44 @@ static unsigned manager_dispatch_start_when_upheld_queue(Manager *m) { return n; } +static unsigned manager_dispatch_stop_when_bound_queue(Manager *m) { + unsigned n = 0; + Unit *u; + int r; + + assert(m); + + while ((u = m->stop_when_bound_queue)) { + _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; + Unit *culprit = NULL; + + assert(u->in_stop_when_bound_queue); + LIST_REMOVE(stop_when_bound_queue, m->stop_when_bound_queue, u); + u->in_stop_when_bound_queue = false; + + n++; + + if (!unit_is_bound_by_inactive(u, &culprit)) + continue; + + log_unit_debug(u, "Unit is stopped because bound to inactive unit %s.", culprit->id); + + /* If stopping a unit fails continuously we might enter a stop loop here, hence stop acting on the + * service being unnecessary after a while. */ + + if (!ratelimit_below(&u->auto_start_stop_ratelimit)) { + log_unit_warning(u, "Unit needs to be stopped because it is bound to inactive unit %s it, but not stopping since we tried this too often recently.", culprit->id); + continue; + } + + r = manager_add_job(u->manager, JOB_STOP, u, JOB_REPLACE, NULL, &error, NULL); + if (r < 0) + log_unit_warning_errno(u, r, "Failed to enqueue stop job, ignoring: %s", bus_error_message(&error, r)); + } + + return n; +} + static void manager_clear_jobs_and_units(Manager *m) { Unit *u; @@ -1366,6 +1404,7 @@ static void manager_clear_jobs_and_units(Manager *m) { assert(!m->gc_job_queue); assert(!m->stop_when_unneeded_queue); assert(!m->start_when_upheld_queue); + assert(!m->stop_when_bound_queue); assert(hashmap_isempty(m->jobs)); assert(hashmap_isempty(m->units)); @@ -2996,6 +3035,9 @@ int manager_loop(Manager *m) { if (manager_dispatch_start_when_upheld_queue(m) > 0) continue; + if (manager_dispatch_stop_when_bound_queue(m) > 0) + continue; + if (manager_dispatch_stop_when_unneeded_queue(m) > 0) continue; diff --git a/src/core/manager.h b/src/core/manager.h index bfb8b628e7..c4d4b2c375 100644 --- a/src/core/manager.h +++ b/src/core/manager.h @@ -190,6 +190,9 @@ struct Manager { /* Units which are upheld by another other which we might need to act on */ LIST_HEAD(Unit, start_when_upheld_queue); + /* Units that have BindsTo= another unit, and might need to be shutdown because the bound unit is not active. */ + LIST_HEAD(Unit, stop_when_bound_queue); + sd_event *event; /* This maps PIDs we care about to units that are interested in. We allow multiple units to he interested in diff --git a/src/core/unit-dependency-atom.c b/src/core/unit-dependency-atom.c index 60939ce956..761835828a 100644 --- a/src/core/unit-dependency-atom.c +++ b/src/core/unit-dependency-atom.c @@ -62,6 +62,7 @@ static const UnitDependencyAtom atom_map[_UNIT_DEPENDENCY_MAX] = { UNIT_ATOM_PROPAGATE_RESTART | UNIT_ATOM_PROPAGATE_START_FAILURE | UNIT_ATOM_PINS_STOP_WHEN_UNNEEDED | + UNIT_ATOM_ADD_CANNOT_BE_ACTIVE_WITHOUT_QUEUE | UNIT_ATOM_DEFAULT_TARGET_DEPENDENCIES, [UNIT_UPHELD_BY] = UNIT_ATOM_START_STEADILY | @@ -172,7 +173,9 @@ UnitDependency unit_dependency_from_unique_atom(UnitDependencyAtom atom) { UNIT_ATOM_PROPAGATE_RESTART | UNIT_ATOM_PROPAGATE_START_FAILURE | UNIT_ATOM_PINS_STOP_WHEN_UNNEEDED | + UNIT_ATOM_ADD_CANNOT_BE_ACTIVE_WITHOUT_QUEUE | UNIT_ATOM_DEFAULT_TARGET_DEPENDENCIES: + case UNIT_ATOM_ADD_CANNOT_BE_ACTIVE_WITHOUT_QUEUE: return UNIT_BOUND_BY; case UNIT_ATOM_START_STEADILY | diff --git a/src/core/unit-dependency-atom.h b/src/core/unit-dependency-atom.h index 3e684f01c4..c5f8f5d400 100644 --- a/src/core/unit-dependency-atom.h +++ b/src/core/unit-dependency-atom.h @@ -31,6 +31,8 @@ typedef enum UnitDependencyAtom { /* Stop our unit if the other unit happens to inactive */ UNIT_ATOM_CANNOT_BE_ACTIVE_WITHOUT = UINT64_C(1) << 7, + /* If our unit enters inactive state, add the other unit to the BoundBy= queue */ + UNIT_ATOM_ADD_CANNOT_BE_ACTIVE_WITHOUT_QUEUE = UINT64_C(1) << 8, /* Start this unit whenever we find it inactive and the other unit active */ UNIT_ATOM_START_STEADILY = UINT64_C(1) << 9, diff --git a/src/core/unit.c b/src/core/unit.c index 32fbda9884..7d673bb363 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -523,6 +523,22 @@ void unit_submit_to_start_when_upheld_queue(Unit *u) { u->in_start_when_upheld_queue = true; } +void unit_submit_to_stop_when_bound_queue(Unit *u) { + assert(u); + + if (u->in_stop_when_bound_queue) + return; + + if (!UNIT_IS_ACTIVE_OR_RELOADING(unit_active_state(u))) + return; + + if (!unit_has_dependency(u, UNIT_ATOM_CANNOT_BE_ACTIVE_WITHOUT, NULL)) + return; + + LIST_PREPEND(stop_when_bound_queue, u->manager->stop_when_bound_queue, u); + u->in_stop_when_bound_queue = true; +} + static void unit_clear_dependencies(Unit *u) { assert(u); @@ -738,6 +754,9 @@ Unit* unit_free(Unit *u) { if (u->in_start_when_upheld_queue) LIST_REMOVE(start_when_upheld_queue, u->manager->start_when_upheld_queue, u); + if (u->in_stop_when_bound_queue) + LIST_REMOVE(stop_when_bound_queue, u->manager->stop_when_bound_queue, u); + safe_close(u->ip_accounting_ingress_map_fd); safe_close(u->ip_accounting_egress_map_fd); @@ -2060,6 +2079,38 @@ bool unit_is_upheld_by_active(Unit *u, Unit **ret_culprit) { return false; } +bool unit_is_bound_by_inactive(Unit *u, Unit **ret_culprit) { + Unit *other; + + assert(u); + + /* Checks whether this unit is bound to another unit that is inactive, i.e. whether we should stop + * because the other unit is down.*/ + + if (unit_active_state(u) != UNIT_ACTIVE || u->job) { + /* Don't clean up while the unit is transitioning or is even inactive. */ + if (ret_culprit) + *ret_culprit = NULL; + return false; + } + + UNIT_FOREACH_DEPENDENCY(other, u, UNIT_ATOM_CANNOT_BE_ACTIVE_WITHOUT) { + if (other->job) + continue; + + if (UNIT_IS_INACTIVE_OR_FAILED(unit_active_state(other))) { + if (*ret_culprit) + *ret_culprit = other; + + return true; + } + } + + if (ret_culprit) + *ret_culprit = NULL; + return false; +} + static void check_unneeded_dependencies(Unit *u) { Unit *other; assert(u); @@ -2080,53 +2131,14 @@ static void check_uphold_dependencies(Unit *u) { unit_submit_to_start_when_upheld_queue(other); } -static void unit_check_binds_to(Unit *u) { - _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; - bool stop = false; +static void check_bound_by_dependencies(Unit *u) { Unit *other; - int r; - assert(u); - if (u->job) - return; + /* Add all units this unit depends on to the queue that processes BindsTo= stop behaviour. */ - if (unit_active_state(u) != UNIT_ACTIVE) - return; - - UNIT_FOREACH_DEPENDENCY(other, u, UNIT_ATOM_CANNOT_BE_ACTIVE_WITHOUT) { - if (other->job) - continue; - - if (!other->coldplugged) - /* We might yet create a job for the other unit… */ - continue; - - if (!UNIT_IS_INACTIVE_OR_FAILED(unit_active_state(other))) - continue; - - stop = true; - break; - } - - if (!stop) - return; - - /* If stopping a unit fails continuously we might enter a stop - * loop here, hence stop acting on the service being - * unnecessary after a while. */ - if (!ratelimit_below(&u->auto_start_stop_ratelimit)) { - log_unit_warning(u, "Unit is bound to inactive unit %s, but not stopping since we tried this too often recently.", other->id); - return; - } - - assert(other); - log_unit_info(u, "Unit is bound to inactive unit %s. Stopping, too.", other->id); - - /* A unit we need to run is gone. Sniff. Let's stop this. */ - r = manager_add_job(u->manager, JOB_STOP, u, JOB_FAIL, NULL, &error, NULL); - if (r < 0) - log_unit_warning_errno(u, r, "Failed to enqueue stop job, ignoring: %s", bus_error_message(&error, r)); + UNIT_FOREACH_DEPENDENCY(other, u, UNIT_ATOM_ADD_CANNOT_BE_ACTIVE_WITHOUT_QUEUE) + unit_submit_to_stop_when_bound_queue(other); } static void retroactively_start_dependencies(Unit *u) { @@ -2654,9 +2666,11 @@ void unit_notify(Unit *u, UnitActiveState os, UnitActiveState ns, UnitNotifyFlag retroactively_stop_dependencies(u); } - /* Stop unneeded units regardless if going down was expected or not */ - if (UNIT_IS_INACTIVE_OR_FAILED(ns)) + /* Stop unneeded units and bound-by units regardless if going down was expected or not */ + if (UNIT_IS_INACTIVE_OR_FAILED(ns)) { check_unneeded_dependencies(u); + check_bound_by_dependencies(u); + } /* Start uphold units regardless if going up was expected or not */ if (UNIT_IS_ACTIVE_OR_RELOADING(ns)) @@ -2703,7 +2717,7 @@ void unit_notify(Unit *u, UnitActiveState os, UnitActiveState ns, UnitNotifyFlag /* Maybe we finished startup, but something we needed has vanished? Let's die then. (This happens when * something BindsTo= to a Type=oneshot unit, as these units go directly from starting to inactive, * without ever entering started.) */ - unit_check_binds_to(u); + unit_submit_to_stop_when_bound_queue(u); if (os != UNIT_FAILED && ns == UNIT_FAILED) { reason = strjoina("unit ", u->id, " failed"); diff --git a/src/core/unit.h b/src/core/unit.h index 67367d5e4c..d34d647845 100644 --- a/src/core/unit.h +++ b/src/core/unit.h @@ -235,6 +235,9 @@ typedef struct Unit { /* Queue of units that have an Uphold= dependency from some other unit, and should be checked for starting */ LIST_FIELDS(Unit, start_when_upheld_queue); + /* Queue of units that have a BindTo= dependency on some other unit, and should possibly be shut down */ + LIST_FIELDS(Unit, stop_when_bound_queue); + /* PIDs we keep an eye on. Note that a unit might have many * more, but these are the ones we care enough about to * process SIGCHLD for */ @@ -387,6 +390,7 @@ typedef struct Unit { bool in_target_deps_queue:1; bool in_stop_when_unneeded_queue:1; bool in_start_when_upheld_queue:1; + bool in_stop_when_bound_queue:1; bool sent_dbus_new_signal:1; @@ -745,6 +749,7 @@ void unit_add_to_gc_queue(Unit *u); void unit_add_to_target_deps_queue(Unit *u); void unit_submit_to_stop_when_unneeded_queue(Unit *u); void unit_submit_to_start_when_upheld_queue(Unit *u); +void unit_submit_to_stop_when_bound_queue(Unit *u); int unit_merge(Unit *u, Unit *other); int unit_merge_by_name(Unit *u, const char *other); @@ -875,6 +880,7 @@ bool unit_is_pristine(Unit *u); bool unit_is_unneeded(Unit *u); bool unit_is_upheld_by_active(Unit *u, Unit **ret_culprit); +bool unit_is_bound_by_inactive(Unit *u, Unit **ret_culprit); pid_t unit_control_pid(Unit *u); pid_t unit_main_pid(Unit *u); From 99e9af257a1a3afbb661a0991bf4464285bc8009 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 26 Apr 2021 22:07:24 +0200 Subject: [PATCH 16/18] core: reorder where we add units to queues in unit_notify() This moves all calls that shall do deferred work on detecting whether to start/stop the unit or dependent units after a unit state change to the end of the function, to make things easier to read. So far, these calls were spread all over the function, and conditionalized needlessly on MANAGER_RELOADING(). This is unnecessary, since the queues are not dispatched while reloading anyway, and immediately before acting on a queued unit we'll check if the suggested operation really makes sense. The only conditionalizaiton we leave in is on checking the new unit state itself, since we have that in a local variable anyway. --- src/core/unit.c | 52 ++++++++++++++++++++++++++++--------------------- 1 file changed, 30 insertions(+), 22 deletions(-) diff --git a/src/core/unit.c b/src/core/unit.c index 7d673bb363..9c97a92746 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -2666,16 +2666,6 @@ void unit_notify(Unit *u, UnitActiveState os, UnitActiveState ns, UnitNotifyFlag retroactively_stop_dependencies(u); } - /* Stop unneeded units and bound-by units regardless if going down was expected or not */ - if (UNIT_IS_INACTIVE_OR_FAILED(ns)) { - check_unneeded_dependencies(u); - check_bound_by_dependencies(u); - } - - /* Start uphold units regardless if going up was expected or not */ - if (UNIT_IS_ACTIVE_OR_RELOADING(ns)) - check_uphold_dependencies(u); - if (ns != os && ns == UNIT_FAILED) { log_unit_debug(u, "Unit entered failed state."); @@ -2708,17 +2698,6 @@ void unit_notify(Unit *u, UnitActiveState os, UnitActiveState ns, UnitNotifyFlag unit_trigger_notify(u); if (!MANAGER_IS_RELOADING(m)) { - /* Maybe we finished startup and are now ready for being stopped because unneeded? */ - unit_submit_to_stop_when_unneeded_queue(u); - - /* Maybe someone wants us to remain up? */ - unit_submit_to_start_when_upheld_queue(u); - - /* Maybe we finished startup, but something we needed has vanished? Let's die then. (This happens when - * something BindsTo= to a Type=oneshot unit, as these units go directly from starting to inactive, - * without ever entering started.) */ - unit_submit_to_stop_when_bound_queue(u); - if (os != UNIT_FAILED && ns == UNIT_FAILED) { reason = strjoina("unit ", u->id, " failed"); emergency_action(m, u->failure_action, 0, u->reboot_arg, unit_failure_action_exit_status(u), reason); @@ -2728,7 +2707,36 @@ void unit_notify(Unit *u, UnitActiveState os, UnitActiveState ns, UnitNotifyFlag } } - unit_add_to_gc_queue(u); + /* And now, add the unit or depending units to various queues that will act on the new situation if + * needed. These queues generally check for continous state changes rather than events (like most of + * the state propagation above), and do work deferred instead of instantly, since they typically + * don't want to run during reloading, and usually involve checking combined state of multiple units + * at once. */ + + if (UNIT_IS_INACTIVE_OR_FAILED(ns)) { + /* Stop unneeded units and bound-by units regardless if going down was expected or not */ + check_unneeded_dependencies(u); + check_bound_by_dependencies(u); + + /* Maybe someone wants us to remain up? */ + unit_submit_to_start_when_upheld_queue(u); + + /* Maybe the unit should be GC'ed now? */ + unit_add_to_gc_queue(u); + } + + if (UNIT_IS_ACTIVE_OR_RELOADING(ns)) { + /* Start uphold units regardless if going up was expected or not */ + check_uphold_dependencies(u); + + /* Maybe we finished startup and are now ready for being stopped because unneeded? */ + unit_submit_to_stop_when_unneeded_queue(u); + + /* Maybe we finished startup, but something we needed has vanished? Let's die then. (This happens + * when something BindsTo= to a Type=oneshot unit, as these units go directly from starting to + * inactive, without ever entering started.) */ + unit_submit_to_stop_when_bound_queue(u); + } } int unit_watch_pid(Unit *u, pid_t pid, bool exclusive) { From 3ba471facb15feccc0acb2b2d818154b1e69e72e Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 14 Apr 2021 22:43:15 +0200 Subject: [PATCH 17/18] test-engine: ensure atom bits are properly packed Let's make sure all atoms are actually used, and no holes are left. --- src/test/test-engine.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/src/test/test-engine.c b/src/test/test-engine.c index 8e196907ee..1039709b89 100644 --- a/src/test/test-engine.c +++ b/src/test/test-engine.c @@ -50,6 +50,23 @@ static void verify_dependency_atoms(void) { multi_use_atoms |= combined & a; combined |= a; } + + /* Make sure all atoms are used, i.e. there's at least one dependency type that references it. */ + assert_se(combined == _UNIT_DEPENDENCY_ATOM_MAX); + + for (UnitDependencyAtom a = 1; a <= _UNIT_DEPENDENCY_ATOM_MAX; a <<= 1) { + + if (multi_use_atoms & a) { + /* If an atom is used by multiple dep types, then mapping the atom to a dependency is + * not unique and *must* fail */ + assert_se(unit_dependency_from_unique_atom(a) == _UNIT_DEPENDENCY_INVALID); + continue; + } + + /* If only a single dep type uses specific atom, let's guarantee our mapping table is + complete, and thus the atom can be mapped to the single dep type that is used. */ + assert_se(unit_dependency_from_unique_atom(a) >= 0); + } } int main(int argc, char *argv[]) { From 076036327432c6b5db97fd870614bc4f4db946be Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 14 Apr 2021 17:34:25 +0200 Subject: [PATCH 18/18] test: add test for OnSuccess= + Uphold= + PropagatesStopTo= + BindsTo= --- test/TEST-57-ONSUCCESS-UPHOLD/Makefile | 1 + test/TEST-57-ONSUCCESS-UPHOLD/test.sh | 7 ++ test/units/testsuite-57-binds-to.service | 8 +++ test/units/testsuite-57-bound-by.service | 5 ++ test/units/testsuite-57-fail.service | 6 ++ test/units/testsuite-57-prop-stop-one.service | 9 +++ test/units/testsuite-57-prop-stop-two.service | 5 ++ test/units/testsuite-57-short-lived.service | 10 +++ test/units/testsuite-57-short-lived.sh | 18 +++++ test/units/testsuite-57-success.service | 6 ++ test/units/testsuite-57-uphold.service | 6 ++ test/units/testsuite-57.service | 7 ++ test/units/testsuite-57.sh | 68 +++++++++++++++++++ 13 files changed, 156 insertions(+) create mode 120000 test/TEST-57-ONSUCCESS-UPHOLD/Makefile create mode 100755 test/TEST-57-ONSUCCESS-UPHOLD/test.sh create mode 100644 test/units/testsuite-57-binds-to.service create mode 100644 test/units/testsuite-57-bound-by.service create mode 100644 test/units/testsuite-57-fail.service create mode 100644 test/units/testsuite-57-prop-stop-one.service create mode 100644 test/units/testsuite-57-prop-stop-two.service create mode 100644 test/units/testsuite-57-short-lived.service create mode 100755 test/units/testsuite-57-short-lived.sh create mode 100644 test/units/testsuite-57-success.service create mode 100644 test/units/testsuite-57-uphold.service create mode 100644 test/units/testsuite-57.service create mode 100755 test/units/testsuite-57.sh diff --git a/test/TEST-57-ONSUCCESS-UPHOLD/Makefile b/test/TEST-57-ONSUCCESS-UPHOLD/Makefile new file mode 120000 index 0000000000..e9f93b1104 --- /dev/null +++ b/test/TEST-57-ONSUCCESS-UPHOLD/Makefile @@ -0,0 +1 @@ +../TEST-01-BASIC/Makefile \ No newline at end of file diff --git a/test/TEST-57-ONSUCCESS-UPHOLD/test.sh b/test/TEST-57-ONSUCCESS-UPHOLD/test.sh new file mode 100755 index 0000000000..145c88fcd8 --- /dev/null +++ b/test/TEST-57-ONSUCCESS-UPHOLD/test.sh @@ -0,0 +1,7 @@ +#!/usr/bin/env bash +set -e + +TEST_DESCRIPTION="test OnSuccess= + Uphold= + PropagatesStopTo= + BindsTo=" +. $TEST_BASE_DIR/test-functions + +do_test "$@" 57 diff --git a/test/units/testsuite-57-binds-to.service b/test/units/testsuite-57-binds-to.service new file mode 100644 index 0000000000..e4b6ee3c29 --- /dev/null +++ b/test/units/testsuite-57-binds-to.service @@ -0,0 +1,8 @@ +[Unit] +Description=Unit with BindsTo= +BindsTo=testsuite-57-bound-by.service +After=testsuite-57-bound-by.service + +[Service] +ExecStart=/bin/sleep infinity +ExecStopPost=systemctl kill --kill-who=main -sRTMIN+1 testsuite-57.service diff --git a/test/units/testsuite-57-bound-by.service b/test/units/testsuite-57-bound-by.service new file mode 100644 index 0000000000..df66a8fe84 --- /dev/null +++ b/test/units/testsuite-57-bound-by.service @@ -0,0 +1,5 @@ +[Unit] +Description=Unit with BoundBy= + +[Service] +ExecStart=/bin/sleep 0.7 diff --git a/test/units/testsuite-57-fail.service b/test/units/testsuite-57-fail.service new file mode 100644 index 0000000000..07d8ddde9d --- /dev/null +++ b/test/units/testsuite-57-fail.service @@ -0,0 +1,6 @@ +[Unit] +Description=Failing unit +OnFailure=testsuite-57-uphold.service + +[Service] +ExecStart=/bin/false diff --git a/test/units/testsuite-57-prop-stop-one.service b/test/units/testsuite-57-prop-stop-one.service new file mode 100644 index 0000000000..cdca7d4451 --- /dev/null +++ b/test/units/testsuite-57-prop-stop-one.service @@ -0,0 +1,9 @@ +[Unit] +Description=Stop Propagation Receiver +Wants=testsuite-57-prop-stop-two.service +After=testsuite-57-prop-stop-two.service +StopPropagatedFrom=testsuite-57-prop-stop-two.service + +[Service] +ExecStart=/bin/sleep infinity +ExecStopPost=systemctl kill --kill-who=main -sUSR2 testsuite-57.service diff --git a/test/units/testsuite-57-prop-stop-two.service b/test/units/testsuite-57-prop-stop-two.service new file mode 100644 index 0000000000..a462faf353 --- /dev/null +++ b/test/units/testsuite-57-prop-stop-two.service @@ -0,0 +1,5 @@ +[Unit] +Description=Stop Propagation Sender + +[Service] +ExecStart=/bin/sleep 1.5 diff --git a/test/units/testsuite-57-short-lived.service b/test/units/testsuite-57-short-lived.service new file mode 100644 index 0000000000..7fa268247f --- /dev/null +++ b/test/units/testsuite-57-short-lived.service @@ -0,0 +1,10 @@ +[Unit] +Description=Shortlived Unit +StopWhenUnneeded=yes + +# Bump up the start limit logic, so that we can be restarted frequently enough +StartLimitBurst=15 +StartLimitIntervalSec=1h + +[Service] +ExecStart=/usr/lib/systemd/tests/testdata/units/testsuite-57-short-lived.sh diff --git a/test/units/testsuite-57-short-lived.sh b/test/units/testsuite-57-short-lived.sh new file mode 100755 index 0000000000..8c48d3a2d5 --- /dev/null +++ b/test/units/testsuite-57-short-lived.sh @@ -0,0 +1,18 @@ +#!/usr/bin/env bash + +set -ex + +if [ -f /tmp/testsuite-57.counter ] ; then + read -r counter < /tmp/testsuite-57.counter + counter=$(("$counter" + 1)) +else + counter=0 +fi + +echo "$counter" > /tmp/testsuite-57.counter + +if [ "$counter" -eq 5 ] ; then + systemctl kill --kill-who=main -sUSR1 testsuite-57.service +fi + +exec sleep 1.5 diff --git a/test/units/testsuite-57-success.service b/test/units/testsuite-57-success.service new file mode 100644 index 0000000000..ac1071fbba --- /dev/null +++ b/test/units/testsuite-57-success.service @@ -0,0 +1,6 @@ +[Unit] +Description=Succeeding unit +OnSuccess=testsuite-57-fail.service + +[Service] +ExecStart=/bin/true diff --git a/test/units/testsuite-57-uphold.service b/test/units/testsuite-57-uphold.service new file mode 100644 index 0000000000..d2b3828375 --- /dev/null +++ b/test/units/testsuite-57-uphold.service @@ -0,0 +1,6 @@ +[Unit] +Description=Upholding Unit +Upholds=testsuite-57-short-lived.service + +[Service] +ExecStart=/bin/sleep infinity diff --git a/test/units/testsuite-57.service b/test/units/testsuite-57.service new file mode 100644 index 0000000000..cab23afb44 --- /dev/null +++ b/test/units/testsuite-57.service @@ -0,0 +1,7 @@ +[Unit] +Description=TEST-57-ONSUCCESS-UPHOLD + +[Service] +ExecStartPre=rm -f /failed /testok +ExecStart=/usr/lib/systemd/tests/testdata/units/%N.sh +Type=oneshot diff --git a/test/units/testsuite-57.sh b/test/units/testsuite-57.sh new file mode 100755 index 0000000000..5db82b8437 --- /dev/null +++ b/test/units/testsuite-57.sh @@ -0,0 +1,68 @@ +#!/usr/bin/env bash + +set -eux +set -o pipefail + +systemd-analyze log-level debug +systemd-analyze log-target journal + +# Idea is this: +# 1. we start testsuite-57-success.service +# 2. which through OnSuccess= starts testsuite-57-fail.service, +# 3. which through OnFailure= starts testsuite-57-uphold.service, +# 4. which through Uphold= starts/keeps testsuite-57-short-lived.service running, +# 5. which will sleep 1s when invoked, and on the 5th invocation send us a SIGUSR1 +# 6. once we got that we finish cleanly + +sigusr1=0 +trap sigusr1=1 SIGUSR1 + +systemctl start testsuite-57-success.service + +while [ "$sigusr1" -eq 0 ] ; do + sleep .5 +done + +systemctl stop testsuite-57-uphold.service + +# Idea is this: +# 1. we start testsuite-57-prop-stop-one.service +# 2. which through Wants=/After= pulls in testsuite-57-prop-stop-two.service as well +# 3. testsuite-57-prop-stop-one.service then sleeps indefinitely +# 4. testsuite-57-prop-stop-two.service sleeps a short time and exits +# 5. the StopPropagatedFrom= dependency between the two should ensure *both* will exit as result +# 6. an ExecStopPost= line on testsuite-57-prop-stop-one.service will send us a SIGUSR2 +# 7. once we got that we finish cleanly + +sigusr2=0 +trap sigusr2=1 SIGUSR2 + +systemctl start testsuite-57-prop-stop-one.service + +while [ "$sigusr2" -eq 0 ] ; do + sleep .5 +done + + +# Idea is this: +# 1. we start testsuite-57-binds-to.service +# 2. which through BindsTo=/After= pulls in testsuite-57-bound-by.service as well +# 3. testsuite-57-bound-by.service suddenly dies +# 4. testsuite-57-binds-to.service should then also be pulled down (it otherwise just hangs) +# 6. an ExecStopPost= line on testsuite-57-binds-to.service will send us a SIGRTMIN1+1 +# 7. once we got that we finish cleanly + +sigrtmin1=0 +trap sigrtmin1=1 SIGRTMIN+1 + +systemctl start testsuite-57-binds-to.service + +while [ "$sigrtmin1" -eq 0 ] ; do + sleep .5 +done + +systemd-analyze log-level info + +echo OK >/testok + +exit 0