From 49b34f75e7c801210624e0c7dd00be990873628a Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Mon, 22 May 2023 08:30:30 +0800 Subject: [PATCH 1/2] core: get rid of unused Service.will_auto_restart logic The announced new behavior for OnFailure= never worked properly, and we've fixed the document instead in #27675. Therefore, let's get rid of the unused logic completely. More at #27594. The to-be-added RestartMode= option should cover the use case hopefully. Closes #27594 --- src/core/service.c | 13 +++---------- src/core/service.h | 2 -- src/core/unit.c | 7 ++----- src/core/unit.h | 1 - 4 files changed, 5 insertions(+), 18 deletions(-) diff --git a/src/core/service.c b/src/core/service.c index 41e6198771..ad8ff75b14 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -1946,8 +1946,6 @@ static bool service_will_restart(Unit *u) { assert(s); - if (s->will_auto_restart) - return true; if (IN_SET(s->state, SERVICE_DEAD_BEFORE_AUTO_RESTART, SERVICE_FAILED_BEFORE_AUTO_RESTART, SERVICE_AUTO_RESTART)) return true; @@ -1993,19 +1991,14 @@ static void service_enter_dead(Service *s, ServiceResult f, bool allow_restart) log_unit_debug(UNIT(s), "Service restart not allowed."); else { const char *reason; - bool shall_restart; - shall_restart = service_shall_restart(s, &reason); + allow_restart = service_shall_restart(s, &reason); log_unit_debug(UNIT(s), "Service will %srestart (%s)", - shall_restart ? "" : "not ", + allow_restart ? "" : "not ", reason); - if (shall_restart) - s->will_auto_restart = true; } - if (s->will_auto_restart) { - s->will_auto_restart = false; - + if (allow_restart) { /* We make two state changes here: one that maps to the high-level UNIT_INACTIVE/UNIT_FAILED * state (i.e. a state indicating deactivation), and then one that that maps to the * high-level UNIT_STARTING state (i.e. a state indicating activation). We do this so that diff --git a/src/core/service.h b/src/core/service.h index 55c4127dee..0e578c9280 100644 --- a/src/core/service.h +++ b/src/core/service.h @@ -181,8 +181,6 @@ struct Service { bool main_pid_alien:1; bool bus_name_good:1; bool forbid_restart:1; - /* Keep restart intention between UNIT_FAILED and UNIT_ACTIVATING */ - bool will_auto_restart:1; bool start_timeout_defined:1; bool exec_fd_hot:1; diff --git a/src/core/unit.c b/src/core/unit.c index 3393138bac..49d289c04d 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -2777,9 +2777,7 @@ void unit_notify(Unit *u, UnitActiveState os, UnitActiveState ns, UnitNotifyFlag if (ns != os && ns == UNIT_FAILED) { log_unit_debug(u, "Unit entered failed state."); - - if (!(flags & UNIT_NOTIFY_WILL_AUTO_RESTART)) - unit_start_on_failure(u, "OnFailure=", UNIT_ATOM_ON_FAILURE, u->on_failure_job_mode); + 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)) { @@ -2796,8 +2794,7 @@ void unit_notify(Unit *u, UnitActiveState os, UnitActiveState ns, UnitNotifyFlag unit_log_resources(u); } - if (ns == UNIT_INACTIVE && !IN_SET(os, UNIT_FAILED, UNIT_INACTIVE, UNIT_MAINTENANCE) && - !(flags & UNIT_NOTIFY_WILL_AUTO_RESTART)) + 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); } diff --git a/src/core/unit.h b/src/core/unit.h index 7e85150643..3cab35ea4f 100644 --- a/src/core/unit.h +++ b/src/core/unit.h @@ -904,7 +904,6 @@ void unit_notify_cgroup_oom(Unit *u, bool managed_oom); typedef enum UnitNotifyFlags { UNIT_NOTIFY_RELOAD_FAILURE = 1 << 0, - UNIT_NOTIFY_WILL_AUTO_RESTART = 1 << 1, } UnitNotifyFlags; void unit_notify(Unit *u, UnitActiveState os, UnitActiveState ns, UnitNotifyFlags flags); From 96b09de500f9d658b2e49abf3be15e06f9bd1ca6 Mon Sep 17 00:00:00 2001 From: Mike Yuan Date: Mon, 22 May 2023 08:35:53 +0800 Subject: [PATCH 2/2] core: drop UnitNotifyFlags This essentially reverts 2ad2e41a72ec19159c0746a78e15ff880fe32a63. No longer needed after dropping UNIT_NOTIFY_WILL_AUTO_RESTART. --- src/core/automount.c | 2 +- src/core/device.c | 2 +- src/core/mount.c | 3 +-- src/core/path.c | 2 +- src/core/scope.c | 2 +- src/core/service.c | 4 +--- src/core/slice.c | 2 +- src/core/socket.c | 2 +- src/core/swap.c | 2 +- src/core/target.c | 2 +- src/core/timer.c | 2 +- src/core/unit.c | 10 +++++----- src/core/unit.h | 6 +----- 13 files changed, 17 insertions(+), 24 deletions(-) diff --git a/src/core/automount.c b/src/core/automount.c index 89755e3a9b..5df697bf22 100644 --- a/src/core/automount.c +++ b/src/core/automount.c @@ -283,7 +283,7 @@ static void automount_set_state(Automount *a, AutomountState state) { if (state != old_state) log_unit_debug(UNIT(a), "Changed %s -> %s", automount_state_to_string(old_state), automount_state_to_string(state)); - unit_notify(UNIT(a), state_translation_table[old_state], state_translation_table[state], 0); + unit_notify(UNIT(a), state_translation_table[old_state], state_translation_table[state], /* reload_success = */ true); } static int automount_coldplug(Unit *u) { diff --git a/src/core/device.c b/src/core/device.c index 79082d1688..60ab59c53b 100644 --- a/src/core/device.c +++ b/src/core/device.c @@ -182,7 +182,7 @@ static void device_set_state(Device *d, DeviceState state) { if (state != old_state) log_unit_debug(UNIT(d), "Changed %s -> %s", device_state_to_string(old_state), device_state_to_string(state)); - unit_notify(UNIT(d), state_translation_table[old_state], state_translation_table[state], 0); + unit_notify(UNIT(d), state_translation_table[old_state], state_translation_table[state], /* reload_success = */ true); } static void device_found_changed(Device *d, DeviceFound previous, DeviceFound now) { diff --git a/src/core/mount.c b/src/core/mount.c index f25188681d..8f02bd29d8 100644 --- a/src/core/mount.c +++ b/src/core/mount.c @@ -761,8 +761,7 @@ static void mount_set_state(Mount *m, MountState state) { if (state != old_state) log_unit_debug(UNIT(m), "Changed %s -> %s", mount_state_to_string(old_state), mount_state_to_string(state)); - unit_notify(UNIT(m), state_translation_table[old_state], state_translation_table[state], - m->reload_result == MOUNT_SUCCESS ? 0 : UNIT_NOTIFY_RELOAD_FAILURE); + unit_notify(UNIT(m), state_translation_table[old_state], state_translation_table[state], m->reload_result == MOUNT_SUCCESS); } static int mount_coldplug(Unit *u) { diff --git a/src/core/path.c b/src/core/path.c index c95663c3aa..5fb14d9a10 100644 --- a/src/core/path.c +++ b/src/core/path.c @@ -477,7 +477,7 @@ static void path_set_state(Path *p, PathState state) { if (state != old_state) log_unit_debug(UNIT(p), "Changed %s -> %s", path_state_to_string(old_state), path_state_to_string(state)); - unit_notify(UNIT(p), state_translation_table[old_state], state_translation_table[state], 0); + unit_notify(UNIT(p), state_translation_table[old_state], state_translation_table[state], /* reload_success = */ true); } static void path_enter_waiting(Path *p, bool initial, bool from_trigger_notify); diff --git a/src/core/scope.c b/src/core/scope.c index 5f3b62e021..761eb5ea56 100644 --- a/src/core/scope.c +++ b/src/core/scope.c @@ -127,7 +127,7 @@ static void scope_set_state(Scope *s, ScopeState state) { if (state != old_state) log_debug("%s changed %s -> %s", UNIT(s)->id, scope_state_to_string(old_state), scope_state_to_string(state)); - unit_notify(UNIT(s), state_translation_table[old_state], state_translation_table[state], 0); + unit_notify(UNIT(s), state_translation_table[old_state], state_translation_table[state], /* reload_success = */ true); } static int scope_add_default_dependencies(Scope *s) { diff --git a/src/core/service.c b/src/core/service.c index ad8ff75b14..533d7d3771 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -1277,9 +1277,7 @@ static void service_set_state(Service *s, ServiceState state) { if (old_state != state) log_unit_debug(UNIT(s), "Changed %s -> %s", service_state_to_string(old_state), service_state_to_string(state)); - unit_notify(UNIT(s), table[old_state], table[state], - (s->reload_result == SERVICE_SUCCESS ? 0 : UNIT_NOTIFY_RELOAD_FAILURE) | - (s->will_auto_restart ? UNIT_NOTIFY_WILL_AUTO_RESTART : 0)); + unit_notify(UNIT(s), table[old_state], table[state], s->reload_result == SERVICE_SUCCESS); } static usec_t service_coldplug_timeout(Service *s) { diff --git a/src/core/slice.c b/src/core/slice.c index c87b790a97..490aabfd35 100644 --- a/src/core/slice.c +++ b/src/core/slice.c @@ -43,7 +43,7 @@ static void slice_set_state(Slice *t, SliceState state) { slice_state_to_string(old_state), slice_state_to_string(state)); - unit_notify(UNIT(t), state_translation_table[old_state], state_translation_table[state], 0); + unit_notify(UNIT(t), state_translation_table[old_state], state_translation_table[state], /* reload_success = */ true); } static int slice_add_parent_slice(Slice *s) { diff --git a/src/core/socket.c b/src/core/socket.c index 0bd1b12652..f3dd000e60 100644 --- a/src/core/socket.c +++ b/src/core/socket.c @@ -1844,7 +1844,7 @@ static void socket_set_state(Socket *s, SocketState state) { if (state != old_state) log_unit_debug(UNIT(s), "Changed %s -> %s", socket_state_to_string(old_state), socket_state_to_string(state)); - unit_notify(UNIT(s), state_translation_table[old_state], state_translation_table[state], 0); + unit_notify(UNIT(s), state_translation_table[old_state], state_translation_table[state], /* reload_success = */ true); } static int socket_coldplug(Unit *u) { diff --git a/src/core/swap.c b/src/core/swap.c index 26a950b058..a57176b1a3 100644 --- a/src/core/swap.c +++ b/src/core/swap.c @@ -554,7 +554,7 @@ static void swap_set_state(Swap *s, SwapState state) { if (state != old_state) log_unit_debug(UNIT(s), "Changed %s -> %s", swap_state_to_string(old_state), swap_state_to_string(state)); - unit_notify(UNIT(s), state_translation_table[old_state], state_translation_table[state], 0); + unit_notify(UNIT(s), state_translation_table[old_state], state_translation_table[state], /* reload_success = */ true); /* If there other units for the same device node have a job queued it might be worth checking again if it is runnable diff --git a/src/core/target.c b/src/core/target.c index 6225df5b0d..3519b4b653 100644 --- a/src/core/target.c +++ b/src/core/target.c @@ -31,7 +31,7 @@ static void target_set_state(Target *t, TargetState state) { target_state_to_string(old_state), target_state_to_string(state)); - unit_notify(UNIT(t), state_translation_table[old_state], state_translation_table[state], 0); + unit_notify(UNIT(t), state_translation_table[old_state], state_translation_table[state], /* reload_success = */ true); } static int target_add_default_dependencies(Target *t) { diff --git a/src/core/timer.c b/src/core/timer.c index aab376e5cc..4671e20dd9 100644 --- a/src/core/timer.c +++ b/src/core/timer.c @@ -298,7 +298,7 @@ static void timer_set_state(Timer *t, TimerState state) { if (state != old_state) log_unit_debug(UNIT(t), "Changed %s -> %s", timer_state_to_string(old_state), timer_state_to_string(state)); - unit_notify(UNIT(t), state_translation_table[old_state], state_translation_table[state], 0); + unit_notify(UNIT(t), state_translation_table[old_state], state_translation_table[state], /* reload_success = */ true); } static void timer_enter_waiting(Timer *t, bool time_change); diff --git a/src/core/unit.c b/src/core/unit.c index 49d289c04d..f7b8cf0abc 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -2111,7 +2111,7 @@ int unit_reload(Unit *u) { if (!UNIT_VTABLE(u)->reload) { /* Unit doesn't have a reload function, but we need to propagate the reload anyway */ - unit_notify(u, unit_active_state(u), unit_active_state(u), 0); + unit_notify(u, unit_active_state(u), unit_active_state(u), /* reload_success = */ true); return 0; } @@ -2622,7 +2622,7 @@ static void unit_emit_audit_stop(Unit *u, UnitActiveState state) { } } -static bool unit_process_job(Job *j, UnitActiveState ns, UnitNotifyFlags flags) { +static bool unit_process_job(Job *j, UnitActiveState ns, bool reload_success) { bool unexpected = false; JobResult result; @@ -2664,7 +2664,7 @@ static bool unit_process_job(Job *j, UnitActiveState ns, UnitNotifyFlags flags) if (j->state == JOB_RUNNING) { if (ns == UNIT_ACTIVE) - job_finish_and_invalidate(j, (flags & UNIT_NOTIFY_RELOAD_FAILURE) ? JOB_FAILED : JOB_DONE, true, false); + job_finish_and_invalidate(j, reload_success ? JOB_DONE : JOB_FAILED, true, false); else if (!IN_SET(ns, UNIT_ACTIVATING, UNIT_RELOADING)) { unexpected = true; @@ -2695,7 +2695,7 @@ static bool unit_process_job(Job *j, UnitActiveState ns, UnitNotifyFlags flags) return unexpected; } -void unit_notify(Unit *u, UnitActiveState os, UnitActiveState ns, UnitNotifyFlags flags) { +void unit_notify(Unit *u, UnitActiveState os, UnitActiveState ns, bool reload_success) { const char *reason; Manager *m; @@ -2760,7 +2760,7 @@ void unit_notify(Unit *u, UnitActiveState os, UnitActiveState ns, UnitNotifyFlag /* Let's propagate state changes to the job */ if (u->job) - unexpected = unit_process_job(u->job, ns, flags); + unexpected = unit_process_job(u->job, ns, reload_success); else unexpected = true; diff --git a/src/core/unit.h b/src/core/unit.h index 3cab35ea4f..09af8915dc 100644 --- a/src/core/unit.h +++ b/src/core/unit.h @@ -902,11 +902,7 @@ int unit_kill_common(Unit *u, KillWho who, int signo, int code, int value, pid_t void unit_notify_cgroup_oom(Unit *u, bool managed_oom); -typedef enum UnitNotifyFlags { - UNIT_NOTIFY_RELOAD_FAILURE = 1 << 0, -} UnitNotifyFlags; - -void unit_notify(Unit *u, UnitActiveState os, UnitActiveState ns, UnitNotifyFlags flags); +void unit_notify(Unit *u, UnitActiveState os, UnitActiveState ns, bool reload_success); int unit_watch_pid(Unit *u, pid_t pid, bool exclusive); void unit_unwatch_pid(Unit *u, pid_t pid);