From 4754cd2db6772bc688f9c23c827ca16160addbcb Mon Sep 17 00:00:00 2001 From: Daan De Meyer Date: Wed, 23 Sep 2026 12:00:35 +0200 Subject: [PATCH] sd-future: make sd_fiber_await() report failures rather then future results sd_fiber_await() returned the target future's result on completion and an error on interruption, so a negative value could mean either that the target future resolved with an error or that the wait never finished. Most callers had to check the target future's state afterwards to tell the two apart. Even worse, a target resolving with -ECANCELED was indistinguishable from a cancelled wait. Let's return 0 once the target future has resolved and a negative error only when the wait itself did not complete. The target future's outcome stays available through sd_future_result(), which callers have to additionally inspect now. --- src/libsystemd/sd-bus/bus-future.c | 10 +++--- src/libsystemd/sd-bus/test-bus-fiber.c | 1 + src/libsystemd/sd-bus/test-bus-watch-bind.c | 2 ++ src/libsystemd/sd-event/event-future.c | 4 +++ src/libsystemd/sd-future/fiber-io.c | 15 ++++++++- src/libsystemd/sd-future/fiber.c | 11 ++++--- src/libsystemd/sd-future/test-fiber.c | 27 ++++++++++------ src/libsystemd/sd-future/test-future-group.c | 33 +++++++++++++------- src/shared/qmp-client.c | 12 +++---- src/systemd/sd-future.h | 11 ++++--- src/test/test-qmp-client.c | 5 +-- 11 files changed, 85 insertions(+), 46 deletions(-) diff --git a/src/libsystemd/sd-bus/bus-future.c b/src/libsystemd/sd-bus/bus-future.c index 9e72ba2766..cc8c5f03cc 100644 --- a/src/libsystemd/sd-bus/bus-future.c +++ b/src/libsystemd/sd-bus/bus-future.c @@ -123,14 +123,12 @@ int bus_call_suspend( if (r < 0) return sd_bus_error_set_errno(reterr_error, r); + /* An interruption takes precedence even if the reply arrived in the same tick: the wait + * consumed the queued cancellation or timeout, and returning the reply instead would drop it. */ r = sd_fiber_await(f); - - /* If the future isn't resolved, the suspend was interrupted before a reply arrived (fiber - * cancelled, fiber-wide SD_FIBER_TIMEOUT scope expired, …). There's no reply to extract, - * so surface the resume error directly. When the future is resolved, future_get_bus_reply() - * recovers either the reply or the detailed sd_bus_error from the error reply. */ - if (sd_future_state(f) != SD_FUTURE_RESOLVED) + if (r < 0) return sd_bus_error_set_errno(reterr_error, r); + /* Recovers either the reply or the detailed sd_bus_error from the error reply. */ return future_get_bus_reply(f, reterr_error, ret_reply); } diff --git a/src/libsystemd/sd-bus/test-bus-fiber.c b/src/libsystemd/sd-bus/test-bus-fiber.c index 179f8943da..09dbb7ecf9 100644 --- a/src/libsystemd/sd-bus/test-bus-fiber.c +++ b/src/libsystemd/sd-bus/test-bus-fiber.c @@ -299,6 +299,7 @@ static int interrupted_calls_fiber(void *userdata) { } ASSERT_OK_ZERO(sd_fiber_await(caller)); + ASSERT_OK_ZERO(sd_future_result(caller)); ASSERT_NULL(s->c->pending_call); ASSERT_NULL(s->c->waiter); } diff --git a/src/libsystemd/sd-bus/test-bus-watch-bind.c b/src/libsystemd/sd-bus/test-bus-watch-bind.c index 6561633b8a..bbcd70d1dd 100644 --- a/src/libsystemd/sd-bus/test-bus-watch-bind.c +++ b/src/libsystemd/sd-bus/test-bus-watch-bind.c @@ -189,6 +189,8 @@ static int request_exit(void *userdata) { /* Wait for all client fibers to complete before requesting exit */ ASSERT_OK(sd_fiber_await(args->client1)); ASSERT_OK(sd_fiber_await(args->client2)); + ASSERT_OK(sd_future_result(args->client1)); + ASSERT_OK(sd_future_result(args->client2)); ASSERT_OK(sd_bus_new(&bus)); diff --git a/src/libsystemd/sd-event/event-future.c b/src/libsystemd/sd-event/event-future.c index 24ba19d678..e2e9a9b9c5 100644 --- a/src/libsystemd/sd-event/event-future.c +++ b/src/libsystemd/sd-event/event-future.c @@ -411,6 +411,10 @@ int event_run_suspend(sd_event *e, uint64_t timeout) { if (r < 0) return r; + r = sd_future_result(group); + if (r < 0) + return r; + r = sd_event_prepare(e); if (r == 0) r = sd_event_wait(e, 0); diff --git a/src/libsystemd/sd-future/fiber-io.c b/src/libsystemd/sd-future/fiber-io.c index b5c1f31245..e405fda3eb 100644 --- a/src/libsystemd/sd-future/fiber-io.c +++ b/src/libsystemd/sd-future/fiber-io.c @@ -54,6 +54,10 @@ static ssize_t fiber_io_operation( if (r < 0) return r; + r = sd_future_result(io); + if (r < 0) + return r; + return func(fd, args); } @@ -227,9 +231,13 @@ int sd_fiber_connect(int sockfd, const struct sockaddr *addr, socklen_t addrlen) if (r < 0) return r; + r = sd_fiber_await(io); + if (r < 0) + return r; + /* future_new_io resolves with the revents mask on success; translate any positive value * (e.g. POLLOUT) back to the connect(2) success status. */ - r = sd_fiber_await(io); + r = sd_future_result(io); return r > 0 ? 0 : r; } @@ -445,6 +453,11 @@ int sd_fiber_ppoll(struct pollfd *fds, size_t n_fds, const struct timespec *time r = sd_fiber_await(group); if (r < 0 && r != -ETIME) return r; + if (r >= 0) { + r = sd_future_result(group); + if (r < 0) + return r; + } /* Always sweep fds with a non-blocking ppoll(): the timer and an fd readiness can resolve in * the same event-loop tick (or the fd can become ready between the timer firing and us being diff --git a/src/libsystemd/sd-future/fiber.c b/src/libsystemd/sd-future/fiber.c index 19075c68fa..7c97524c72 100644 --- a/src/libsystemd/sd-future/fiber.c +++ b/src/libsystemd/sd-future/fiber.c @@ -822,7 +822,11 @@ int sd_fiber_sleep(uint64_t usec) { if (r < 0) return r; - return sd_fiber_await(timer); + r = sd_fiber_await(timer); + if (r < 0) + return r; + + return sd_future_result(timer); } static int fiber_wait_callback(sd_future *target, void *userdata) { @@ -861,7 +865,7 @@ int sd_fiber_await(sd_future *target) { assert_return(sd_future_get_event(target) == sd_future_get_event(f), -EINVAL); if (sd_future_state(target) == SD_FUTURE_RESOLVED) - return sd_future_result(target); + return 0; /* The fiber is executing inside event dispatch, so its loop cannot have finished yet. * Waiting during exit is supported by the slot's exit source. */ @@ -879,8 +883,7 @@ int sd_fiber_await(sd_future *target) { if (sd_future_state(target) != SD_FUTURE_RESOLVED) return -EBUSY; - /* Read the result while the slot still pins the target: releasing it may drop the last ref. */ - return sd_future_result(target); + return 0; } sd_future* sd_future_cancel_wait_unref(sd_future *f) { diff --git a/src/libsystemd/sd-future/test-fiber.c b/src/libsystemd/sd-future/test-fiber.c index d9234892e6..80b080bd26 100644 --- a/src/libsystemd/sd-future/test-fiber.c +++ b/src/libsystemd/sd-future/test-fiber.c @@ -379,16 +379,17 @@ TEST(fiber_wait_for_completed) { ASSERT_OK_EQ(sd_future_result(target), 100); } -/* Test: awaiting an already-resolved future returns the future's result directly */ +/* Test: awaiting an already-resolved future completes immediately */ static int await_resolved_fiber(void *userdata) { sd_future *target = userdata; ASSERT_EQ((int) sd_future_state(target), (int) SD_FUTURE_RESOLVED); - ASSERT_OK_EQ(sd_fiber_await(target), 77); + ASSERT_OK_ZERO(sd_fiber_await(target)); + ASSERT_OK_EQ(sd_future_result(target), 77); return 0; } -TEST(fiber_await_resolved_returns_result) { +TEST(fiber_await_resolved) { _cleanup_(sd_event_unrefp) sd_event *e = NULL; ASSERT_OK(sd_event_new(&e)); ASSERT_OK(sd_event_set_exit_on_idle(e, true)); @@ -1667,13 +1668,14 @@ static int await_result_fiber(void *userdata) { _cleanup_(sd_future_unrefp) sd_future *awaited = NULL; ASSERT_OK(sd_future_new_defer(sd_fiber_get_event(), result, &awaited)); - ASSERT_EQ(sd_fiber_await(awaited), result); /* Pending. */ - ASSERT_EQ(sd_fiber_await(awaited), result); /* Already resolved. */ + ASSERT_OK_ZERO(sd_fiber_await(awaited)); /* Pending. */ + ASSERT_EQ(sd_future_result(awaited), result); + ASSERT_OK_ZERO(sd_fiber_await(awaited)); /* Already resolved. */ ASSERT_OK_ZERO(sd_fiber_yield()); /* An already-resolved future must leave an unrelated pending interruption alone. */ ASSERT_OK(sd_fiber_resume(sd_fiber_get_current(), -ECANCELED)); - ASSERT_EQ(sd_fiber_await(awaited), result); + ASSERT_OK_ZERO(sd_fiber_await(awaited)); ASSERT_ERROR(sd_fiber_yield(), ECANCELED); } @@ -1693,7 +1695,14 @@ TEST(fiber_await_result) { } static int await_borrowed_fiber(void *userdata) { - return sd_fiber_await(userdata); + _cleanup_(sd_future_unrefp) sd_future *target = sd_future_ref(ASSERT_PTR(userdata)); + int r; + + r = sd_fiber_await(target); + if (r < 0) + return r; + + return sd_future_result(target); } TEST(fiber_await_last_reference) { @@ -1709,11 +1718,11 @@ TEST(fiber_await_last_reference) { ASSERT_OK(sd_fiber_new(e, "await-borrowed", await_borrowed_fiber, target, /* destroy= */ NULL, &waiter)); - /* Suspend in await, so its non-floating callback slot holds a reference to target. */ + /* Suspend in await, so the waiter's own reference and the wait's slot keep target alive. */ ASSERT_OK_POSITIVE(sd_event_run(e, 0)); ASSERT_EQ(sd_future_state(waiter), SD_FUTURE_PENDING); - /* Leave the slot as the sole owner. Await must read the result before freeing it. */ + /* Drop our reference while the wait is in flight: the waiter still reads the result. */ ASSERT_OK(sd_future_resolve(target, result)); target = sd_future_unref(target); diff --git a/src/libsystemd/sd-future/test-future-group.c b/src/libsystemd/sd-future/test-future-group.c index b33f83348a..a924b8a1e9 100644 --- a/src/libsystemd/sd-future/test-future-group.c +++ b/src/libsystemd/sd-future/test-future-group.c @@ -404,7 +404,7 @@ TEST(future_group_cancels_parent_on_child_error) { ASSERT_ERROR(s.group_result, EINVAL); } -/* Awaiting a group must return the child's error instead of cancelling the waiting parent. */ +/* Awaiting a group must complete with the child's error instead of cancelling the waiting parent. */ static int await_gets_error_driver(void *userdata) { _cleanup_(sd_future_unrefp) sd_future *group = NULL, *errorer = NULL; int *await_result = ASSERT_PTR(userdata); @@ -413,7 +413,8 @@ static int await_gets_error_driver(void *userdata) { ASSERT_OK(sd_future_new_defer(sd_fiber_get_event(), -EINVAL, &errorer)); ASSERT_OK(sd_future_group_add(group, errorer)); - *await_result = sd_fiber_await(group); + ASSERT_OK_ZERO(sd_fiber_await(group)); + *await_result = sd_future_result(group); ASSERT_OK_ZERO(sd_fiber_yield()); return 0; } @@ -462,10 +463,9 @@ typedef struct AddResolvedState { static int add_resolved_driver(void *userdata) { AddResolvedState *s = ASSERT_PTR(userdata); - /* Drive the pre-built child to completion. The defer resolves with -EINVAL, which - * makes sd_fiber_await return -EINVAL — we don't ASSERT_OK that. The child is now - * RESOLVED and we can add it to the group. */ - (void) sd_fiber_await(s->child); + /* Drive the pre-built child to completion. The defer resolves with -EINVAL, but the wait + * itself succeeds. The child is now RESOLVED and we can add it to the group. */ + ASSERT_OK_ZERO(sd_fiber_await(s->child)); ASSERT_EQ(sd_future_state(s->child), SD_FUTURE_RESOLVED); s->add_result = sd_future_group_add(s->group, s->child); @@ -473,7 +473,8 @@ static int add_resolved_driver(void *userdata) { /* Await drives the loop one more tick so the defer that wraps the RESOLVED child can * fire group_child_resolved and settle the group. */ - s->await_result = sd_fiber_await(s->group); + ASSERT_OK_ZERO(sd_fiber_await(s->group)); + s->await_result = sd_future_result(s->group); return 0; } @@ -510,7 +511,8 @@ typedef struct AddManyState { static int add_many_driver(void *userdata) { AddManyState *s = ASSERT_PTR(userdata); ASSERT_OK(sd_future_group_add_many(s->group, s->a, s->b, s->c)); - s->join_result = sd_fiber_await(s->group); + ASSERT_OK_ZERO(sd_fiber_await(s->group)); + s->join_result = sd_future_result(s->group); return 0; } @@ -744,12 +746,12 @@ static int parent_await_driver(void *userdata) { s->ready = true; ASSERT_EQ(sd_fiber_suspend(), s->result); - /* Parent cancellation does not replace the group's result. A later await of the resolved - * group returns that result, just like sd_future_result(). */ + /* Parent cancellation does not replace the group's result, and a later await of the + * resolved group completes immediately. */ if (s->result == -ECANCELED) { ASSERT_EQ(sd_future_state(s->group), SD_FUTURE_RESOLVED); ASSERT_ERROR(sd_future_result(s->group), EIO); - ASSERT_ERROR(sd_fiber_await(s->group), EIO); + ASSERT_OK_ZERO(sd_fiber_await(s->group)); } return 0; } @@ -830,7 +832,14 @@ TEST(future_group_external_cancel_leaves_parent_running) { } static int peer_await_driver(void *userdata) { - return sd_fiber_await(userdata); + sd_future *group = ASSERT_PTR(userdata); + int r; + + r = sd_fiber_await(group); + if (r < 0) + return r; + + return sd_future_result(group); } TEST(future_group_peer_await_does_not_suppress_parent_cancel) { diff --git a/src/shared/qmp-client.c b/src/shared/qmp-client.c index 42e7eb32bd..fa911cbfd6 100644 --- a/src/shared/qmp-client.c +++ b/src/shared/qmp-client.c @@ -977,16 +977,14 @@ static int qmp_client_call_suspend( if (r < 0) return r; + /* An interruption takes precedence even if the reply arrived in the same tick: the wait + * consumed the queued cancellation or timeout, and returning the reply instead would drop it. */ r = sd_fiber_await(call); - - /* If the future isn't resolved, the suspend was interrupted before a reply arrived (fiber - * cancelled, fiber-wide SD_FIBER_TIMEOUT scope expired, …). There's no reply to extract, - * so surface the resume error directly. When the future is resolved, future_get_qmp_reply() - * already encodes success (1), QMP-level error (-EIO with the desc captured if asked for), - * and no-reply (negative future result) — pass it through. */ - if (sd_future_state(call) != SD_FUTURE_RESOLVED) + if (r < 0) return r; + /* future_get_qmp_reply() encodes success (1), QMP-level error (-EIO with the desc captured if + * asked for), and no-reply (negative future result) — pass it through. */ return future_get_qmp_reply(call, ret_result, ret_error_desc); } diff --git a/src/systemd/sd-future.h b/src/systemd/sd-future.h index 93b76e4f50..a7e8b936de 100644 --- a/src/systemd/sd-future.h +++ b/src/systemd/sd-future.h @@ -141,11 +141,12 @@ sd_future* sd_fiber_get_awaiting(sd_future *f); int sd_fiber_yield(void); int sd_fiber_sleep(uint64_t usec); -/* Return the target's result on completion, or a wait error if the calling fiber is interrupted or - * the wait fails. An interrupted wait does not imply that the target has resolved; once it has, - * sd_future_result() gives its outcome independently of the wait's return value. An already-resolved - * target returns its result without consuming a pending interruption. The target must belong to the - * calling fiber's event loop. */ +/* Suspend until the target has resolved. Returns 0 once it has, and a negative error only if the wait + * itself did not complete: the calling fiber was interrupted (-ECANCELED, -ETIME), woken for an + * unrelated reason (-EBUSY), or the wait could not be set up. An interrupted wait does not imply that + * the target has resolved. The target's outcome is available through sd_future_result() afterwards; + * the caller must hold its own reference to read it. An already-resolved target returns 0 without + * consuming a pending interruption. The target must belong to the calling fiber's event loop. */ int sd_fiber_await(sd_future *target); int sd_fiber_suspend(void); int sd_fiber_resume(sd_future *f, int result); diff --git a/src/test/test-qmp-client.c b/src/test/test-qmp-client.c index 9f6429ea2c..a9203a7e2a 100644 --- a/src/test/test-qmp-client.c +++ b/src/test/test-qmp-client.c @@ -419,7 +419,7 @@ static int qmp_client_call_fiber(void *userdata) { sd_json_variant *status = ASSERT_NOT_NULL(sd_json_variant_by_key(result, "status")); ASSERT_STREQ(sd_json_variant_string(status), "running"); - /* QMP-level error: future resolves with -EIO, sd_fiber_await() returns -EIO, and + /* QMP-level error: the wait completes, the future resolves with -EIO, and * future_get_qmp_reply() also returns -EIO (mirroring future_get_bus_reply()) — with the * detailed description captured via error_desc on top, and result left NULL. */ f = sd_future_unref(f); @@ -427,7 +427,7 @@ static int qmp_client_call_fiber(void *userdata) { error_desc = mfree(error_desc); ASSERT_OK(qmp_client_call_future(client, "stop", NULL, &f)); - ASSERT_ERROR(sd_fiber_await(f), EIO); + ASSERT_OK_ZERO(sd_fiber_await(f)); ASSERT_ERROR(sd_future_result(f), EIO); ASSERT_ERROR(future_get_qmp_reply(f, &result, &error_desc), EIO); @@ -541,6 +541,7 @@ static int qmp_client_call_interrupted_fiber(void *userdata) { ASSERT_NOT_NULL(result); ASSERT_NULL(error_desc); ASSERT_OK_ZERO(sd_fiber_await(mock)); + ASSERT_OK_ZERO(sd_future_result(mock)); ASSERT_OK_ZERO(sd_fiber_yield()); ASSERT_OK(sd_event_exit(sd_fiber_get_event(), 0)); return 0;