Re: [RESEND PATCH V10 14/15] iothread: simplify API by merging iothread_get_aio_context variants

On Fri, Jul 31, 2026, 3:43 PM Markus Armbruster <[email protected]> wrote:

> Zhang Chen <[email protected]> writes:
>
> > Simplify the interface by merging iothread_ref_and_get_aio_context()
> > into iothread_get_aio_context(). The updated function now requires a
> > 'holder' parameter, ensuring that every retrieval of an AioContext for
> > long-term use is automatically registered in the IOThread's holder list.
> >
> > Update all callers across block, virtio, scsi, net, and monitor
> > subsystems to match the new signature. This cleanup reduces code
> > redundancy and improves the reliability of IOThread introspection.
> >
> > Signed-off-by: Zhang Chen <[email protected]>
>
> [...]
>
> > diff --git a/include/system/iothread.h b/include/system/iothread.h
> > index 3cce5eeb09..3a6010b9f0 100644
> > --- a/include/system/iothread.h
> > +++ b/include/system/iothread.h
> > @@ -73,22 +73,22 @@ DECLARE_INSTANCE_CHECKER(IOThread, IOTHREAD,
> >
> >  char *iothread_get_id(IOThread *iothread);
> >  IOThread *iothread_by_id(const char *id);
> > -/*
> > - * The iothread_get_aio_context() and iothread_put_aio_context() are not
> > - * thread-safe and must be called under the Big QEMU Lock (BQL).
> > - */
> > -AioContext *iothread_get_aio_context(IOThread *iothread);
> > +
> >  /*
> >   * Normally the IOThread's AioContext is fetched using
> > - * iothread_ref_and_get_aio_context(), but there are legacy callers
> > + * iothread_get_aio_context(), but there are legacy callers
> >   * without a clear ref/unref lifecycle. They cannot unref (e.g. because
> >   * they do not have an IOThread pointer), so provide an unsafe way to
> >   * fetch the AioContext without holding a reference count. This unsafe
> >   * API serves legacy callers - do not use it in new code.
> >   */
> >  AioContext *iothread_unsafe_get_aio_context(IOThread *iothread);
> > -AioContext *iothread_ref_and_get_aio_context(IOThread *iothread,
> > -                                             const IOThreadHolder
> *holder);
> > +/*
> > + * The iothread_get_aio_context() and iothread_put_aio_context() are not
> > + * thread-safe and must be called under the Big QEMU Lock (BQL).
> > + */
> > +AioContext *iothread_get_aio_context(IOThread *iothread,
> > +                                     const IOThreadHolder *holder);
> >  void iothread_put_aio_context(IOThread *iothread, const IOThreadHolder
> *holder);
> >  GMainContext *iothread_get_g_main_context(IOThread *iothread);
> >
> > diff --git a/iothread.c b/iothread.c
> > index 157bb4d302..9d278efc45 100644
> > --- a/iothread.c
> > +++ b/iothread.c
> > @@ -429,13 +429,8 @@ char *iothread_get_id(IOThread *iothread)
> >      return
> g_strdup(object_get_canonical_path_component(OBJECT(iothread)));
> >  }
> >
> > -AioContext *iothread_get_aio_context(IOThread *iothread)
> > -{
> > -    return iothread->ctx;
> > -}
> > -
> > -AioContext *iothread_ref_and_get_aio_context(IOThread *iothread,
> > -                                             const IOThreadHolder
> *holder)
> > +AioContext *iothread_get_aio_context(IOThread *iothread,
> > +                                     const IOThreadHolder *holder)
> >  {
> >      /* Add IOThreadHolder to the list */
> >      iothread_ref(iothread, holder);
>
> Together with PATCH 04, this renames iothread_get_aio_context() to
> iothread_unsafe_get_aio_context(), and
> iothread_ref_and_get_aio_context() to iothread_get_aio_context().
>
> iothread_get_aio_context() takes a reference now.  That's a very
> significant change.
>
> In my opinion, every function that takes or releases a reference needs a
> function comment unless the function name makes it completely obvious.
> I know we have plenty of functions lacking such comments, but that
> doesn't make me wrong :)
>
> There are similarly named functions that, as far as I can tell, return a
> thing's AioContext without taking a reference:
>
>     bdrv_get_aio_context()
>     blk_get_aio_context()
>     block_job_get_aio_context()
>     iohandler_get_aio_context()
>     qemu_coroutine_get_aio_context()
>     qemu_get_aio_context()
>
> I fear this is going to be confusing.
>
> But I'm not the maintainer.  Who is the maintainer?  MAINTAINERS doesn't
> cover iothread.c, even though it is a core piece of infrastructure.
>
>
Thanks for the detailed review!

1. Documentation:
   I completely agree that functions modifying
 reference counts must be explicitly documented.
I will add comprehensive function comments in
include/sysemu/iothread.h in the next version
to clearly state that iothread_get_aio_context()
takes a reference and requires a matching
iothread_put_aio_context().

2. API naming & consistency:
   The main motivation for merging iothread_ref_and_get_aio_context()
into iothread_get_aio_context(..., holder) was
to enforce holder registration and avoid untracked
AioContext retrievals across subsystems.
The mandatory 'holder' parameter serves as
a explicit hint that this is an acquiring operation.

This series based on Stefan's suggestions, and
he also contributed some of the IOThread relevant work. I'll wait for
feedback from Stefan/Paolo/Markus or others regarding the naming
convention. If they prefer keeping a distinct name (e.g.
iothread_ref_get_aio_context), I am open to adjusting it.

I think Stefan is the missed IOThread maintainer role? (By the way, if he
or anyone else has no objections, I can add it at the end.).

Thanks
Chen



> [...]
>
>