From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43416) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bwmSS-0003Jw-H3 for qemu-devel@nongnu.org; Wed, 19 Oct 2016 04:42:46 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bwmSP-00063A-9z for qemu-devel@nongnu.org; Wed, 19 Oct 2016 04:42:44 -0400 Received: from mx1.redhat.com ([209.132.183.28]:33592) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1bwmSO-00062l-W9 for qemu-devel@nongnu.org; Wed, 19 Oct 2016 04:42:41 -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 99B48C05681A for ; Wed, 19 Oct 2016 08:42:39 +0000 (UTC) Date: Wed, 19 Oct 2016 09:42:35 +0100 From: "Daniel P. Berrange" Message-ID: <20161019084235.GE11194@redhat.com> Reply-To: "Daniel P. Berrange" 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> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20161019081210.GA2035@work-vm> Content-Transfer-Encoding: quoted-printable 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 09:12:11AM +0100, Dr. David Alan Gilbert wrote: > * Markus Armbruster (armbru@redhat.com) wrote: > > "Daniel P. Berrange" writes: > >=20 > > > On Tue, Oct 18, 2016 at 02:52:13PM +0100, Dr. David Alan Gilbert wr= ote: > > >> * Daniel P. Berrange (berrange@redhat.com) wrote: > > >> > On Tue, Oct 18, 2016 at 02:25:25PM +0100, Dr. David Alan Gilbert= wrote: > > >> > > * Daniel P. Berrange (berrange@redhat.com) wrote: > > >> > > > On Tue, Oct 18, 2016 at 12:32:02PM +0100, Dr. David Alan Gil= bert wrote: > > >> > > > > * Daniel P. Berrange (berrange@redhat.com) wrote: > > >> > > > > > On Wed, Oct 12, 2016 at 08:15:02PM +0100, Dr. David Alan= Gilbert wrote: > > >> > > > > > > Hi, > > >> > > > > > > I had a look at a couple of readline like libraries; > > >> > > > > > > editline and linenoise. A difficulty with using them = is that > > >> > > > > > > they both want fd's or FILE*'s; editline takes either = but > > >> > > > > > > from a brief look I think it's expecting to extract th= e fd. > > >> > > > > > > That makes them tricky to integrate into qemu, where > > >> > > > > > > the chardev's hide a whole bunch of non-fd things; in = particular > > >> > > > > > > tls, mux, ringbuffers etc. > > >> > > > > > >=20 > > >> > > > > > > If we could get away with just a FILE* then we could u= se fopencookie, > > >> > > > > > > but that's GNU only. > > >> > > > > > >=20 > > >> > > > > > > Is there any sane way of shepherding all chardev's int= o having an > > >> > > > > > > fd? > > >> > > > > >=20 > > >> > > > > > The entire chardev abstraction model exists precisely be= cause we cannot > > >> > > > > > make all chardevs look like a single fd. Even those whic= h are fd based > > >> > > > > > may have separate FDs for input and output. > > >> > > > >=20 > > >> > > > > Note that editline takes separate in/out streams, but it d= oes want those streams > > >> > > > > to be FILE*'s. > > >> > > > >=20 > > >> > > > > > IMHO the only viable approach would be to enhance lineno= ise/editline to > > >> > > > > > not assume use of fd* or FILE * abstractions. > > >> > > > >=20 > > >> > > > > I think if it came to that then we'd probably end up stick= ing with what we > > >> > > > > had for a very long time; I'd assume it would take a long = time before > > >> > > > > any mods we made to the libraries would come around to be = generally useful. > > >> > > > >=20 > > >> > > > > > BTW, what is the actual thread issue you are facing ? Ch= ardevs at least > > >> > > > > > ought to be usable from a separate thread, as long as ea= ch distinct > > >> > > > > > chardev object instance was only used from one thread at= a time ? > > >> > > > >=20 > > >> > > > > Marc-Andr=C3=A9 pointed that out; I hadn't realised they w= ere thread safe. > > >> > > > > But what are the rules? You say 'only used from one thread= at a time' - > > >> > > > > what happens if we have a mux and the different streams to= the mux come > > >> > > > > from different threads? > > >> > > >=20 > > >> > > > Well there is no mutex locking on the CharDriverState object= s, so the > > >> > > > exact rule is "you mustn't do anything from multiple threads= that will > > >> > > > race on contents of CharDriverState". That's too fuzzy to be= useful to > > >> > > > developers though, so I think the only sensible option right= now is to > > >> > > > say any "top level" CharDriverState should only be touch fro= m one thread > > >> > > > at a time. IOW, if you have a mux, that that rule would appl= y to the > > >> > > > mux itself and the various children it owns as if they were = a single > > >> > > > unnit. > > >> > >=20 > > >> > > OK; I think we're probably saved by the big lock at the moment= , so that > > >> > > all device emulation that outputs text is probably holding it = and the monitor > > >> > > is also. What about something like an error_report from a dif= ferent thread > > >> > > while something is happening in the monitor? > > >> >=20 > > >> > If we moved execution of monitor commands to separate thread fro= m the > > >> > thread handling monitor I/O, then we'd have to modify error_repo= rt so > > >> > that it queued the text in some manner, such that it was only th= en > > >> > fed back to the client once the command thread completed. Altern= atively > > >> > we'd have to introduced locking in the Monitor object, that seri= alized > > >> > access to the underling CharDriverState I/O funcs. > > >>=20 > > >> I already use error_report's in places in migration threads of var= ious > > >> 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. > >=20 > > cur_mon dates back to single-threaded times. > >=20 > > The idea is to print to the monitor when running within an HMP comman= d, > > else to stderr. > >=20 > > The current solution is to set cur_mon around monitor commands. Fine > > with a single thread, not fine at all with multiple threads. > >=20 > > Making cur_mon thread-local should fix things. > >=20 > > 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 m= e > > 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? > >=20 > > If I'm wrong and it is sensible, we need locking. >=20 > 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. 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 resul= t is going to the right place. Historically the migration hasn't use error_setg because where was no mechanism to feed the Error **errp object back to the caller, due to the "migrate" QMP command always being in the background. Similarly in the incoming migration side, we've always wanted errors to go to stderr. I fixed the outgoing side in commit d59ce6f34434bf47a9b26138c908650bf9a24be1 Author: Daniel P. Berrange Date: Wed Apr 27 11:05:00 2016 +0100 migration: add reporting of errors for outgoing migration So we could useful convert the migration code to use error_setg everywher= e and then use error_report_err() on the incoming side to spit that Error onto stderr. The block layer has almost no use of Error **errp parameters either and this has long been an issue preventing useful error reporting there. Regards, Daniel --=20 |: http://berrange.com -o- http://www.flickr.com/photos/dberrange= / :| |: http://libvirt.org -o- http://virt-manager.or= g :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr= / :|