From d2ebf5cc1d59e29139f06efaa3a9b2c184cdaa25 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 30 Oct 2024 15:31:08 +0100 Subject: [PATCH] sd-varlink: change sd_varlink_error() to always return an error MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit 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. --- src/libsystemd/sd-varlink/sd-varlink.c | 20 +++++++++++++----- src/test/test-varlink.c | 28 ++++++++++++++++++-------- 2 files changed, 35 insertions(+), 13 deletions(-) diff --git a/src/libsystemd/sd-varlink/sd-varlink.c b/src/libsystemd/sd-varlink/sd-varlink.c index a1b3648266..73fcc71f86 100644 --- 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 cafe98871b..40d972b6b6 100644 --- 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); -- 2.25.1