From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53363) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1byg6R-0005dy-Ch for qemu-devel@nongnu.org; Mon, 24 Oct 2016 10:19:55 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1byg6N-0002Dp-EV for qemu-devel@nongnu.org; Mon, 24 Oct 2016 10:19:51 -0400 Received: from mx1.redhat.com ([209.132.183.28]:39384) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1byg6N-0002Cc-8e for qemu-devel@nongnu.org; Mon, 24 Oct 2016 10:19:47 -0400 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 8D888C05678C for ; Mon, 24 Oct 2016 14:19:46 +0000 (UTC) From: Markus Armbruster References: <1477301639-386-1-git-send-email-pbonzini@redhat.com> <20161024103400.GA4014@work-vm> <37942764-b3c8-1ac1-5121-894ada7300f2@redhat.com> <87vawhevxp.fsf@dusky.pond.sub.org> <477e068b-d445-2296-25c6-976be1e2e319@redhat.com> Date: Mon, 24 Oct 2016 16:19:44 +0200 In-Reply-To: <477e068b-d445-2296-25c6-976be1e2e319@redhat.com> (Paolo Bonzini's message of "Mon, 24 Oct 2016 15:17:17 +0200") Message-ID: <87ziltbzi7.fsf@dusky.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH] qemu-error: remove dependency of stubs on monitor List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: "Dr. David Alan Gilbert" , qemu-devel@nongnu.org Paolo Bonzini writes: > On 24/10/2016 15:08, Markus Armbruster wrote: >> Paolo Bonzini writes: >> >>> On 24/10/2016 12:34, Dr. David Alan Gilbert wrote: >>>> * Paolo Bonzini (pbonzini@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 >>>>> --- >>>>> 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.