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

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2828 bytes --]

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 <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.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]