Re: [Qemu-devel] [PATCH v4] monitor: let cur_mon be per-thread

[Markus Armbruster]

Peter Xu <address@hidden> writes:

> On Thu, Jul 19, 2018 at 02:14:56PM +0200, Markus Armbruster wrote:
[...]
>> Here's my try:
>> 
>>     monitor: Fix unsafe sharing of @cur_mon among threads
>> 
>>     @cur_mon is null unless the main thread is running monitor code, either
>>     HMP code within monitor_read(), or QMP code within
>>     monitor_qmp_dispatch().
>> 
>>     Use of @cur_mon outside the main thread is therefore unsafe.
>> 
>>     Most of its uses are in monitor command handlers.  These run in the main
>>     thread.
>> 
>>     However, there are also uses hiding elsewhere, such as in
>>     error_vprintf(), and thus error_report(), making these functions unsafe
>>     outside the main thread.  No such unsafe uses are known at this time.
>>     Regardless, this is an unnecessary trap.  It's an ancient trap, though.
>> 
>>     More recently, commit cf869d53172 "qmp: support out-of-band (oob)
>>     execution" spiced things up: the monitor I/O thread assigns to @cur_mon
>>     when executing commands out-of-band.  Having two threads save, set and
>>     restore @cur_mon without synchronization is definitely unsafe.  We can
>>     end up with @cur_mon null while the main thread runs monitor code, or
>>     non-null while it runs non-monitor code.
>> 
>>     We could fix this by making the I/O thread not mess with @cur_mon, but
>>     that would leave the trap armed and ready.
>> 
>>     Instead, make @cur_mon thread-local.  It's now reliably null unless the
>>     thread is running monitor code.
>
> Looks good to me.

With that commit message:
Reviewed-by: Markus Armbruster <address@hidden>