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

[Igor Mammedov <imammedo@redhat.com>]

On Mon, 4 Jun 2018 16:21:48 +0200
Michal Privoznik <mprivozn@redhat.com> wrote:

> On 06/04/2018 02:03 PM, Daniel P. Berrangé 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.
not calling 1st main_loop(), solves the issue if no --preconfig
is specified like Michal has suggested. So moving os_setup_post()
earlier isn't the only option.

With Michal's patch it should work fine with old libvirt versions,
however it would mean more changes to libvirt when adding
--preconfig option handling as it would need to connect to qmp
socket earlier if the option is used.

> > 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.
Moving post board input validation might be problematic as
it might require existing board to create a device so it could verify
user provided parameters.

Does mgmt application starts QEMU with log file where QEMU would
write errors if any after os_setup_post() and would mgmt app look
into it/report from it to user if QEMU disappears?


> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  vl.c | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)  
> 
> Yup, this fixes the problem I've raised in my patch. Thanks!
I'd prefer your patch, as it doesn't break error handling,
or maybe even shorter version which keeps state transitions in one place:

diff --git a/vl.c b/vl.c
index c4fe255..fa44138 100644
--- a/vl.c
+++ b/vl.c
@@ -1956,7 +1956,7 @@ static void main_loop(void)
 #ifdef CONFIG_PROFILER
     int64_t ti;
 #endif
-    do {
+    while (!main_loop_should_exit()) {
 #ifdef CONFIG_PROFILER
         ti = profile_getclock();
 #endif
@@ -1964,7 +1964,7 @@ static void main_loop(void)
 #ifdef CONFIG_PROFILER
         dev_time += profile_getclock() - ti;
 #endif
-    } while (!main_loop_should_exit());
+    }
 }

 
> Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
> 
> (if my R-b line means anything here :-))
> 
> Michal