sd-varlink: change sd_varlink_error() to always return an error

Let's make sure that sd_varlink_error() always returns an error code, so
that we can use it in a style "return sd_varlink_error(…);" everywhere,
which has two effects: return a good error reply to clients, and exit
the current stack frame with a failure code.

Interestingly sd_varlink_error_invalid_parameter() already worked like
this in some cases, but sd_varlink_error() itself didn't.

This is an alternative to the error handling tweak proposed in #34882,
but I think is a lot more generically useful, since it establishes a
pattern.

I checked our codebase, and this change should generally be OK without
breaking callsites, since the current callers (with exception of the
machined case from #34882) called sd_varlink_error() in the outermost
varlink method call dispatch stack frame, where this behaviour change
does not alter anything.

This is similar btw, how sd_bus_error_setf() and friends always return
error codes too, synthesized from its parameters.

diff --git a/src/libsystemd/sd-varlink/sd-varlink.c b/src/libsystemd/sd-varlink/sd-varlink.c
index a1b36482666b4d67b6247f35bb40f21e0b183572..73fcc71f867d16f99d9511ed3caa7abd0dc3675f 100644 (file)
--- a/src/libsystemd/sd-varlink/sd-varlink.c
+++ b/src/libsystemd/sd-varlink/sd-varlink.c
@@ -1361,7 +1361,11 @@ static int varlink_dispatch_method(sd_varlink *v) {
 
                                 if (v->state == VARLINK_PROCESSING_METHOD) {
                                         r = sd_varlink_error(v, SD_VARLINK_ERROR_EXPECTED_MORE, NULL);
-                                        if (r < 0)
+                                        /* If we didn't manage to enqueue an error response, then fail the
+                                         * connection completely. Otherwise ignore the error from
+                                         * sd_varlink_error() here, as it is synthesized from the function's
+                                         * parameters. */
+                                        if (r < 0 && VARLINK_STATE_WANTS_REPLY(v->state))
                                                 goto fail;
                                 }
                         } else if (r < 0) {
@@ -1371,7 +1375,8 @@ static int varlink_dispatch_method(sd_varlink *v) {
 
                                 if (VARLINK_STATE_WANTS_REPLY(v->state)) {
                                         r = sd_varlink_error_invalid_parameter_name(v, bad_field);
-                                        if (r < 0)
+                                        /* If we didn't manage to enqueue an error response, then fail the connection completely. */
+                                        if (r < 0 && VARLINK_STATE_WANTS_REPLY(v->state))
                                                 goto fail;
                                 }
                         }
@@ -1388,14 +1393,16 @@ static int varlink_dispatch_method(sd_varlink *v) {
                                  * method call remains unanswered. */
                                 if (VARLINK_STATE_WANTS_REPLY(v->state)) {
                                         r = sd_varlink_error_errno(v, r);
-                                        if (r < 0)
+                                        /* If we didn't manage to enqueue an error response, then fail the connection completely. */
+                                        if (r < 0 && VARLINK_STATE_WANTS_REPLY(v->state))
                                                 goto fail;
                                 }
                         }
                 }
         } else if (VARLINK_STATE_WANTS_REPLY(v->state)) {
                 r = sd_varlink_errorbo(v, SD_VARLINK_ERROR_METHOD_NOT_FOUND, SD_JSON_BUILD_PAIR("method", SD_JSON_BUILD_STRING(method)));
-                if (r < 0)
+                /* If we didn't manage to enqueue an error response, then fail the connection completely. */
+                if (r < 0 && VARLINK_STATE_WANTS_REPLY(v->state))
                         goto fail;
         }
 
@@ -2559,7 +2566,10 @@ _public_ int sd_varlink_error(sd_varlink *v, const char *error_id, sd_json_varia
         } else
                 varlink_set_state(v, VARLINK_PROCESSED_METHOD);
 
-        return 1;
+        /* Everything worked. Let's now return the error we got passed as input as negative errno, so that
+         * programs can just do "return sd_varlink_error();" and get both: a friendly error reply to clients,
+         * and an error return from the current stack frame. */
+        return sd_varlink_error_to_errno(error_id, parameters);
 }
 
 _public_ int sd_varlink_errorb(sd_varlink *v, const char *error_id, ...) {

diff --git a/src/test/test-varlink.c b/src/test/test-varlink.c
index cafe98871bbc3489aea619c8d797d23266331604..40d972b6b64193aa44c3cb250c35d7928dd2a87b 100644 (file)
--- a/src/test/test-varlink.c
+++ b/src/test/test-varlink.c
@@ -33,14 +33,20 @@ static int method_something(sd_varlink *link, sd_json_variant *parameters, sd_va
         int r;
 
         a = sd_json_variant_by_key(parameters, "a");
-        if (!a)
-                return sd_varlink_error(link, "io.test.BadParameters", NULL);
+        if (!a) {
+                r = sd_varlink_error(link, "io.test.BadParameters", NULL);
+                assert_se(r == -EBADR);
+                return r;
+        }
 
         x = sd_json_variant_integer(a);
 
         b = sd_json_variant_by_key(parameters, "b");
-        if (!b)
-                return sd_varlink_error(link, "io.test.BadParameters", NULL);
+        if (!b) {
+                r = sd_varlink_error(link, "io.test.BadParameters", NULL);
+                assert_se(r == -EBADR);
+                return r;
+        }
 
         y = sd_json_variant_integer(b);
 
@@ -105,8 +111,11 @@ static int method_passfd(sd_varlink *link, sd_json_variant *parameters, sd_varli
         int r;
 
         a = sd_json_variant_by_key(parameters, "fd");
-        if (!a)
-                return sd_varlink_error(link, "io.test.BadParameters", NULL);
+        if (!a) {
+                r = sd_varlink_error_invalid_parameter_name(link, "fd");
+                assert_se(r == -EINVAL);
+                return r;
+        }
 
         ASSERT_STREQ(sd_json_variant_string(a), "whoop");
 
@@ -242,8 +251,7 @@ static void *thread(void *arg) {
         _cleanup_(sd_json_variant_unrefp) sd_json_variant *i = NULL;
         _cleanup_(sd_json_variant_unrefp) sd_json_variant *wrong = NULL;
         sd_json_variant *o = NULL, *k = NULL, *j = NULL;
-        const char *error_id;
-        const char *e;
+        const char *error_id, *e;
         int x = 0;
 
         assert_se(sd_json_build(&i, SD_JSON_BUILD_OBJECT(SD_JSON_BUILD_PAIR("a", SD_JSON_BUILD_INTEGER(88)),
@@ -288,6 +296,7 @@ static void *thread(void *arg) {
         assert_se(sd_varlink_push_fd(c, fd3) == 2);
 
         assert_se(sd_varlink_callb(c, "io.test.PassFD", &o, &e, SD_JSON_BUILD_OBJECT(SD_JSON_BUILD_PAIR("fd", SD_JSON_BUILD_STRING("whoop")))) >= 0);
+        assert_se(!e);
 
         int fd4 = sd_varlink_peek_fd(c, 0);
         int fd5 = sd_varlink_peek_fd(c, 1);
@@ -298,6 +307,9 @@ static void *thread(void *arg) {
         test_fd(fd4, "miau", 4);
         test_fd(fd5, "wuff", 4);
 
+        assert_se(sd_varlink_callb(c, "io.test.PassFD", &o, &e, SD_JSON_BUILD_OBJECT(SD_JSON_BUILD_PAIR("fdx", SD_JSON_BUILD_STRING("whoopx")))) >= 0);
+        ASSERT_TRUE(sd_varlink_error_is_invalid_parameter(e, o, "fd"));
+
         assert_se(sd_varlink_callb(c, "io.test.IDontExist", &o, &e, SD_JSON_BUILD_OBJECT(SD_JSON_BUILD_PAIR("x", SD_JSON_BUILD_REAL(5.5)))) >= 0);
         ASSERT_STREQ(sd_json_variant_string(sd_json_variant_by_key(o, "method")), "io.test.IDontExist");
         ASSERT_STREQ(e, SD_VARLINK_ERROR_METHOD_NOT_FOUND);