From c436a4981eb376338beaa2042a1d0293fc8e7637 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 1 Oct 2019 14:13:35 +0200 Subject: [PATCH 01/11] core: minor formatting adjustment --- src/core/kill.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/core/kill.c b/src/core/kill.c index 6fe96cfc07..e7608ac81f 100644 --- a/src/core/kill.c +++ b/src/core/kill.c @@ -25,7 +25,7 @@ void kill_context_dump(KillContext *c, FILE *f, const char *prefix) { "%sKillSignal: SIG%s\n" "%sFinalKillSignal: SIG%s\n" "%sSendSIGKILL: %s\n" - "%sSendSIGHUP: %s\n", + "%sSendSIGHUP: %s\n", prefix, kill_mode_to_string(c->kill_mode), prefix, signal_to_string(c->kill_signal), prefix, signal_to_string(c->final_kill_signal), @@ -35,20 +35,20 @@ void kill_context_dump(KillContext *c, FILE *f, const char *prefix) { static const char* const kill_mode_table[_KILL_MODE_MAX] = { [KILL_CONTROL_GROUP] = "control-group", - [KILL_PROCESS] = "process", - [KILL_MIXED] = "mixed", - [KILL_NONE] = "none" + [KILL_PROCESS] = "process", + [KILL_MIXED] = "mixed", + [KILL_NONE] = "none", }; DEFINE_STRING_TABLE_LOOKUP(kill_mode, KillMode); static const char* const kill_who_table[_KILL_WHO_MAX] = { - [KILL_MAIN] = "main", - [KILL_CONTROL] = "control", - [KILL_ALL] = "all", - [KILL_MAIN_FAIL] = "main-fail", + [KILL_MAIN] = "main", + [KILL_CONTROL] = "control", + [KILL_ALL] = "all", + [KILL_MAIN_FAIL] = "main-fail", [KILL_CONTROL_FAIL] = "control-fail", - [KILL_ALL_FAIL] = "all-fail" + [KILL_ALL_FAIL] = "all-fail", }; DEFINE_STRING_TABLE_LOOKUP(kill_who, KillWho); From fa036b61144129a7efffe44b054c1eb639d8603b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 1 Oct 2019 14:25:10 +0200 Subject: [PATCH 02/11] core: remove unused prototypes --- src/core/load-fragment.h | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/core/load-fragment.h b/src/core/load-fragment.h index e29d58f706..f369c4dc4f 100644 --- a/src/core/load-fragment.h +++ b/src/core/load-fragment.h @@ -43,8 +43,6 @@ CONFIG_PARSER_PROTOTYPE(config_parse_exec_cpu_sched_prio); CONFIG_PARSER_PROTOTYPE(config_parse_exec_cpu_affinity); CONFIG_PARSER_PROTOTYPE(config_parse_exec_secure_bits); CONFIG_PARSER_PROTOTYPE(config_parse_capability_set); -CONFIG_PARSER_PROTOTYPE(config_parse_kill_signal); -CONFIG_PARSER_PROTOTYPE(config_parse_final_kill_signal); CONFIG_PARSER_PROTOTYPE(config_parse_exec_mount_flags); CONFIG_PARSER_PROTOTYPE(config_parse_timer); CONFIG_PARSER_PROTOTYPE(config_parse_trigger_unit); From 28a2dfe8014327a884bef36667c1cf94f547e716 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 1 Oct 2019 14:58:55 +0200 Subject: [PATCH 03/11] core: add helper function to check job status Since job.h includes unit.h, and unit.h includes job.h, imports need to be adjusted to make sure unit.h is included first if the helper is used. --- src/core/dbus-job.h | 2 +- src/core/dbus-unit.h | 1 - src/core/service.c | 2 +- src/core/unit.c | 9 ++------- src/core/unit.h | 4 ++++ src/test/test-engine.c | 6 +++--- src/test/test-job-type.c | 1 - 7 files changed, 11 insertions(+), 14 deletions(-) diff --git a/src/core/dbus-job.h b/src/core/dbus-job.h index c9f6fc7187..380e117fca 100644 --- a/src/core/dbus-job.h +++ b/src/core/dbus-job.h @@ -4,7 +4,7 @@ #include "sd-bus.h" #include "sd-bus-vtable.h" -#include "job.h" +#include "unit.h" extern const sd_bus_vtable bus_job_vtable[]; diff --git a/src/core/dbus-unit.h b/src/core/dbus-unit.h index 24abf3c088..91711311a7 100644 --- a/src/core/dbus-unit.h +++ b/src/core/dbus-unit.h @@ -4,7 +4,6 @@ #include "sd-bus.h" #include "sd-bus-vtable.h" -#include "job.h" #include "unit.h" extern const sd_bus_vtable bus_unit_vtable[]; diff --git a/src/core/service.c b/src/core/service.c index 71befcddc8..8e313f8e90 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -2245,7 +2245,7 @@ static void service_enter_restart(Service *s) { assert(s); - if (UNIT(s)->job && UNIT(s)->job->type == JOB_STOP) { + if (unit_has_job_type(UNIT(s), JOB_STOP)) { /* Don't restart things if we are going down anyway */ log_unit_info(UNIT(s), "Stop job pending for unit, delaying automatic restart."); diff --git a/src/core/unit.c b/src/core/unit.c index b9fb7dbc0e..71fe7a6e23 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -4022,7 +4022,7 @@ bool unit_stop_pending(Unit *u) { * different from unit_inactive_or_pending() which checks both * the current state and for a queued job. */ - return u->job && u->job->type == JOB_STOP; + return unit_has_job_type(u, JOB_STOP); } bool unit_inactive_or_pending(Unit *u) { @@ -4057,12 +4057,7 @@ bool unit_active_or_pending(Unit *u) { bool unit_will_restart_default(Unit *u) { assert(u); - if (!u->job) - return false; - if (u->job->type == JOB_START) - return true; - - return false; + return unit_has_job_type(u, JOB_START); } bool unit_will_restart(Unit *u) { diff --git a/src/core/unit.h b/src/core/unit.h index 3b0042a9eb..793ee638a6 100644 --- a/src/core/unit.h +++ b/src/core/unit.h @@ -842,6 +842,10 @@ const char *unit_label_path(Unit *u); int unit_pid_attachable(Unit *unit, pid_t pid, sd_bus_error *error); +static inline bool unit_has_job_type(Unit *u, JobType type) { + return u && u->job && u->job->type == type; +} + /* unit_log_skip is for cases like ExecCondition= where a unit is considered "done" * after some execution, rather than succeeded or failed. */ void unit_log_skip(Unit *u, const char *result); diff --git a/src/test/test-engine.c b/src/test/test-engine.c index 633cc42534..1064811497 100644 --- a/src/test/test-engine.c +++ b/src/test/test-engine.c @@ -106,9 +106,9 @@ int main(int argc, char *argv[]) { printf("Test11: (Start/stop job ordering, execution cycle)\n"); assert_se(manager_add_job(m, JOB_START, i, JOB_FAIL, NULL, NULL, &j) == 0); - assert_se(a->job && a->job->type == JOB_STOP); - assert_se(d->job && d->job->type == JOB_STOP); - assert_se(b->job && b->job->type == JOB_START); + assert_se(unit_has_job_type(a, JOB_STOP)); + assert_se(unit_has_job_type(d, JOB_STOP)); + assert_se(unit_has_job_type(b, JOB_START)); manager_dump_jobs(m, stdout, "\t"); printf("Load6:\n"); diff --git a/src/test/test-job-type.c b/src/test/test-job-type.c index d51e0d94fd..33a95c6b52 100644 --- a/src/test/test-job-type.c +++ b/src/test/test-job-type.c @@ -2,7 +2,6 @@ #include -#include "job.h" #include "service.h" #include "unit.h" From a232ebcc2cd3d5e16b0d0fb0be61549f25fb8239 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 1 Oct 2019 15:15:06 +0200 Subject: [PATCH 04/11] core: add support for RestartKillSignal= to override signal used for restart jobs v2: - if RestartKillSignal= is not specified, fall back to KillSignal=. This is necessary to preserve backwards compatibility (and keep KillSignal= generally useful). --- shell-completion/bash/systemd-run | 2 +- shell-completion/zsh/_systemd-run | 2 +- src/core/dbus-kill.c | 22 ++++++++++++++++++++ src/core/kill.c | 3 +++ src/core/kill.h | 7 +++++++ src/core/load-fragment-gperf.gperf.m4 | 1 + src/core/mount.c | 2 ++ src/core/service.c | 8 +++++-- src/core/socket.c | 13 ++++++++++-- src/core/swap.c | 23 +++++++++++++++------ src/core/unit.c | 3 +++ src/core/unit.h | 1 + src/shared/bus-unit-util.c | 5 +---- test/TEST-23-TYPE-EXEC/testsuite.sh | 8 +++++++ test/fuzz/fuzz-unit-file/directives.service | 1 + 15 files changed, 85 insertions(+), 16 deletions(-) diff --git a/shell-completion/bash/systemd-run b/shell-completion/bash/systemd-run index 4670973032..707b038d9a 100644 --- a/shell-completion/bash/systemd-run +++ b/shell-completion/bash/systemd-run @@ -80,7 +80,7 @@ _systemd_run() { SendSIGKILL= MemoryLimit= CPUShares= BlockIOWeight= User= Group= DevicePolicy= KillMode= DeviceAllow= BlockIOReadBandwidth= BlockIOWriteBandwidth= BlockIODeviceWeight= Nice= Environment= - KillSignal= FinalKillSignal= LimitCPU= LimitFSIZE= LimitDATA= + KillSignal= RestartKillSignal= FinalKillSignal= LimitCPU= LimitFSIZE= LimitDATA= LimitSTACK= LimitCORE= LimitRSS= LimitNOFILE= LimitAS= LimitNPROC= LimitMEMLOCK= LimitLOCKS= LimitSIGPENDING= LimitMSGQUEUE= LimitNICE= LimitRTPRIO= LimitRTTIME= PrivateTmp= PrivateDevices= diff --git a/shell-completion/zsh/_systemd-run b/shell-completion/zsh/_systemd-run index 6a703a075b..ca0faa1484 100644 --- a/shell-completion/zsh/_systemd-run +++ b/shell-completion/zsh/_systemd-run @@ -35,7 +35,7 @@ _arguments \ SendSIGKILL= MemoryLimit= CPUShares= BlockIOWeight= User= Group= \ DevicePolicy= KillMode= DeviceAllow= BlockIOReadBandwidth= \ BlockIOWriteBandwidth= BlockIODeviceWeight= Nice= Environment= \ - KillSignal= FinalKillSignal= LimitCPU= LimitFSIZE= LimitDATA= \ + KillSignal= RestartKillSignal= FinalKillSignal= LimitCPU= LimitFSIZE= LimitDATA= \ LimitSTACK= LimitCORE= LimitRSS= LimitNOFILE= LimitAS= LimitNPROC= \ LimitMEMLOCK= LimitLOCKS= LimitSIGPENDING= LimitMSGQUEUE= \ LimitNICE= LimitRTPRIO= LimitRTTIME= PrivateTmp= PrivateDevices= \ diff --git a/src/core/dbus-kill.c b/src/core/dbus-kill.c index e2b3a0d517..30597e86f0 100644 --- a/src/core/dbus-kill.c +++ b/src/core/dbus-kill.c @@ -8,10 +8,28 @@ static BUS_DEFINE_PROPERTY_GET_ENUM(property_get_kill_mode, kill_mode, KillMode); +static int property_get_restart_kill_signal( + sd_bus *bus, + const char *path, + const char *interface, + const char *property, + sd_bus_message *reply, + void *userdata, + sd_bus_error *error) { + KillContext *c = userdata; + int s; + + assert(c); + + s = restart_kill_signal(c); + return sd_bus_message_append_basic(reply, 'i', &s); +} + const sd_bus_vtable bus_kill_vtable[] = { SD_BUS_VTABLE_START(0), SD_BUS_PROPERTY("KillMode", "s", property_get_kill_mode, offsetof(KillContext, kill_mode), SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("KillSignal", "i", bus_property_get_int, offsetof(KillContext, kill_signal), SD_BUS_VTABLE_PROPERTY_CONST), + SD_BUS_PROPERTY("RestartKillSignal", "i", property_get_restart_kill_signal, 0, SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("FinalKillSignal", "i", bus_property_get_int, offsetof(KillContext, final_kill_signal), SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("SendSIGKILL", "b", bus_property_get_bool, offsetof(KillContext, send_sigkill), SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("SendSIGHUP", "b", bus_property_get_bool, offsetof(KillContext, send_sighup), SD_BUS_VTABLE_PROPERTY_CONST), @@ -21,6 +39,7 @@ const sd_bus_vtable bus_kill_vtable[] = { static BUS_DEFINE_SET_TRANSIENT_PARSE(kill_mode, KillMode, kill_mode_from_string); static BUS_DEFINE_SET_TRANSIENT_TO_STRING(kill_signal, "i", int32_t, int, "%" PRIi32, signal_to_string_with_check); +static BUS_DEFINE_SET_TRANSIENT_TO_STRING(restart_kill_signal, "i", int32_t, int, "%" PRIi32, signal_to_string_with_check); static BUS_DEFINE_SET_TRANSIENT_TO_STRING(final_kill_signal, "i", int32_t, int, "%" PRIi32, signal_to_string_with_check); static BUS_DEFINE_SET_TRANSIENT_TO_STRING(watchdog_signal, "i", int32_t, int, "%" PRIi32, signal_to_string_with_check); @@ -51,6 +70,9 @@ int bus_kill_context_set_transient_property( if (streq(name, "KillSignal")) return bus_set_transient_kill_signal(u, name, &c->kill_signal, message, flags, error); + if (streq(name, "RestartKillSignal")) + return bus_set_transient_restart_kill_signal(u, name, &c->restart_kill_signal, message, flags, error); + if (streq(name, "FinalKillSignal")) return bus_set_transient_final_kill_signal(u, name, &c->final_kill_signal, message, flags, error); diff --git a/src/core/kill.c b/src/core/kill.c index e7608ac81f..a9f468e29e 100644 --- a/src/core/kill.c +++ b/src/core/kill.c @@ -9,6 +9,7 @@ void kill_context_init(KillContext *c) { assert(c); c->kill_signal = SIGTERM; + /* restart_kill_signal is unset by default and we fall back to kill_signal */ c->final_kill_signal = SIGKILL; c->send_sigkill = true; c->send_sighup = false; @@ -23,11 +24,13 @@ void kill_context_dump(KillContext *c, FILE *f, const char *prefix) { fprintf(f, "%sKillMode: %s\n" "%sKillSignal: SIG%s\n" + "%sRestartKillSignal: SIG%s\n" "%sFinalKillSignal: SIG%s\n" "%sSendSIGKILL: %s\n" "%sSendSIGHUP: %s\n", prefix, kill_mode_to_string(c->kill_mode), prefix, signal_to_string(c->kill_signal), + prefix, signal_to_string(restart_kill_signal(c)), prefix, signal_to_string(c->final_kill_signal), prefix, yes_no(c->send_sigkill), prefix, yes_no(c->send_sighup)); diff --git a/src/core/kill.h b/src/core/kill.h index 43648b6d8a..1deb0aff9b 100644 --- a/src/core/kill.h +++ b/src/core/kill.h @@ -21,6 +21,7 @@ typedef enum KillMode { struct KillContext { KillMode kill_mode; int kill_signal; + int restart_kill_signal; int final_kill_signal; int watchdog_signal; bool send_sigkill; @@ -47,3 +48,9 @@ KillMode kill_mode_from_string(const char *s) _pure_; const char *kill_who_to_string(KillWho k) _const_; KillWho kill_who_from_string(const char *s) _pure_; + +static inline int restart_kill_signal(const KillContext *c) { + if (c->restart_kill_signal != 0) + return c->restart_kill_signal; + return c->kill_signal; +} diff --git a/src/core/load-fragment-gperf.gperf.m4 b/src/core/load-fragment-gperf.gperf.m4 index 62dff99d86..d057c0d18b 100644 --- a/src/core/load-fragment-gperf.gperf.m4 +++ b/src/core/load-fragment-gperf.gperf.m4 @@ -161,6 +161,7 @@ m4_define(`KILL_CONTEXT_CONFIG_ITEMS', $1.SendSIGHUP, config_parse_bool, 0, offsetof($1, kill_context.send_sighup) $1.KillMode, config_parse_kill_mode, 0, offsetof($1, kill_context.kill_mode) $1.KillSignal, config_parse_signal, 0, offsetof($1, kill_context.kill_signal) +$1.RestartKillSignal, config_parse_signal, 0, offsetof($1, kill_context.restart_kill_signal) $1.FinalKillSignal, config_parse_signal, 0, offsetof($1, kill_context.final_kill_signal) $1.WatchdogSignal, config_parse_signal, 0, offsetof($1, kill_context.watchdog_signal)' )m4_dnl diff --git a/src/core/mount.c b/src/core/mount.c index fb3467c350..8a0c8be120 100644 --- a/src/core/mount.c +++ b/src/core/mount.c @@ -866,6 +866,8 @@ static int state_to_kill_operation(MountState state) { switch (state) { case MOUNT_REMOUNTING_SIGTERM: + return KILL_RESTART; + case MOUNT_UNMOUNTING_SIGTERM: return KILL_TERMINATE; diff --git a/src/core/service.c b/src/core/service.c index 8e313f8e90..1c677b4355 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -1838,13 +1838,17 @@ fail: service_enter_signal(s, SERVICE_FINAL_SIGTERM, SERVICE_FAILURE_RESOURCES); } -static int state_to_kill_operation(ServiceState state) { +static int state_to_kill_operation(Service *s, ServiceState state) { switch (state) { case SERVICE_STOP_WATCHDOG: return KILL_WATCHDOG; case SERVICE_STOP_SIGTERM: + if (unit_has_job_type(UNIT(s), JOB_RESTART)) + return KILL_RESTART; + _fallthrough_; + case SERVICE_FINAL_SIGTERM: return KILL_TERMINATE; @@ -1875,7 +1879,7 @@ static void service_enter_signal(Service *s, ServiceState state, ServiceResult f r = unit_kill_context( UNIT(s), &s->kill_context, - state_to_kill_operation(state), + state_to_kill_operation(s, state), s->main_pid, s->control_pid, s->main_pid_alien); diff --git a/src/core/socket.c b/src/core/socket.c index d7ff7d1501..7696490f8a 100644 --- a/src/core/socket.c +++ b/src/core/socket.c @@ -2077,6 +2077,16 @@ fail: socket_enter_signal(s, SOCKET_FINAL_SIGTERM, SOCKET_FAILURE_RESOURCES); } +static int state_to_kill_operation(Socket *s, SocketState state) { + if (state == SOCKET_STOP_PRE_SIGTERM && unit_has_job_type(UNIT(s), JOB_RESTART)) + return KILL_RESTART; + + if (state == SOCKET_FINAL_SIGTERM) + return KILL_TERMINATE; + + return KILL_KILL; +} + static void socket_enter_signal(Socket *s, SocketState state, SocketResult f) { int r; @@ -2088,8 +2098,7 @@ static void socket_enter_signal(Socket *s, SocketState state, SocketResult f) { r = unit_kill_context( UNIT(s), &s->kill_context, - !IN_SET(state, SOCKET_STOP_PRE_SIGTERM, SOCKET_FINAL_SIGTERM) ? - KILL_KILL : KILL_TERMINATE, + state_to_kill_operation(s, state), -1, s->control_pid, false); diff --git a/src/core/swap.c b/src/core/swap.c index 6d0cd51750..0924b35050 100644 --- a/src/core/swap.c +++ b/src/core/swap.c @@ -712,21 +712,32 @@ static void swap_enter_dead_or_active(Swap *s, SwapResult f) { swap_enter_dead(s, f); } +static int state_to_kill_operation(Swap *s, SwapState state) { + if (state == SWAP_DEACTIVATING_SIGTERM) { + if (unit_has_job_type(UNIT(s), JOB_RESTART)) + return KILL_RESTART; + else + return KILL_TERMINATE; + } + + return KILL_KILL; +} + static void swap_enter_signal(Swap *s, SwapState state, SwapResult f) { int r; - KillOperation kop; assert(s); if (s->result == SWAP_SUCCESS) s->result = f; - if (state == SWAP_DEACTIVATING_SIGTERM) - kop = KILL_TERMINATE; - else - kop = KILL_KILL; - r = unit_kill_context(UNIT(s), &s->kill_context, kop, -1, s->control_pid, false); + r = unit_kill_context(UNIT(s), + &s->kill_context, + state_to_kill_operation(s, state), + -1, + s->control_pid, + false); if (r < 0) goto fail; diff --git a/src/core/unit.c b/src/core/unit.c index 71fe7a6e23..494185a046 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -4695,6 +4695,9 @@ static int operation_to_signal(KillContext *c, KillOperation k) { case KILL_TERMINATE_AND_LOG: return c->kill_signal; + case KILL_RESTART: + return restart_kill_signal(c); + case KILL_KILL: return c->final_kill_signal; diff --git a/src/core/unit.h b/src/core/unit.h index 793ee638a6..96f718acdc 100644 --- a/src/core/unit.h +++ b/src/core/unit.h @@ -18,6 +18,7 @@ typedef struct UnitRef UnitRef; typedef enum KillOperation { KILL_TERMINATE, KILL_TERMINATE_AND_LOG, + KILL_RESTART, KILL_KILL, KILL_WATCHDOG, _KILL_OPERATION_MAX, diff --git a/src/shared/bus-unit-util.c b/src/shared/bus-unit-util.c index 2cd5adfd30..0b0772c972 100644 --- a/src/shared/bus-unit-util.c +++ b/src/shared/bus-unit-util.c @@ -1355,15 +1355,12 @@ static int bus_append_execute_property(sd_bus_message *m, const char *field, con static int bus_append_kill_property(sd_bus_message *m, const char *field, const char *eq) { if (streq(field, "KillMode")) - return bus_append_string(m, field, eq); if (STR_IN_SET(field, "SendSIGHUP", "SendSIGKILL")) - return bus_append_parse_boolean(m, field, eq); - if (STR_IN_SET(field, "KillSignal", "FinalKillSignal", "WatchdogSignal")) - + if (STR_IN_SET(field, "KillSignal", "RestartKillSignal", "FinalKillSignal", "WatchdogSignal")) return bus_append_signal_from_string(m, field, eq); return 0; diff --git a/test/TEST-23-TYPE-EXEC/testsuite.sh b/test/TEST-23-TYPE-EXEC/testsuite.sh index b161d97a7c..50d6754b96 100755 --- a/test/TEST-23-TYPE-EXEC/testsuite.sh +++ b/test/TEST-23-TYPE-EXEC/testsuite.sh @@ -19,6 +19,14 @@ systemd-run --unit=four -p Type=exec /bin/sleep infinity ! systemd-run --unit=five -p Type=exec -p User=idontexist /bin/sleep infinity ! systemd-run --unit=six -p Type=exec /tmp/brokenbinary +systemd-run --unit=seven -p KillSignal=SIGTERM -p RestartKillSignal=SIGINT -p Type=exec /bin/sleep infinity +# Both TERM and SIGINT happen to have the same number on all architectures +test $(systemctl show --value -p KillSignal seven.service) -eq 15 +test $(systemctl show --value -p RestartKillSignal seven.service) -eq 2 + +systemctl restart seven.service +systemctl stop seven.service + systemd-analyze log-level info echo OK > /testok diff --git a/test/fuzz/fuzz-unit-file/directives.service b/test/fuzz/fuzz-unit-file/directives.service index fe9d451b41..068f4398b9 100644 --- a/test/fuzz/fuzz-unit-file/directives.service +++ b/test/fuzz/fuzz-unit-file/directives.service @@ -185,6 +185,7 @@ RequiresMountsFor= Requisite= Restart= RestartForceExitStatus= +RestartKillSignal= RestartPreventExitStatus= RestartSec= ReusePort= From b557f1c14760cd6f11a07352f972972b9edf9d62 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 2 Oct 2019 13:53:49 +0200 Subject: [PATCH 05/11] man: document the RestartKillSignal= setting --- man/systemd.kill.xml | 40 ++++++++++++++++++++++------------------ man/systemd.service.xml | 32 +++++++++++++++----------------- 2 files changed, 37 insertions(+), 35 deletions(-) diff --git a/man/systemd.kill.xml b/man/systemd.kill.xml index 2c6fea7493..6a1c67d406 100644 --- a/man/systemd.kill.xml +++ b/man/systemd.kill.xml @@ -85,32 +85,36 @@ unless it is empty. Processes will first be terminated via SIGTERM (unless the signal to send - is changed via KillSignal=). Optionally, this is immediately followed by a - SIGHUP (if enabled with SendSIGHUP=). If processes still - remain after the main process of a unit has exited or the delay configured via the - TimeoutStopSec= has passed, the termination request is repeated with the - SIGKILL signal or the signal specified via FinalKillSignal= - (unless this is disabled via the SendSIGKILL= option). See - kill2 for more - information. + is changed via KillSignal= or RestartKillSignal=). Optionally, + this is immediately followed by a SIGHUP (if enabled with + SendSIGHUP=). If processes still remain after the main process of a unit has + exited or the delay configured via the TimeoutStopSec= has passed, the termination + request is repeated with the SIGKILL signal or the signal specified via + FinalKillSignal= (unless this is disabled via the SendSIGKILL= + option). See kill2 + for more information. Defaults to . KillSignal= - Specifies which signal to use when killing a - service. This controls the signal that is sent as first step - of shutting down a unit (see above), and is usually followed - by SIGKILL (see above and below). For a - list of valid signals, see + Specifies which signal to use when stopping a service. This controls the signal that + is sent as first step of shutting down a unit (see above), and is usually followed by + SIGKILL (see above and below). For a list of valid signals, see signal7. - Defaults to SIGTERM. + Defaults to SIGTERM. - Note that, right after sending the signal specified in - this setting, systemd will always send - SIGCONT, to ensure that even suspended - tasks can be terminated cleanly. + Note that, right after sending the signal specified in this setting, systemd will always send + SIGCONT, to ensure that even suspended tasks can be terminated cleanly. + + + + + RestartKillSignal= + Specifies which signal to use when restarting a service. The same as + KillSignal= described above, with the exception that this setting is used in a + restart job. Not set by default, and the value of KillSignal= is used. diff --git a/man/systemd.service.xml b/man/systemd.service.xml index 14840c2e16..af14eedfcc 100644 --- a/man/systemd.service.xml +++ b/man/systemd.service.xml @@ -477,26 +477,24 @@ ExecStop= - Commands to execute to stop the service - started via ExecStart=. This argument takes - multiple command lines, following the same scheme as described - for ExecStart= above. Use of this setting - is optional. After the commands configured in this option are - run, it is implied that the service is stopped, and any processes - remaining for it are terminated - according to the KillMode= setting (see + Commands to execute to stop the service started via + ExecStart=. This argument takes multiple command lines, following the same scheme + as described for ExecStart= above. Use of this setting is optional. After the + commands configured in this option are run, it is implied that the service is stopped, and any + processes remaining for it are terminated according to the KillMode= setting (see systemd.kill5). - If this option is not specified, the process is terminated by - sending the signal specified in KillSignal= - when service stop is requested. Specifier and environment - variable substitution is supported (including + If this option is not specified, the process is terminated by sending the signal specified in + KillSignal= or RestartKillSignal= when service stop is + requested. Specifier and environment variable substitution is supported (including $MAINPID, see above). - Note that it is usually not sufficient to specify a command for this setting that only asks the service - to terminate (for example, by queuing some form of termination signal for it), but does not wait for it to do - so. Since the remaining processes of the services are killed according to KillMode= and - KillSignal= as described above immediately after the command exited, this may not result in - a clean stop. The specified command should hence be a synchronous operation, not an asynchronous one. + Note that it is usually not sufficient to specify a command for this setting that only asks the + service to terminate (for example, by sending some form of termination signal to it), but does not + wait for it to do so. Since the remaining processes of the services are killed according to + KillMode= and KillSignal= or + RestartKillSignal= as described above immediately after the command exited, this + may not result in a clean stop. The specified command should hence be a synchronous operation, not an + asynchronous one. Note that the commands specified in ExecStop= are only executed when the service started successfully first. They are not invoked if the service was never started at all, or in case its From 4ab1670f3de2debb5fd373ab3320925eccfc417d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 1 Oct 2019 15:53:42 +0200 Subject: [PATCH 06/11] core: rework how logging level is calculated for kill operations Setting the log level based on the signal made sense when signals that were used were fixed. Since we allow signals to be configured, it doesn't make sense to log at notice level about e.g. a restart or stop operation just because the signal used is different. This avoids messages like: six.service: Killing process 210356 (sleep) with signal SIGINT. --- src/core/unit.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/core/unit.c b/src/core/unit.c index 494185a046..0d93b94b81 100644 --- a/src/core/unit.c +++ b/src/core/unit.c @@ -4686,22 +4686,26 @@ static int log_kill(pid_t pid, int sig, void *userdata) { return 1; } -static int operation_to_signal(KillContext *c, KillOperation k) { +static int operation_to_signal(const KillContext *c, KillOperation k, bool *noteworthy) { assert(c); switch (k) { case KILL_TERMINATE: case KILL_TERMINATE_AND_LOG: + *noteworthy = false; return c->kill_signal; case KILL_RESTART: + *noteworthy = false; return restart_kill_signal(c); case KILL_KILL: + *noteworthy = true; return c->final_kill_signal; case KILL_WATCHDOG: + *noteworthy = true; return c->watchdog_signal; default: @@ -4730,16 +4734,16 @@ int unit_kill_context( if (c->kill_mode == KILL_NONE) return 0; - sig = operation_to_signal(c, k); + bool noteworthy; + sig = operation_to_signal(c, k, ¬eworthy); + if (noteworthy) + log_func = log_kill; send_sighup = c->send_sighup && IN_SET(k, KILL_TERMINATE, KILL_TERMINATE_AND_LOG) && sig != SIGHUP; - if (k != KILL_TERMINATE || IN_SET(sig, SIGKILL, SIGABRT)) - log_func = log_kill; - if (main_pid > 0) { if (log_func) log_func(main_pid, sig, u); From ab76be55819268968c0a987bf187db9568e09dbd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 1 Oct 2019 16:23:19 +0200 Subject: [PATCH 07/11] networkd: use SIGUSR2 to do a restart The code supports SIGTERM and SIGINT to termiante the process. It would be possible to reporpose one of those signals for the restart operation, but I think it's better to use a completely different signal to avoid misunderstandings. --- src/network/networkd-manager.c | 30 ++++++++++++++++++++++++++++-- src/network/networkd-manager.h | 1 + units/systemd-networkd.service.in | 1 + 3 files changed, 30 insertions(+), 2 deletions(-) diff --git a/src/network/networkd-manager.c b/src/network/networkd-manager.c index e0c095d022..546bb2375c 100644 --- a/src/network/networkd-manager.c +++ b/src/network/networkd-manager.c @@ -30,6 +30,7 @@ #include "ordered-set.h" #include "path-util.h" #include "set.h" +#include "signal-util.h" #include "strv.h" #include "sysctl-util.h" #include "tmpfile-util.h" @@ -1561,6 +1562,28 @@ static int manager_dirty_handler(sd_event_source *s, void *userdata) { return 1; } +static int signal_terminate_callback(sd_event_source *s, const struct signalfd_siginfo *si, void *userdata) { + Manager *m = userdata; + + assert(m); + m->restarting = false; + + log_debug("Terminate operation initiated."); + + return sd_event_exit(sd_event_source_get_event(s), 0); +} + +static int signal_restart_callback(sd_event_source *s, const struct signalfd_siginfo *si, void *userdata) { + Manager *m = userdata; + + assert(m); + m->restarting = true; + + log_debug("Restart operation initiated."); + + return sd_event_exit(sd_event_source_get_event(s), 0); +} + int manager_new(Manager **ret) { _cleanup_(manager_freep) Manager *m = NULL; int r; @@ -1581,9 +1604,12 @@ int manager_new(Manager **ret) { if (r < 0) return r; + assert_se(sigprocmask_many(SIG_SETMASK, NULL, SIGINT, SIGTERM, SIGUSR2, -1) >= 0); + (void) sd_event_set_watchdog(m->event, true); - (void) sd_event_add_signal(m->event, NULL, SIGTERM, NULL, NULL); - (void) sd_event_add_signal(m->event, NULL, SIGINT, NULL, NULL); + (void) sd_event_add_signal(m->event, NULL, SIGTERM, signal_terminate_callback, m); + (void) sd_event_add_signal(m->event, NULL, SIGINT, signal_terminate_callback, m); + (void) sd_event_add_signal(m->event, NULL, SIGUSR2, signal_restart_callback, m); r = sd_event_add_post(m->event, NULL, manager_dirty_handler, m); if (r < 0) diff --git a/src/network/networkd-manager.h b/src/network/networkd-manager.h index f5166dad12..7b5557f5bf 100644 --- a/src/network/networkd-manager.h +++ b/src/network/networkd-manager.h @@ -29,6 +29,7 @@ struct Manager { bool enumerating:1; bool dirty:1; + bool restarting:1; Set *dirty_links; diff --git a/units/systemd-networkd.service.in b/units/systemd-networkd.service.in index 9ea3bb914e..5c6275e5b3 100644 --- a/units/systemd-networkd.service.in +++ b/units/systemd-networkd.service.in @@ -42,6 +42,7 @@ SystemCallArchitectures=native SystemCallErrorNumber=EPERM SystemCallFilter=@system-service Type=notify +RestartKillSignal=SIGUSR2 User=systemd-network WatchdogSec=3min From 800603524a7ab076d40450286ff7802a04289b7f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 1 Oct 2019 17:12:31 +0200 Subject: [PATCH 08/11] networkd: drop DHCPv4 config on stop, keep on restart, by default This partially reverts 95355a281c06c5970b7355c38b066910c3be4958. It seems that other parts of link_stop_clients() should be skipped when restarting, but I don't know enough about those other clients to have an opinion if it is better to stop&start them on restart or not. Anyway, that can be done in later patches now that the support for restarts is there. Fixes #13625. --- man/systemd.network.xml | 4 ++-- src/network/networkd-link.c | 8 ++++++-- src/network/networkd-network.c | 6 ++---- 3 files changed, 10 insertions(+), 8 deletions(-) diff --git a/man/systemd.network.xml b/man/systemd.network.xml index e5f9d6f470..cd86a4f4be 100644 --- a/man/systemd.network.xml +++ b/man/systemd.network.xml @@ -876,8 +876,8 @@ lease expires. This is contrary to the DHCP specification, but may be the best choice if, e.g., the root filesystem relies on this connection. The setting dhcp implies dhcp-on-stop, and yes implies - dhcp and static. Defaults to - dhcp-on-stop. + dhcp and static. Defaults to no. + diff --git a/src/network/networkd-link.c b/src/network/networkd-link.c index 4c0daa4802..a23bddde9b 100644 --- a/src/network/networkd-link.c +++ b/src/network/networkd-link.c @@ -770,8 +770,12 @@ int link_stop_clients(Link *link, bool may_keep_dhcp) { dhcp4_release_old_lease(link); - if (link->dhcp_client && (!may_keep_dhcp || !link->network || - !FLAGS_SET(link->network->keep_configuration, KEEP_CONFIGURATION_DHCP_ON_STOP))) { + bool keep_dhcp = may_keep_dhcp && + link->network && + (link->manager->restarting || + FLAGS_SET(link->network->keep_configuration, KEEP_CONFIGURATION_DHCP_ON_STOP)); + + if (link->dhcp_client && !keep_dhcp) { k = sd_dhcp_client_stop(link->dhcp_client); if (k < 0) r = log_link_warning_errno(link, k, "Could not stop DHCPv4 client: %m"); diff --git a/src/network/networkd-network.c b/src/network/networkd-network.c index 23a21d8c9e..a1f285b8e5 100644 --- a/src/network/networkd-network.c +++ b/src/network/networkd-network.c @@ -268,13 +268,11 @@ int network_verify(Network *network) { /* CriticalConnection=yes also preserve foreign static configurations. */ network->keep_configuration = KEEP_CONFIGURATION_YES; else - /* For backward compatibility, we do not release DHCP addresses on manager stop. */ - network->keep_configuration = KEEP_CONFIGURATION_DHCP_ON_STOP; + network->keep_configuration = KEEP_CONFIGURATION_NO; } if (network->keep_configuration < 0) - /* For backward compatibility, we do not release DHCP addresses on manager stop. */ - network->keep_configuration = KEEP_CONFIGURATION_DHCP_ON_STOP; + network->keep_configuration = KEEP_CONFIGURATION_NO; LIST_FOREACH_SAFE(addresses, address, address_next, network->static_addresses) if (address_section_verify(address) < 0) From 5f3b5f190fe5b106d18924e14a5148ed1519f6d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 1 Oct 2019 17:25:31 +0200 Subject: [PATCH 09/11] networkd: change SendRelease default to true Now that we don't drop DHCP config on restart, this seems appropriate. --- man/systemd.network.xml | 2 +- src/network/networkd-network.c | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/man/systemd.network.xml b/man/systemd.network.xml index cd86a4f4be..aa53eb0c0c 100644 --- a/man/systemd.network.xml +++ b/man/systemd.network.xml @@ -1568,7 +1568,7 @@ SendRelease= When true, the DHCPv4 client sends a DHCP release packet when it stops. - Defaults to false. + Defaults to true. diff --git a/src/network/networkd-network.c b/src/network/networkd-network.c index a1f285b8e5..3ea76a034a 100644 --- a/src/network/networkd-network.c +++ b/src/network/networkd-network.c @@ -371,6 +371,7 @@ int network_load_one(Manager *manager, const char *filename) { .dhcp_use_routes = true, /* NOTE: this var might be overwritten by network_apply_anonymize_if_set */ .dhcp_send_hostname = true, + .dhcp_send_release = true, /* To enable/disable RFC7844 Anonymity Profiles */ .dhcp_anonymize = false, .dhcp_route_metric = DHCP_ROUTE_METRIC, From 8ff4585ffa870bce1a02dbf53cfcae7f559c3e83 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 2 Oct 2019 10:22:49 +0200 Subject: [PATCH 10/11] sd-dhcp-client: merge client_send_release() into sd_dhcp_client_send_release() The public function and the implementation were split into two for no particular reason. We would assert() on the internal state of the client. This should not be done in a function that is directly called from a public function. (I.e., we should not crash if the public function is called at the wrong time.) assert() is changed to assert_return(). And before anyone asks: I put the assert_returns() *above* the internal variables on purpose. This makes it easier to see that the assert_returns() are about the state that is passed in, and if they are not satisfied, the function returns immediately. The compiler doesn't care either way, so the ordering that is clearest to the reader should be chosen. --- src/libsystemd-network/sd-dhcp-client.c | 63 +++++++++++-------------- 1 file changed, 27 insertions(+), 36 deletions(-) diff --git a/src/libsystemd-network/sd-dhcp-client.c b/src/libsystemd-network/sd-dhcp-client.c index 2d511f7feb..3f386db2f5 100644 --- a/src/libsystemd-network/sd-dhcp-client.c +++ b/src/libsystemd-network/sd-dhcp-client.c @@ -870,41 +870,6 @@ static int client_send_discover(sd_dhcp_client *client) { return 0; } -static int client_send_release(sd_dhcp_client *client) { - _cleanup_free_ DHCPPacket *release = NULL; - size_t optoffset, optlen; - int r; - - assert(client); - assert(!IN_SET(client->state, DHCP_STATE_STOPPED)); - - r = client_message_init(client, &release, DHCP_RELEASE, - &optlen, &optoffset); - if (r < 0) - return r; - - /* Fill up release IP and MAC */ - release->dhcp.ciaddr = client->lease->address; - memcpy(&release->dhcp.chaddr, &client->mac_addr, client->mac_addr_len); - - r = dhcp_option_append(&release->dhcp, optlen, &optoffset, 0, - SD_DHCP_OPTION_END, 0, NULL); - if (r < 0) - return r; - - r = dhcp_network_send_udp_socket(client->fd, - client->lease->server_address, - DHCP_PORT_SERVER, - &release->dhcp, - sizeof(DHCPMessage) + optoffset); - if (r < 0) - return r; - - log_dhcp_client(client, "RELEASE"); - - return 0; -} - static int client_send_request(sd_dhcp_client *client) { _cleanup_free_ DHCPPacket *request = NULL; size_t optoffset, optlen; @@ -1929,8 +1894,34 @@ int sd_dhcp_client_start(sd_dhcp_client *client) { int sd_dhcp_client_send_release(sd_dhcp_client *client) { assert_return(client, -EINVAL); + assert_return(client->state != DHCP_STATE_STOPPED, -ESTALE); - client_send_release(client); + _cleanup_free_ DHCPPacket *release = NULL; + size_t optoffset, optlen; + int r; + + r = client_message_init(client, &release, DHCP_RELEASE, &optlen, &optoffset); + if (r < 0) + return r; + + /* Fill up release IP and MAC */ + release->dhcp.ciaddr = client->lease->address; + memcpy(&release->dhcp.chaddr, &client->mac_addr, client->mac_addr_len); + + r = dhcp_option_append(&release->dhcp, optlen, &optoffset, 0, + SD_DHCP_OPTION_END, 0, NULL); + if (r < 0) + return r; + + r = dhcp_network_send_udp_socket(client->fd, + client->lease->server_address, + DHCP_PORT_SERVER, + &release->dhcp, + sizeof(DHCPMessage) + optoffset); + if (r < 0) + return r; + + log_dhcp_client(client, "RELEASE"); return 0; } From 8bea7e700f5e73d9a92583f71647c9788defb843 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 2 Oct 2019 10:29:41 +0200 Subject: [PATCH 11/11] sd-dhcp-client: do not crash if sd_dhcp_client_send_release() is called with no lease Again, a public function should not crash if called at an inopportune moment. Also, make sure we don't call the function if we have no lease. --- src/libsystemd-network/sd-dhcp-client.c | 1 + src/network/networkd-dhcp4.c | 6 +++--- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/libsystemd-network/sd-dhcp-client.c b/src/libsystemd-network/sd-dhcp-client.c index 3f386db2f5..02f3569edc 100644 --- a/src/libsystemd-network/sd-dhcp-client.c +++ b/src/libsystemd-network/sd-dhcp-client.c @@ -1895,6 +1895,7 @@ int sd_dhcp_client_start(sd_dhcp_client *client) { int sd_dhcp_client_send_release(sd_dhcp_client *client) { assert_return(client, -EINVAL); assert_return(client->state != DHCP_STATE_STOPPED, -ESTALE); + assert_return(client->lease, -EUNATCH); _cleanup_free_ DHCPPacket *release = NULL; size_t optoffset, optlen; diff --git a/src/network/networkd-dhcp4.c b/src/network/networkd-dhcp4.c index 78cd241140..db762163a8 100644 --- a/src/network/networkd-dhcp4.c +++ b/src/network/networkd-dhcp4.c @@ -864,10 +864,10 @@ static int dhcp4_handler(sd_dhcp_client *client, int event, void *userdata) { return 0; } - if (link->network->dhcp_send_release) - (void) sd_dhcp_client_send_release(client); - if (link->dhcp_lease) { + if (link->network->dhcp_send_release) + (void) sd_dhcp_client_send_release(client); + r = dhcp_lease_lost(link); if (r < 0) { link_enter_failed(link);