From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55682) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bwnTl-0002Hu-3p for qemu-devel@nongnu.org; Wed, 19 Oct 2016 05:48:10 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bwnTi-0003T5-2e for qemu-devel@nongnu.org; Wed, 19 Oct 2016 05:48:09 -0400 Received: from mx1.redhat.com ([209.132.183.28]:50872) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1bwnTh-0003Sr-S4 for qemu-devel@nongnu.org; Wed, 19 Oct 2016 05:48:06 -0400 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) (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 2318D8123D for ; Wed, 19 Oct 2016 09:48:04 +0000 (UTC) From: Markus Armbruster References: <20161012191502.GC16187@work-vm> <20161018100409.GH4349@redhat.com> <20161018113202.GE2190@work-vm> <20161018120121.GN4349@redhat.com> <20161018132524.GG2190@work-vm> <20161018133528.GD12728@redhat.com> <20161018135213.GI2190@work-vm> <20161018140141.GF12728@redhat.com> <87wph4g44n.fsf@dusky.pond.sub.org> <20161019081210.GA2035@work-vm> <20161019084235.GE11194@redhat.com> Date: Wed, 19 Oct 2016 11:48:01 +0200 In-Reply-To: <20161019084235.GE11194@redhat.com> (Daniel P. Berrange's message of "Wed, 19 Oct 2016 09:42:35 +0100") Message-ID: <87twc8d60e.fsf@dusky.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] chardev's and fd's in monitors List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Daniel P. Berrange" Cc: "Dr. David Alan Gilbert" , qemu-devel@nongnu.org "Daniel P. Berrange" writes: > On Wed, Oct 19, 2016 at 09:12:11AM +0100, Dr. David Alan Gilbert wrote: >> * Markus Armbruster (armbru@redhat.com) wrote: >> > "Daniel P. Berrange" writes: >> > >> > > On Tue, Oct 18, 2016 at 02:52:13PM +0100, Dr. David Alan Gilbert wrote: [...] >> > >> I already use error_report's in places in migration threads of various >> > >> types; I'm not sure if that's a problem. >> > > >> > > Unless those places are protected by the big qemu lock, that sounds >> > > not good. error_report calls into error_vprintf which checks the >> > > 'cur_mon' global "Monitor" pointer. This variable is updated at >> > > runtime - eg in qmp_human_monitor_command(), monitor_qmp_read(), >> > > monitor_read(), etc. So if migration threads outside the BQL are >> > > calling error_report() that could well cause problems. If you >> > > are lucky messages will merely end up going to stderr instead of >> > > the monitor, but in worst case I wouldn't be surprised if there >> > > is a crash possibility in some race conditions. >> > >> > cur_mon dates back to single-threaded times. >> > >> > The idea is to print to the monitor when running within an HMP command, >> > else to stderr. >> > >> > The current solution is to set cur_mon around monitor commands. Fine >> > with a single thread, not fine at all with multiple threads. >> > >> > Making cur_mon thread-local should fix things. >> > >> > If you do want to report errors from another thread in a monitor, you >> > should use error_setg() & friends to get them into the monitor, in my >> > opinion. Asynchronously barfing output to a monitor doesn't strike me >> > as a sensible design. Not least because it doesn't work at all with >> > QMP! If an error message is important enough for the human monitor's >> > user to make use route it to the human monitor, why is hiding it from >> > the QMP client okay? >> > >> > If I'm wrong and it is sensible, we need locking. >> >> The difficulty is that we've long tried to be consistent and use error_report >> rather than fprintf's; now that is turning out to be wrong if we're in >> other threads. No, the two are equally wrong as wrong as far as threading is concerned: unless that other thread is executing an HMP command, error_report() calls vfprintf(). >> It's even trickier for the cases of routines that might >> be called in either the main thread or another thread - we have no >> right answer as to how they should print na error. > > Pretty much all code should be using error_setg together with an Error **errp > parameter to the method. The only places that should use error_report are at > top of the call chain where they unambigously know that the printed result > is going to the right place. When you know your code's running on behalf of startup code, a monitor command or similar, go ahead and error_report(). When you don't know your context, error reporting should be left to something up the stack that does. This means returning a suitable error value in simple cases (null, -1, -errno, whatever makes sense, just document it), and error_setg() when error values don't provide enough information to the caller to report the error in a useful way. [...]