Re: [PATCH v4 06/12] aio: free AioContext when aio_context_new() fails

[Kevin Wolf <kwolf@redhat.com>]

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 <stefanha@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
> 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