"Daniel P. Berrange" <berra...@redhat.com> writes: > On Wed, Oct 19, 2016 at 09:12:11AM +0100, Dr. David Alan Gilbert wrote: >> * Markus Armbruster (arm...@redhat.com) wrote: >> > "Daniel P. Berrange" <berra...@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. [...]