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

[Igor Mammedov <imammedo@redhat.com>]

On Tue, 5 Jun 2018 13:00:54 +0100
Daniel P. Berrangé <berrange@redhat.com> wrote:

> 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é <berrange@redhat.com> 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é <berrange@redhat.com>  
> > [...]
> > 
> > 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.
I've posted v3 series with this variant as both turned out
to be somewhat related and it avoids forgetting to call os_setup_post()
before main_loop() is called which is non obvious requirement from
libvirt side. Resulting code ended up  cleaner and still keeps runstate
transitions in one place main_loop_should_exit().

But I don't really have a preference here, so if we don't hide
it inside of main_loop() we need to duplicate exit condition
from main_loop_should_exit(), but that's it.
It would look a bit more messier, like:

diff --git a/vl.c b/vl.c
index d6fa67f..f1bda99 100644
--- a/vl.c
+++ b/vl.c
@@ -3039,6 +3039,7 @@ int main(int argc, char **argv, char **envp)
     Error *err = NULL;
     bool list_data_dirs = false;
     char *dir, **dirs;
+    bool os_setup_post_done = false;
     typedef struct BlockdevOptions_queue {
         BlockdevOptions *bdo;
         Location loc;
@@ -4579,7 +4580,16 @@ 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) {
+        if (runstate_check(RUN_STATE_PRECONFIG)) {
+            runstate_set(RUN_STATE_PRELAUNCH);
+        }
+        preconfig_exit_requested = false;
+    } else {
+        os_setup_post();
+        os_setup_post_done = true;
+        main_loop();
+    }
 
     /* from here on runstate is RUN_STATE_PRELAUNCH */
     machine_run_board_init(current_machine);
@@ -4709,6 +4719,9 @@ int main(int argc, char **argv, char **envp)
 
     accel_setup_post(current_machine);
 
+    if (!os_setup_post_done) {
+       os_setup_post();
+    }
     main_loop();

but if that's what is preferred I can repost v4.

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