Daniel P. Berrangé <berra...@redhat.com> writes: > 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.
Fair. The sane way to avoid that is keeping constructors super-simple. Ideally, not call anything. Next best, not call anything but simple initialization functions from well-known system libraries, and our own portability wrappers for them. > 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. Oh yes. So much simpler, easier, and safer than constructors. >> Do we actually do it? > > Probably not, but I can't be that confident as I have not auditted > all constructors. More evidence for us abusing constructors. The constructor audit I'd like to see: dumb them down to super-simple, ... > 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. ... so nobody will need to put debug prints there. > 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 :-( The spiked pits we set up for ourselves... >> 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> I suggest to record our low opinion on constructor abuse in the commit message. As is, it almost sounds as if we considered it normal. Starting threads there definitely isn't! Reviewed-by: Markus Armbruster <arm...@redhat.com>
