Re: [PATCH V6 06/14] monitor: Update tracking iothread users with holder name

[Markus Armbruster <armbru@redhat.com>]

Zhang Chen <zhangckid@gmail.com> writes:

> On Thu, Apr 23, 2026 at 8:44 PM Markus Armbruster <armbru@redhat.com> wrote:
>> Oh.
>>
>> PATCH 02 defines QAPI type IoThreadHolder as a union with two branches
>> for the two kinds of holders: one for block node holder, and one for QOM
>> object holder.  Both branches use a string to identify the holder.
>>
>> The C code in PATCH 02 uses just a string to identify the holder.  This
>> silently assumes that the strings for block nodes are distinct from the
>> strings for QOM objects.  True as long as we only use absolute QOM
>> paths: these start with '/', and block nodes can't.
>>
>> This patch shows me there's actually a third kind: monitor.  You use the
>> QOM object branch for it.  That's confusing; a monitor is not a QOM
>> object.  Moreover, you make yet another silent assumption: absolute QOM
>> paths do not start with "/monitor/".
>>
>> This is too much for me.
>>
>> Please use a separate IoThreadHolderKind value and IoThreadHolder branch
>> for each kind of holder.  The kinds I've seen so far are block-node, QOM
>> object, monitor.
>>
>> If Daniel Berrangé's "[PATCH RFC 00/17] monitor: turn QMP and HMP into
>> QOM objects" gets merged, kind monitor can go away.
>>
>> Use of just a string to identify the holder requires strings for
>> different kinds to be distinct.  The argument why they are must be
>> written down, simple, and likely to remain true.
>>
>> But I'd prefer to use IoThreadHolder instead of string.  No assumptions
>> necessary then.
>>
>> If this IoThreadHolder arguments make the calls ugly or overly verbose,
>> consider thin wrappers for each kind, i.e.
>>
>>     new_ctx = iothread_get_aio_context_block(iothread, bs);
>>
>> instead of
>>
>>     holder_name = bdrv_get_node_name(bs);
>>     new_ctx = iothread_get_aio_context(iothread, holder_name);
>>
>> and so forth.
>>
>> Questions?
>
> Thank you for the detailed explanation.
> You are right, in this patch I silent assumption: absolute QOM paths
> do not start with "/monitor/".
> Because the Daniel Berrangé's RFC patch and the monitor implementation
> is very special,
> it creates the iothread for itself. The iothread lifecycle is same
> with the monitor.
> It is not a general case like QOM and block node.
> Maybe no need for this case add a third kind here?

I don't know.  Or maybe I don't understand the question.