On Mon, Jun 11, 2018 at 03:16:25PM +0200, Igor Mammedov wrote:
> On Fri, 8 Jun 2018 10:21:05 -0300
> Eduardo Habkost <ehabk...@redhat.com> 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é <berra...@redhat.com>
> > > Signed-off-by: Igor Mammedov <imamm...@redhat.com>
> > > ---
> > > 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: berra...@redhat.com
> > > CC: mre...@redhat.com
> > > CC: pbonz...@redhat.com
> > > CC: ehabk...@redhat.com
> > > CC: ldok...@redhat.com
> > > CC: ebl...@redhat.com
> > > ---
> > > 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
