* Markus Armbruster (arm...@redhat.com) wrote: > "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.
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 / dgilb...@redhat.com / Manchester, UK
