Re: [PATCH v5 2/4] vl: Initialise main loop earlier

[Kevin Wolf <kwolf@redhat.com>]

Am 19.02.2020 um 15:07 hat Wolfgang Bumiller geschrieben:
> On Tue, Feb 18, 2020 at 04:40:34PM +0100, Kevin Wolf wrote:
> > We want to be able to use qemu_aio_context in the monitor
> > initialisation.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > Reviewed-by: Markus Armbruster <armbru@redhat.com>
> > ---
> >  vl.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/vl.c b/vl.c
> > index 794f2e5733..98bc51e089 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -2894,6 +2894,11 @@ int main(int argc, char **argv, char **envp)
> >      runstate_init();
> >      precopy_infrastructure_init();
> >      postcopy_infrastructure_init();
> > +
> > +    if (qemu_init_main_loop(&main_loop_err)) {
> > +        error_report_err(main_loop_err);
> > +        exit(1);
> > +    }
> >      monitor_init_globals();
> 
> This is a tiny bit scary, as we now have around 1kloc of code between
> here and os_daemonize() where in the future we may accidentally cause
> the aio context's on-demand thread pool to spawn before fork()ing
> (silently losing the threads again - we did have such an issue right
> there in monitor_init_globals() in the past)

I don't think it's that bad, because while it is many lines, it's just
boring option parsing code. I certainly don't think it's likely to start
using a thread pool anywhere.

However, I also wonder now if this patch is actually necessary. Maybe
some intermediate version of the series actually used qemu_aio_context
in monitor_init_globals_core(), but none of the versions actually sent
to the list do. They all use iohandler_get_aio_context(), which should
be safe before the main loop is initialised. To make sure that I didn't
miss anything, 'make check' passes without the patch.

So maybe we can just drop this patch after all.

Kevin