From 631b676bb7636971c2e1c42528f49378d8970696 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 24 Nov 2016 18:47:48 +0100 Subject: [PATCH] core: explicitly verify that BindsTo= deps are in order before dispatch start operation of a unit Let's make sure we verify that all BindsTo= are in order before we actually go and dispatch a start operation to a unit. Normally the job queue should already have made sure all deps are in order, but this might not have been sufficient in two cases: a) when the user changes deps during runtime and reloads the daemon, and b) when the user placed BindsTo= dependencies without matching After= dependencies, so that we don't actually wait for the bound to unit to be up before upping also the binding unit. See: #4725 --- NEWS | 4 ++++ src/core/job.c | 2 ++ src/core/unit.c | 33 +++++++++++++++++++++++++++++++++ 3 files changed, 39 insertions(+) diff --git a/NEWS b/NEWS index ec50c0c..23f555f 100644 --- a/NEWS +++ b/NEWS @@ -35,6 +35,10 @@ CHANGES WITH 233 in spe names of remote hosts and to reply to mDNS's A and AAAA requests from the hosts. + * When units are about to be started an additional check is now done to + ensure that all dependencies of type BindsTo= (when used in + combination with After=) have been started. + CHANGES WITH 232: * The new RemoveIPC= option can be used to remove IPC objects owned by diff --git a/src/core/job.c b/src/core/job.c index 00f7d79..07f4b74 100644 --- a/src/core/job.c +++ b/src/core/job.c @@ -627,6 +627,8 @@ int job_run_and_invalidate(Job *j) { r = job_finish_and_invalidate(j, JOB_ASSERT, true, false); else if (r == -EOPNOTSUPP) r = job_finish_and_invalidate(j, JOB_UNSUPPORTED, true, false); + else if (r == -ENOLINK) + r = job_finish_and_invalidate(j, JOB_DEPENDENCY, true, false); else if (r == -EAGAIN) job_set_state(j, JOB_WAITING); else if (r < 0) diff --git a/src/core/unit.c b/src/core/unit.c index 5e4b156..0b680e9 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -1527,6 +1527,7 @@ int unit_start_limit_test(Unit *u) { } bool unit_shall_confirm_spawn(Unit *u) { + assert(u); if (manager_is_confirm_spawn_disabled(u->manager)) return false; @@ -1537,6 +1538,31 @@ bool unit_shall_confirm_spawn(Unit *u) { return !unit_get_exec_context(u)->same_pgrp; } +static bool unit_verify_deps(Unit *u) { + Unit *other; + Iterator j; + + 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. */ + + SET_FOREACH(other, u->dependencies[UNIT_BINDS_TO], j) { + + if (!set_contains(u->dependencies[UNIT_AFTER], other)) + continue; + + if (!UNIT_IS_ACTIVE_OR_RELOADING(unit_active_state(other))) { + log_unit_notice(u, "Bound to unit %s, but unit isn't active.", other->id); + return false; + } + } + + return true; +} + /* Errors: * -EBADR: This unit type does not support starting. * -EALREADY: Unit is already started. @@ -1545,6 +1571,7 @@ bool unit_shall_confirm_spawn(Unit *u) { * -EPROTO: Assert failed * -EINVAL: Unit not loaded * -EOPNOTSUPP: Unit type not supported + * -ENOLINK: The necessary dependencies are not fulfilled. */ int unit_start(Unit *u) { UnitActiveState state; @@ -1590,6 +1617,12 @@ int unit_start(Unit *u) { if (!unit_supported(u)) return -EOPNOTSUPP; + /* Let's make sure that the deps really are in order before we start this. Normally the job engine should have + * taken care of this already, but let's check this here again. After all, our dependencies might not be in + * effect anymore, due to a reload or due to a failed condition. */ + if (!unit_verify_deps(u)) + return -ENOLINK; + /* Forward to the main object, if we aren't it. */ following = unit_following(u); if (following) { -- 2.7.4