Re: [Qemu-devel] chardev's and fd's in monitors

[Daniel P. Berrange]

On Wed, Oct 19, 2016 at 11:05:53AM +0100, Dr. David Alan Gilbert wrote:
> * Markus Armbruster (address@hidden) wrote:
> > "Daniel P. Berrange" <address@hidden> writes:
> > 
> > > On Wed, Oct 19, 2016 at 09:12:11AM +0100, Dr. David Alan Gilbert wrote:
> > >> * Markus Armbruster (address@hidden) wrote:
> > >> > "Daniel P. Berrange" <address@hidden> 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/ :|