From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59029) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bwnl5-00036a-Sk for qemu-devel@nongnu.org; Wed, 19 Oct 2016 06:06:05 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bwnl0-0004PV-SY for qemu-devel@nongnu.org; Wed, 19 Oct 2016 06:06:03 -0400 Received: from mx1.redhat.com ([209.132.183.28]:44212) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1bwnl0-0004OS-KN for qemu-devel@nongnu.org; Wed, 19 Oct 2016 06:05:58 -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 6C5CFC04B317 for ; Wed, 19 Oct 2016 10:05:56 +0000 (UTC) Date: Wed, 19 Oct 2016 11:05:53 +0100 From: "Dr. David Alan Gilbert" Message-ID: <20161019100552.GD2035@work-vm> References: <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> <87twc8d60e.fsf@dusky.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87twc8d60e.fsf@dusky.pond.sub.org> Subject: Re: [Qemu-devel] chardev's and fd's in monitors List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: "Daniel P. Berrange" , qemu-devel@nongnu.org * Markus Armbruster (armbru@redhat.com) wrote: > "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. We need a way to be able to report an error without plumbing error_setg up the stack; if you're saying error_report isn't suitable then we should just recommend we switch everything in migration back to fprintf(stderr, Dave > [...] -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK