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 <mpriv...@redhat.com>
> ---
> 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 :|
