Re: [PATCH V9 02/17] iothread: introduce iothread_ref/unref to track attached devices

[Stefan Hajnoczi <stefanha@redhat.com>]

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

On Wed, Jun 24, 2026 at 03:08:36PM +0800, Zhang Chen wrote:
> Currently, IOThreads do not maintain a record of which devices are
> associated with them. This makes it difficult to monitor the
> workload distribution of IOThreads, especially in complex
> hotplug scenarios involving multiple virtio-blk or virtio-scsi devices.
> 
> This patch introduces a reference counting and tracking mechanism
> within the IOThread object:
> 
> - iothread_ref(): Prepends the device's IOThreadHolder to a list.
> - iothread_unref(): Searches for the IOThreadHolder using a custom
>   string comparison (g_strcmp0), releases the associated memory
>   upon a successful match.
> - holders: A GList storing the IOThreadHolder of attached devices
>   for runtime introspection.
> 
> A later commit will add QMP commands to let management applications
> query the attachment status of IOThreads.
> 
> Signed-off-by: Zhang Chen <zhangckid@gmail.com>
> ---
>  include/system/iothread.h |  5 +++
>  iothread.c                | 94 +++++++++++++++++++++++++++++++++++++++
>  qapi/misc.json            | 61 +++++++++++++++++++++++++
>  3 files changed, 160 insertions(+)

Minor comments below. This patch looks good overall:

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

> diff --git a/include/system/iothread.h b/include/system/iothread.h
> index a1ef7696cb..b9207ad829 100644
> --- a/include/system/iothread.h
> +++ b/include/system/iothread.h
> @@ -50,6 +50,11 @@ struct IOThread {
>      bool stopping;              /* has iothread_stop() been called? */
>      bool running;               /* should iothread_run() continue? */
>      int thread_id;
> +    /*
> +     * The list elements are of type IOThreadHolder, which can
> +     * represent either a QOM path or a block node name.

This comment duplicates the IOThreadHolder types and will become
outdated easily. It might already be outdated (there are 3 types, not
just the 2 mentioned in the comment). I suggest replacing it with
something like:

  /* List of iothread_ref() holders (elements are of type IOThreadHolder) */

> +static int iothread_holder_compare(gconstpointer a, gconstpointer b)
> +{
> +    const IOThreadHolder *holder_a = a;
> +    const IOThreadHolder *holder_b = b;
> +    const char *name_a, *name_b;
> +
> +    if (holder_a->type != holder_b->type) {
> +        return -1;
> +    }
> +
> +    switch (holder_a->type) {
> +    case IO_THREAD_HOLDER_KIND_QOM_OBJECT:
> +        name_a = holder_a->u.qom_object.qom_path;
> +        name_b = holder_b->u.qom_object.qom_path;
> +        break;
> +    case IO_THREAD_HOLDER_KIND_BLOCK_NODE:
> +        name_a = holder_a->u.block_node.node_name;
> +        name_b = holder_b->u.block_node.node_name;
> +        break;
> +    case IO_THREAD_HOLDER_KIND_MONITOR_NAME:
> +        name_a = holder_a->u.monitor_name.monitor_name;
> +        name_b = holder_b->u.monitor_name.monitor_name;
> +        break;
> +    default:
> +       /*
> +        * This should not happen. If it does, name_a/b remains
> +        * NULL and g_strcmp0 will handle it safely.
> +        */
> +        name_a = NULL;
> +        name_b = NULL;

return -1 would be simpler and safer. g_strcmp0(NULL, NULL) returns 0,
so callers will think they have found a match and proceed with their
operation - it will likely result in undefined behavior since memory is
probably corrupted if we got here. It's safer to fail the comparison so
the caller does not proceed.

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