On Wed, Sep 17, 2025 at 04:07:06PM +0200, Markus Armbruster wrote: > Daniel P. Berrangé <berra...@redhat.com> writes: > > > Some monitor functions, most notably, monitor_cur() rely on global > > data being initialized by 'monitor_init_globals()'. The latter is > > called relatively late in startup. If code triggers error_report() > > before monitor_init_globals() is called, QEMU will abort when > > accessing the uninitialized monitor mutex. > > > > The critical monitor global data must be initialized from a > > constructor function, to improve the guarantee that it is done > > before any possible calls to monitor_cur(). Not only that, but > > the constructor must be marked to run before the default > > constructor in case any of them trigger error reporting. > > Is error reporting from constructors a good idea? I feel they're best > used for simple initializations only.
When you're down in the weeds on a given piece of code it might not occurr that it could be used in a constructor. The biggest usage is QOM type registration, which we've obviously been careful (lucky) enough to keep safe. The other common use if initializing global mutexes. I rather wish our mutex APIs supported a static initializer like you get with pthreads and/or glib mutexes. That would have avoided this ordernig problem. > > Do we actually do it? Probably not, but I can't be that confident as I have not auditted all constructors. I accidentally created a problem myself by putting an error_report call into the rcu constructor to debug something never realized that would result in pain. And then I put error_report into the RCU thread itself and thus discovered that was running concurrently with other constructors. > > Note in particular that the RCU constructor will spawn a background > > thread so we might even have non-constructor QEMU code running > > concurrently with other constructors. > > Ugh! Indeed, that was my thought when discovernig this :-( > > Arguably > > Fixes: e69ee454b5f9 (monitor: Make current monitor a per-coroutine property) > > I never liked the @coroutine_mon hash table (which is what broke early > monitor_cur()), but accepted it for want of better ideas. I spent a little time wondering if we could replace coroutine_mon with a "__thread Monitor cur' and then update that in monitor_set_cur, but I couldn't convince myself it would be entirely safe. So for sake of getting the series done I took this approach and left the current monitor stuff for another day. > > > Reviewed-by: Richard Henderson <richard.hender...@linaro.org> > > Reviewed-by: Dr. David Alan Gilbert <d...@treblig.org> > > Signed-off-by: Daniel P. Berrangé <berra...@redhat.com> > With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
