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

[Eduardo Habkost]

On Mon, Jun 11, 2018 at 03:16:25PM +0200, Igor Mammedov wrote:
> On Fri, 8 Jun 2018 10:21:05 -0300
> Eduardo Habkost <address@hidden> wrote:
> 
> > On Thu, Jun 07, 2018 at 02:00:09PM +0200, Igor Mammedov 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 --preconfig is used. 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. Moving as much user
> > > input validation as possible to before the main_loop() call might help,
> > > but mgmt application should stop assuming that QEMU has started
> > > successfuly and use other means to collect errors from QEMU (logfile).
> > > 
> > > Signed-off-by: Daniel P. Berrangé <address@hidden>
> > > Signed-off-by: Igor Mammedov <address@hidden>
> > > ---
> > > v5:
> > >   * use original Daniel's patch [1], but addapt it to apply on top of
> > >     "[PATCH v3 1/2] cli: Don't run early event loop if no  --preconfig 
> > > was specified"
> > >     with extra comment and massage commit message a little bit.
> > > v6:
> > >   * hide os_setup_post_done flag inside of os_setup_post() as it was in v4
> > > 
> > > CC: address@hidden
> > > CC: address@hidden
> > > CC: address@hidden
> > > CC: address@hidden
> > > CC: address@hidden
> > > CC: address@hidden
> > > ---
> > >  os-posix.c | 6 ++++++
> > >  vl.c       | 6 ++++++
> > >  2 files changed, 12 insertions(+)
> > > 
> > > diff --git a/os-posix.c b/os-posix.c
> > > index 9ce6f74..0246195 100644
> > > --- a/os-posix.c
> > > +++ b/os-posix.c
> > > @@ -309,8 +309,14 @@ void os_daemonize(void)
> > >  
> > >  void os_setup_post(void)
> > >  {
> > > +    static bool os_setup_post_done;
> > >      int fd = 0;
> > >  
> > > +    if (os_setup_post_done) {
> > > +        return;
> > > +    }
> > > +    os_setup_post_done = true;
> > > +
> > >      if (daemonize) {
> > >          if (chdir("/")) {
> > >              error_report("not able to chdir to /: %s", strerror(errno));
> > > diff --git a/vl.c b/vl.c
> > > index fa44138..457ff2a 100644
> > > --- a/vl.c
> > > +++ b/vl.c
> > > @@ -4578,6 +4578,12 @@ int main(int argc, char **argv, char **envp)
> > >      parse_numa_opts(current_machine);
> > >  
> > >      /* do monitor/qmp handling at preconfig state if requested */
> > > +    if (!preconfig_exit_requested && is_daemonized()) {
> > > +        /* signal parent QEMU to exit, libvirt treats it as a sign
> > > +         * that monitor socket is ready to accept connections
> > > +         */
> > > +        os_setup_post();
> > > +    }  
> > 
> > I was looking at the daemonize logic, and noticed it we have a
> > huge amount of code between this line and the next
> > os_setup_post() call that could either:
> > 
> > * call exit() and/or error_report(); or
> logging would work to the extent mentioned in commit message,
> i.e. it' would work fine when log file is used otherwise it
> errors will go to /dev/null
> 
> so it should be more or less fine on this point

My worry is that most users of error_report() involve an exit()
call too.

Once we have an active monitor, we must never call exit()
directly.  Even qmp_quit() doesn't call exit() directly.

> 
> > * be unable to finish machine initialization because of
> >   chdir("/"), change_root(), or change_process_uid().
> this one really no go.
> I see 2 options here,
> 
>  * move init code that opens files to early stage (before preconfig monitor)
>    or split it to open files early.
>    (I've spotted several obvious places fwcfg/vnc/replay_start/migration)
>    but there might be code somewhere in callbacks that would do it too,
>    so it rather risky to go this route.
>    (I'd do this anyways one place at the time using sanitizing
>     initialization sequence pretext.)

We might have QMP commands that take file paths as input, so is
this really an option?


> 
>  * split out signaling part that tells parent process to exit into
>    separate helper that's called once before/from main_loop().
>    This option seems low risk and additionally error output to
>    stderr will work as it does currently (until os_setup_post())

My assumption is that separating the chdir()/stdout/stderr logic
from the fork/daemonize/exit steps wouldn't be possible without
breaking expectations about -daemonize.

-- 
Eduardo