From abaf5edd083bdb8ff8d12ff526f28f9e836b4478 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 30 Jun 2021 14:21:33 +0200 Subject: [PATCH] Revert "Introduce ExitType" This reverts commit cb0e818f7cc2499d81ef143e5acaa00c6e684711. After this was merged, some design and implementation issues were discovered, see the discussion in #18782 and #19385. They certainly can be fixed, but so far nobody has stepped up, and we're nearing a release. Hopefully, this feature can be merged again after a rework. Fixes #19345. --- NEWS | 7 -- docs/TRANSIENT-SETTINGS.md | 1 - man/org.freedesktop.systemd1.xml | 6 -- man/systemd.service.xml | 25 ----- shell-completion/bash/systemd-run | 2 +- shell-completion/zsh/_systemd-run | 2 +- src/core/dbus-service.c | 6 -- src/core/load-fragment-gperf.gperf.in | 1 - src/core/load-fragment.c | 2 - src/core/load-fragment.h | 1 - src/core/service.c | 37 ++----- src/core/service.h | 11 --- src/shared/bus-unit-util.c | 1 - .../xdg-autostart-service.c | 1 - test/TEST-56-EXIT-TYPE/Makefile | 1 - test/TEST-56-EXIT-TYPE/test.sh | 9 -- test/fuzz/fuzz-unit-file/directives.service | 1 - test/units/testsuite-56.service | 6 -- test/units/testsuite-56.sh | 98 ------------------- 19 files changed, 10 insertions(+), 208 deletions(-) delete mode 120000 test/TEST-56-EXIT-TYPE/Makefile delete mode 100755 test/TEST-56-EXIT-TYPE/test.sh delete mode 100644 test/units/testsuite-56.service delete mode 100755 test/units/testsuite-56.sh diff --git a/NEWS b/NEWS index 814e3e47e90..d7ba481fe1b 100644 --- a/NEWS +++ b/NEWS @@ -34,13 +34,6 @@ CHANGES WITH 249 in spe: previously unprovisioned images (i.e. images with a mostly empty /etc/). - * Services gained a new ExitType= setting which can configure how to - determine when a service exited: the default is "main" which defines - the runtime by the service's main process lifetime (this matches the - only behaviour implemented in v248 and before), but with "cgroup" the - runtime is defined by the existence of any process in the service's - cgroup. - * The systemd-machine-id-setup tool now supports a --image= switch for provisioning a machine ID file into an OS disk image, similar to how --root= operates on an OS file tree. This matches the existing switch diff --git a/docs/TRANSIENT-SETTINGS.md b/docs/TRANSIENT-SETTINGS.md index 070ad5a2859..3a75627ca99 100644 --- a/docs/TRANSIENT-SETTINGS.md +++ b/docs/TRANSIENT-SETTINGS.md @@ -304,7 +304,6 @@ Most service unit settings are available for transient units. ✓ ExecStartPre= ✓ ExecStop= ✓ ExecStopPost= -✓ ExitType= ✓ FileDescriptorStoreMax= ✓ GuessMainPID= ✓ NonBlocking= diff --git a/man/org.freedesktop.systemd1.xml b/man/org.freedesktop.systemd1.xml index 8249e31d07a..74a920273af 100644 --- a/man/org.freedesktop.systemd1.xml +++ b/man/org.freedesktop.systemd1.xml @@ -2298,8 +2298,6 @@ node /org/freedesktop/systemd1/unit/avahi_2ddaemon_2eservice { @org.freedesktop.DBus.Property.EmitsChangedSignal("const") readonly s Type = '...'; @org.freedesktop.DBus.Property.EmitsChangedSignal("const") - readonly s ExitType = '...'; - @org.freedesktop.DBus.Property.EmitsChangedSignal("const") readonly s Restart = '...'; @org.freedesktop.DBus.Property.EmitsChangedSignal("const") readonly s PIDFile = '...'; @@ -2866,8 +2864,6 @@ node /org/freedesktop/systemd1/unit/avahi_2ddaemon_2eservice { - - @@ -3386,8 +3382,6 @@ node /org/freedesktop/systemd1/unit/avahi_2ddaemon_2eservice { - - diff --git a/man/systemd.service.xml b/man/systemd.service.xml index 50d1a1d85d6..350bc5f8e5b 100644 --- a/man/systemd.service.xml +++ b/man/systemd.service.xml @@ -255,31 +255,6 @@ - - ExitType= - - - Configures the process exit type for this service unit. One of or - : - - - If set to (the default), the service manager - will consider the unit stopped when the main process, which is determined according to the `Type`, exits. - - - The exit type is meant for applications whose forking model is not - known ahead of time and which might not have a specific main process. The service will stay running as long - as at least one process in the cgroup is running. The exit status of the service is that of the last - process in the cgroup to exit. - - - It is generally recommended to use ExitType= when a service has - a known forking model and a main process can reliably be determined. ExitType= - is well suited for transient or automatically generated services, such as graphical - applications inside of a desktop environment. - - - RemainAfterExit= diff --git a/shell-completion/bash/systemd-run b/shell-completion/bash/systemd-run index c5db8b77bd8..76b9700f79f 100644 --- a/shell-completion/bash/systemd-run +++ b/shell-completion/bash/systemd-run @@ -78,7 +78,7 @@ _systemd_run() { -p|--property) local comps='CPUAccounting= MemoryAccounting= BlockIOAccounting= SendSIGHUP= SendSIGKILL= MemoryLimit= CPUShares= BlockIOWeight= User= Group= - DevicePolicy= KillMode= ExitType= DeviceAllow= BlockIOReadBandwidth= + DevicePolicy= KillMode= DeviceAllow= BlockIOReadBandwidth= BlockIOWriteBandwidth= BlockIODeviceWeight= Nice= Environment= KillSignal= RestartKillSignal= FinalKillSignal= LimitCPU= LimitFSIZE= LimitDATA= LimitSTACK= LimitCORE= LimitRSS= LimitNOFILE= LimitAS= LimitNPROC= diff --git a/shell-completion/zsh/_systemd-run b/shell-completion/zsh/_systemd-run index 322bb60e1ca..cd0ad8245f7 100644 --- a/shell-completion/zsh/_systemd-run +++ b/shell-completion/zsh/_systemd-run @@ -45,7 +45,7 @@ _arguments \ {-p+,--property=}'[Set unit property]:NAME=VALUE:(( \ CPUAccounting= MemoryAccounting= BlockIOAccounting= SendSIGHUP= \ SendSIGKILL= MemoryLimit= CPUShares= BlockIOWeight= User= Group= \ - DevicePolicy= KillMode= ExitType= DeviceAllow= BlockIOReadBandwidth= \ + DevicePolicy= KillMode= DeviceAllow= BlockIOReadBandwidth= \ BlockIOWriteBandwidth= BlockIODeviceWeight= Nice= Environment= \ KillSignal= RestartKillSignal= FinalKillSignal= LimitCPU= LimitFSIZE= LimitDATA= \ LimitSTACK= LimitCORE= LimitRSS= LimitNOFILE= LimitAS= LimitNPROC= \ diff --git a/src/core/dbus-service.c b/src/core/dbus-service.c index 13d555041e5..02628cd39ea 100644 --- a/src/core/dbus-service.c +++ b/src/core/dbus-service.c @@ -27,7 +27,6 @@ #include "unit.h" static BUS_DEFINE_PROPERTY_GET_ENUM(property_get_type, service_type, ServiceType); -static BUS_DEFINE_PROPERTY_GET_ENUM(property_get_exit_type, service_exit_type, ServiceExitType); static BUS_DEFINE_PROPERTY_GET_ENUM(property_get_result, service_result, ServiceResult); static BUS_DEFINE_PROPERTY_GET_ENUM(property_get_restart, service_restart, ServiceRestart); static BUS_DEFINE_PROPERTY_GET_ENUM(property_get_notify_access, notify_access, NotifyAccess); @@ -193,7 +192,6 @@ int bus_service_method_mount_image(sd_bus_message *message, void *userdata, sd_b const sd_bus_vtable bus_service_vtable[] = { SD_BUS_VTABLE_START(0), SD_BUS_PROPERTY("Type", "s", property_get_type, offsetof(Service, type), SD_BUS_VTABLE_PROPERTY_CONST), - SD_BUS_PROPERTY("ExitType", "s", property_get_exit_type, offsetof(Service, exit_type), SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("Restart", "s", property_get_restart, offsetof(Service, restart), SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("PIDFile", "s", NULL, offsetof(Service, pid_file), SD_BUS_VTABLE_PROPERTY_CONST), SD_BUS_PROPERTY("NotifyAccess", "s", property_get_notify_access, offsetof(Service, notify_access), SD_BUS_VTABLE_PROPERTY_CONST), @@ -379,7 +377,6 @@ static int bus_set_transient_std_fd( } static BUS_DEFINE_SET_TRANSIENT_PARSE(notify_access, NotifyAccess, notify_access_from_string); static BUS_DEFINE_SET_TRANSIENT_PARSE(service_type, ServiceType, service_type_from_string); -static BUS_DEFINE_SET_TRANSIENT_PARSE(service_exit_type, ServiceExitType, service_exit_type_from_string); static BUS_DEFINE_SET_TRANSIENT_PARSE(service_restart, ServiceRestart, service_restart_from_string); static BUS_DEFINE_SET_TRANSIENT_PARSE(oom_policy, OOMPolicy, oom_policy_from_string); static BUS_DEFINE_SET_TRANSIENT_STRING_WITH_CHECK(bus_name, sd_bus_service_name_is_valid); @@ -417,9 +414,6 @@ static int bus_service_set_transient_property( if (streq(name, "Type")) return bus_set_transient_service_type(u, name, &s->type, message, flags, error); - if (streq(name, "ExitType")) - return bus_set_transient_service_exit_type(u, name, &s->exit_type, message, flags, error); - if (streq(name, "OOMPolicy")) return bus_set_transient_oom_policy(u, name, &s->oom_policy, message, flags, error); diff --git a/src/core/load-fragment-gperf.gperf.in b/src/core/load-fragment-gperf.gperf.in index d343145fa99..42441eab6ef 100644 --- a/src/core/load-fragment-gperf.gperf.in +++ b/src/core/load-fragment-gperf.gperf.in @@ -383,7 +383,6 @@ Service.StartLimitAction, config_parse_emergency_action, Service.FailureAction, config_parse_emergency_action, 0, offsetof(Unit, failure_action) Service.RebootArgument, config_parse_unit_string_printf, 0, offsetof(Unit, reboot_arg) Service.Type, config_parse_service_type, 0, offsetof(Service, type) -Service.ExitType, config_parse_service_exit_type, 0, offsetof(Service, exit_type) Service.Restart, config_parse_service_restart, 0, offsetof(Service, restart) Service.PermissionsStartOnly, config_parse_bool, 0, offsetof(Service, permissions_start_only) Service.RootDirectoryStartOnly, config_parse_bool, 0, offsetof(Service, root_directory_start_only) diff --git a/src/core/load-fragment.c b/src/core/load-fragment.c index 6bb61431631..8fb3c378ee4 100644 --- a/src/core/load-fragment.c +++ b/src/core/load-fragment.c @@ -135,7 +135,6 @@ DEFINE_CONFIG_PARSE_ENUM(config_parse_protect_home, protect_home, ProtectHome, " DEFINE_CONFIG_PARSE_ENUM(config_parse_protect_system, protect_system, ProtectSystem, "Failed to parse protect system value"); DEFINE_CONFIG_PARSE_ENUM(config_parse_runtime_preserve_mode, exec_preserve_mode, ExecPreserveMode, "Failed to parse runtime directory preserve mode"); DEFINE_CONFIG_PARSE_ENUM(config_parse_service_type, service_type, ServiceType, "Failed to parse service type"); -DEFINE_CONFIG_PARSE_ENUM(config_parse_service_exit_type, service_exit_type, ServiceExitType, "Failed to parse service exit type"); DEFINE_CONFIG_PARSE_ENUM(config_parse_service_restart, service_restart, ServiceRestart, "Failed to parse service restart specifier"); DEFINE_CONFIG_PARSE_ENUM(config_parse_service_timeout_failure_mode, service_timeout_failure_mode, ServiceTimeoutFailureMode, "Failed to parse timeout failure mode"); DEFINE_CONFIG_PARSE_ENUM(config_parse_socket_bind, socket_address_bind_ipv6_only_or_bool, SocketAddressBindIPv6Only, "Failed to parse bind IPv6 only value"); @@ -5845,7 +5844,6 @@ void unit_dump_config_items(FILE *f) { { config_parse_unit_deps, "UNIT [...]" }, { config_parse_exec, "PATH [ARGUMENT [...]]" }, { config_parse_service_type, "SERVICETYPE" }, - { config_parse_service_exit_type, "SERVICEEXITTYPE" }, { config_parse_service_restart, "SERVICERESTART" }, { config_parse_service_timeout_failure_mode, "TIMEOUTMODE" }, { config_parse_kill_mode, "KILLMODE" }, diff --git a/src/core/load-fragment.h b/src/core/load-fragment.h index d722041f962..45e9c397e4e 100644 --- a/src/core/load-fragment.h +++ b/src/core/load-fragment.h @@ -32,7 +32,6 @@ CONFIG_PARSER_PROTOTYPE(config_parse_service_timeout); CONFIG_PARSER_PROTOTYPE(config_parse_service_timeout_abort); CONFIG_PARSER_PROTOTYPE(config_parse_service_timeout_failure_mode); CONFIG_PARSER_PROTOTYPE(config_parse_service_type); -CONFIG_PARSER_PROTOTYPE(config_parse_service_exit_type); CONFIG_PARSER_PROTOTYPE(config_parse_service_restart); CONFIG_PARSER_PROTOTYPE(config_parse_socket_bindtodevice); CONFIG_PARSER_PROTOTYPE(config_parse_exec_output); diff --git a/src/core/service.c b/src/core/service.c index af5a2decf11..c6e667b5de8 100644 --- a/src/core/service.c +++ b/src/core/service.c @@ -1621,25 +1621,18 @@ static int control_pid_good(Service *s) { return s->control_pid > 0; } -static int cgroup_empty(Service *s) { - assert(s); - - /* Returns 0 if there is no cgroup, > 0 if is empty or doesn't exist, < 0 if we can't figure it out */ - - if (!UNIT(s)->cgroup_path) - return 0; - - return cg_is_empty_recursive(SYSTEMD_CGROUP_CONTROLLER, UNIT(s)->cgroup_path); -} - - static int cgroup_good(Service *s) { int r; + assert(s); + /* Returns 0 if the cgroup is empty or doesn't exist, > 0 if it is exists and is populated, < 0 if we can't * figure it out */ - r = cgroup_empty(s); + if (!UNIT(s)->cgroup_path) + return 0; + + r = cg_is_empty_recursive(SYSTEMD_CGROUP_CONTROLLER, UNIT(s)->cgroup_path); if (r < 0) return r; @@ -3402,14 +3395,7 @@ static void service_sigchld_event(Unit *u, pid_t pid, int code, int status) { else assert_not_reached("Unknown code"); - /* Services with ExitType=cgroup ignore the main PID for purposes of exit status */ - if (s->exit_type == SERVICE_EXIT_CGROUP && s->main_pid == pid) { - service_unwatch_main_pid(s); - s->main_pid_known = false; - } - - if ((s->exit_type == SERVICE_EXIT_MAIN && s->main_pid == pid) || - (s->exit_type == SERVICE_EXIT_CGROUP && cgroup_empty(s) && !control_pid_good(s))) { + if (s->main_pid == pid) { /* Forking services may occasionally move to a new PID. * As long as they update the PID file before exiting the old * PID, they're fine. */ @@ -3442,7 +3428,7 @@ static void service_sigchld_event(Unit *u, pid_t pid, int code, int status) { unit_log_process_exit( u, - s->exit_type == SERVICE_EXIT_CGROUP ? "Last process" : "Main process", + "Main process", service_exec_command_to_string(SERVICE_EXEC_START), f == SERVICE_SUCCESS, code, status); @@ -4465,13 +4451,6 @@ 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_CGROUP] = "cgroup", -}; - -DEFINE_STRING_TABLE_LOOKUP(service_exit_type, ServiceExitType); - static const char* const service_exec_command_table[_SERVICE_EXEC_COMMAND_MAX] = { [SERVICE_EXEC_CONDITION] = "ExecCondition", [SERVICE_EXEC_START_PRE] = "ExecStartPre", diff --git a/src/core/service.h b/src/core/service.h index 0d51fc31535..6d931c3d5e4 100644 --- a/src/core/service.h +++ b/src/core/service.h @@ -35,13 +35,6 @@ typedef enum ServiceType { _SERVICE_TYPE_INVALID = -EINVAL, } ServiceType; -typedef enum ServiceExitType { - SERVICE_EXIT_MAIN, /* we consider the main PID when deciding if the service exited */ - SERVICE_EXIT_CGROUP, /* we wait for the last process in the cgroup to exit */ - _SERVICE_EXIT_TYPE_MAX, - _SERVICE_EXIT_TYPE_INVALID = -EINVAL, -} ServiceExitType; - typedef enum ServiceExecCommand { SERVICE_EXEC_CONDITION, SERVICE_EXEC_START_PRE, @@ -104,7 +97,6 @@ struct Service { Unit meta; ServiceType type; - ServiceExitType exit_type; ServiceRestart restart; ExitStatusSet restart_prevent_status; ExitStatusSet restart_force_status; @@ -234,9 +226,6 @@ ServiceRestart service_restart_from_string(const char *s) _pure_; const char* service_type_to_string(ServiceType i) _const_; ServiceType service_type_from_string(const char *s) _pure_; -const char* service_exit_type_to_string(ServiceExitType i) _const_; -ServiceExitType service_exit_type_from_string(const char *s) _pure_; - const char* service_exec_command_to_string(ServiceExecCommand i) _const_; ServiceExecCommand service_exec_command_from_string(const char *s) _pure_; diff --git a/src/shared/bus-unit-util.c b/src/shared/bus-unit-util.c index 31a6c63f0c9..d3a5b25d186 100644 --- a/src/shared/bus-unit-util.c +++ b/src/shared/bus-unit-util.c @@ -1992,7 +1992,6 @@ static int bus_append_service_property(sd_bus_message *m, const char *field, con if (STR_IN_SET(field, "PIDFile", "Type", - "ExitType", "Restart", "BusName", "NotifyAccess", diff --git a/src/xdg-autostart-generator/xdg-autostart-service.c b/src/xdg-autostart-generator/xdg-autostart-service.c index 5068c0ebb39..fe73bfe9db3 100644 --- a/src/xdg-autostart-generator/xdg-autostart-service.c +++ b/src/xdg-autostart-generator/xdg-autostart-service.c @@ -598,7 +598,6 @@ int xdg_autostart_service_generate_unit( fprintf(f, "\n[Service]\n" "Type=exec\n" - "ExitType=cgroup\n" "ExecStart=:%s\n" "Restart=no\n" "TimeoutSec=5s\n" diff --git a/test/TEST-56-EXIT-TYPE/Makefile b/test/TEST-56-EXIT-TYPE/Makefile deleted file mode 120000 index e9f93b1104c..00000000000 --- a/test/TEST-56-EXIT-TYPE/Makefile +++ /dev/null @@ -1 +0,0 @@ -../TEST-01-BASIC/Makefile \ No newline at end of file diff --git a/test/TEST-56-EXIT-TYPE/test.sh b/test/TEST-56-EXIT-TYPE/test.sh deleted file mode 100755 index 0f84dca1ba6..00000000000 --- a/test/TEST-56-EXIT-TYPE/test.sh +++ /dev/null @@ -1,9 +0,0 @@ -#!/usr/bin/env bash -set -e - -TEST_DESCRIPTION="test ExitType=cgroup" - -# shellcheck source=test/test-functions -. "${TEST_BASE_DIR:?}/test-functions" - -do_test "$@" diff --git a/test/fuzz/fuzz-unit-file/directives.service b/test/fuzz/fuzz-unit-file/directives.service index b5df300a6bc..de7d2c7dafe 100644 --- a/test/fuzz/fuzz-unit-file/directives.service +++ b/test/fuzz/fuzz-unit-file/directives.service @@ -160,7 +160,6 @@ ExecStartPost= ExecStartPre= ExecStop= ExecStopPost= -ExitType= ExtensionImages= FailureAction= FileDescriptorStoreMax= diff --git a/test/units/testsuite-56.service b/test/units/testsuite-56.service deleted file mode 100644 index d8ad589ca05..00000000000 --- a/test/units/testsuite-56.service +++ /dev/null @@ -1,6 +0,0 @@ -[Unit] -Description=TEST-56-EXIT-TYPE - -[Service] -ExecStart=/usr/lib/systemd/tests/testdata/units/%N.sh -Type=oneshot diff --git a/test/units/testsuite-56.sh b/test/units/testsuite-56.sh deleted file mode 100755 index 27bd3ca8f73..00000000000 --- a/test/units/testsuite-56.sh +++ /dev/null @@ -1,98 +0,0 @@ -#!/usr/bin/env bash -set -eux - -systemd-analyze log-level debug - -# Multiple level process tree, parent process stays up -cat >/tmp/test56-exit-cgroup.sh < sleep -sleep infinity & -disown - -# process tree: systemd -> bash -> bash -> sleep -((sleep infinity); true) & - -# process tree: systemd -> bash -> sleep -sleep infinity -EOF -chmod +x /tmp/test56-exit-cgroup.sh - -# service should be stopped cleanly -(sleep 1; systemctl stop one) & -systemd-run --wait --unit=one -p ExitType=cgroup /tmp/test56-exit-cgroup.sh - -# same thing with a truthy exec condition -(sleep 1; systemctl stop two) & -systemd-run --wait --unit=two -p ExitType=cgroup -p ExecCondition=true /tmp/test56-exit-cgroup.sh - -# false exec condition: systemd-run should exit immediately with status code: 1 -systemd-run --wait --unit=three -p ExitType=cgroup -p ExecCondition=false /tmp/test56-exit-cgroup.sh \ - && { echo 'unexpected success'; exit 1; } - -# service should exit uncleanly -(sleep 1; systemctl kill --signal 9 four) & -systemd-run --wait --unit=four -p ExitType=cgroup /tmp/test56-exit-cgroup.sh \ - && { echo 'unexpected success'; exit 1; } - - -# Multiple level process tree, parent process exits quickly -cat >/tmp/test56-exit-cgroup-parentless.sh < sleep -sleep infinity & - -# process tree: systemd -> bash -> sleep -((sleep infinity); true) & -EOF -chmod +x /tmp/test56-exit-cgroup-parentless.sh - -# service should be stopped cleanly -(sleep 1; systemctl stop five) & -systemd-run --wait --unit=five -p ExitType=cgroup /tmp/test56-exit-cgroup-parentless.sh - -# service should exit uncleanly -(sleep 1; systemctl kill --signal 9 six) & -systemd-run --wait --unit=six -p ExitType=cgroup /tmp/test56-exit-cgroup-parentless.sh \ - && { echo 'unexpected success'; exit 1; } - - -# Multiple level process tree, parent process exits uncleanly but last process exits cleanly -cat >/tmp/test56-exit-cgroup-clean.sh < bash -> sleep -(sleep 1; true) & - -exit 255 -EOF -chmod +x /tmp/test56-exit-cgroup-clean.sh - -# service should exit cleanly and be garbage-collected -systemd-run --wait --unit=seven -p ExitType=cgroup /tmp/test56-exit-cgroup-clean.sh - - -# Multiple level process tree, parent process exits cleanly but last process exits uncleanly -cat >/tmp/test56-exit-cgroup-unclean.sh < bash -> sleep -(sleep 1; exit 255) & -EOF -chmod +x /tmp/test56-exit-cgroup-unclean.sh - -# service should exit uncleanly after 1 second -systemd-run --wait --unit=eight -p ExitType=cgroup /tmp/test56-exit-cgroup-unclean.sh \ - && { echo 'unexpected success'; exit 1; } - -systemd-analyze log-level info - -echo OK >/testok - -exit 0