Re: [RFC 06/11] aio: free AioContext when aio_context_new() fails

[Stefan Hajnoczi]

On Wed, May 28, 2025 at 04:06:13PM -0500, Eric Blake wrote:
> On Wed, May 28, 2025 at 03:09:11PM -0400, Stefan Hajnoczi wrote:
> > 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 
> > ---
> >  include/block/aio.h |  3 +++
> >  util/async.c| 12 ++--
> >  2 files changed, 13 insertions(+), 2 deletions(-)
> > 
> 
> > +++ b/util/async.c
> > @@ -369,6 +369,10 @@ aio_ctx_finalize(GSource *source)
> 
> While you're here, is it worth cleaning up that odd spacing in the
> function parameter list?

Yes.

Stefan


signature.asc
Description: PGP signature

Re: [RFC 06/11] aio: free AioContext when aio_context_new() fails

[Eric Blake]

On Wed, May 28, 2025 at 03:09:11PM -0400, Stefan Hajnoczi wrote:
> 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 
> ---
>  include/block/aio.h |  3 +++
>  util/async.c| 12 ++--
>  2 files changed, 13 insertions(+), 2 deletions(-)
> 

> +++ b/util/async.c
> @@ -369,6 +369,10 @@ aio_ctx_finalize(GSource *source)

While you're here, is it worth cleaning up that odd spacing in the
function parameter list?

>  QEMUBH *bh;
>  unsigned flags;
>  
> +if (!ctx->initialized) {
> +return;
> +}
> +
>  thread_pool_free_aio(ctx->thread_pool);

Reviewed-by: Eric Blake 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

[RFC 06/11] aio: free AioContext when aio_context_new() fails

[Stefan Hajnoczi]

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 
---
 include/block/aio.h |  3 +++
 util/async.c| 12 ++--
 2 files changed, 13 insertions(+), 2 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..bc841eeb4f 100644
--- a/util/async.c
+++ b/util/async.c
@@ -369,6 +369,10 @@ aio_ctx_finalize(GSource *source)
 QEMUBH *bh;
 unsigned flags;
 
+if (!ctx->initialized) {
+return;
+}
+
 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);
+
 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;
 }
 
-- 
2.49.0