Re: [Qemu-devel] [PATCH v7 4/4] monitor: add lock to protect mon_fdsets

[Peter Xu]

On Mon, May 28, 2018 at 05:19:08PM +0200, Markus Armbruster wrote:

[...]

> >> 
> >> > + * Meanwhile it can also be used even at the end of main.  Let's keep
> >> > + * it initialized for the whole lifecycle of QEMU.
> >> > + */
> >> 
> >> Awkward question, since our main() is such a tangled mess, but here goes
> >> anyway...  The existing place to initialize monitor.c's globals is
> >> monitor_init_globals().  But that one runs too late, I guess:
> >> parse_add_fd() runs earlier, and calls monitor_fdset_add_fd().  Unclean
> >> even without this lock; no module should be used before its
> >> initialization function runs.  Sure we can't run monitor_init_globals()
> >> sufficiently early?
> >
> > Please see the comment for monitor_init_globals():
> >
> >     /*
> >      * Note: qtest_enabled() (which is used in monitor_qapi_event_init())
> >      * depends on configure_accelerator() above.
> >      */
> >     monitor_init_globals();
> >
> > So I guess it won't work to directly move it earlier.  The init
> > dependency of QEMU is really complicated.  I'll be fine now that we
> > mark totally independent init functions (like this one, which is a
> > mutex init only) as constructor, then we can save some time on
> > ordering issue.
> 
> Let me rephrase.  There's a preexisting issue: main() calls monitor.c
> functions before calling its initialization function
> monitor_init_globals().  This needs to be cleaned up.  Would you be
> willing to do it?
> 
> Possible solutions:
> 
> * Perhaps we can move monitor_init_globals() up and/or the code calling
>   into monitor.c early down sufficiently.
> 
> * Calculate event_clock_type on each use instead of ahead of time.  It's
>   qtest_enabled ? QEMU_CLOCK_VIRTUAL : QEMU_CLOCK_REALTIME, and neither
>   of its users needs to be fast.  Then move monitor_init_globals before
>   the code calling into monitor.c.

Indeed.  Obviously you thought a step further. :)

> 
> I'm not opposed to use of constructors for self-contained initialization
> (no calls to other modules).  But I don't like initialization spread
> over multiple functions.

Since this work will actually decide where I should init this new
fdset lock, so I'll try to do that altogether within the series.

Thanks for your suggestions!  It makes sense.

-- 
Peter Xu