Am 10.09.2025 um 19:56 hat Stefan Hajnoczi geschrieben:
> g_source_destroy() only removes the GSource from the GMainContext it's
> attached to, if any. It does not free it.
>
> Use g_source_unref() instead so that the AioContext (which embeds a
> GSource) is freed. There is no need to call g_source_destroy() in
> aio_context_new() because the GSource isn't attached to a GMainContext
> yet.
>
> aio_ctx_finalize() expects everything to be set up already, so introduce
> the new ctx->initialized boolean and do nothing when called with
> !initialized. This also requires moving aio_context_setup() down after
> event_notifier_init() since aio_ctx_finalize() won't release any
> resources that aio_context_setup() acquired.
>
> Signed-off-by: Stefan Hajnoczi <[email protected]>
> Reviewed-by: Eric Blake <[email protected]>
> ---
> v2:
> - Fix spacing in aio_ctx_finalize() argument list [Eric]
> ---
> include/block/aio.h | 3 +++
> util/async.c | 14 +++++++++++---
> 2 files changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/include/block/aio.h b/include/block/aio.h
> index 1657740a0e..2760f308f5 100644
> --- a/include/block/aio.h
> +++ b/include/block/aio.h
> @@ -291,6 +291,9 @@ struct AioContext {
> gpointer epollfd_tag;
>
> const FDMonOps *fdmon_ops;
> +
> + /* Was aio_context_new() successful? */
> + bool initialized;
> };
>
> /**
> diff --git a/util/async.c b/util/async.c
> index a39410d675..34aaab4e9e 100644
> --- a/util/async.c
> +++ b/util/async.c
> @@ -363,12 +363,16 @@ aio_ctx_dispatch(GSource *source,
> }
>
> static void
> -aio_ctx_finalize(GSource *source)
> +aio_ctx_finalize(GSource *source)
> {
> AioContext *ctx = (AioContext *) source;
> QEMUBH *bh;
> unsigned flags;
>
> + if (!ctx->initialized) {
> + return;
> + }
You had to move aio_context_setup() down in aio_context_new() to make
sure that this doesn't leak things.
How will we make sure that nobody adds another error path after
allocating something after g_source_new()? g_source_new() doesn't seem
to guarantee that AioContext starts zeroed, which is annoying if we
wanted to just make aio_ctx_finalize() safe to be called from before the
first error path.
> thread_pool_free_aio(ctx->thread_pool);
>
> #ifdef CONFIG_LINUX_AIO
> @@ -579,13 +583,15 @@ AioContext *aio_context_new(Error **errp)
> ctx = (AioContext *) g_source_new(&aio_source_funcs, sizeof(AioContext));
> QSLIST_INIT(&ctx->bh_list);
> QSIMPLEQ_INIT(&ctx->bh_slice_list);
> - aio_context_setup(ctx);
>
> ret = event_notifier_init(&ctx->notifier, false);
> if (ret < 0) {
> error_setg_errno(errp, -ret, "Failed to initialize event notifier");
> goto fail;
> }
>
> +
> + aio_context_setup(ctx);
Can we at least add a comment here saying that there must be no error
after this aio_context_setup() call any more because we would then leak
things?
> g_source_set_can_recurse(&ctx->source, true);
> qemu_lockcnt_init(&ctx->list_lock);
>
> @@ -619,9 +625,11 @@ AioContext *aio_context_new(Error **errp)
>
> register_aiocontext(ctx);
>
> + ctx->initialized = true;
> +
> return ctx;
> fail:
> - g_source_destroy(&ctx->source);
> + g_source_unref(&ctx->source);
> return NULL;
> }
Kevin
