Re: [PATCH v7 07/13] monitor: Make current monitor a per-coroutine property

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 14.09.2020 um 17:11 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>> 
>> > This way, a monitor command handler will still be able to access the
>> > current monitor, but when it yields, all other code code will correctly
>> > get NULL from monitor_cur().
>> >
>> > This uses a hash table to map the coroutine pointer to the current
>> > monitor of that coroutine.  Outside of coroutine context, we associate
>> > the current monitor with the leader coroutine of the current thread.
>> 
>> In qemu-system-FOO, the hash table can have only these entries:
>> 
>> * (OOB) One mapping @mon_iothread's thread leader to a QMP monitor, while
>>   executing a QMP command out-of-band.
>> 
>> * (QMP-CO) One mapping @qmp_dispatcher_co (a coroutine in the main
>>   thread) to a QMP monitor, while executing a QMP command in-band and in
>>   coroutine context.
>> 
>> * (QMP) One mapping the main thread's leader to a QMP monitor, while
>>   executing a QMP command in-band and out of coroutine context, in a
>>   bottom half.
>> 
>> * (HMP) One mapping the main thread's leader to an HMP monitor, while
>>   executing an HMP command out of coroutine context.
>> 
>> * (HMP-CO) One mapping a transient coroutine in the main thread to an
>>   HMP monitor, while executing an HMP command in coroutine context.
>> 
>> In-band execution is one command after the other.
>> 
>> Therefore, at most one monitor command can be executing in-band at any
>> time.
>> 
>> Therefore, the hash table has at most *two* entries: one (OOB), and one
>> of the other four.
>> 
>> Can you shoot any holes into my argument?
>
> I think with human-monitor-command, you can have three mappings:
>
> 1. The main thread's leader (it is a non-coroutine QMP command) to the
>    QMP monitor

This is (QMP).

> 2. With a coroutine HMP command, one mapping from the transient HMP
>    coroutine to the transient HMP monitor (with a non-coroutine HMP
>    command, we'd instead temporarily change the mapping from 1.)

This is (HMP-CO).

> 3. The OOB entry

This is (OOB).

To get 1. (QMP) and 2, (HMP-CO) at the same time, the in-band,
non-coroutine QMP command needs to execute interleaved with the in-band,
coroutine HMP command.

Such an interleaving contradicts "In-band execution is one command after
the other", which is a fundamental assumption in-band commands may make.
If the assumption is invalid, we got a problem.  Is it?

>> I suspect there's a simpler solution struggling to get out.  But this
>> solution works, so in it goes.  Should the simpler one succeed at
>> getting out, it can go in on top.  If not, I'll probably add even more
>> comments to remind myself of these facts.
>
> I think the simple approach you had posted could work if you can fill in
> the HMP part, but I think it wasn't completely trivial and you told me
> not to bother for now,

Correct.  I decided to go with the code you already tested.

>                        so I didn't. It may still be a viable path
> forward if you like it better.