From f6f271f0db802c628f95d853ceb4f1ef254d4a74 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Mon, 3 Jun 2024 06:11:02 +0200 Subject: [PATCH 1/4] core/unit: add one assertion, reduce variable scope --- src/core/unit.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/core/unit.c b/src/core/unit.c index 2d40618fcb..2da5876237 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -2597,9 +2597,6 @@ static bool unit_process_job(Job *j, UnitActiveState ns, bool reload_success) { } void unit_notify(Unit *u, UnitActiveState os, UnitActiveState ns, bool reload_success) { - const char *reason; - Manager *m; - assert(u); assert(os < _UNIT_ACTIVE_STATE_MAX); assert(ns < _UNIT_ACTIVE_STATE_MAX); @@ -2608,7 +2605,7 @@ void unit_notify(Unit *u, UnitActiveState os, UnitActiveState ns, bool reload_su * UnitActiveState! That means that ns == os is an expected behavior here. For example: if a mount point is * remounted this function will be called too! */ - m = u->manager; + Manager *m = ASSERT_PTR(u->manager); /* Let's enqueue the change signal early. In case this unit has a job associated we want that this unit is in * the bus queue, so that any job change signal queued will force out the unit change signal first. */ @@ -2707,6 +2704,8 @@ void unit_notify(Unit *u, UnitActiveState os, UnitActiveState ns, bool reload_su unit_trigger_notify(u); if (!MANAGER_IS_RELOADING(m)) { + const char *reason; + 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); From b984151e502ffbc31c61eb9eec5f2046d77608df Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Sun, 9 Jun 2024 14:48:37 +0200 Subject: [PATCH 2/4] core/unit: merge nested if statements, use else where appropriate We already use `else if` for unit state checks above. Let's use that at one more place to make mutually exclusive cases more distinct. --- src/core/unit.c | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/src/core/unit.c b/src/core/unit.c index 2da5876237..a11c075c7f 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -2611,17 +2611,16 @@ void unit_notify(Unit *u, UnitActiveState os, UnitActiveState ns, bool reload_su * the bus queue, so that any job change signal queued will force out the unit change signal first. */ unit_add_to_dbus_queue(u); - /* Update systemd-oomd on the property/state change */ - if (os != ns) { - /* Always send an update if the unit is going into an inactive state so systemd-oomd knows to stop - * monitoring. - * Also send an update whenever the unit goes active; this is to handle a case where an override file - * sets one of the ManagedOOM*= properties to "kill", then later removes it. systemd-oomd needs to - * know to stop monitoring when the unit changes from "kill" -> "auto" on daemon-reload, but we don't - * have the information on the property. Thus, indiscriminately send an update. */ - if (UNIT_IS_INACTIVE_OR_FAILED(ns) || UNIT_IS_ACTIVE_OR_RELOADING(ns)) - (void) manager_varlink_send_managed_oom_update(u); - } + /* Update systemd-oomd on the property/state change. + * + * Always send an update if the unit is going into an inactive state so systemd-oomd knows to + * stop monitoring. + * Also send an update whenever the unit goes active; this is to handle a case where an override file + * sets one of the ManagedOOM*= properties to "kill", then later removes it. systemd-oomd needs to + * know to stop monitoring when the unit changes from "kill" -> "auto" on daemon-reload, but we don't + * have the information on the property. Thus, indiscriminately send an update. */ + if (os != ns && (UNIT_IS_INACTIVE_OR_FAILED(ns) || UNIT_IS_ACTIVE_OR_RELOADING(ns))) + (void) manager_varlink_send_managed_oom_update(u); /* Update timestamps for state changes */ if (!MANAGER_IS_RELOADING(m)) { @@ -2734,9 +2733,8 @@ void unit_notify(Unit *u, UnitActiveState os, UnitActiveState ns, bool reload_su /* Maybe we can release some resources now? */ unit_submit_to_release_resources_queue(u); - } - if (UNIT_IS_ACTIVE_OR_RELOADING(ns)) { + } else if (UNIT_IS_ACTIVE_OR_RELOADING(ns)) { /* Start uphold units regardless if going up was expected or not */ check_uphold_dependencies(u); From 8c4514b258f35b9e9a05dd483b7605adac732b6f Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Mon, 3 Jun 2024 06:15:15 +0200 Subject: [PATCH 3/4] core/unit: place OnFailure= handling close to OnSuccess= While at it, the duplicate log of "Unit entered failed state" is dropped. Since the state change would be logged by each unit type already, before calling unit_notify(). --- src/core/unit.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/core/unit.c b/src/core/unit.c index a11c075c7f..def621ef6d 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -2672,20 +2672,14 @@ void unit_notify(Unit *u, UnitActiveState os, UnitActiveState ns, bool reload_su retroactively_stop_dependencies(u); } - if (ns != os && ns == UNIT_FAILED) { - log_unit_debug(u, "Unit entered failed state."); - 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)) { /* This unit just finished starting up */ unit_emit_audit_start(u); manager_send_unit_plymouth(m, u); manager_send_unit_supervisor(m, u, /* active= */ true); - } - if (UNIT_IS_INACTIVE_OR_FAILED(ns) && !UNIT_IS_INACTIVE_OR_FAILED(os)) { + } else if (UNIT_IS_INACTIVE_OR_FAILED(ns) && !UNIT_IS_INACTIVE_OR_FAILED(os)) { /* This unit just stopped/failed. */ unit_emit_audit_stop(u, ns); @@ -2695,6 +2689,8 @@ void unit_notify(Unit *u, UnitActiveState os, UnitActiveState ns, bool reload_su if (ns == UNIT_INACTIVE && !IN_SET(os, UNIT_FAILED, UNIT_INACTIVE, UNIT_MAINTENANCE)) unit_start_on_failure(u, "OnSuccess=", UNIT_ATOM_ON_SUCCESS, u->on_success_job_mode); + else if (ns != os && ns == UNIT_FAILED) + unit_start_on_failure(u, "OnFailure=", UNIT_ATOM_ON_FAILURE, u->on_failure_job_mode); } manager_recheck_journal(m); From 9faeb2d024540ac4b94dedb00a13606ec262cc31 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Sat, 8 Jun 2024 00:02:26 +0200 Subject: [PATCH 4/4] core: clean up OnFailure= and OnSuccess= handling a bit - Replace "on_failure" in function names with "on_termination" - Only pass UnitDependencyAtom in, as other info can be determined from that --- src/core/job.c | 7 ++-- src/core/unit.c | 97 +++++++++++++++++++++++++++---------------------- src/core/unit.h | 2 +- 3 files changed, 58 insertions(+), 48 deletions(-) diff --git a/src/core/job.c b/src/core/job.c index 2f19468a95..fb32223e18 100644 --- a/src/core/job.c +++ b/src/core/job.c @@ -1056,8 +1056,9 @@ int job_finish_and_invalidate(Job *j, JobResult result, bool recursive, bool alr !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. */ + /* Trigger OnFailure= dependencies manually here. We need to do that because a failed job might not + * cause a unit state change. Note that we don't treat JOB_CANCELED as failure in this context. + * And JOB_FAILURE is already handled by the unit itself (unit_notify). */ if (IN_SET(result, JOB_TIMEOUT, JOB_DEPENDENCY)) { log_unit_struct(u, LOG_NOTICE, "JOB_TYPE=%s", job_type_to_string(t), @@ -1067,7 +1068,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, "OnFailure=", UNIT_ATOM_ON_FAILURE, u->on_failure_job_mode); + unit_start_on_termination_deps(u, UNIT_ATOM_ON_FAILURE); } unit_trigger_notify(u); diff --git a/src/core/unit.c b/src/core/unit.c index def621ef6d..7e5f56cb21 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -1600,26 +1600,36 @@ 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) { +static const struct { + UnitDependencyAtom atom; + size_t job_mode_offset; + const char *dependency_name; + const char *job_mode_setting_name; +} on_termination_settings[] = { + { UNIT_ATOM_ON_SUCCESS, offsetof(Unit, on_success_job_mode), "OnSuccess=", "OnSuccessJobMode=" }, + { UNIT_ATOM_ON_FAILURE, offsetof(Unit, on_failure_job_mode), "OnFailure=", "OnFailureJobMode=" }, +}; - Unit *other, *found = NULL; +static int unit_validate_on_termination_job_modes(Unit *u) { + assert(u); - if (job_mode != JOB_ISOLATE) - return 0; + /* Verify that if On{Success,Failure}JobMode=isolate, only one unit gets specified. */ - 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); + FOREACH_ELEMENT(setting, on_termination_settings) { + JobMode job_mode = *(JobMode*) ((uint8_t*) u + setting->job_mode_offset); + + if (job_mode != JOB_ISOLATE) + continue; + + Unit *other, *found = NULL; + UNIT_FOREACH_DEPENDENCY(other, u, setting->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.", + setting->dependency_name, setting->job_mode_setting_name); + } } return 0; @@ -1678,11 +1688,7 @@ int unit_load(Unit *u) { if (r < 0) goto fail; - r = unit_validate_on_failure_job_mode(u, "OnSuccessJobMode=", u->on_success_job_mode, "OnSuccess=", UNIT_ATOM_ON_SUCCESS); - if (r < 0) - goto fail; - - r = unit_validate_on_failure_job_mode(u, "OnFailureJobMode=", u->on_failure_job_mode, "OnFailure=", UNIT_ATOM_ON_FAILURE); + r = unit_validate_on_termination_job_modes(u); if (r < 0) goto fail; @@ -2246,40 +2252,43 @@ static void retroactively_stop_dependencies(Unit *u) { (void) manager_add_job(u->manager, JOB_STOP, other, JOB_REPLACE, NULL, NULL, NULL); } -void unit_start_on_failure( - Unit *u, - const char *dependency_name, - UnitDependencyAtom atom, - JobMode job_mode) { - - int n_jobs = -1; - Unit *other; +void unit_start_on_termination_deps(Unit *u, UnitDependencyAtom atom) { + const char *dependency_name = NULL; + JobMode job_mode; + unsigned n_jobs = 0; int r; - assert(u); - assert(dependency_name); - assert(IN_SET(atom, UNIT_ATOM_ON_SUCCESS, UNIT_ATOM_ON_FAILURE)); - /* Act on OnFailure= and OnSuccess= dependencies */ + assert(u); + assert(u->manager); + assert(IN_SET(atom, UNIT_ATOM_ON_SUCCESS, UNIT_ATOM_ON_FAILURE)); + + FOREACH_ELEMENT(setting, on_termination_settings) + if (atom == setting->atom) { + job_mode = *(JobMode*) ((uint8_t*) u + setting->job_mode_offset); + dependency_name = setting->dependency_name; + break; + } + + assert(dependency_name); + + Unit *other; UNIT_FOREACH_DEPENDENCY(other, u, atom) { _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; - if (n_jobs < 0) { + if (n_jobs == 0) log_unit_info(u, "Triggering %s dependencies.", dependency_name); - n_jobs = 0; - } 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)); + log_unit_warning_errno(u, r, "Failed to enqueue %s%s job, ignoring: %s", + dependency_name, other->id, bus_error_message(&error, r)); n_jobs++; } - if (n_jobs >= 0) - log_unit_debug(u, "Triggering %s dependencies done (%i %s).", + if (n_jobs > 0) + log_unit_debug(u, "Triggering %s dependencies done (%u %s).", dependency_name, n_jobs, n_jobs == 1 ? "job" : "jobs"); } @@ -2688,9 +2697,9 @@ void unit_notify(Unit *u, UnitActiveState os, UnitActiveState ns, bool reload_su } if (ns == UNIT_INACTIVE && !IN_SET(os, UNIT_FAILED, UNIT_INACTIVE, UNIT_MAINTENANCE)) - unit_start_on_failure(u, "OnSuccess=", UNIT_ATOM_ON_SUCCESS, u->on_success_job_mode); + unit_start_on_termination_deps(u, UNIT_ATOM_ON_SUCCESS); else if (ns != os && ns == UNIT_FAILED) - unit_start_on_failure(u, "OnFailure=", UNIT_ATOM_ON_FAILURE, u->on_failure_job_mode); + unit_start_on_termination_deps(u, UNIT_ATOM_ON_FAILURE); } manager_recheck_journal(m); diff --git a/src/core/unit.h b/src/core/unit.h index b135fecc51..b0ffbbcebe 100644 --- a/src/core/unit.h +++ b/src/core/unit.h @@ -916,7 +916,7 @@ bool unit_will_restart(Unit *u); int unit_add_default_target_dependency(Unit *u, Unit *target); -void unit_start_on_failure(Unit *u, const char *dependency_name, UnitDependencyAtom atom, JobMode job_mode); +void unit_start_on_termination_deps(Unit *u, UnitDependencyAtom atom); void unit_trigger_notify(Unit *u); UnitFileState unit_get_unit_file_state(Unit *u);