On Wed, Oct 19, 2016 at 11:05:53AM +0100, Dr. David Alan Gilbert wrote: > * 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,
Well both error_report() + fprintf are broken from POV of anything using QMP. error_report() is slightly less broken for HMP, but doesn't help QMP. In the short term we should just make error_report be threadsafe in its usage of the monitor. 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. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
