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

[Markus Armbruster <armbru@redhat.com>]

"Daniel P. Berrange" <berrange@redhat.com> writes:

> On Wed, Oct 19, 2016 at 11:05:53AM +0100, Dr. David Alan Gilbert wrote:
>> * 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,

In the cases where error_report() isn't suitable, fprintf() is just as
unsuitable for the exact same reasons.

> Well both error_report() + fprintf  are broken from POV of anything
> using QMP. error_report() is slightly less broken for HMP,

error_report() is not broken at all for HMP code.  The trouble is code
that can't know whether it's running in a context where error_report()
is suitable.

>                                                            but doesn't
> help QMP.

Correct.

> In the short term we should just make error_report be  threadsafe in
> its usage of the monitor.

Any problems left once cur_mon is thread-local (which it should be
anyway)?

>                           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.

Actually, no choice but propagate errors up the stack until they reach a
spot that knows how to report them.  error_setg() is *one* way to do
that.  Its prime advantage is ability to carry an error message.  Its
disadvantage is boilerplate.  Use it as needed.  Just don't convert back
and forth between Error and other representations while propagating up
the stack.