From 0bdb94bfbb204b95d75d759a5104fdd507d889c4 Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Tue, 15 Sep 2026 17:38:29 +0100 Subject: [PATCH] Bug 40901: (QA follow-up) Harden the service abstraction in koha-functions.sh - koha_init_backend relied on "systemctl list-unit-files" exiting non-zero when nothing matches. That behaviour is version dependent, and on a systemd host running koha-sysv a false positive would send every koha-* helper to units that do not exist. Test for the koha-plack@.service unit file instead: unambiguous, and no systemctl fork per call. The _KOHA_INIT_BACKEND cache is dropped; every caller ran the function in a $(...) subshell so it never took effect anyway. - The _sysv_worker and _sysv_z3950 code moved here from bash-only scripts brought "((error_count++))" and "[[ ! $VAR ]]" into a #!/bin/sh library that koha-disable, koha-enable, koha-remove, koha-list and the init script source under dash. Use POSIX equivalents. - _sysv_restart_zebra and _sysv_restart_sip lost the explicit "return 0" of the original quiet not-running branch, so a start failure now aborted the caller's whole instance loop under set -e (including the init script's restart across all instances). Restore it. - systemctl status is run with --no-pager from koha_service_ctl and koha-systemd-ctl so the helpers never block on a pager. - koha-plack --enable/--disable ran systemctl before editing the Apache configuration, so a systemctl failure under set -e left the vhost untouched. Do the systemd half after the Apache edits and warn instead of aborting. Co-Authored-By: Claude Fable 5.1 Signed-off-by: Martin Renvoize Signed-off-by: Pedro Amorim --- debian/scripts/koha-functions.sh | 46 +++++++++++++++++++------------- debian/scripts/koha-plack | 23 ++++++++++------ debian/systemd/koha-systemd-ctl | 5 +++- 3 files changed, 46 insertions(+), 28 deletions(-) diff --git a/debian/scripts/koha-functions.sh b/debian/scripts/koha-functions.sh index 27319ca9dd..66bae8d247 100755 --- a/debian/scripts/koha-functions.sh +++ b/debian/scripts/koha-functions.sh @@ -170,19 +170,21 @@ is_sitemap_enabled() # --- Service management abstraction --- # Systemd is the primary path. Legacy daemon/start-stop-daemon is the fallback. -_KOHA_INIT_BACKEND="" - +# Prints "systemd" when systemd is PID 1 *and* the koha-systemd package's +# units are installed, "sysv" otherwise. A systemd host running koha-sysv +# must take the SysV path, so the presence of the unit files is what +# decides, not the init system alone. Callers use $(koha_init_backend), so +# keep this cheap: plain file tests, no systemctl round trip. koha_init_backend() { - if [ -z "$_KOHA_INIT_BACKEND" ]; then - if [ -d /run/systemd/system ] && \ - systemctl list-unit-files koha-plack@.service >/dev/null 2>&1; then - _KOHA_INIT_BACKEND="systemd" - else - _KOHA_INIT_BACKEND="sysv" - fi + if [ -d /run/systemd/system ] && \ + { [ -f /lib/systemd/system/koha-plack@.service ] || \ + [ -f /usr/lib/systemd/system/koha-plack@.service ] || \ + [ -f /etc/systemd/system/koha-plack@.service ]; }; then + echo "systemd" + else + echo "sysv" fi - echo "$_KOHA_INIT_BACKEND" } # Map service + queue to systemd unit name @@ -210,7 +212,7 @@ koha_service_ctl() reload) systemctl reload-or-restart "$unit" ;; status) - systemctl status "$unit" ;; + systemctl --no-pager status "$unit" ;; esac } @@ -787,8 +789,11 @@ _sysv_restart_zebra() else if [ "$verbose" != "no" ]; then log_warning_msg "Zebra not running for ${name}." + _sysv_start_zebra ${name} + else + _sysv_start_zebra ${name} + return 0 fi - _sysv_start_zebra ${name} fi } @@ -838,11 +843,11 @@ _sysv_start_worker() echo "Starting Koha worker daemon for ${name} (${queue})" if ! daemon $DAEMONOPTS -- "$worker_DAEMON" --queue "$queue"; then - ((error_count++)) + error_count=$((error_count+1)) fi else echo "Error: worker already running for ${name} (${queue})" - ((error_count++)) + error_count=$((error_count+1)) fi done log_end_msg $error_count @@ -868,11 +873,11 @@ _sysv_stop_worker() echo "Stopping Koha worker daemon for ${name} (${queue})" if ! daemon $DAEMONOPTS --stop -- "$worker_DAEMON" --queue "$queue"; then - ((error_count++)) + error_count=$((error_count+1)) fi else echo "Error: worker not running for ${name} (${queue})" - ((error_count++)) + error_count=$((error_count+1)) fi done log_end_msg $error_count @@ -898,7 +903,7 @@ _sysv_restart_worker() echo "Restarting Koha worker daemon for ${name} (${queue})" if ! daemon $DAEMONOPTS --restart -- "$worker_DAEMON" --queue "$queue"; then - ((error_count++)) + error_count=$((error_count+1)) fi else echo "Worker not running for ${name} (${queue})." @@ -1199,8 +1204,11 @@ _sysv_restart_sip() else if [ "$verbose" != "no" ]; then log_warning_msg "Warning: SIP server not running for ${name}." + _sysv_start_sip ${name} + else + _sysv_start_sip ${name} + return 0 fi - _sysv_start_sip ${name} fi } @@ -1264,7 +1272,7 @@ _sysv_start_z3950() if ! is_z3950_running ${instancename}; then export KOHA_CONF="/etc/koha/sites/${instancename}/koha-conf.xml" - if [[ ! $Z3950_ADDITIONAL_OPTS ]]; then + if [ -z "$Z3950_ADDITIONAL_OPTS" ]; then Z3950_ADDITIONAL_OPTS="$( xmlstarlet sel -t -v 'yazgfs/config/z3950_responder_options' "$CONFIGDIR/config.xml" || true )" fi diff --git a/debian/scripts/koha-plack b/debian/scripts/koha-plack index f9092d7b04..27b68032b5 100755 --- a/debian/scripts/koha-plack +++ b/debian/scripts/koha-plack @@ -124,10 +124,6 @@ enable_plack() local instancename=$1 local instancefile=$(get_apache_config_for "$instancename") - if [ "$(koha_init_backend)" = "systemd" ]; then - koha_service_ctl enable plack "$instancename" - fi - alreadyopac=0 alreadyintra=0 failopac=0 @@ -159,6 +155,14 @@ enable_plack() alreadyintra=1 fi + # Under systemd the unit has to be enabled as well, whatever the Apache + # side looked like. Done after the config edits so a systemctl failure + # cannot leave them half-applied under set -e. + if [ "$(koha_init_backend)" = "systemd" ]; then + koha_service_ctl enable plack "$instancename" || \ + warn "Could not enable koha-plack@${instancename}.service" + fi + # Fail if it was already plack enabled. if [ $alreadyopac -eq 1 ] && [ $alreadyintra -eq 1 ] ; then return 1 @@ -179,10 +183,6 @@ disable_plack() local instancename=$1 local instancefile=$(get_apache_config_for "$instancename") - if [ "$(koha_init_backend)" = "systemd" ]; then - koha_service_ctl disable plack "$instancename" - fi - alreadyopac=0 alreadyintra=0 failopac=0 @@ -214,6 +214,13 @@ disable_plack() alreadyintra=1 fi + # Under systemd the unit has to be disabled as well; done after the + # Apache edits for the same reason as in enable_plack. + if [ "$(koha_init_backend)" = "systemd" ]; then + koha_service_ctl disable plack "$instancename" || \ + warn "Could not disable koha-plack@${instancename}.service" + fi + # Fail if it was already plack disabled. if [ $alreadyopac -eq 1 ] && [ $alreadyintra -eq 1 ] ; then return 1 diff --git a/debian/systemd/koha-systemd-ctl b/debian/systemd/koha-systemd-ctl index b75c3a4f4c..a9bca50e55 100755 --- a/debian/systemd/koha-systemd-ctl +++ b/debian/systemd/koha-systemd-ctl @@ -60,7 +60,10 @@ else fi case "$CMD" in - enable|disable|start|stop|restart|status) + status) + systemctl --no-pager "$CMD" "$UNIT" + ;; + enable|disable|start|stop|restart) systemctl "$CMD" "$UNIT" ;; *)