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

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Markus Armbruster (armbru@redhat.com) wrote:
> "Daniel P. Berrange" <berrange@redhat.com> 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" <berrange@redhat.com> 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