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

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.

[...]