From 95c81c55b2eb6063d79ad343738a6240bbccd100 Mon Sep 17 00:00:00 2001 From: Luca Boccassi Date: Wed, 9 Feb 2022 11:48:30 +0000 Subject: [PATCH 01/12] core: split $MONITOR_METADATA and return it only if a single unit triggers OnFailure/OnSuccess Remove the list logic, and simply skip passing metadata if more than one unit triggered an OnFailure/OnSuccess handler. Instead of a single env var to loop over, provide each separate item as its own variable. Fixes https://github.com/systemd/systemd/issues/22370 --- man/systemd.exec.xml | 80 +++++++--------- src/core/job.c | 12 --- src/core/job.h | 4 - src/core/service.c | 132 ++++++++++---------------- src/core/unit-dependency-atom.c | 24 +++-- src/core/unit-dependency-atom.h | 35 +++---- src/core/unit.c | 23 +---- src/core/unit.h | 3 - src/test/test-engine.c | 7 ++ test/units/testsuite-68.sh | 160 +++++++++----------------------- 10 files changed, 163 insertions(+), 317 deletions(-) diff --git a/man/systemd.exec.xml b/man/systemd.exec.xml index 3b57f8d2f1..defca12d1b 100644 --- a/man/systemd.exec.xml +++ b/man/systemd.exec.xml @@ -3404,7 +3404,7 @@ StandardInputData=SWNrIHNpdHplIGRhIHVuJyBlc3NlIEtsb3BzLAp1ZmYgZWVtYWwga2xvcHAncy $SERVICE_RESULT - Only defined for the service unit type, this environment variable is passed to all + Only used for the service unit type. This environment variable is passed to all ExecStop= and ExecStopPost= processes, and encodes the service "result". Currently, the following values are defined: @@ -3472,7 +3472,7 @@ StandardInputData=SWNrIHNpdHplIGRhIHVuJyBlc3NlIEtsb3BzLAp1ZmYgZWVtYWwga2xvcHAncy $EXIT_CODE $EXIT_STATUS - Only defined for the service unit type, these environment variables are passed to all + Only defined for the service unit type. These environment variables are passed to all ExecStop=, ExecStopPost= processes and contain exit status/code information of the main process of the service. For the precise definition of the exit code and status, see wait2. $EXIT_CODE @@ -3585,38 +3585,28 @@ StandardInputData=SWNrIHNpdHplIGRhIHVuJyBlc3NlIEtsb3BzLAp1ZmYgZWVtYWwga2xvcHAncy - $MONITOR_METADATA + $MONITOR_SERVICE_RESULT + $MONITOR_EXIT_CODE + $MONITOR_EXIT_STATUS + $MONITOR_INVOCATION_ID + $MONITOR_UNIT - Only defined for the service unit type, this environment variable is passed to all - ExecStart= and ExecStartPre= processes which run in services - triggered by OnFailure= or OnSuccess= dependencies. - - - The contents of this variable consists of a semi-colon separated list of metadata fields associated with the triggering - service. For each service which triggered the OnFailure= or OnSuccess= - dependency the following fields will be set: + Only defined for the service unit type. Those environment variable are passed to + all ExecStart= and ExecStartPre= processes which run in + services triggered by OnFailure= or OnSuccess= dependencies. - - SERVICE_RESULT - EXIT_CODE - EXIT_STATUS - INVOCATION_ID - UNIT - - - The fields SERVICE_RESULT, EXIT_CODE and - EXIT_STATUS may take the same values that are allowed when set for - ExecStop= and ExecStopPost= processes. The fields - INVOCATION_ID and UNIT are the invocaton id and unit - name of the service which triggered the dependency. Each field is comma separated, i.e. - - -SERVICE_RESULT=result-string,EXIT_CODE=exit-code,EXIT_STATUS=exit-status,INVOCATION_ID=invocation-id,UNIT=triggering.service - - - + Variables $MONITOR_SERVICE_RESULT, $MONITOR_EXIT_CODE + and $MONITOR_EXIT_STATUS take the same values as for + ExecStop= and ExecStopPost= processes. Variables + $MONITOR_INVOCATION_ID and $MONITOR_UNIT are set to the + invocaton id and unit name of the service which triggered the dependency. + Note that when multiple services trigger the same unit, those variables will be + not be passed. Consider using a template handler unit for that case instead: + OnFailure=handler@%n.service for non-templated units, + or OnFailure=handler@%p-%i.service for templated + units. @@ -4060,7 +4050,7 @@ SERVICE_RESULT=result-string,EXIT_CODE=exit-code,EXIT_STATUS=exit-status,INVOCAT Examples - <varname>$MONITOR_METADATA</varname> usage + <varname>$MONITOR_<replaceable>*</replaceable></varname> usage A service myfailer.service which can trigger an OnFailure= dependency. @@ -4094,33 +4084,31 @@ ExecStart=/bin/mysecondprogram Description=Acts on service failing or succeeding [Service] -ExecStart=/bin/bash -c "echo $MONITOR_METADATA" +ExecStart=/bin/bash -c "echo $MONITOR_SERVICE_RESULT $MONITOR_EXIT_CODE $MONITOR_EXIT_STATUS $MONITOR_INVOCATION_ID $MONITOR_UNIT" If myfailer.service were to run and exit in failure, then myhandler.service would be triggered and the - $MONITOR_METADATA variable would be set as follows: + monitor variables would be set as follows: -MONITOR_METADATA=SERVICE_RESULT=result-string,EXIT_CODE=exit-code,EXIT_STATUS=exit-status,INVOCATION_ID=invocation-id,UNIT=myfailer.service +MONITOR_SERVICE_RESULT=exit-code +MONITOR_EXIT_CODE=exited +MONITOR_EXIT_STATUS=1 +MONITOR_INVOCATION_ID=cc8fdc149b2b4ca698d4f259f4054236 +MONITOR_UNIT=myfailer.service If mysuccess.service were to run and exit in success, then myhandler.service would be triggered and the - $MONITOR_METADATA variable would be set as follows: + monitor variables would be set as follows: -MONITOR_METADATA=SERVICE_RESULT=result-string,EXIT_CODE=exit-code,EXIT_STATUS=exit-status,INVOCATION_ID=invocation-id,UNIT=mysuccess.service - - - If myfailer.service and mysuccess.service were to run and exit, - there is a chance that the triggered dependency start job might be merged. Thus only a single invocation of - myhandler.service would be triggered. In this case the $MONITOR_METADATA variable - would be a list containing exit metadata for both of myfailer.service - and mysuccess.service. - - -MONITOR_METADATA=SERVICE_RESULT=result-string,EXIT_CODE=exit-code,EXIT_STATUS=exit-status,INVOCATION_ID=invocation-id,UNIT=myfailer.service;SERVICE_RESULT=result-string,EXIT_CODE=exit-code,EXIT_STATUS=exit-status,INVOCATION_ID=invocation-id,UNIT=mysuccess.service +MONITOR_SERVICE_RESULT=success +MONITOR_EXIT_CODE=exited +MONITOR_EXIT_STATUS=0 +MONITOR_INVOCATION_ID=6ab9af147b8c4a3ebe36e7a5f8611697 +MONITOR_UNIT=mysuccess.service diff --git a/src/core/job.c b/src/core/job.c index f28821071b..94ab381626 100644 --- a/src/core/job.c +++ b/src/core/job.c @@ -99,9 +99,6 @@ Job* job_free(Job *j) { assert(!j->subject_list); assert(!j->object_list); - while (!LIST_IS_EMPTY(j->triggered_by)) - LIST_POP(triggered_by, j->triggered_by); - job_unlink(j); sd_bus_track_unref(j->bus_track); @@ -110,13 +107,6 @@ Job* job_free(Job *j) { return mfree(j); } -void job_add_triggering_unit(Job *j, Unit *u) { - assert(j); - assert(u); - - LIST_APPEND(triggered_by, j->triggered_by, u); -} - static void job_set_state(Job *j, JobState state) { assert(j); assert(state >= 0); @@ -197,8 +187,6 @@ static void job_merge_into_installed(Job *j, Job *other) { j->irreversible = j->irreversible || other->irreversible; j->ignore_order = j->ignore_order || other->ignore_order; - if (other->triggered_by) - LIST_JOIN(triggered_by, j->triggered_by, other->triggered_by); } Job* job_install(Job *j) { diff --git a/src/core/job.h b/src/core/job.h index 762b0bb19b..a66e5985b8 100644 --- a/src/core/job.h +++ b/src/core/job.h @@ -124,8 +124,6 @@ struct Job { LIST_HEAD(JobDependency, subject_list); LIST_HEAD(JobDependency, object_list); - LIST_HEAD(Unit, triggered_by); - /* Used for graph algs as a "I have been here" marker */ Job* marker; unsigned generation; @@ -246,5 +244,3 @@ 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, UnitDependencyAtom assume_dep); - -void job_add_triggering_unit(Job *j, Unit *u); diff --git a/src/core/service.c b/src/core/service.c index 92af448ff4..551f3df355 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -1447,81 +1447,37 @@ static bool service_exec_needs_notify_socket(Service *s, ExecFlags flags) { return s->notify_access != NOTIFY_NONE; } -static int service_create_monitor_md_env(Job *j, char **ret) { - _cleanup_free_ char *var = NULL; - const char *list_delim = ";"; - bool first = true; - Unit *tu; +static Service *service_get_triggering_service(Service *s) { + Unit *candidate = NULL, *other; - assert(j); - assert(ret); + assert(s); - /* Create an environment variable 'MONITOR_METADATA', if creation is successful - * a pointer to it is returned via ret. + /* Return the service which triggered service 's', this means dependency + * types which include the UNIT_ATOM_ON_{FAILURE,SUCCESS}_OF atoms. * - * This variable contains a space separated set of fields which relate to - * the service(s) which triggered job 'j'. Job 'j' is the JOB_START job for - * an OnFailure= or OnSuccess= dependency. Format of the MONITOR_METADATA - * variable is as follows: - * - * MONITOR_METADATA="SERVICE_RESULT=,EXIT_CODE=,EXIT_STATUS=, - * INVOCATION_ID=,UNIT=; - * SERVICE_RESULT=,EXIT_CODE=,EXIT_STATUS=, - * INVOCATION_ID=,UNIT=" - * - * Multiple results may be passed as in the above example if jobs are merged, i.e. - * some services a and b contain an OnFailure= or OnSuccess= dependency on the same - * service. - * - * For example: - * - * MONITOR_METADATA="SERVICE_RESULT=exit-code,EXIT_CODE=exited,EXIT_STATUS=1,INVOCATION_ID=02dd868af2f344b18edaf74b618b2f90,UNIT=failure.service; - * SERVICE_RESULT=exit-code,EXIT_CODE=exited,EXIT_STATUS=1,INVOCATION_ID=80cb228bd7344f77a090eda603a3cfe2,UNIT=failure2.service" - */ + * N.B. if there are multiple services which could trigger 's' via OnFailure= + * or OnSuccess= then we return NULL. This is since we don't know from which + * one to propagate the exit status. */ - LIST_FOREACH(triggered_by, tu, j->triggered_by) { - Service *env_source = SERVICE(tu); - int r; - - if (!env_source) - continue; - - /* Add the environment variable name first. */ - if (first && !strextend(&var, "MONITOR_METADATA=")) - return -ENOMEM; - - if (!strextend(&var, !first ? list_delim : "", "SERVICE_RESULT=", service_result_to_string(env_source->result))) - return -ENOMEM; - - first = false; - - if (env_source->main_exec_status.pid > 0 && - dual_timestamp_is_set(&env_source->main_exec_status.exit_timestamp)) { - if (!strextend(&var, ",EXIT_CODE=", sigchld_code_to_string(env_source->main_exec_status.code))) - return -ENOMEM; - - if (env_source->main_exec_status.code == CLD_EXITED) { - r = strextendf(&var, ",EXIT_STATUS=%i", - env_source->main_exec_status.status); - if (r < 0) - return r; - } else if (!strextend(&var, ",EXIT_STATUS=", signal_to_string(env_source->main_exec_status.status))) - return -ENOMEM; + UNIT_FOREACH_DEPENDENCY(other, UNIT(s), UNIT_ATOM_ON_FAILURE_OF) { + if (candidate) { + log_unit_debug(UNIT(s), "multiple trigger source candidates for exit status propagation, skipping."); + return NULL; } - if (!sd_id128_is_null(UNIT(env_source)->invocation_id)) { - r = strextendf(&var, ",INVOCATION_ID=" SD_ID128_FORMAT_STR, - SD_ID128_FORMAT_VAL(UNIT(env_source)->invocation_id)); - if (r < 0) - return r; - } - - if (!strextend(&var, ",UNIT=", UNIT(env_source)->id)) - return -ENOMEM; + candidate = other; } - *ret = TAKE_PTR(var); - return 0; + UNIT_FOREACH_DEPENDENCY(other, UNIT(s), UNIT_ATOM_ON_SUCCESS_OF) { + if (candidate) { + log_unit_debug(UNIT(s), "multiple trigger source candidates for exit status propagation, skipping."); + return NULL; + } + + candidate = other; + } + + return SERVICE(candidate); } static int service_spawn( @@ -1588,7 +1544,7 @@ static int service_spawn( if (r < 0) return r; - our_env = new0(char*, 10); + our_env = new0(char*, 12); if (!our_env) return -ENOMEM; @@ -1645,30 +1601,44 @@ static int service_spawn( } } + Service *env_source = NULL; + const char *monitor_prefix; if (flags & EXEC_SETENV_RESULT) { - if (asprintf(our_env + n_env++, "SERVICE_RESULT=%s", service_result_to_string(s->result)) < 0) + env_source = s; + monitor_prefix = ""; + } else if (flags & EXEC_SETENV_MONITOR_RESULT) { + env_source = service_get_triggering_service(s); + monitor_prefix = "MONITOR_"; + } + + if (env_source) { + if (asprintf(our_env + n_env++, "%sSERVICE_RESULT=%s", monitor_prefix, service_result_to_string(env_source->result)) < 0) return -ENOMEM; - if (s->main_exec_status.pid > 0 && - dual_timestamp_is_set(&s->main_exec_status.exit_timestamp)) { - if (asprintf(our_env + n_env++, "EXIT_CODE=%s", sigchld_code_to_string(s->main_exec_status.code)) < 0) + if (env_source->main_exec_status.pid > 0 && + dual_timestamp_is_set(&env_source->main_exec_status.exit_timestamp)) { + if (asprintf(our_env + n_env++, "%sEXIT_CODE=%s", monitor_prefix, sigchld_code_to_string(env_source->main_exec_status.code)) < 0) return -ENOMEM; - if (s->main_exec_status.code == CLD_EXITED) - r = asprintf(our_env + n_env++, "EXIT_STATUS=%i", s->main_exec_status.status); + if (env_source->main_exec_status.code == CLD_EXITED) + r = asprintf(our_env + n_env++, "%sEXIT_STATUS=%i", monitor_prefix, env_source->main_exec_status.status); else - r = asprintf(our_env + n_env++, "EXIT_STATUS=%s", signal_to_string(s->main_exec_status.status)); + r = asprintf(our_env + n_env++, "%sEXIT_STATUS=%s", monitor_prefix, signal_to_string(env_source->main_exec_status.status)); if (r < 0) return -ENOMEM; } - } else if (flags & EXEC_SETENV_MONITOR_RESULT) { - Job *j = UNIT(s)->job; - if (j) { - r = service_create_monitor_md_env(j, our_env + n_env++); - if (r < 0) - return r; + if (env_source != s) { + if (!sd_id128_is_null(UNIT(env_source)->invocation_id)) { + r = asprintf(our_env + n_env++, "%sINVOCATION_ID=" SD_ID128_FORMAT_STR, + monitor_prefix, SD_ID128_FORMAT_VAL(UNIT(env_source)->invocation_id)); + if (r < 0) + return -ENOMEM; + } + + if (asprintf(our_env + n_env++, "%sUNIT=%s", monitor_prefix, UNIT(env_source)->id) < 0) + return -ENOMEM; } } diff --git a/src/core/unit-dependency-atom.c b/src/core/unit-dependency-atom.c index 333eea6c3d..81333523e7 100644 --- a/src/core/unit-dependency-atom.c +++ b/src/core/unit-dependency-atom.c @@ -82,13 +82,11 @@ static const UnitDependencyAtom atom_map[_UNIT_DEPENDENCY_MAX] = { [UNIT_PROPAGATES_STOP_TO] = UNIT_ATOM_RETROACTIVE_STOP_ON_STOP | UNIT_ATOM_PROPAGATE_STOP, - [UNIT_ON_FAILURE] = UNIT_ATOM_ON_FAILURE | - UNIT_ATOM_BACK_REFERENCE_IMPLIED, - - [UNIT_ON_SUCCESS] = UNIT_ATOM_ON_SUCCESS | - UNIT_ATOM_BACK_REFERENCE_IMPLIED, - /* These are simple dependency types: they consist of a single atom only */ + [UNIT_ON_FAILURE] = UNIT_ATOM_ON_FAILURE, + [UNIT_ON_SUCCESS] = UNIT_ATOM_ON_SUCCESS, + [UNIT_ON_FAILURE_OF] = UNIT_ATOM_ON_FAILURE_OF, + [UNIT_ON_SUCCESS_OF] = UNIT_ATOM_ON_SUCCESS_OF, [UNIT_BEFORE] = UNIT_ATOM_BEFORE, [UNIT_AFTER] = UNIT_ATOM_AFTER, [UNIT_TRIGGERS] = UNIT_ATOM_TRIGGERS, @@ -104,8 +102,6 @@ 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, }; @@ -200,17 +196,19 @@ UnitDependency unit_dependency_from_unique_atom(UnitDependencyAtom atom) { case UNIT_ATOM_PROPAGATE_STOP_FAILURE: return UNIT_CONFLICTED_BY; - case UNIT_ATOM_ON_FAILURE | - UNIT_ATOM_BACK_REFERENCE_IMPLIED: + /* And now, the simple ones */ + case UNIT_ATOM_ON_FAILURE: return UNIT_ON_FAILURE; - case UNIT_ATOM_ON_SUCCESS | - UNIT_ATOM_BACK_REFERENCE_IMPLIED: case UNIT_ATOM_ON_SUCCESS: return UNIT_ON_SUCCESS; - /* And now, the simple ones */ + case UNIT_ATOM_ON_SUCCESS_OF: + return UNIT_ON_SUCCESS_OF; + + case UNIT_ATOM_ON_FAILURE_OF: + return UNIT_ON_FAILURE_OF; case UNIT_ATOM_BEFORE: return UNIT_BEFORE; diff --git a/src/core/unit-dependency-atom.h b/src/core/unit-dependency-atom.h index 1cf9795786..02532e57d6 100644 --- a/src/core/unit-dependency-atom.h +++ b/src/core/unit-dependency-atom.h @@ -66,27 +66,22 @@ typedef enum UnitDependencyAtom { /* Recheck default target deps on other units (which are target units) */ UNIT_ATOM_DEFAULT_TARGET_DEPENDENCIES = UINT64_C(1) << 21, - /* Dependencies which include this atom automatically get a reverse - * REFERENCES/REFERENCED_BY dependency. */ - UNIT_ATOM_BACK_REFERENCE_IMPLIED = UINT64_C(1) << 22, - - /* Trigger a dependency on successful service exit. */ - UNIT_ATOM_ON_SUCCESS = UINT64_C(1) << 23, - /* Trigger a dependency on unsuccessful service exit. */ - UNIT_ATOM_ON_FAILURE = UINT64_C(1) << 24, - /* The remaining atoms map 1:1 to the equally named high-level deps */ - UNIT_ATOM_BEFORE = UINT64_C(1) << 25, - UNIT_ATOM_AFTER = UINT64_C(1) << 26, - UNIT_ATOM_TRIGGERS = UINT64_C(1) << 27, - UNIT_ATOM_TRIGGERED_BY = UINT64_C(1) << 28, - UNIT_ATOM_PROPAGATES_RELOAD_TO = UINT64_C(1) << 29, - UNIT_ATOM_JOINS_NAMESPACE_OF = UINT64_C(1) << 30, - UNIT_ATOM_REFERENCES = UINT64_C(1) << 31, - UNIT_ATOM_REFERENCED_BY = UINT64_C(1) << 32, - UNIT_ATOM_IN_SLICE = UINT64_C(1) << 33, - UNIT_ATOM_SLICE_OF = UINT64_C(1) << 34, - _UNIT_DEPENDENCY_ATOM_MAX = (UINT64_C(1) << 35) - 1, + UNIT_ATOM_ON_FAILURE = UINT64_C(1) << 22, + UNIT_ATOM_ON_SUCCESS = UINT64_C(1) << 23, + UNIT_ATOM_ON_FAILURE_OF = UINT64_C(1) << 24, + UNIT_ATOM_ON_SUCCESS_OF = UINT64_C(1) << 25, + UNIT_ATOM_BEFORE = UINT64_C(1) << 26, + UNIT_ATOM_AFTER = UINT64_C(1) << 27, + UNIT_ATOM_TRIGGERS = UINT64_C(1) << 28, + UNIT_ATOM_TRIGGERED_BY = UINT64_C(1) << 29, + UNIT_ATOM_PROPAGATES_RELOAD_TO = UINT64_C(1) << 30, + UNIT_ATOM_JOINS_NAMESPACE_OF = UINT64_C(1) << 31, + UNIT_ATOM_REFERENCES = UINT64_C(1) << 32, + UNIT_ATOM_REFERENCED_BY = UINT64_C(1) << 33, + UNIT_ATOM_IN_SLICE = UINT64_C(1) << 34, + UNIT_ATOM_SLICE_OF = UINT64_C(1) << 35, + _UNIT_DEPENDENCY_ATOM_MAX = (UINT64_C(1) << 36) - 1, _UNIT_DEPENDENCY_ATOM_INVALID = -EINVAL, } UnitDependencyAtom; diff --git a/src/core/unit.c b/src/core/unit.c index 2cddc924f3..af6cf097fc 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -2222,24 +2222,17 @@ void unit_start_on_failure( UNIT_FOREACH_DEPENDENCY(other, u, atom) { _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; - Job *job = NULL; if (!logged) { log_unit_info(u, "Triggering %s dependencies.", dependency_name); logged = true; } - r = manager_add_job(u->manager, JOB_START, other, job_mode, NULL, &error, &job); + 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 %s job, ignoring: %s", dependency_name, bus_error_message(&error, r)); - else if (job) - /* u will be kept pinned since both UNIT_ON_FAILURE and UNIT_ON_SUCCESS includes - * UNIT_ATOM_BACK_REFERENCE_IMPLIED. We save the triggering unit here since we - * want to be able to reference it when we come to run the OnFailure= or OnSuccess= - * dependency. */ - job_add_triggering_unit(job, u); } if (logged) @@ -3121,20 +3114,6 @@ int unit_add_dependency( noop = false; } - if (FLAGS_SET(a, UNIT_ATOM_BACK_REFERENCE_IMPLIED)) { - r = unit_add_dependency_hashmap(&other->dependencies, UNIT_REFERENCES, u, 0, mask); - if (r < 0) - return r; - if (r) - noop = false; - - r = unit_add_dependency_hashmap(&u->dependencies, UNIT_REFERENCED_BY, other, 0, mask); - if (r < 0) - return r; - if (r) - noop = false; - } - if (add_reference) { r = unit_add_dependency_hashmap(&u->dependencies, UNIT_REFERENCES, other, mask, 0); if (r < 0) diff --git a/src/core/unit.h b/src/core/unit.h index 786c15d623..94f2180951 100644 --- a/src/core/unit.h +++ b/src/core/unit.h @@ -242,9 +242,6 @@ typedef struct Unit { /* 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); - /* Queue of units which have triggered an OnFailure= or OnSuccess= dependency job. */ - LIST_FIELDS(Unit, triggered_by); - /* 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 */ diff --git a/src/test/test-engine.c b/src/test/test-engine.c index 673c665612..473e896acc 100644 --- a/src/test/test-engine.c +++ b/src/test/test-engine.c @@ -223,6 +223,7 @@ int main(int argc, char *argv[]) { 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); + assert_se(unit_add_dependency_by_name(stub, UNIT_ON_SUCCESS, "non-existing-on-success.target", true, UNIT_DEPENDENCY_FILE) >= 0); log_info("/* Merging a+stub, dumps before */"); unit_dump(a, stderr, NULL); @@ -237,7 +238,13 @@ int main(int argc, char *argv[]) { 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(manager_get_unit(m, "non-existing-on-failure.target"), UNIT_ATOM_ON_FAILURE_OF, a)); + assert_se(unit_has_dependency(a, UNIT_ATOM_ON_SUCCESS, manager_get_unit(m, "non-existing-on-success.target"))); + assert_se(unit_has_dependency(manager_get_unit(m, "non-existing-on-success.target"), UNIT_ATOM_ON_SUCCESS_OF, a)); assert_se(!unit_has_dependency(a, UNIT_ATOM_ON_FAILURE, manager_get_unit(m, "basic.target"))); + assert_se(!unit_has_dependency(a, UNIT_ATOM_ON_SUCCESS, manager_get_unit(m, "basic.target"))); + assert_se(!unit_has_dependency(a, UNIT_ATOM_ON_FAILURE_OF, manager_get_unit(m, "basic.target"))); + assert_se(!unit_has_dependency(a, UNIT_ATOM_ON_SUCCESS_OF, 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")); diff --git a/test/units/testsuite-68.sh b/test/units/testsuite-68.sh index 15e2e0353f..d9584e4539 100755 --- a/test/units/testsuite-68.sh +++ b/test/units/testsuite-68.sh @@ -36,16 +36,6 @@ OnFailure=testservice-failure-exit-handler-68.service ExecStart=/bin/bash -c "exit 1" EOF -# Another service which triggers testservice-failure-exit-handler-68.service -cat >/run/systemd/system/testservice-failure-68-additional.service </run/systemd/system/testservice-success-68.service </run/systemd/system/testservice-success-68-additional.service <= 1 service -# details since jobs may merge. +# MONITOR* env variables are passed. cat >/tmp/check_on_success.sh <=1 service -# details since jobs may merge. +# MONITOR* env variables are passed. cat >/tmp/check_on_failure.sh </testok From c19c4ab14840e93ebd4d5650a16453daa7dd872d Mon Sep 17 00:00:00 2001 From: Luca Boccassi Date: Wed, 9 Feb 2022 11:50:19 +0000 Subject: [PATCH 02/12] test: cover template OnFailure/OnSuccess handlers in TEST-68-PROPAGATE-EXIT-STATUS --- test/units/testsuite-68.sh | 50 ++++++++++++++++++++++++++++++++++++-- 1 file changed, 48 insertions(+), 2 deletions(-) diff --git a/test/units/testsuite-68.sh b/test/units/testsuite-68.sh index d9584e4539..aa02d66dcf 100755 --- a/test/units/testsuite-68.sh +++ b/test/units/testsuite-68.sh @@ -36,6 +36,15 @@ OnFailure=testservice-failure-exit-handler-68.service ExecStart=/bin/bash -c "exit 1" EOF +cat >/run/systemd/system/testservice-failure-68-template.service </run/systemd/system/testservice-success-68.service </run/systemd/system/testservice-success-68-template.service </tmp/check_on_success.sh </run/systemd/system/testservice-success-exit-handler-68-template@.service </tmp/check_on_failure.sh </run/systemd/system/testservice-failure-exit-handler-68-template@.service </testok From 3fbd5f2007983a221468432db51d85165dfdac62 Mon Sep 17 00:00:00 2001 From: Luca Boccassi Date: Wed, 9 Feb 2022 11:58:30 +0000 Subject: [PATCH 03/12] NEWS: note backward-incompatible MONITOR_METADATA change --- NEWS | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/NEWS b/NEWS index 95ffc555a2..d0714157c5 100644 --- a/NEWS +++ b/NEWS @@ -17,6 +17,14 @@ CHANGES WITH 251: for breaking backward compatibility instead, as nobody should be affected, given the state of the current interface. + * Service monitor environment variables will only be passed to OnFailure=/OnSuccess= + handlers if exactly one unit lists the handler unit as OnFailure=/OnSuccess=. + Therefore, $MONITOR_METADATA is no longer used, and instead separate + variables are used: $MONITOR_SERVICE_RESULT, $MONITOR_EXIT_CODE, + $MONITOR_EXIT_STATUS, $MONITOR_INVOCATION_ID and $MONITOR_UNIT. + For cases when a single handler needs to watch multiple units, use a + templated handler. + * Services with Restart=always and a failing ExecCondition= will no longer be restarted, to bring ExecCondition= in line with Condition*= settings. From ff7b9a26930dd80f5567dc9d4c3f94f1737c9138 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 1 Mar 2022 10:07:23 +0100 Subject: [PATCH 04/12] TEST-68-PROPAGATE-EXIT-STATUS: deobfuscate shell code and fix typo After the cleanup, it was fairly easy to see the wrong variable name ;) --- test/units/testsuite-68.sh | 90 ++++++++++++++++++++------------------ 1 file changed, 48 insertions(+), 42 deletions(-) diff --git a/test/units/testsuite-68.sh b/test/units/testsuite-68.sh index aa02d66dcf..c93f870395 100755 --- a/test/units/testsuite-68.sh +++ b/test/units/testsuite-68.sh @@ -33,7 +33,7 @@ Description=TEST-68-PROPAGATE-EXIT-STATUS with OnFailure= trigger OnFailure=testservice-failure-exit-handler-68.service [Service] -ExecStart=/bin/bash -c "exit 1" +ExecStart=sh -c "exit 1" EOF cat >/run/systemd/system/testservice-failure-68-template.service </run/systemd/system/testservice-success-68-template.service </tmp/check_on_success.sh </tmp/check_on_success.sh <<"EOF" +#!/bin/sh set -ex env -if [ "\$MONITOR_SERVICE_RESULT" != "success" ]; then - echo "MONITOR_SERVICE_RESULT was \"\$MONITOR_SERVICE_RESULT\", expected \"success\""; - exit 1; +if [ "$MONITOR_SERVICE_RESULT" != "success" ]; then + echo "MONITOR_SERVICE_RESULT was '$MONITOR_SERVICE_RESULT', expected 'success'" + exit 1 fi -if [ "\$MONITOR_EXIT_CODE" != "exited" ]; then - echo "MONITOR_EXIT_CODE was \"\$MONITOR_EXIT_CODE\", expected \"exited\""; - exit 1; +if [ "$MONITOR_EXIT_CODE" != "exited" ]; then + echo "MONITOR_EXIT_CODE was '$MONITOR_EXIT_CODE', expected 'exited'" + exit 1 fi -if [ "\$MONITOR_EXIT_STATUS" != "0" ]; then - echo "MONITOR_EXIT_STATUS was \"\$MONITOR_EXIT_STATUS\", expected \"0\""; - exit 1; +if [ "$MONITOR_EXIT_STATUS" != "0" ]; then + echo "MONITOR_EXIT_STATUS was '$MONITOR_EXIT_STATUS', expected '0'" + exit 1 fi -if [ -z "\$MONITOR_INVOCATION_ID" ]; then - echo "MONITOR_INVOCATION_ID unset"; - exit 1; +if [ -z "$MONITOR_INVOCATION_ID" ]; then + echo "MONITOR_INVOCATION_ID unset" + exit 1 fi -if [[ "\$MONITOR_UNIT" != "testservice-success-68.service" && "\$UNIT" != "testservice-success-68-template.service" && "\$MONITOR_UNIT" != "testservice-transient-success-68.service" ]]; then - echo "MONITOR_UNIT was \"\$MONITOR_UNIT\", expected \"testservice-success-68{-transient}.service\""; - exit 1; +if [ "$MONITOR_UNIT" != "testservice-success-68.service" ] && + [ "$MONITOR_UNIT" != "testservice-success-68-template.service" ] && + [ "$MONITOR_UNIT" != "testservice-transient-success-68.service" ]; then + + echo "MONITOR_UNIT was '$MONITOR_UNIT', expected 'testservice[-transient]-success-68[-template].service'" + exit 1 fi -exit 0; +exit 0 EOF chmod +x /tmp/check_on_success.sh @@ -123,37 +126,40 @@ EOF # Script to check that when an OnFailure= dependency fires, the correct # MONITOR* env variables are passed. -cat >/tmp/check_on_failure.sh </tmp/check_on_failure.sh <<"EOF" +#!/bin/sh set -ex env -if [ "\$MONITOR_SERVICE_RESULT" != "exit-code" ]; then - echo "MONITOR_SERVICE_RESULT was \"\$MONITOR_SERVICE_RESULT\", expected \"exit-code\""; - exit 1; +if [ "$MONITOR_SERVICE_RESULT" != "exit-code" ]; then + echo "MONITOR_SERVICE_RESULT was '$MONITOR_SERVICE_RESULT', expected 'exit-code'" + exit 1 fi -if [ "\$MONITOR_EXIT_CODE" != "exited" ]; then - echo "MONITOR_EXIT_CODE was \"\$MONITOR_EXIT_CODE\", expected \"exited\""; - exit 1; +if [ "$MONITOR_EXIT_CODE" != "exited" ]; then + echo "MONITOR_EXIT_CODE was '$MONITOR_EXIT_CODE', expected 'exited'" + exit 1 fi -if [ "\$MONITOR_EXIT_STATUS" != "1" ]; then - echo "MONITOR_EXIT_STATUS was \"\$MONITOR_EXIT_STATUS\", expected \"1\""; - exit 1; +if [ "$MONITOR_EXIT_STATUS" != "1" ]; then + echo "MONITOR_EXIT_STATUS was '$MONITOR_EXIT_STATUS', expected '1'" + exit 1 fi -if [ -z "\$MONITOR_INVOCATION_ID" ]; then - echo "MONITOR_INVOCATION_ID unset"; - exit 1; +if [ -z "$MONITOR_INVOCATION_ID" ]; then + echo "MONITOR_INVOCATION_ID unset" + exit 1 fi -if [[ "\$MONITOR_UNIT" != "testservice-failure-68.service" && "\$UNIT" != "testservice-failure-68-template.service" && "\$MONITOR_UNIT" != "testservice-transient-failure-68.service" ]]; then - echo "MONITOR_UNIT was \"\$MONITOR_UNIT\", expected \"testservice-failure-68{-transient}.service\""; - exit 1; +if [ "$MONITOR_UNIT" != "testservice-failure-68.service" ] && + [ "$MONITOR_UNIT" != "testservice-failure-68-template.service" ] && + [ "$MONITOR_UNIT" != "testservice-transient-failure-68.service" ]; then + + echo "MONITOR_UNIT was '$MONITOR_UNIT', expected 'testservice[-transient]-failure-68[-template].service'" + exit 1 fi -exit 0; +exit 0 EOF chmod +x /tmp/check_on_failure.sh @@ -187,9 +193,9 @@ systemctl start testservice-success-68.service wait_on_state_or_fail "testservice-success-exit-handler-68.service" "inactive" "10" # Test some transient units since these exit very quickly. -systemd-run --unit=testservice-transient-success-68 --property=OnSuccess=testservice-success-exit-handler-68.service /bin/bash -c "exit 0;" +systemd-run --unit=testservice-transient-success-68 --property=OnSuccess=testservice-success-exit-handler-68.service sh -c "exit 0" wait_on_state_or_fail "testservice-success-exit-handler-68.service" "inactive" "10" -systemd-run --unit=testservice-transient-failure-68 --property=OnFailure=testservice-failure-exit-handler-68.service /bin/bash -c "exit 1;" +systemd-run --unit=testservice-transient-failure-68 --property=OnFailure=testservice-failure-exit-handler-68.service sh -c "exit 1" wait_on_state_or_fail "testservice-failure-exit-handler-68.service" "inactive" "10" # Test template handlers too From f086cca2483b5275fd2f7a423c157422a723ac60 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Thu, 10 Mar 2022 14:47:41 +0000 Subject: [PATCH 05/12] TEST-68: enhance testing of chained commands The test would fail when the the same handler was used for multiple *failing* units. We need to call 'reset-failed' to let the manager forget about the earlier ones. systemd-analyze log-target console is removed, because it's easier to follow the logs if logging it to the journal. --- test/units/testsuite-68.sh | 26 +++++++++++++++++++++----- 1 file changed, 21 insertions(+), 5 deletions(-) diff --git a/test/units/testsuite-68.sh b/test/units/testsuite-68.sh index c93f870395..89c5d2618c 100755 --- a/test/units/testsuite-68.sh +++ b/test/units/testsuite-68.sh @@ -24,7 +24,6 @@ wait_on_state_or_fail () { } systemd-analyze log-level debug -systemd-analyze log-target console # Trigger testservice-failure-exit-handler-68.service cat >/run/systemd/system/testservice-failure-68.service </tmp/check_on_success.sh <<"EOF" #!/bin/sh set -ex -env +env | sort if [ "$MONITOR_SERVICE_RESULT" != "success" ]; then echo "MONITOR_SERVICE_RESULT was '$MONITOR_SERVICE_RESULT', expected 'success'" exit 1 @@ -130,7 +129,7 @@ cat >/tmp/check_on_failure.sh <<"EOF" #!/bin/sh set -ex -env +env | sort if [ "$MONITOR_SERVICE_RESULT" != "exit-code" ]; then echo "MONITOR_SERVICE_RESULT was '$MONITOR_SERVICE_RESULT', expected 'exit-code'" exit 1 @@ -187,23 +186,40 @@ EOF systemctl daemon-reload +: "-------I----------------------------------------------------" systemctl start testservice-failure-68.service wait_on_state_or_fail "testservice-failure-exit-handler-68.service" "inactive" "10" + +: "-------II---------------------------------------------------" systemctl start testservice-success-68.service wait_on_state_or_fail "testservice-success-exit-handler-68.service" "inactive" "10" +# Let's get rid of the failed units so the tests below don't fail, and daemon-reload +# to force garbace collection of the units. +systemctl reset-failed +systemctl daemon-reload + # Test some transient units since these exit very quickly. +: "-------III--------------------------------------------------" systemd-run --unit=testservice-transient-success-68 --property=OnSuccess=testservice-success-exit-handler-68.service sh -c "exit 0" wait_on_state_or_fail "testservice-success-exit-handler-68.service" "inactive" "10" + +: "-------IIII-------------------------------------------------" systemd-run --unit=testservice-transient-failure-68 --property=OnFailure=testservice-failure-exit-handler-68.service sh -c "exit 1" wait_on_state_or_fail "testservice-failure-exit-handler-68.service" "inactive" "10" +systemctl reset-failed +systemctl daemon-reload + # Test template handlers too -systemctl start testservice-failure-68-template.service -wait_on_state_or_fail "testservice-failure-exit-handler-68-template@testservice-failure-68-template.service.service" "inactive" "10" +: "-------V---------------------------------------------------" systemctl start testservice-success-68-template.service wait_on_state_or_fail "testservice-success-exit-handler-68-template@testservice-success-68-template.service.service" "inactive" "10" +: "-------VI----------------------------------------------------" +systemctl start testservice-failure-68-template.service +wait_on_state_or_fail "testservice-failure-exit-handler-68-template@testservice-failure-68-template.service.service" "inactive" "10" + systemd-analyze log-level info echo OK >/testok From fb1381662b61ae5aa87925964e4eb7805db54d47 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 1 Mar 2022 13:03:31 +0100 Subject: [PATCH 06/12] various: align vertically for ease of reading --- src/core/service.c | 2 +- src/core/unit.c | 60 +++++++++++++++++++++--------------------- src/test/test-engine.c | 38 +++++++++++++------------- 3 files changed, 50 insertions(+), 50 deletions(-) diff --git a/src/core/service.c b/src/core/service.c index 551f3df355..b71c26ca33 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -4641,7 +4641,7 @@ static const char* const service_type_table[_SERVICE_TYPE_MAX] = { DEFINE_STRING_TABLE_LOOKUP(service_type, ServiceType); static const char* const service_exit_type_table[_SERVICE_EXIT_TYPE_MAX] = { - [SERVICE_EXIT_MAIN] = "main", + [SERVICE_EXIT_MAIN] = "main", [SERVICE_EXIT_CGROUP] = "cgroup", }; diff --git a/src/core/unit.c b/src/core/unit.c index af6cf097fc..a164a9d305 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -3009,37 +3009,37 @@ int unit_add_dependency( UnitDependencyMask mask) { static const UnitDependency inverse_table[_UNIT_DEPENDENCY_MAX] = { - [UNIT_REQUIRES] = UNIT_REQUIRED_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, - [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, - [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_TRIGGERS] = UNIT_TRIGGERED_BY, - [UNIT_TRIGGERED_BY] = UNIT_TRIGGERS, - [UNIT_PROPAGATES_RELOAD_TO] = UNIT_RELOAD_PROPAGATED_FROM, + [UNIT_REQUIRES] = UNIT_REQUIRED_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, + [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, + [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_TRIGGERS] = UNIT_TRIGGERED_BY, + [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_REFERENCES] = UNIT_REFERENCED_BY, - [UNIT_REFERENCED_BY] = UNIT_REFERENCES, - [UNIT_IN_SLICE] = UNIT_SLICE_OF, - [UNIT_SLICE_OF] = UNIT_IN_SLICE, + [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, }; Unit *original_u = u, *original_other = other; UnitDependencyAtom a; diff --git a/src/test/test-engine.c b/src/test/test-engine.c index 473e896acc..cd01d1358b 100644 --- a/src/test/test-engine.c +++ b/src/test/test-engine.c @@ -192,17 +192,17 @@ int main(int argc, char *argv[]) { 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(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( 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(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( 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); @@ -232,15 +232,15 @@ int main(int argc, char *argv[]) { 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(manager_get_unit(m, "non-existing-on-failure.target"), UNIT_ATOM_ON_FAILURE_OF, a)); - assert_se(unit_has_dependency(a, UNIT_ATOM_ON_SUCCESS, manager_get_unit(m, "non-existing-on-success.target"))); - assert_se(unit_has_dependency(manager_get_unit(m, "non-existing-on-success.target"), UNIT_ATOM_ON_SUCCESS_OF, a)); + 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(manager_get_unit(m, "non-existing-on-failure.target"), UNIT_ATOM_ON_FAILURE_OF, a)); + assert_se( unit_has_dependency(a, UNIT_ATOM_ON_SUCCESS, manager_get_unit(m, "non-existing-on-success.target"))); + assert_se( unit_has_dependency(manager_get_unit(m, "non-existing-on-success.target"), UNIT_ATOM_ON_SUCCESS_OF, a)); assert_se(!unit_has_dependency(a, UNIT_ATOM_ON_FAILURE, manager_get_unit(m, "basic.target"))); assert_se(!unit_has_dependency(a, UNIT_ATOM_ON_SUCCESS, manager_get_unit(m, "basic.target"))); assert_se(!unit_has_dependency(a, UNIT_ATOM_ON_FAILURE_OF, manager_get_unit(m, "basic.target"))); @@ -282,19 +282,19 @@ int main(int argc, char *argv[]) { assert_se(UNIT_GET_SLICE(tomato) == zupa); assert_se(!unit_has_dependency(tomato, UNIT_ATOM_IN_SLICE, sauce)); assert_se(!unit_has_dependency(tomato, UNIT_ATOM_IN_SLICE, fruit)); - assert_se(unit_has_dependency(tomato, UNIT_ATOM_IN_SLICE, zupa)); + assert_se( unit_has_dependency(tomato, UNIT_ATOM_IN_SLICE, zupa)); assert_se(!unit_has_dependency(tomato, UNIT_ATOM_REFERENCES, sauce)); assert_se(!unit_has_dependency(tomato, UNIT_ATOM_REFERENCES, fruit)); - assert_se(unit_has_dependency(tomato, UNIT_ATOM_REFERENCES, zupa)); + assert_se( unit_has_dependency(tomato, UNIT_ATOM_REFERENCES, zupa)); assert_se(!unit_has_dependency(sauce, UNIT_ATOM_SLICE_OF, tomato)); assert_se(!unit_has_dependency(fruit, UNIT_ATOM_SLICE_OF, tomato)); - assert_se(unit_has_dependency(zupa, UNIT_ATOM_SLICE_OF, tomato)); + assert_se( unit_has_dependency(zupa, UNIT_ATOM_SLICE_OF, tomato)); assert_se(!unit_has_dependency(sauce, UNIT_ATOM_REFERENCED_BY, tomato)); assert_se(!unit_has_dependency(fruit, UNIT_ATOM_REFERENCED_BY, tomato)); - assert_se(unit_has_dependency(zupa, UNIT_ATOM_REFERENCED_BY, tomato)); + assert_se( unit_has_dependency(zupa, UNIT_ATOM_REFERENCED_BY, tomato)); return 0; } From adf769b06c4cdee200eb854d28982a0d356aab80 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 1 Mar 2022 11:33:28 +0100 Subject: [PATCH 07/12] manager: adjust comment --- src/core/execute.c | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/src/core/execute.c b/src/core/execute.c index c731dec0bd..027e5473b6 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -4459,12 +4459,9 @@ static int exec_child( return log_oom(); } - /* The PATH variable is set to the default path in params->environment. - * However, this is overridden if user specified fields have PATH set. - * The intention is to also override PATH if the user does - * not specify PATH and the user has specified ExecSearchPath - */ - + /* The $PATH variable is set to the default path in params->environment. However, this is overridden + * if user-specified fields have $PATH set. The intention is to also override $PATH if the unit does + * not specify PATH but the unit has ExecSearchPath. */ if (!strv_isempty(context->exec_search_path)) { _cleanup_free_ char *joined = NULL; @@ -4501,22 +4498,26 @@ static int exec_child( return log_unit_error_errno(unit, r, "Failed to set up kernel keyring: %m"); } - /* We need sandboxing if the caller asked us to apply it and the command isn't explicitly excepted from it */ + /* We need sandboxing if the caller asked us to apply it and the command isn't explicitly excepted + * from it. */ needs_sandboxing = (params->flags & EXEC_APPLY_SANDBOXING) && !(command->flags & EXEC_COMMAND_FULLY_PRIVILEGED); - /* We need the ambient capability hack, if the caller asked us to apply it and the command is marked for it, and the kernel doesn't actually support ambient caps */ + /* We need the ambient capability hack, if the caller asked us to apply it and the command is marked + * for it, and the kernel doesn't actually support ambient caps. */ needs_ambient_hack = (params->flags & EXEC_APPLY_SANDBOXING) && (command->flags & EXEC_COMMAND_AMBIENT_MAGIC) && !ambient_capabilities_supported(); - /* We need setresuid() if the caller asked us to apply sandboxing and the command isn't explicitly excepted from either whole sandboxing or just setresuid() itself, and the ambient hack is not desired */ + /* We need setresuid() if the caller asked us to apply sandboxing and the command isn't explicitly + * excepted from either whole sandboxing or just setresuid() itself, and the ambient hack is not + * desired. */ if (needs_ambient_hack) needs_setuid = false; else needs_setuid = (params->flags & EXEC_APPLY_SANDBOXING) && !(command->flags & (EXEC_COMMAND_FULLY_PRIVILEGED|EXEC_COMMAND_NO_SETUID)); if (needs_sandboxing) { - /* MAC enablement checks need to be done before a new mount ns is created, as they rely on /sys being - * present. The actual MAC context application will happen later, as late as possible, to avoid - * impacting our own code paths. */ + /* MAC enablement checks need to be done before a new mount ns is created, as they rely on + * /sys being present. The actual MAC context application will happen later, as late as + * possible, to avoid impacting our own code paths. */ #if HAVE_SELINUX use_selinux = mac_selinux_use(); From 82acee149cd24ea6825870206bb1743f6506ce64 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 1 Mar 2022 11:33:07 +0100 Subject: [PATCH 08/12] manager: log how many OnSuccess/OnFailure jobs were started --- src/core/unit.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/core/unit.c b/src/core/unit.c index a164a9d305..c9f88c81ef 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -2210,7 +2210,7 @@ void unit_start_on_failure( UnitDependencyAtom atom, JobMode job_mode) { - bool logged = false; + int n_jobs = -1; Unit *other; int r; @@ -2223,9 +2223,9 @@ void unit_start_on_failure( UNIT_FOREACH_DEPENDENCY(other, u, atom) { _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; - if (!logged) { + if (n_jobs < 0) { log_unit_info(u, "Triggering %s dependencies.", dependency_name); - logged = true; + n_jobs = 0; } r = manager_add_job(u->manager, JOB_START, other, job_mode, NULL, &error, NULL); @@ -2233,10 +2233,12 @@ void unit_start_on_failure( log_unit_warning_errno( u, r, "Failed to enqueue %s job, ignoring: %s", dependency_name, bus_error_message(&error, r)); + n_jobs ++; } - if (logged) - log_unit_debug(u, "Triggering %s dependencies done.", dependency_name); + if (n_jobs >= 0) + log_unit_debug(u, "Triggering %s dependencies done (%u %s).", + dependency_name, n_jobs, n_jobs == 1 ? "job" : "jobs"); } void unit_trigger_notify(Unit *u) { From edbf8984a48de047df9f7172d73ee6b232f559f6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 1 Mar 2022 11:59:48 +0100 Subject: [PATCH 09/12] manager/service: when we spawn, say why We already logged what we are spawning, but not so much why. Let's add this, so it's easier to distinguish execstartpre/execstart/execstartpost and such. --- src/core/service.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/core/service.c b/src/core/service.c index b71c26ca33..f5fe683cf2 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -43,6 +43,8 @@ #include "utf8.h" #include "util.h" +#define service_spawn(...) service_spawn_internal(__func__, __VA_ARGS__) + static const UnitActiveState state_translation_table[_SERVICE_STATE_MAX] = { [SERVICE_DEAD] = UNIT_INACTIVE, [SERVICE_CONDITION] = UNIT_ACTIVATING, @@ -1480,7 +1482,8 @@ static Service *service_get_triggering_service(Service *s) { return SERVICE(candidate); } -static int service_spawn( +static int service_spawn_internal( + const char *caller, Service *s, ExecCommand *c, usec_t timeout, @@ -1500,10 +1503,13 @@ static int service_spawn( pid_t pid; int r; + assert(caller); assert(s); assert(c); assert(ret_pid); + log_unit_debug(UNIT(s), "Will spawn child (%s): %s", caller, c->path); + r = unit_prepare_exec(UNIT(s)); /* This realizes the cgroup, among other things */ if (r < 0) return r; From 7a5049c780454197c4f03632e67ea735ea803265 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 2 Mar 2022 13:05:50 +0100 Subject: [PATCH 10/12] manager/service: when we have multiple candidates to handle, warn This would be very confusing to users, so let's warn if they configured the same handler for multiple units and we're not running it as expected. --- src/core/service.c | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/src/core/service.c b/src/core/service.c index f5fe683cf2..942bc7c0ff 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -1462,24 +1462,23 @@ static Service *service_get_triggering_service(Service *s) { * one to propagate the exit status. */ UNIT_FOREACH_DEPENDENCY(other, UNIT(s), UNIT_ATOM_ON_FAILURE_OF) { - if (candidate) { - log_unit_debug(UNIT(s), "multiple trigger source candidates for exit status propagation, skipping."); - return NULL; - } - + if (candidate) + goto have_other; candidate = other; } UNIT_FOREACH_DEPENDENCY(other, UNIT(s), UNIT_ATOM_ON_SUCCESS_OF) { - if (candidate) { - log_unit_debug(UNIT(s), "multiple trigger source candidates for exit status propagation, skipping."); - return NULL; - } - + if (candidate) + goto have_other; candidate = other; } return SERVICE(candidate); + + have_other: + log_unit_warning(UNIT(s), "multiple trigger source candidates for exit status propagation (%s, %s), skipping.", + candidate->id, other->id); + return NULL; } static int service_spawn_internal( From 02de9614d4a541032da3039c2cae918ec852be28 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 2 Mar 2022 13:09:06 +0100 Subject: [PATCH 11/12] manager: prevent cleanup of triggering units before we start the handler This fixes the following case: OnFailure= would be spawned correctly, but OnSuccess= would be spawned without the MONITOR_* metadata, because we'd "collect" the unit that started successfully. So let's block cleanup while we have a job running for the handler. The job cannot last infinitely, so at some point we'll be able to collect both. --- src/core/unit.c | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/src/core/unit.c b/src/core/unit.c index c9f88c81ef..bd11049c77 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -375,6 +375,20 @@ int unit_set_description(Unit *u, const char *description) { return 0; } +static bool unit_success_failure_handler_has_jobs(Unit *unit) { + Unit *other; + + UNIT_FOREACH_DEPENDENCY(other, unit, UNIT_ATOM_ON_SUCCESS) + if (other->job || other->nop_job) + return true; + + UNIT_FOREACH_DEPENDENCY(other, unit, UNIT_ATOM_ON_FAILURE) + if (other->job || other->nop_job) + return true; + + return false; +} + bool unit_may_gc(Unit *u) { UnitActiveState state; int r; @@ -389,10 +403,7 @@ bool unit_may_gc(Unit *u) { * in unit_gc_sweep(), but using markers to properly collect dependency loops. */ - if (u->job) - return false; - - if (u->nop_job) + if (u->job || u->nop_job) return false; state = unit_active_state(u); @@ -427,6 +438,10 @@ bool unit_may_gc(Unit *u) { assert_not_reached(); } + /* Check if any OnFailure= or on Success= jobs may be pending */ + if (unit_success_failure_handler_has_jobs(u)) + return false; + if (u->cgroup_path) { /* If the unit has a cgroup, then check whether there's anything in it. If so, we should stay * around. Units with active processes should never be collected. */ From a257c941adffa3632081c34b5504182cbd5151d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 1 Mar 2022 10:49:26 +0100 Subject: [PATCH 12/12] manager: pass monitor metadata in more cases The first ExecStartPre or the first ExecStart commands would get the metadata, but not the subsequent ones. Also check that we do not pass it in ExecStartPost. --- src/core/service.c | 3 ++- test/units/testsuite-68.sh | 9 +++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/src/core/service.c b/src/core/service.c index 942bc7c0ff..acf3df77da 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -2459,6 +2459,7 @@ static void service_run_next_control(Service *s) { EXEC_APPLY_SANDBOXING|EXEC_APPLY_CHROOT|EXEC_IS_CONTROL| (IN_SET(s->control_command_id, SERVICE_EXEC_CONDITION, SERVICE_EXEC_START_PRE, SERVICE_EXEC_STOP_POST) ? EXEC_APPLY_TTY_STDIN : 0)| (IN_SET(s->control_command_id, SERVICE_EXEC_STOP, SERVICE_EXEC_STOP_POST) ? EXEC_SETENV_RESULT : 0)| + (IN_SET(s->control_command_id, SERVICE_EXEC_START_PRE, SERVICE_EXEC_START) ? EXEC_SETENV_MONITOR_RESULT : 0)| (IN_SET(s->control_command_id, SERVICE_EXEC_START_POST, SERVICE_EXEC_RELOAD, SERVICE_EXEC_STOP, SERVICE_EXEC_STOP_POST) ? EXEC_CONTROL_CGROUP : 0), &s->control_pid); if (r < 0) @@ -2495,7 +2496,7 @@ static void service_run_next_main(Service *s) { r = service_spawn(s, s->main_command, s->timeout_start_usec, - EXEC_PASS_FDS|EXEC_APPLY_SANDBOXING|EXEC_APPLY_CHROOT|EXEC_APPLY_TTY_STDIN|EXEC_SET_WATCHDOG, + EXEC_PASS_FDS|EXEC_APPLY_SANDBOXING|EXEC_APPLY_CHROOT|EXEC_APPLY_TTY_STDIN|EXEC_SET_WATCHDOG|EXEC_SETENV_MONITOR_RESULT, &pid); if (r < 0) goto fail; diff --git a/test/units/testsuite-68.sh b/test/units/testsuite-68.sh index 89c5d2618c..3072a94575 100755 --- a/test/units/testsuite-68.sh +++ b/test/units/testsuite-68.sh @@ -169,8 +169,13 @@ cat >/run/systemd/system/testservice-failure-exit-handler-68.service </run/systemd/system/testservice-failure-exit-handler-68-template@.service < Description=TEST-68-PROPAGATE-EXIT-STATUS handle service exiting in failure (template) [Service] +Type=oneshot ExecStartPre=echo "triggered by %i" ExecStartPre=/tmp/check_on_failure.sh +ExecStartPre=/tmp/check_on_failure.sh ExecStart=/tmp/check_on_failure.sh +ExecStart=/tmp/check_on_failure.sh +ExecStartPost=test -z '$MONITOR_SERVICE_RESULT' EOF systemctl daemon-reload