Re: [Qemu-devel] [PATCH] qemu-error: remove dependency of stubs on monitor

[Markus Armbruster]

Paolo Bonzini <address@hidden> writes:

> On 24/10/2016 15:08, Markus Armbruster wrote:
>> Paolo Bonzini <address@hidden> writes:
>> 
>>> On 24/10/2016 12:34, Dr. David Alan Gilbert wrote:
>>>> * Paolo Bonzini (address@hidden) wrote:
>>>>> Leave the implementation of error_printf, error_printf_unless_qmp
>>>>> and error_vprintf to libqemustub.a and monitor.c, so that we can
>>>>> remove the monitor_printf and monitor_vprintf stubs.
>>>>>
>>>>> Signed-off-by: Paolo Bonzini <address@hidden>
>>>>> ---
>>>>>         This should help shutting up the vmstate unit tests.
>>>>
>>>> Why does this make it any easier than my patch?
>>>
>>>> You're still going to need to add something stub specific to turn
>>>> the output on and off.
>>>
>>> It makes it possible to override the functions independent of the rest
>>> of util/qemu-error.c. You can implement the functions in the test, simply as
>>>
>>>     had_stderr_output = true;
>>>
>>> and then assert that had_stderr_output is false or true depending on the
>>> test.
>> 
>> I buy that when I see a test using it :)
>
> Ok, so should I rewrite the test-vmstate patch to do this?

You need to rewrite an existing test or create a new one to demonstrate
your approach is worthwhile.  Details are up to you.

>>> (It's also a useful starting point to fix the cur_mon race).
>> 
>> Uh, the fix for the cur_mon race is making it thread-local, isn't it?
>
> Or just old-school mutex.  There is monitor_lock, let's make it protect
> cur_mon.

cur_mon is semantically thread-local: it's non-null while we're
executing a monitor command.  That's a property of the stack, thus the
thread.  The fact that it's not actually thread-local now is a bug I
blame on inertia.

Further evidence: if a thread calls error_report(), it should honor
cur_mon *only* when *this* thread executes a monitor command.  It should
not spew to some unrelated monitor just because some other thread
happens to execute a monitor command right now.

monitor_lock is different: it's for protecting data that's *shared*
among monitors, such as mon_list.  I suspect it's not quite used that
way.  But it should.