From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33042) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bwnv7-0003Pp-7v for qemu-devel@nongnu.org; Wed, 19 Oct 2016 06:16:26 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bwnv3-0000f0-Bi for qemu-devel@nongnu.org; Wed, 19 Oct 2016 06:16:25 -0400 Received: from mx1.redhat.com ([209.132.183.28]:35496) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1bwnv3-0000eT-4K for qemu-devel@nongnu.org; Wed, 19 Oct 2016 06:16:21 -0400 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) (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 EFFCDBAEF for ; Wed, 19 Oct 2016 10:16:19 +0000 (UTC) Date: Wed, 19 Oct 2016 11:16:16 +0100 From: "Daniel P. Berrange" Message-ID: <20161019101616.GL11194@redhat.com> Reply-To: "Daniel P. Berrange" References: <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> <20161019100552.GD2035@work-vm> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20161019100552.GD2035@work-vm> Subject: Re: [Qemu-devel] chardev's and fd's in monitors List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Dr. David Alan Gilbert" Cc: Markus Armbruster , qemu-devel@nongnu.org On Wed, Oct 19, 2016 at 11:05:53AM +0100, Dr. David Alan Gilbert wrote: > * 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, Well both error_report() + fprintf are broken from POV of anything using QMP. error_report() is slightly less broken for HMP, but doesn't help QMP. In the short term we should just make error_report be threadsafe in its usage of the monitor. Beyond the short term we have no choice but to plumb in error_setg throughout, otherwise QMP will continue to have useless error reporting in this area of code. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|