Paolo Bonzini <pbonz...@redhat.com> writes: > On 24/10/2016 15:08, Markus Armbruster wrote: >> Paolo Bonzini <pbonz...@redhat.com> writes: >> >>> On 24/10/2016 12:34, Dr. David Alan Gilbert wrote: >>>> * Paolo Bonzini (pbonz...@redhat.com) 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 <pbonz...@redhat.com> >>>>> --- >>>>> 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.
