Re: [Qemu-devel] [PATCH v2 2/2] vl: fix use of --daemonize with --preconfig

[Daniel P . Berrangé]

On Mon, Jun 04, 2018 at 11:53:15PM +0200, Igor Mammedov wrote:
> On Mon,  4 Jun 2018 13:03:45 +0100
> Daniel P. Berrangé <address@hidden> wrote:
> 
> > When using --daemonize, the initial lead process will fork a child and
> > then wait to be notified that setup is complete via a pipe, before it
> > exits.  When using --preconfig there is an extra call to main_loop()
> > before the notification is done from os_setup_post(). Thus the parent
> > process won't exit until the mgmt application connects to the monitor
> > and tells QEMU to leave the RUN_STATE_PRECONFIG. The mgmt application
> > won't connect to the monitor until daemonizing has completed though.
> > 
> > This is a chicken and egg problem, leading to deadlock at startup.
> > 
> > The only viable way to fix this is to call os_setup_post() before
> > the early main_loop() call when in RUN_STATE_PRECONFIG. This has the
> > downside that any errors from this point onwards won't be handled
> > well by the mgmt application, because it will think QEMU has started
> > successfully, so not be expecting an abrupt exit. The only way to
> > deal with that is to move as much user input validation as possible
> > to before the main_loop() call. This is left as an exercise for
> > future interested developers.
> > 
> > Signed-off-by: Daniel P. Berrangé <address@hidden>
> [...]
> 
> How about combining ideas from yours and Michal's patches?
> It should fix broken iotests/libvirt sync point and
> we can think about NONE runstate idea at without rushing it.
> If it looks acceptable, I'll post proper patches + test case for it
> after some testing (to ensure that iotests Max pointed out are working
> as expected)
> 
> diff --git a/vl.c b/vl.c
> index c4fe255..a2062d6 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1953,10 +1953,15 @@ static bool main_loop_should_exit(void)
>  
>  static void main_loop(void)
>  {
> +    static bool os_setup_post_done = false;
>  #ifdef CONFIG_PROFILER
>      int64_t ti;
>  #endif
> -    do {
> +    if (!os_setup_post_done) {
> +        os_setup_post();
> +        os_setup_post_done = true;
> +    }

I don't really like hiding the os_setup_post() call in
the main_loop() method, since they're really independant
functionality.

> +    while (!main_loop_should_exit()) {
>  #ifdef CONFIG_PROFILER
>          ti = profile_getclock();
>  #endif
> @@ -1964,7 +1969,7 @@ static void main_loop(void)
>  #ifdef CONFIG_PROFILER
>          dev_time += profile_getclock() - ti;
>  #endif
> -    } while (!main_loop_should_exit());
> +    }
>  }
>  
>  static void version(void)

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 :|