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

[Markus Armbruster]

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

[...]