Re: [PATCH v3 02/20] monitor: initialize global data from a constructor

["Daniel P. Berrangé" <berrange@redhat.com>]

On Wed, Sep 17, 2025 at 04:07:06PM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@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.henderson@linaro.org>
> > Reviewed-by: Dr. David Alan Gilbert <dave@treblig.org>
> > Signed-off-by: Daniel P. Berrangé <berrange@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 :|