From 8ea8e23f4013dbc4f4a66c81eb786f0505434f2e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 28 Jul 2023 17:54:59 +0200 Subject: [PATCH 1/4] manager: fix reloading in reload-or-restart --marked bus_unit_queue_job_one has two callers: - bus_unit_queue_job which would do the appropriate transormations to turn JOB_TRY_RESTART into JOB_TRY_RELOAD, - and method_enqueue_marked_jobs which did not. In effect, method_enqueue_marked_jobs() would queue restart jobs for units which has Markers= needs-reload or needs-restart. When the chunk of code which does the transformations is moved from bus_unit_queue_job to bus_unit_queue_job_one, there is no change for bus_unit_queue_job, and method_enqueue_marked_jobs is fixed. The additional checks that are done seem reasonable to do from method_enqueue_marked_jobs: we shouldn't be restarting units which are configured to not allow that, or force unwanted start of dbus-broker. --- src/core/dbus-unit.c | 82 ++++++++++++++++++++++---------------------- 1 file changed, 41 insertions(+), 41 deletions(-) diff --git a/src/core/dbus-unit.c b/src/core/dbus-unit.c index 629f08ebcc..1f673fe26d 100644 --- a/src/core/dbus-unit.c +++ b/src/core/dbus-unit.c @@ -1770,6 +1770,47 @@ int bus_unit_queue_job_one( Job *j, *a; int r; + if (FLAGS_SET(flags, BUS_UNIT_QUEUE_RELOAD_IF_POSSIBLE) && unit_can_reload(u)) { + if (type == JOB_RESTART) + type = JOB_RELOAD_OR_START; + else if (type == JOB_TRY_RESTART) + type = JOB_TRY_RELOAD; + } + + if (type == JOB_STOP && + IN_SET(u->load_state, UNIT_NOT_FOUND, UNIT_ERROR, UNIT_BAD_SETTING) && + unit_active_state(u) == UNIT_INACTIVE) + return sd_bus_error_setf(error, BUS_ERROR_NO_SUCH_UNIT, "Unit %s not loaded.", u->id); + + if ((type == JOB_START && u->refuse_manual_start) || + (type == JOB_STOP && u->refuse_manual_stop) || + (IN_SET(type, JOB_RESTART, JOB_TRY_RESTART) && (u->refuse_manual_start || u->refuse_manual_stop)) || + (type == JOB_RELOAD_OR_START && job_type_collapse(type, u) == JOB_START && u->refuse_manual_start)) + return sd_bus_error_setf(error, + BUS_ERROR_ONLY_BY_DEPENDENCY, + "Operation refused, unit %s may be requested by dependency only (it is configured to refuse manual start/stop).", + u->id); + + /* dbus-broker issues StartUnit for activation requests, and Type=dbus services automatically + * gain dependency on dbus.socket. Therefore, if dbus has a pending stop job, the new start + * job that pulls in dbus again would cause job type conflict. Let's avoid that by rejecting + * job enqueuing early. + * + * Note that unlike signal_activation_request(), we can't use unit_inactive_or_pending() + * here. StartUnit is a more generic interface, and thus users are allowed to use e.g. systemctl + * to start Type=dbus services even when dbus is inactive. */ + if (type == JOB_START && u->type == UNIT_SERVICE && SERVICE(u)->type == SERVICE_DBUS) + FOREACH_STRING(dbus_unit, SPECIAL_DBUS_SOCKET, SPECIAL_DBUS_SERVICE) { + Unit *dbus; + + dbus = manager_get_unit(u->manager, dbus_unit); + if (dbus && unit_stop_pending(dbus)) + return sd_bus_error_setf(error, + BUS_ERROR_SHUTTING_DOWN, + "Operation for unit %s refused, D-Bus is shutting down.", + u->id); + } + if (FLAGS_SET(flags, BUS_UNIT_QUEUE_VERBOSE_REPLY)) { affected = set_new(NULL); if (!affected) @@ -1862,47 +1903,6 @@ int bus_unit_queue_job( if (r < 0) return r; - if (FLAGS_SET(flags, BUS_UNIT_QUEUE_RELOAD_IF_POSSIBLE) && unit_can_reload(u)) { - if (type == JOB_RESTART) - type = JOB_RELOAD_OR_START; - else if (type == JOB_TRY_RESTART) - type = JOB_TRY_RELOAD; - } - - if (type == JOB_STOP && - IN_SET(u->load_state, UNIT_NOT_FOUND, UNIT_ERROR, UNIT_BAD_SETTING) && - unit_active_state(u) == UNIT_INACTIVE) - return sd_bus_error_setf(error, BUS_ERROR_NO_SUCH_UNIT, "Unit %s not loaded.", u->id); - - if ((type == JOB_START && u->refuse_manual_start) || - (type == JOB_STOP && u->refuse_manual_stop) || - (IN_SET(type, JOB_RESTART, JOB_TRY_RESTART) && (u->refuse_manual_start || u->refuse_manual_stop)) || - (type == JOB_RELOAD_OR_START && job_type_collapse(type, u) == JOB_START && u->refuse_manual_start)) - return sd_bus_error_setf(error, - BUS_ERROR_ONLY_BY_DEPENDENCY, - "Operation refused, unit %s may be requested by dependency only (it is configured to refuse manual start/stop).", - u->id); - - /* dbus-broker issues StartUnit for activation requests, and Type=dbus services automatically - * gain dependency on dbus.socket. Therefore, if dbus has a pending stop job, the new start - * job that pulls in dbus again would cause job type conflict. Let's avoid that by rejecting - * job enqueuing early. - * - * Note that unlike signal_activation_request(), we can't use unit_inactive_or_pending() - * here. StartUnit is a more generic interface, and thus users are allowed to use e.g. systemctl - * to start Type=dbus services even when dbus is inactive. */ - if (type == JOB_START && u->type == UNIT_SERVICE && SERVICE(u)->type == SERVICE_DBUS) - FOREACH_STRING(dbus_unit, SPECIAL_DBUS_SOCKET, SPECIAL_DBUS_SERVICE) { - Unit *dbus; - - dbus = manager_get_unit(u->manager, dbus_unit); - if (dbus && unit_stop_pending(dbus)) - return sd_bus_error_setf(error, - BUS_ERROR_SHUTTING_DOWN, - "Operation for unit %s refused, D-Bus is shutting down.", - u->id); - } - r = sd_bus_message_new_method_return(message, &reply); if (r < 0) return r; From 631d2b05ec5195d1f8f8fbff8a2dfcbf23d0b7aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Wed, 26 Jul 2023 09:02:04 +0200 Subject: [PATCH 2/4] rpm: add %systemd_postun_with_reload and %systemd_user_postun_with_reload For some units, the package would like to issue a reload. The machinery was already in place since c9615f73521986b3607b852c139036d58973043c: systemctl reload-or-restart --marked Enqueues restart jobs for all units that have the 'needs-restart' mark, and reload jobs for units that have the 'needs-reload' mark. When a unit marked for reload does not support reload, restart will be queued. The new macros allow a reload to be issued instead of a restart. Based on the discussion on fedora-devel: https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org/message/IJSUGIEJNYZZRE53FF4YFUEBRHRAVIXR/ Tested using dummy package https://github.com/keszybz/rpm-test-reload. --- src/rpm/macros.systemd.in | 16 ++++++++++++++++ src/rpm/systemd-update-helper.in | 22 ++++++++++++++++++++++ 2 files changed, 38 insertions(+) diff --git a/src/rpm/macros.systemd.in b/src/rpm/macros.systemd.in index c07541c728..f05553f557 100644 --- a/src/rpm/macros.systemd.in +++ b/src/rpm/macros.systemd.in @@ -101,6 +101,22 @@ if [ $1 -ge 1 ] && [ -x "{{SYSTEMD_UPDATE_HELPER_PATH}}" ]; then \ fi \ %{nil} +%systemd_postun_with_reload() \ +%{expand:%%{?__systemd_someargs_%#:%%__systemd_someargs_%# systemd_postun_with_reload}} \ +if [ $1 -ge 1 ] && [ -x "{{SYSTEMD_UPDATE_HELPER_PATH}}" ]; then \ + # Package upgrade, not uninstall \ + {{SYSTEMD_UPDATE_HELPER_PATH}} mark-reload-system-units %{?*} || : \ +fi \ +%{nil} + +%systemd_user_postun_with_reload() \ +%{expand:%%{?__systemd_someargs_%#:%%__systemd_someargs_%# systemd_user_postun_with_reload}} \ +if [ $1 -ge 1 ] && [ -x "{{SYSTEMD_UPDATE_HELPER_PATH}}" ]; then \ + # Package upgrade, not uninstall \ + {{SYSTEMD_UPDATE_HELPER_PATH}} mark-reload-user-units %{?*} || : \ +fi \ +%{nil} + %udev_hwdb_update() %{nil} %udev_rules_update() %{nil} diff --git a/src/rpm/systemd-update-helper.in b/src/rpm/systemd-update-helper.in index c623a5ea17..c81e16c3d3 100755 --- a/src/rpm/systemd-update-helper.in +++ b/src/rpm/systemd-update-helper.in @@ -47,6 +47,15 @@ case "$command" in wait ;; + mark-reload-system-units) + [ -d /run/systemd/system ] || exit 0 + + for unit in "$@"; do + systemctl set-property "$unit" Markers=+needs-reload & + done + wait + ;; + mark-restart-user-units) [ -d /run/systemd/system ] || exit 0 @@ -60,6 +69,19 @@ case "$command" in wait ;; + mark-reload-user-units) + [ -d /run/systemd/system ] || exit 0 + + users=$(systemctl list-units 'user@*' --legend=no | sed -n -r 's/.*user@([0-9]+).service.*/\1/p') + for user in $users; do + for unit in "$@"; do + SYSTEMD_BUS_TIMEOUT={{UPDATE_HELPER_USER_TIMEOUT_SEC}}s \ + systemctl --user -M "$user@" set-property "$unit" Markers=+needs-reload & + done + done + wait + ;; + system-reload-restart|system-reload|system-restart) if [ -n "$*" ]; then echo "Unexpected arguments for '$command': $*" From c6699598e28f812a11712162d9d2648735af7ac4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 28 Jul 2023 19:03:21 +0200 Subject: [PATCH 3/4] rpm: use rpm.execute() in more cases 7bde8293a4f4ccfe4b5c5000869068e3c1ac45c6 converted all the posix.execp() calls, but I do not see why we shouldn't also convert the posix.exec() calls. --- src/rpm/triggers.systemd.in | 56 ++++++------------------------------- 1 file changed, 8 insertions(+), 48 deletions(-) diff --git a/src/rpm/triggers.systemd.in b/src/rpm/triggers.systemd.in index 24108a77bb..d480ab84b6 100644 --- a/src/rpm/triggers.systemd.in +++ b/src/rpm/triggers.systemd.in @@ -13,20 +13,10 @@ -- upgraded. We care about the case where a package is initially -- installed, because other cases are covered by the *un scriptlets, -- so sometimes we will reload needlessly. -pid = posix.fork() -if pid == 0 then - assert(posix.exec("{{SYSTEMD_UPDATE_HELPER_PATH}}", "system-reload-restart")) -elseif pid > 0 then - posix.wait(pid) -end +assert(rpm.execute("{{SYSTEMD_UPDATE_HELPER_PATH}}", "system-reload-restart")) %transfiletriggerin -P 900899 -p -- {{USER_DATA_UNIT_DIR}} /etc/systemd/user -pid = posix.fork() -if pid == 0 then - assert(posix.exec("{{SYSTEMD_UPDATE_HELPER_PATH}}", "user-reload-restart")) -elseif pid > 0 then - posix.wait(pid) -end +assert(rpm.execute("{{SYSTEMD_UPDATE_HELPER_PATH}}", "user-reload-restart")) %transfiletriggerpostun -P 1000100 -p -- {{SYSTEM_DATA_UNIT_DIR}} /etc/systemd/system -- On removal, we need to run daemon-reload after any units have been @@ -34,39 +24,19 @@ end -- On upgrade, we need to run daemon-reload after any new unit files -- have been installed, but before %postun scripts in packages get -- executed. -pid = posix.fork() -if pid == 0 then - assert(posix.exec("{{SYSTEMD_UPDATE_HELPER_PATH}}", "system-reload")) -elseif pid > 0 then - posix.wait(pid) -end +assert(rpm.execute("{{SYSTEMD_UPDATE_HELPER_PATH}}", "system-reload")) %transfiletriggerpostun -P 1000100 -p -- {{SYSTEM_DATA_UNIT_DIR}} /etc/systemd/system -- Execute daemon-reload in user managers. -pid = posix.fork() -if pid == 0 then - assert(posix.exec("{{SYSTEMD_UPDATE_HELPER_PATH}}", "user-reload")) -elseif pid > 0 then - posix.wait(pid) -end +assert(rpm.execute("{{SYSTEMD_UPDATE_HELPER_PATH}}", "user-reload")) %transfiletriggerpostun -P 10000 -p -- {{SYSTEM_DATA_UNIT_DIR}} /etc/systemd/system -- We restart remaining system services that should be restarted here. -pid = posix.fork() -if pid == 0 then - assert(posix.exec("{{SYSTEMD_UPDATE_HELPER_PATH}}", "system-restart")) -elseif pid > 0 then - posix.wait(pid) -end +assert(rpm.execute("{{SYSTEMD_UPDATE_HELPER_PATH}}", "system-restart")) %transfiletriggerpostun -P 9999 -p -- {{USER_DATA_UNIT_DIR}} /etc/systemd/user -- We restart remaining user services that should be restarted here. -pid = posix.fork() -if pid == 0 then - assert(posix.exec("{{SYSTEMD_UPDATE_HELPER_PATH}}", "user-restart")) -elseif pid > 0 then - posix.wait(pid) -end +assert(rpm.execute("{{SYSTEMD_UPDATE_HELPER_PATH}}", "user-restart")) %transfiletriggerin -P 1000700 -p -- {{SYSUSERS_DIR}} -- This script will process files installed in {{SYSUSERS_DIR}} to create @@ -88,12 +58,7 @@ assert(rpm.execute("journalctl", "--update-catalog")) -- This script will automatically apply binfmt rules if files have been -- installed or updated in {{BINFMT_DIR}}. if posix.access("/run/systemd/system") then - pid = posix.fork() - if pid == 0 then - assert(posix.exec("{{LIBEXECDIR}}/systemd-binfmt")) - elseif pid > 0 then - posix.wait(pid) - end + assert(rpm.execute("{{LIBEXECDIR}}/systemd-binfmt")) end %transfiletriggerin -P 1000600 -p -- {{TMPFILES_DIR}} @@ -113,10 +78,5 @@ end -- This script will automatically apply sysctl rules if files have been -- installed or updated in {{SYSCTL_DIR}}. if posix.access("/run/systemd/system") then - pid = posix.fork() - if pid == 0 then - assert(posix.exec("{{LIBEXECDIR}}/systemd-sysctl")) - elseif pid > 0 then - posix.wait(pid) - end + assert(rpm.execute("{{LIBEXECDIR}}/systemd-sysctl")) end From 9ff28e312bffe7567aa5d3f2c41303dd456f1691 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 28 Jul 2023 19:24:58 +0200 Subject: [PATCH 4/4] rpm: add %systemd_user_daemon_reexec This macros wraps the call to daemon-reexec in all user managers. It would be called for example from systemd %post right after the call to systemctl daemon-reexec. This will be used in the Fedora systemd package to fix a long-standing FIXME. Tested via building and reinstalling the systemd package with the patches. --- src/rpm/macros.systemd.in | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/rpm/macros.systemd.in b/src/rpm/macros.systemd.in index f05553f557..241e4b9c49 100644 --- a/src/rpm/macros.systemd.in +++ b/src/rpm/macros.systemd.in @@ -117,6 +117,13 @@ if [ $1 -ge 1 ] && [ -x "{{SYSTEMD_UPDATE_HELPER_PATH}}" ]; then \ fi \ %{nil} +%systemd_user_daemon_reexec() \ +if [ $1 -ge 1 ] && [ -x "{{SYSTEMD_UPDATE_HELPER_PATH}}" ]; then \ + # Package upgrade, not uninstall \ + {{SYSTEMD_UPDATE_HELPER_PATH}} user-reexec || : \ +fi \ +%{nil} + %udev_hwdb_update() %{nil} %udev_rules_update() %{nil}