From 631b676bb7636971c2e1c42528f49378d8970696 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 24 Nov 2016 18:47:48 +0100 Subject: [PATCH 1/2] 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 ec50c0c403..23f555fc53 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 00f7d7998f..07f4b74c5c 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 5e4b1567d8..0b680e9544 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) { From 62d3ca2469328b59f533bdc955cc6738cb053176 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 29 Nov 2016 22:03:44 +0100 Subject: [PATCH 2/2] man: extend the docs on BindsTo= and Requires= a bit Let's emphasize that both really should be combined with After=. --- man/systemd.unit.xml | 127 ++++++++++++++++++++----------------------- 1 file changed, 58 insertions(+), 69 deletions(-) diff --git a/man/systemd.unit.xml b/man/systemd.unit.xml index 417840e6c2..2208b6a287 100644 --- a/man/systemd.unit.xml +++ b/man/systemd.unit.xml @@ -436,32 +436,30 @@ Requires= - Configures requirement dependencies on other - units. If this unit gets activated, the units listed here will - be activated as well. If one of the other units gets - deactivated or its activation fails, this unit will be - deactivated. This option may be specified more than once or - multiple space-separated units may be specified in one option - in which case requirement dependencies for all listed names - will be created. Note that requirement dependencies do not - influence the order in which services are started or stopped. - This has to be configured independently with the - After= or Before= - options. If a unit foo.service requires a - unit bar.service as configured with - Requires= and no ordering is configured - with After= or Before=, - then both units will be started simultaneously and without any - delay between them if foo.service is - activated. Often, it is a better choice to use - Wants= instead of - Requires= in order to achieve a system that - is more robust when dealing with failing services. + Configures requirement dependencies on other units. If this unit gets activated, the units + listed here will be activated as well. If one of the other units gets deactivated or its activation fails, this + unit will be deactivated. This option may be specified more than once or multiple space-separated units may be + specified in one option in which case requirement dependencies for all listed names will be created. Note that + requirement dependencies do not influence the order in which services are started or stopped. This has to be + configured independently with the After= or Before= options. If a unit + foo.service requires a unit bar.service as configured with + Requires= and no ordering is configured with After= or + Before=, then both units will be started simultaneously and without any delay between them + if foo.service is activated. Often, it is a better choice to use Wants= + instead of Requires= in order to achieve a system that is more robust when dealing with + failing services. - Note that dependencies of this type may also be - configured outside of the unit configuration file by adding a - symlink to a .requires/ directory - accompanying the unit file. For details, see + Note that this dependency type does not imply that the other unit always has to be in active state when + this unit is running. Specifically: failing condition checks (such as ConditionPathExists=, + ConditionPathExists=, … — see below) do not cause the start job of a unit with a + Requires= dependency on it to fail. Also, some unit types may deactivate on their own (for + example, a service process may decide to exit cleanly, or a device may be unplugged by the user), which is not + propagated to units having a Requires= dependency. Use the BindsTo= + dependency type together with After= to ensure that a unit may never be in active state + without a specific other unit also in active state (see below). + + Note that dependencies of this type may also be configured outside of the unit configuration file by + adding a symlink to a .requires/ directory accompanying the unit file. For details, see above. @@ -495,14 +493,21 @@ BindsTo= - Configures requirement dependencies, very - similar in style to Requires=, however in - addition to this behavior, it also declares that this unit is - stopped when any of the units listed suddenly disappears. - Units can suddenly, unexpectedly disappear if a service - terminates on its own choice, a device is unplugged or a mount - point unmounted without involvement of - systemd. + Configures requirement dependencies, very similar in style to + Requires=. However, this dependency type is stronger: in addition to the effect of + Requires= it declares that if the unit bound to is stopped, this unit will be stopped + too. This means a unit bound to another unit that suddenly enters inactive state will be stopped too. + Units can suddenly, unexpectedly enter inactive state for different reasons: the main process of a service unit + might terminate on its own choice, the backing device of a device unit might be unplugged or the mount point of + a mount unit might be unmounted without involvement of the system and service manager. + + When used in conjunction with After= on the same unit the behaviour of + BindsTo= is even stronger. In this case, the unit bound to strictly has to be in active + state for this unit to also be in active state. This not only means a unit bound to another unit that suddenly + enters inactive state, but also one that is bound to another unit that gets skipped due to a failed condition + check (such as ConditionPathExists=, ConditionPathIsSymbolicLink=, … — + see below) will be stopped, should it be running. Hence, in many cases it is best to combine + BindsTo= with After=. @@ -541,42 +546,26 @@ Before= After= - A space-separated list of unit names. - Configures ordering dependencies between units. If a unit - foo.service contains a setting - and both units are being - started, bar.service's start-up is - delayed until foo.service is started up. - Note that this setting is independent of and orthogonal to the - requirement dependencies as configured by - Requires=. It is a common pattern to - include a unit name in both the After= and - Requires= option, in which case the unit - listed will be started before the unit that is configured with - these options. This option may be specified more than once, in - which case ordering dependencies for all listed names are - created. After= is the inverse of - Before=, i.e. while - After= ensures that the configured unit is - started after the listed unit finished starting up, - Before= ensures the opposite, i.e. that the - configured unit is fully started up before the listed unit is - started. Note that when two units with an ordering dependency - between them are shut down, the inverse of the start-up order - is applied. i.e. if a unit is configured with - After= on another unit, the former is - stopped before the latter if both are shut down. Given two units - with any ordering dependency between them, if one unit is shut - down and the other is started up, the shutdown is ordered - before the start-up. It doesn't matter if the ordering - dependency is After= or - Before=. It also doesn't matter which of the - two is shut down, as long as one is shut down and the other is - started up. The shutdown is ordered before the start-up in all - cases. If two units have no ordering dependencies between them, - they are shut down or started up simultaneously, and no ordering - takes place. - + A space-separated list of unit names. Configures ordering dependencies between units. If a + unit foo.service contains a setting and both units are + being started, bar.service's start-up is delayed until foo.service is + started up. Note that this setting is independent of and orthogonal to the requirement dependencies as + configured by Requires=, Wants= or BindsTo=. It is a + common pattern to include a unit name in both the After= and Requires= + option, in which case the unit listed will be started before the unit that is configured with these + options. This option may be specified more than once, in which case ordering dependencies for all listed names + are created. After= is the inverse of Before=, i.e. while + After= ensures that the configured unit is started after the listed unit finished starting + up, Before= ensures the opposite, i.e. that the configured unit is fully started up before + the listed unit is started. Note that when two units with an ordering dependency between them are shut down, + the inverse of the start-up order is applied. i.e. if a unit is configured with After= on + another unit, the former is stopped before the latter if both are shut down. Given two units with any ordering + dependency between them, if one unit is shut down and the other is started up, the shutdown is ordered before + the start-up. It doesn't matter if the ordering dependency is After= or + Before=, in this case. It also doesn't matter which of the two is shut down, as long as one + is shut down and the other is started up. The shutdown is ordered before the start-up in all cases. If two + units have no ordering dependencies between them, they are shut down or started up simultaneously, and no + ordering takes place.