From fb98c75e0ea1c38dd8f47833e6dbd766f5d950ec Mon Sep 17 00:00:00 2001 From: =?utf8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Fri, 4 Jul 2025 19:32:51 +0200 Subject: [PATCH] shared/bus-unit-util: tweak bus_append_exec_command to use Ex prop only if necessary This changes little in behaviour, the conceptual part is more important. The non-Ex variant is the actual name on the command line, and we should use the non-Ex D-Bus property too, if it works. This increases compatibility with old versions. But the code was mostly doing the right thing. Even the tests tested the right thing. Follow-up for b3d593673c5b8b0b7d781fd26ab2062ca6e7dbdb and 898fc00e794d714e2f01409bef440d910c22502a. The test is simplified by taking advantage of the fact that both names on the commandline are supposed to behave identically. Partially resolves https://github.com/systemd/systemd/issues/37174. --- src/shared/bus-unit-util.c | 45 +++++++++---------- .../TEST-23-UNIT-FILE.exec-command-ex.sh | 21 +++------ 2 files changed, 27 insertions(+), 39 deletions(-) diff --git a/src/shared/bus-unit-util.c b/src/shared/bus-unit-util.c index fde4f239bec..48bee05bb3b 100644 --- a/src/shared/bus-unit-util.c +++ b/src/shared/bus-unit-util.c @@ -561,9 +561,8 @@ static int bus_append_socket_filter(sd_bus_message *m, const char *field, const static int bus_append_exec_command(sd_bus_message *m, const char *field, const char *eq) { bool explicit_path = false, done = false, ambient_hack = false; _cleanup_strv_free_ char **cmdline = NULL, **ex_opts = NULL; - _cleanup_free_ char *_path = NULL, *upgraded_name = NULL; + _cleanup_free_ char *_path = NULL; ExecCommandFlags flags = 0; - bool is_ex_prop = endswith(field, "Ex"); int r; do { @@ -638,20 +637,18 @@ static int bus_append_exec_command(sd_bus_message *m, const char *field, const c } } while (!done); - if (!is_ex_prop && (flags & (EXEC_COMMAND_NO_ENV_EXPAND|EXEC_COMMAND_FULLY_PRIVILEGED|EXEC_COMMAND_NO_SETUID|EXEC_COMMAND_VIA_SHELL))) { - /* Upgrade the ExecXYZ= property to ExecXYZEx= for convenience */ - is_ex_prop = true; + bool ex_prop = flags & (EXEC_COMMAND_NO_ENV_EXPAND|EXEC_COMMAND_FULLY_PRIVILEGED|EXEC_COMMAND_NO_SETUID|EXEC_COMMAND_VIA_SHELL); + if (ex_prop) { + /* We need to use ExecXYZEx=. */ + if (!endswith(field, "Ex")) + field = strjoina(field, "Ex"); - upgraded_name = strjoin(field, "Ex"); - if (!upgraded_name) - return log_oom(); - field = upgraded_name; - } - - if (is_ex_prop) { r = exec_command_flags_to_strv(flags, &ex_opts); if (r < 0) return log_error_errno(r, "Failed to serialize ExecCommand flags: %m"); + } else { + if (endswith(field, "Ex")) + field = strndupa_safe(field, strlen(field) - 2); } const char *path = NULL; @@ -686,16 +683,16 @@ static int bus_append_exec_command(sd_bus_message *m, const char *field, const c if (r < 0) return bus_log_create_error(r); - r = sd_bus_message_open_container(m, 'v', is_ex_prop ? "a(sasas)" : "a(sasb)"); + r = sd_bus_message_open_container(m, 'v', ex_prop ? "a(sasas)" : "a(sasb)"); if (r < 0) return bus_log_create_error(r); - r = sd_bus_message_open_container(m, 'a', is_ex_prop ? "(sasas)" : "(sasb)"); + r = sd_bus_message_open_container(m, 'a', ex_prop ? "(sasas)" : "(sasb)"); if (r < 0) return bus_log_create_error(r); if (!strv_isempty(cmdline)) { - r = sd_bus_message_open_container(m, 'r', is_ex_prop ? "sasas" : "sasb"); + r = sd_bus_message_open_container(m, 'r', ex_prop ? "sasas" : "sasb"); if (r < 0) return bus_log_create_error(r); @@ -707,8 +704,8 @@ static int bus_append_exec_command(sd_bus_message *m, const char *field, const c if (r < 0) return bus_log_create_error(r); - r = is_ex_prop ? sd_bus_message_append_strv(m, ex_opts) : - sd_bus_message_append(m, "b", FLAGS_SET(flags, EXEC_COMMAND_IGNORE_FAILURE)); + r = ex_prop ? sd_bus_message_append_strv(m, ex_opts) : + sd_bus_message_append(m, "b", FLAGS_SET(flags, EXEC_COMMAND_IGNORE_FAILURE)); if (r < 0) return bus_log_create_error(r); @@ -2584,19 +2581,19 @@ static const BusProperty service_properties[] = { { "FileDescriptorStoreMax", bus_append_safe_atou }, { "RestartSteps", bus_append_safe_atou }, { "ExecCondition", bus_append_exec_command }, + { "ExecConditionEx", bus_append_exec_command }, /* compat */ { "ExecStartPre", bus_append_exec_command }, + { "ExecStartPreEx", bus_append_exec_command }, /* compat */ { "ExecStart", bus_append_exec_command }, + { "ExecStartEx", bus_append_exec_command }, /* compat */ { "ExecStartPost", bus_append_exec_command }, - { "ExecConditionEx", bus_append_exec_command }, - { "ExecStartPreEx", bus_append_exec_command }, - { "ExecStartEx", bus_append_exec_command }, - { "ExecStartPostEx", bus_append_exec_command }, + { "ExecStartPostEx", bus_append_exec_command }, /* compat */ { "ExecReload", bus_append_exec_command }, + { "ExecReloadEx", bus_append_exec_command }, /* compat */ { "ExecStop", bus_append_exec_command }, + { "ExecStopEx", bus_append_exec_command }, /* compat */ { "ExecStopPost", bus_append_exec_command }, - { "ExecReloadEx", bus_append_exec_command }, - { "ExecStopEx", bus_append_exec_command }, - { "ExecStopPostEx", bus_append_exec_command }, + { "ExecStopPostEx", bus_append_exec_command }, /* compat */ { "RestartPreventExitStatus", bus_append_exit_status }, { "RestartForceExitStatus", bus_append_exit_status }, { "SuccessExitStatus", bus_append_exit_status }, diff --git a/test/units/TEST-23-UNIT-FILE.exec-command-ex.sh b/test/units/TEST-23-UNIT-FILE.exec-command-ex.sh index f926e7dea87..f8dceb1dcce 100755 --- a/test/units/TEST-23-UNIT-FILE.exec-command-ex.sh +++ b/test/units/TEST-23-UNIT-FILE.exec-command-ex.sh @@ -20,25 +20,16 @@ property[7_seven]=ExecStopPost # These should all get upgraded to the corresponding Ex property as the non-Ex variant # does not support the ":" prefix (no-env-expand). for c in "${!property[@]}"; do - systemd-run --unit="$c" -r -p "Type=oneshot" -p "${property[$c]}=:/bin/echo \${$c}" /bin/true + systemd-run --unit="$c" -r -p "Type=oneshot" -p "${property[$c]}=:/bin/echo \${$c}" true systemctl show -p "${property[$c]}" "$c" | grep -F "path=/bin/echo ; argv[]=/bin/echo \${$c} ; ignore_errors=no" systemctl show -p "${property[$c]}Ex" "$c" | grep -F "path=/bin/echo ; argv[]=/bin/echo \${$c} ; flags=no-env-expand" done -declare -A property_ex - -property_ex[1_one_ex]=ExecConditionEx -property_ex[2_two_ex]=ExecStartPreEx -property_ex[3_three_ex]=ExecStartEx -property_ex[4_four_ex]=ExecStartPostEx -property_ex[5_five_ex]=ExecReloadEx -property_ex[6_six_ex]=ExecStopEx -property_ex[7_seven_ex]=ExecStopPostEx - -for c in "${!property_ex[@]}"; do - systemd-run --unit="$c" -r -p "Type=oneshot" -p "${property_ex[$c]}=:/bin/echo \${$c}" /bin/true - systemctl show -p "${property_ex[$c]%??}" "$c" | grep -F "path=/bin/echo ; argv[]=/bin/echo \${$c} ; ignore_errors=no" - systemctl show -p "${property_ex[$c]}" "$c" | grep -F "path=/bin/echo ; argv[]=/bin/echo \${$c} ; flags=no-env-expand" +# Ex names on the commandline are supported for backward compat. +for c in "${!property[@]}"; do + systemd-run --unit="${c}_ex" -r -p "Type=oneshot" -p "${property[$c]}Ex=:/bin/echo \${$c}" true + systemctl show -p "${property[$c]}" "$c" | grep -F "path=/bin/echo ; argv[]=/bin/echo \${$c} ; ignore_errors=no" + systemctl show -p "${property[$c]}Ex" "$c" | grep -F "path=/bin/echo ; argv[]=/bin/echo \${$c} ; flags=no-env-expand" done systemd-analyze log-level info -- 2.47.3