core: tweak error handling around VARLINK_ERROR_UNIT_BAD_SETTING

This commit move the erros in StartTransient from
VARLINK_ERROR_UNIT_BAD_SETTING to SD_VARLINK_ERROR_INVALID_PARAMETER
and it also ensures we have the bad field in the error.

Thanks to Ivan Kruglov for suggesting this.

diff --git a/src/core/varlink-unit.c b/src/core/varlink-unit.c
index da0c0d234a51e23e8cb9a5875a67175dd6ef1696..142de0db8f4207cd3152577c4d32e12db96eabd1 100644 (file)
--- a/src/core/varlink-unit.c
+++ b/src/core/varlink-unit.c
@@ -976,7 +976,12 @@ static int transient_apply_set_credentials(
         return 0;
 }
 
-static int transient_exec_context_apply_properties(Unit *u, ExecContext *c, TransientExecContextParameters *p) {
+static int transient_exec_context_apply_properties(
+                Unit *u,
+                ExecContext *c,
+                TransientExecContextParameters *p,
+                const char **reterr_field) {
+
         int r;
 
         assert(u);
@@ -987,23 +992,35 @@ static int transient_exec_context_apply_properties(Unit *u, ExecContext *c, Tran
                 TransientWorkingDirectory *wd = &p->working_directory;
                 _cleanup_free_ char *simplified = NULL;
 
-                if (wd->home && wd->path)
+                if (wd->home && wd->path) {
+                        if (reterr_field)
+                                *reterr_field = "Exec.WorkingDirectory";
                         return log_debug_errno(SYNTHETIC_ERRNO(EINVAL),
                                                "WorkingDirectory: 'home' and 'path' are mutually exclusive");
-                if (!wd->home && !wd->path)
+                }
+                if (!wd->home && !wd->path) {
+                        if (reterr_field)
+                                *reterr_field = "Exec.WorkingDirectory";
                         return log_debug_errno(SYNTHETIC_ERRNO(EINVAL),
                                                "WorkingDirectory: must specify either 'home' or 'path'");
+                }
 
                 if (!wd->home) {
-                        if (!path_is_absolute(wd->path))
+                        if (!path_is_absolute(wd->path)) {
+                                if (reterr_field)
+                                        *reterr_field = "Exec.WorkingDirectory";
                                 return log_debug_errno(SYNTHETIC_ERRNO(EINVAL),
                                                        "WorkingDirectory: expects an absolute path");
+                        }
                         r = path_simplify_alloc(wd->path, &simplified);
                         if (r < 0)
                                 return r;
-                        if (!path_is_normalized(simplified))
+                        if (!path_is_normalized(simplified)) {
+                                if (reterr_field)
+                                        *reterr_field = "Exec.WorkingDirectory";
                                 return log_debug_errno(SYNTHETIC_ERRNO(EINVAL),
                                                        "WorkingDirectory: expects a normalized path");
+                        }
                 }
 
                 free_and_replace(c->working_directory, simplified);
@@ -1018,22 +1035,29 @@ static int transient_exec_context_apply_properties(Unit *u, ExecContext *c, Tran
 
         if (p->environment_set) {
                 r = exec_context_apply_environment(u, c, p->environment, UNIT_RUNTIME|UNIT_PRIVATE);
-                if (r == -E2BIG)
-                        return log_debug_errno(r, "Too many environment assignments.");
-                if (r == -EINVAL)
-                        return log_debug_errno(r, "Invalid Environment list.");
+                if (IN_SET(r, -E2BIG, -EINVAL)) {
+                        if (reterr_field)
+                                *reterr_field = "Exec.Environment";
+                        /* Convert E2BIG into EINVAL to ensure upper layers get the right error context */
+                        return log_debug_errno(SYNTHETIC_ERRNO(EINVAL),
+                                               r == -E2BIG ? "Too many environment assignments." : "Invalid Environment list.");
+                }
                 if (r < 0)
                         return r;
         }
 
         if (p->set_credentials_set) {
                 r = transient_apply_set_credentials(u, c, p->set_credentials, p->n_set_credentials, /* encrypted= */ false);
+                if (r == -EINVAL && reterr_field)
+                        *reterr_field = "Exec.SetCredential";
                 if (r < 0)
                         return r;
         }
 
         if (p->set_credentials_encrypted_set) {
                 r = transient_apply_set_credentials(u, c, p->set_credentials_encrypted, p->n_set_credentials_encrypted, /* encrypted= */ true);
+                if (r == -EINVAL && reterr_field)
+                        *reterr_field = "Exec.SetCredentialEncrypted";
                 if (r < 0)
                         return r;
         }
@@ -1062,9 +1086,12 @@ static int transient_exec_context_apply_properties(Unit *u, ExecContext *c, Tran
                 _cleanup_free_ char *joined = NULL;
 
                 STRV_FOREACH(g, p->supplementary_groups)
-                        if (!isempty(*g) && !valid_user_group_name(*g, VALID_USER_ALLOW_NUMERIC|VALID_USER_RELAX))
+                        if (!valid_user_group_name(*g, VALID_USER_ALLOW_NUMERIC|VALID_USER_RELAX)) {
+                                if (reterr_field)
+                                        *reterr_field = "Exec.SupplementaryGroups";
                                 return log_debug_errno(SYNTHETIC_ERRNO(EINVAL),
                                                        "Invalid supplementary group name: %s", *g);
+                        }
 
                 r = strv_extend_strv(&c->supplementary_groups, p->supplementary_groups, /* filter_duplicates= */ true);
                 if (r < 0)
@@ -1079,8 +1106,11 @@ static int transient_exec_context_apply_properties(Unit *u, ExecContext *c, Tran
         }
 
         if (p->nice != INT_MAX) {
-                if (!nice_is_valid(p->nice))
+                if (!nice_is_valid(p->nice)) {
+                        if (reterr_field)
+                                *reterr_field = "Exec.Nice";
                         return log_debug_errno(SYNTHETIC_ERRNO(EINVAL), "Invalid Nice= value: %i", p->nice);
+                }
 
                 c->nice = p->nice;
                 c->nice_set = true;
@@ -1091,7 +1121,7 @@ static int transient_exec_context_apply_properties(Unit *u, ExecContext *c, Tran
         return 0;
 }
 
-static int transient_service_apply_properties(Service *s, TransientServiceParameters *sp) {
+static int transient_service_apply_properties(Service *s, TransientServiceParameters *sp, const char **reterr_field) {
         Unit *u = UNIT(ASSERT_PTR(s));
         int r;
 
@@ -1111,8 +1141,11 @@ static int transient_service_apply_properties(Service *s, TransientServiceParame
                 _cleanup_(exec_command_freep) ExecCommand *c = NULL;
                 _cleanup_strv_free_ char **argv = NULL;
 
-                if (!filename_or_absolute_path_is_valid(item->path))
+                if (!filename_or_absolute_path_is_valid(item->path)) {
+                        if (reterr_field)
+                                *reterr_field = "Service.ExecStart";
                         return log_debug_errno(SYNTHETIC_ERRNO(EINVAL), "Invalid ExecStart path: %s", item->path);
+                }
 
                 if (!strv_isempty(item->arguments)) {
                         argv = strv_copy(item->arguments);
@@ -1243,16 +1276,17 @@ int vl_method_start_transient_unit(sd_varlink *link, sd_json_variant *parameters
         /* Apply unit-level properties from context */
         r = transient_unit_apply_properties(u, &p.context);
         if (r == -EINVAL)
-                return sd_varlink_error(link, VARLINK_ERROR_UNIT_BAD_SETTING, NULL);
+                return sd_varlink_error_invalid_parameter_name(link, "context");
         if (r < 0)
                 return sd_varlink_error_errno(link, r);
 
         /* Apply exec-specific properties from context.Exec */
         ExecContext *c = unit_get_exec_context(u);
         if (c) {
-                r = transient_exec_context_apply_properties(u, c, &p.context.exec);
+                bad_field = NULL;
+                r = transient_exec_context_apply_properties(u, c, &p.context.exec, &bad_field);
                 if (r == -EINVAL)
-                        return sd_varlink_error(link, VARLINK_ERROR_UNIT_BAD_SETTING, NULL);
+                        return sd_varlink_error_invalid_parameter_name(link, bad_field ?: "Exec");
                 if (r < 0)
                         return sd_varlink_error_errno(link, r);
         } else if (p.context.exec.present)
@@ -1261,9 +1295,10 @@ int vl_method_start_transient_unit(sd_varlink *link, sd_json_variant *parameters
         /* Apply service-specific properties from context.Service */
         Service *s = SERVICE(u);
         if (s) {
-                r = transient_service_apply_properties(s, &p.context.service);
+                bad_field = NULL;
+                r = transient_service_apply_properties(s, &p.context.service, &bad_field);
                 if (r == -EINVAL)
-                        return sd_varlink_error(link, VARLINK_ERROR_UNIT_BAD_SETTING, NULL);
+                        return sd_varlink_error_invalid_parameter_name(link, bad_field ?: "Service");
                 if (r < 0)
                         return sd_varlink_error_errno(link, r);
         } else if (p.context.service.present)
@@ -1273,7 +1308,7 @@ int vl_method_start_transient_unit(sd_varlink *link, sd_json_variant *parameters
         manager_dispatch_load_queue(manager);
 
         if (u->load_state == UNIT_BAD_SETTING)
-                return sd_varlink_error(link, VARLINK_ERROR_UNIT_BAD_SETTING, NULL);
+                return sd_varlink_error_invalid_parameter_name(link, "context");
         if (!UNIT_IS_LOAD_COMPLETE(u->load_state))
                 return sd_varlink_error(link, VARLINK_ERROR_UNIT_NO_SUCH_UNIT, NULL);
 

diff --git a/test/units/TEST-26-SYSTEMCTL.sh b/test/units/TEST-26-SYSTEMCTL.sh
index 93afa655a2094ab963edb81869ffe6b3b8129a78..d50ae68ef83b37e21bfca1ce7667a4cee5bb0d0a 100755 (executable)
--- a/test/units/TEST-26-SYSTEMCTL.sh
+++ b/test/units/TEST-26-SYSTEMCTL.sh
@@ -682,37 +682,55 @@ varlinkctl call "$MANAGER_SOCKET" io.systemd.Unit.StartTransient \
     '{"context":{"ID":"varlink-transient-exists.service","Service":{"ExecStart":[{"path":"/usr/bin/sleep","arguments":["/usr/bin/sleep","infinity"]}]}}}' |& grep "io.systemd.Unit.UnitExists"
 varlinkctl call "$MANAGER_SOCKET" io.systemd.Unit.StartTransient \
     '{"context":{"ID":"varlink-transient-test.target","Description":"test"}}' |& grep "io.systemd.Unit.UnitTypeNotSupported"
+# Apply-time and dispatch-time validation errors both surface as
+# org.varlink.service.InvalidParameter, with the offending field name in the
+# response parameters. Use --graceful to treat the expected error as success
+# so jq can assert on the dumped parameters JSON directly.
+expect_invalid_parameter() {
+    local payload="$1" field="$2"
+    varlinkctl call --graceful=org.varlink.service.InvalidParameter \
+                    "$MANAGER_SOCKET" io.systemd.Unit.StartTransient "$payload" \
+        | jq -e --arg f "$field" '.parameter == $f' >/dev/null
+}
 defer_transient_cleanup varlink-transient-bad.service
-varlinkctl call "$MANAGER_SOCKET" io.systemd.Unit.StartTransient \
-    '{"context":{"ID":"varlink-transient-bad.service","Service":{"Type":"simple"}}}' |& grep "io.systemd.Unit.BadUnitSetting"
+expect_invalid_parameter \
+    '{"context":{"ID":"varlink-transient-bad.service","Service":{"Type":"simple"}}}' \
+    "context"
 # Invalid ExecStart path: exercises filename_or_absolute_path_is_valid() in transient_service_apply_properties()
 defer_transient_cleanup varlink-transient-badpath.service
-varlinkctl call "$MANAGER_SOCKET" io.systemd.Unit.StartTransient \
-    '{"context":{"ID":"varlink-transient-badpath.service","Service":{"Type":"simple","ExecStart":[{"path":""}]}}}' |& grep "io.systemd.Unit.BadUnitSetting"
+expect_invalid_parameter \
+    '{"context":{"ID":"varlink-transient-badpath.service","Service":{"Type":"simple","ExecStart":[{"path":""}]}}}' \
+    "Service.ExecStart"
 # Relative WorkingDirectory path is rejected
 defer_transient_cleanup varlink-transient-bad-wd.service
-varlinkctl call "$MANAGER_SOCKET" io.systemd.Unit.StartTransient \
-    '{"context":{"ID":"varlink-transient-bad-wd.service","Exec":{"WorkingDirectory":{"path":"relative/path","missingOK":false}},"Service":{"Type":"oneshot","ExecStart":[{"path":"/bin/true"}]}}}' |& grep "io.systemd.Unit.BadUnitSetting"
+expect_invalid_parameter \
+    '{"context":{"ID":"varlink-transient-bad-wd.service","Exec":{"WorkingDirectory":{"path":"relative/path","missingOK":false}},"Service":{"Type":"oneshot","ExecStart":[{"path":"/bin/true"}]}}}' \
+    "Exec.WorkingDirectory"
 # Malformed environment entry (not KEY=VALUE)
 defer_transient_cleanup varlink-transient-bad-env.service
-varlinkctl call "$MANAGER_SOCKET" io.systemd.Unit.StartTransient \
-    '{"context":{"ID":"varlink-transient-bad-env.service","Exec":{"Environment":["not_an_env_var"]},"Service":{"Type":"oneshot","ExecStart":[{"path":"/bin/true"}]}}}' |& grep "io.systemd.Unit.BadUnitSetting"
+expect_invalid_parameter \
+    '{"context":{"ID":"varlink-transient-bad-env.service","Exec":{"Environment":["not_an_env_var"]},"Service":{"Type":"oneshot","ExecStart":[{"path":"/bin/true"}]}}}' \
+    "Exec.Environment"
 # Invalid User= name is rejected at JSON dispatch time as a parameter error
 defer_transient_cleanup varlink-transient-bad-user.service
-varlinkctl call "$MANAGER_SOCKET" io.systemd.Unit.StartTransient \
-    '{"context":{"ID":"varlink-transient-bad-user.service","Exec":{"User":"bad/user"},"Service":{"Type":"oneshot","ExecStart":[{"path":"/bin/true"}]}}}' |& grep "Invalid argument"
+expect_invalid_parameter \
+    '{"context":{"ID":"varlink-transient-bad-user.service","Exec":{"User":"bad/user"},"Service":{"Type":"oneshot","ExecStart":[{"path":"/bin/true"}]}}}' \
+    "context"
 # Out-of-range Nice= value is rejected
 defer_transient_cleanup varlink-transient-bad-nice.service
-varlinkctl call "$MANAGER_SOCKET" io.systemd.Unit.StartTransient \
-    '{"context":{"ID":"varlink-transient-bad-nice.service","Exec":{"Nice":100},"Service":{"Type":"oneshot","ExecStart":[{"path":"/bin/true"}]}}}' |& grep "io.systemd.Unit.BadUnitSetting"
+expect_invalid_parameter \
+    '{"context":{"ID":"varlink-transient-bad-nice.service","Exec":{"Nice":100},"Service":{"Type":"oneshot","ExecStart":[{"path":"/bin/true"}]}}}' \
+    "Exec.Nice"
 # Invalid credential ID
 defer_transient_cleanup varlink-transient-bad-cred-id.service
-varlinkctl call "$MANAGER_SOCKET" io.systemd.Unit.StartTransient \
-    '{"context":{"ID":"varlink-transient-bad-cred-id.service","Exec":{"SetCredential":[{"id":"bad/id","value":"YWJj"}]},"Service":{"Type":"oneshot","ExecStart":[{"path":"/bin/true"}]}}}' |& grep "io.systemd.Unit.BadUnitSetting"
+expect_invalid_parameter \
+    '{"context":{"ID":"varlink-transient-bad-cred-id.service","Exec":{"SetCredential":[{"id":"bad/id","value":"YWJj"}]},"Service":{"Type":"oneshot","ExecStart":[{"path":"/bin/true"}]}}}' \
+    "Exec.SetCredential"
 # Invalid base64 value for credential (rejected at JSON dispatch time as a parameter error)
 defer_transient_cleanup varlink-transient-bad-cred-value.service
-varlinkctl call "$MANAGER_SOCKET" io.systemd.Unit.StartTransient \
-    '{"context":{"ID":"varlink-transient-bad-cred-value.service","Exec":{"SetCredential":[{"id":"mycred","value":"!!!not_base64!!!"}]},"Service":{"Type":"oneshot","ExecStart":[{"path":"/bin/true"}]}}}' |& grep "Invalid argument"
+expect_invalid_parameter \
+    '{"context":{"ID":"varlink-transient-bad-cred-value.service","Exec":{"SetCredential":[{"id":"mycred","value":"!!!not_base64!!!"}]},"Service":{"Type":"oneshot","ExecStart":[{"path":"/bin/true"}]}}}' \
+    "context"
 # Exec on a unit type without an exec context (.slice) is rejected
 varlinkctl call "$MANAGER_SOCKET" io.systemd.Unit.StartTransient \
     '{"context":{"ID":"varlink-transient-exec.slice","Exec":{"WorkingDirectory":{"path":"/tmp","missingOK":false}}}}' |& grep "io.systemd.Unit.UnitTypeNotSupported"