Re: [PATCH V15 13/14] iothread: make legacy AioContext getter explicitly unsafe

[Markus Armbruster <armbru@redhat.com>]

Zhang Chen <zhangckid@gmail.com> writes:

> Now that holder-aware callers have been converted, rename
> iothread_get_aio_context() to iothread_unsafe_get_aio_context() and
> update qmp_x_blockdev_set_iothread(). This avoids adding a duplicate
> accessor only to remove the old one later.

The last sentence appears to justify the change since v14.  If that's
the intent, drop it, please.  If not, what is the intent?

> The block graph retains only the AioContext, while detachment happens
> outside qmp_x_blockdev_set_iothread(), so this call site cannot provide
> a matching holder lifecycle. The new name makes its existing lifetime
> assumption explicit.

Suggest to start like "The only remaining user is
qmp_x_blockdev_set_iothread()" and then explain why this caller cannot
use iothread_ref_and_get_aio_context().

> New code should use iothread_ref_and_get_aio_context().
>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Zhang Chen <zhangckid@gmail.com>
> ---
>  blockdev.c                |  7 ++++++-
>  include/system/iothread.h | 11 ++++++++++-
>  iothread.c                |  2 +-
>  3 files changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index 6e86c6262f..97d161079a 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3683,7 +3683,12 @@ void qmp_x_blockdev_set_iothread(const char *node_name, StrOrNull *iothread,
>              goto out;
>          }
>  
> -        new_context = iothread_get_aio_context(obj);
> +        /*
> +         * The block graph retains only the AioContext, not the IOThread.
> +         * Context detachment happens outside this command, so there is no
> +         * point here where a matching IOThread holder can be released.
> +         */

This is fine for readers who already know what this is about: why we
can't use iothread_ref_and_get_aio_context() /
iothread_unsafe_get_aio_context() here.  Others will lack context.

Suggest to start like

           /*
            * We cannot use iothread_ref_and_get_aio_context() /
            * iothread_unref_and_put_aio_context() here.

then explain why.

> +        new_context = iothread_unsafe_get_aio_context(obj);
>      } else {
>          new_context = qemu_get_aio_context();
>      }

[...]