Re: [Qemu-devel] [PATCH] cli: Don't run early event loop if no --preconfig was specified

[Daniel P . Berrangé]

On Sat, Jun 02, 2018 at 12:34:52PM +0200, Michal Privoznik wrote:
> After 047f7038f586d215 it is possible for event loop to run two
> times. First time whilst parsing command line options (the idea
> is to bring up monitor early so that management applications can
> tweak config before machine is initialized). And the second time
> is after everything is set up (this is the usual place). In both
> cases the event loop is called as main_loop_wait(nonblocking =
> false) which causes the event loop to block until at least one
> event occurred.
> 
> Now, consider that somebody (i.e. libvirt) calls us with
> -daemonize. This operation is split in two steps. The main()
> calls os_daemonize() which fork()-s and then waits in read()
> until child notifies it via write():
> 
> /qemu.git $ ./x86_64-softmmu/qemu-system-x86_64 -S -daemonize \
>   -no-user-config -nodefaults -nographic
> 
>   main():                child:
>     os_daemonize():
>       read(pipe[0])
> 
>                            main_loop():
>                              main_loop_wait(false)
> 
>                            os_setup_post():
>                              write(pipe[1])
> 
>                            main_loop():
>                              main_loop_wait(false)
> 
> Here it can be clearly seen that main() does not exit until an
> event occurs, but at the same time nobody will touch the monitor
> socket until their exec("qemu-system-*") finishes. So the whole
> thing deadlocks.
> 
> The solution is to not call main_loop() unless --preconfig was
> specified (in which case caller knows they must connect to the
> socket before exec() finishes).
> 
> Signed-off-by: Michal Privoznik <address@hidden>
> ---
>  vl.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/vl.c b/vl.c
> index 70f090c823..cde2934c40 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4469,8 +4469,13 @@ int main(int argc, char **argv, char **envp)
>      }
>      parse_numa_opts(current_machine);
>  
> -    /* do monitor/qmp handling at preconfig state if requested */
> -    main_loop();
> +    if (preconfig_exit_requested) {
> +        runstate_set(RUN_STATE_PRELAUNCH);
> +        preconfig_exit_requested = false;
> +    } else {
> +        /* do monitor/qmp handling at preconfig state if requested */
> +        main_loop();
> +    }

Avoiding the double-run of main_loop is good, however, I think we should
also not have put current_run_state in RUN_STATE_PRECONFIG in the first
place if --preconfig wasn't set.  I've sent a patch to fix that problem
too, so if yours is also applied, it could be changed to just do:

    if (current_run_state == RNU_STATE_PRECONFIG) {
        main_loop();
    }

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