On Thu, Oct 09, 2025 at 06:06:00PM +0200, Kevin Wolf wrote: > 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.
Calling aio_ctx_finalize() in all cases was my first thought when writing this patch, but not all of the cleanup code works even when resources are NULL. That's why I took the ->initialized field approach. I will add comments explaining how to handle resource cleanup and the ordering with aio_context_setup(). Stefan
signature.asc
Description: PGP signature
