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.

diff --git a/src/shared/bus-unit-util.c b/src/shared/bus-unit-util.c
index fde4f239bec97b94c48ee102e7625b73df49539d..48bee05bb3bdc91d9383aef6ebe467d34f824915 100644 (file)
--- 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 f926e7dea87351f7212336dc3730b79fad888364..f8dceb1dcce47e630c55d6a1d9df3f49384879ff 100755 (executable)
--- 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