[Qemu-devel] [PATCH v3 0/2] fix -nodefaults and -daemonize regressions caused by --preconfig introduction

[Qemu-devel] [PATCH v3 0/2] fix -nodefaults and -daemonize regressions caused by --preconfig introduction

[Igor Mammedov]

Commit 047f7038f586d2150f16c6d9ba9cfd0479f0f6ac broke iotests that were using
 -nodefault option, which lead to hang in the early main_loop_wait().
1/2 fixes it by not calling main_loop_wait() unless --preconfig option was on CLI.
1/2 also fixes issue where libvirt starting qemu as daemon waits on qemu parent
process to exit which doens't happen at the early main_loop().
2/2 fixes the same deamon issue but with --preconfig option on CLI

With this QEMU passes make check, make check-block and manual testing with
-daemonize

CC: berrange@redhat.com
CC: mreitz@redhat.com
CC: pbonzini@redhat.com
CC: ehabkost@redhat.com
CC: ldoktor@redhat.com

Igor Mammedov (2):
  cli: Don't run early event loop if no  --preconfig was specified
  vl: fix use of --daemonize with --preconfig

 os-posix.c | 6 ++++++
 vl.c       | 6 +++---
 2 files changed, 9 insertions(+), 3 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH v3 1/2] cli: Don't run early event loop if no --preconfig was specified

[Igor Mammedov]

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_wait() unless --preconfig was
specified (in which case caller knows they must connect to the
socket before exec() finishes).

Patch also fixes hang when -nodefaults option is used, which were
causing QEMU hang in the early main_loop_wait() indefinitely by
the same means (not calling main_loop_wait() unless --preconfig
is present on CLI)

Based on
  From: Michal Privoznik <mprivozn@redhat.com>
  Subject: [PATCH] cli: Don't run early event loop if no --preconfig was specified
  Message-Id: <ad910973c593c5ac2fed3a10ea958f7e9c12f82c.1527935663.git.mprivozn@redhat.com>
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v2:
  - fix iotests that involve migration,
    rewrite v1 so it would take into account -incoming use case
---
 vl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

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());
+    }
 }
 
 static void version(void)
-- 
2.7.4

Re: [Qemu-devel] [PATCH v3 1/2] cli: Don't run early event loop if no --preconfig was specified

[Eduardo Habkost]

On Tue, Jun 05, 2018 at 04:00:42PM +0200, Igor Mammedov wrote:
[...]
> Based on
>   From: Michal Privoznik <mprivozn@redhat.com>
>   Subject: [PATCH] cli: Don't run early event loop if no --preconfig was specified
>   Message-Id: <ad910973c593c5ac2fed3a10ea958f7e9c12f82c.1527935663.git.mprivozn@redhat.com>

Michal's patch is already queued on machine-next.  It should be
dropped if this patch gets included, correct?

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 1/2] cli: Don't run early event loop if no --preconfig was specified

[Igor Mammedov]

On Tue, 5 Jun 2018 15:12:46 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Tue, Jun 05, 2018 at 04:00:42PM +0200, Igor Mammedov wrote:
> [...]
> > Based on
> >   From: Michal Privoznik <mprivozn@redhat.com>
> >   Subject: [PATCH] cli: Don't run early event loop if no --preconfig was specified
> >   Message-Id: <ad910973c593c5ac2fed3a10ea958f7e9c12f82c.1527935663.git.mprivozn@redhat.com>  
> 
> Michal's patch is already queued on machine-next.  It should be
> dropped if this patch gets included, correct?
> 
yep, it should be dropped as it has migrations issues.

Re: [Qemu-devel] [PATCH v3 1/2] cli: Don't run early event loop if no --preconfig was specified

[Eduardo Habkost]

On Wed, Jun 06, 2018 at 09:22:47AM +0200, Igor Mammedov wrote:
> On Tue, 5 Jun 2018 15:12:46 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Tue, Jun 05, 2018 at 04:00:42PM +0200, Igor Mammedov wrote:
> > [...]
> > > Based on
> > >   From: Michal Privoznik <mprivozn@redhat.com>
> > >   Subject: [PATCH] cli: Don't run early event loop if no --preconfig was specified
> > >   Message-Id: <ad910973c593c5ac2fed3a10ea958f7e9c12f82c.1527935663.git.mprivozn@redhat.com>  
> > 
> > Michal's patch is already queued on machine-next.  It should be
> > dropped if this patch gets included, correct?
> > 
> yep, it should be dropped as it has migrations issues.

OK, previous patch dequeued, and this one queued.  It will be on
a pull request today.

-- 
Eduardo

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

[Igor Mammedov]

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_wait() 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.

Based on:
  From: Daniel P. Berrangé <berrange@redhat.com>
  Subject: [PATCH v2 2/2] vl: fix use of --daemonize with --preconfig
  Message-Id: <20180604120345.12955-3-berrange@redhat.com>

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v3:
  - rewrite to apply on top of 1/2
---
 os-posix.c | 6 ++++++
 vl.c       | 2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/os-posix.c b/os-posix.c
index 9ce6f74..ee06a8d 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 = false;
     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..d6fa67f 100644
--- a/vl.c
+++ b/vl.c
@@ -1960,6 +1960,7 @@ static void main_loop(void)
 #ifdef CONFIG_PROFILER
         ti = profile_getclock();
 #endif
+        os_setup_post();
         main_loop_wait(false);
 #ifdef CONFIG_PROFILER
         dev_time += profile_getclock() - ti;
@@ -4707,7 +4708,6 @@ int main(int argc, char **argv, char **envp)
     }
 
     accel_setup_post(current_machine);
-    os_setup_post();
 
     main_loop();
 
-- 
2.7.4

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

[Eric Blake]

On 06/05/2018 09:00 AM, 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_wait() 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.
> 
> Based on:
>    From: Daniel P. Berrangé <berrange@redhat.com>
>    Subject: [PATCH v2 2/2] vl: fix use of --daemonize with --preconfig
>    Message-Id: <20180604120345.12955-3-berrange@redhat.com>
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> v3:
>    - rewrite to apply on top of 1/2
> ---
>   os-posix.c | 6 ++++++
>   vl.c       | 2 +-
>   2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/os-posix.c b/os-posix.c
> index 9ce6f74..ee06a8d 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 = false;

The '= false' is not technically necessary; all static variables default 
to zero-initialization.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

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

[Igor Mammedov]

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_wait() 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.

Based on:
  From: Daniel P. Berrangé <berrange@redhat.com>
  Subject: [PATCH v2 2/2] vl: fix use of --daemonize with --preconfig
  Message-Id: <20180604120345.12955-3-berrange@redhat.com>

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v3:
  - rewrite to apply on top of 1/2
v4:
  - do not init static boolean to false as it's aready false
    (Eric Blake <eblake@redhat.com>)

CC: berrange@redhat.com
CC: mreitz@redhat.com
CC: pbonzini@redhat.com
CC: ehabkost@redhat.com
CC: ldoktor@redhat.com
CC: eblake@redhat.com

---
 os-posix.c | 6 ++++++
 vl.c       | 2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)

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..d6fa67f 100644
--- a/vl.c
+++ b/vl.c
@@ -1960,6 +1960,7 @@ static void main_loop(void)
 #ifdef CONFIG_PROFILER
         ti = profile_getclock();
 #endif
+        os_setup_post();
         main_loop_wait(false);
 #ifdef CONFIG_PROFILER
         dev_time += profile_getclock() - ti;
@@ -4707,7 +4708,6 @@ int main(int argc, char **argv, char **envp)
     }
 
     accel_setup_post(current_machine);
-    os_setup_post();
 
     main_loop();
 
-- 
2.7.4

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

[Eduardo Habkost]

On Tue, Jun 05, 2018 at 04:00:43PM +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_wait() 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.
> 
> Based on:
>   From: Daniel P. Berrangé <berrange@redhat.com>
>   Subject: [PATCH v2 2/2] vl: fix use of --daemonize with --preconfig
>   Message-Id: <20180604120345.12955-3-berrange@redhat.com>
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> v3:
>   - rewrite to apply on top of 1/2
> ---
>  os-posix.c | 6 ++++++
>  vl.c       | 2 +-
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/os-posix.c b/os-posix.c
> index 9ce6f74..ee06a8d 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 = false;
>      int fd = 0;
>  
> +    if (os_setup_post_done) {
> +        return;
> +    }
> +    os_setup_post_done = true;
> +

This part is nice because it allows the os_setup_post() call in
the second main loop to be unconditional, but:

>      if (daemonize) {
>          if (chdir("/")) {
>              error_report("not able to chdir to /: %s", strerror(errno));
> diff --git a/vl.c b/vl.c
> index fa44138..d6fa67f 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1960,6 +1960,7 @@ static void main_loop(void)
>  #ifdef CONFIG_PROFILER
>          ti = profile_getclock();
>  #endif
> +        os_setup_post();
>          main_loop_wait(false);

Ensuring the correctness of this os_setup_post() call depends on
reading the whole body of main_loop_should_exit(), which is a
complex and large function.  I think this is too fragile for my
taste.

I prefer Daniel's approach where we have two
os_setup_post()/main_loop() call sites, and the first one is
conditional on --preconfig.


>  #ifdef CONFIG_PROFILER
>          dev_time += profile_getclock() - ti;
> @@ -4707,7 +4708,6 @@ int main(int argc, char **argv, char **envp)
>      }
>  
>      accel_setup_post(current_machine);
> -    os_setup_post();
>  
>      main_loop();
>  
> -- 
> 2.7.4
> 

-- 
Eduardo

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

[Igor Mammedov]

On Tue, 5 Jun 2018 15:30:01 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Tue, Jun 05, 2018 at 04:00:43PM +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_wait() 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.
> > 
> > Based on:
> >   From: Daniel P. Berrangé <berrange@redhat.com>
> >   Subject: [PATCH v2 2/2] vl: fix use of --daemonize with --preconfig
> >   Message-Id: <20180604120345.12955-3-berrange@redhat.com>
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > v3:
> >   - rewrite to apply on top of 1/2
> > ---
> >  os-posix.c | 6 ++++++
> >  vl.c       | 2 +-
> >  2 files changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/os-posix.c b/os-posix.c
> > index 9ce6f74..ee06a8d 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 = false;
> >      int fd = 0;
> >  
> > +    if (os_setup_post_done) {
> > +        return;
> > +    }
> > +    os_setup_post_done = true;
> > +  
> 
> This part is nice because it allows the os_setup_post() call in
> the second main loop to be unconditional, but:
> 
> >      if (daemonize) {
> >          if (chdir("/")) {
> >              error_report("not able to chdir to /: %s", strerror(errno));
> > diff --git a/vl.c b/vl.c
> > index fa44138..d6fa67f 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -1960,6 +1960,7 @@ static void main_loop(void)
> >  #ifdef CONFIG_PROFILER
> >          ti = profile_getclock();
> >  #endif
> > +        os_setup_post();
> >          main_loop_wait(false);  
> 
> Ensuring the correctness of this os_setup_post() call depends on
> reading the whole body of main_loop_should_exit(), which is a
> complex and large function.  I think this is too fragile for my
> taste.
Fragility was the reason why I moved it into main_loop(),
as os_setup_post() was already overlooked once, since
one would have to make very non-obvious connection with
libvirt requirement to call it before main_loop_wait()
This way call to os_setup_post() will not be forgotten,
and would get an attention whenever main_loop() is concerned.


> I prefer Daniel's approach where we have two
> os_setup_post()/main_loop() call sites, and the first one is
> conditional on --preconfig.
Considering we are unlikely to add one more invocation of main_loop().
I'll post here Daniel's version that applies on top of 1/2 with
a comment so we won't forget about libvirt's requirement
(not the best way to write something robust but better then nothing).
So pick whatever variant would seem the best.


> >  #ifdef CONFIG_PROFILER
> >          dev_time += profile_getclock() - ti;
> > @@ -4707,7 +4708,6 @@ int main(int argc, char **argv, char **envp)
> >      }
> >  
> >      accel_setup_post(current_machine);
> > -    os_setup_post();
> >  
> >      main_loop();
> >  
> > -- 
> > 2.7.4
> >   
> 

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

[Igor Mammedov]

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é <berrange@redhat.com>
Signed-off-by: Igor Mammedov <imammedo@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.

CC: berrange@redhat.com
CC: mreitz@redhat.com
CC: pbonzini@redhat.com
CC: ehabkost@redhat.com
CC: ldoktor@redhat.com
CC: eblake@redhat.com

---
 vl.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/vl.c b/vl.c
index fa44138..ee359e6 100644
--- a/vl.c
+++ b/vl.c
@@ -3038,6 +3038,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;
@@ -4578,6 +4579,13 @@ 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();
+        os_setup_post_done = true;
+    }
     main_loop();
 
     /* from here on runstate is RUN_STATE_PRELAUNCH */
@@ -4707,8 +4715,10 @@ int main(int argc, char **argv, char **envp)
     }
 
     accel_setup_post(current_machine);
-    os_setup_post();
 
+    if (!os_setup_post_done) {
+        os_setup_post();
+    }
     main_loop();
 
     gdbserver_cleanup();
-- 
2.7.4

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

[Eduardo Habkost]

On Wed, Jun 06, 2018 at 10:37:05AM +0200, Igor Mammedov wrote:
[...]
> @@ -4578,6 +4579,13 @@ 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();
> +        os_setup_post_done = true;
> +    }

I liked your version of os_setup_post() in v3, where the
os_setup_post_done check is done inside os_setup_post().

>      main_loop();
>  
>      /* from here on runstate is RUN_STATE_PRELAUNCH */
> @@ -4707,8 +4715,10 @@ int main(int argc, char **argv, char **envp)
>      }
>  
>      accel_setup_post(current_machine);
> -    os_setup_post();
>  
> +    if (!os_setup_post_done) {
> +        os_setup_post();
> +    }
>      main_loop();
>  
>      gdbserver_cleanup();
> -- 
> 2.7.4
> 

-- 
Eduardo

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

[Igor Mammedov]

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é <berrange@redhat.com>
Signed-off-by: Igor Mammedov <imammedo@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: berrange@redhat.com
CC: mreitz@redhat.com
CC: pbonzini@redhat.com
CC: ehabkost@redhat.com
CC: ldoktor@redhat.com
CC: eblake@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();
+    }
     main_loop();
 
     /* from here on runstate is RUN_STATE_PRELAUNCH */
-- 
2.7.4

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

[Eduardo Habkost]

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é <berrange@redhat.com>
> Signed-off-by: Igor Mammedov <imammedo@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: berrange@redhat.com
> CC: mreitz@redhat.com
> CC: pbonzini@redhat.com
> CC: ehabkost@redhat.com
> CC: ldoktor@redhat.com
> CC: eblake@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
* be unable to finish machine initialization because of
  chdir("/"), change_root(), or change_process_uid().

Doesn't this make -preconfig and -daemonize fundamentally
incompatible?


>      main_loop();
>  
>      /* from here on runstate is RUN_STATE_PRELAUNCH */
> -- 
> 2.7.4
> 
> 

-- 
Eduardo

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

[Igor Mammedov]

On Fri, 8 Jun 2018 10:21:05 -0300
Eduardo Habkost <ehabkost@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é <berrange@redhat.com>
> > Signed-off-by: Igor Mammedov <imammedo@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: berrange@redhat.com
> > CC: mreitz@redhat.com
> > CC: pbonzini@redhat.com
> > CC: ehabkost@redhat.com
> > CC: ldoktor@redhat.com
> > CC: eblake@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

> * 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.)

 * 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())

> Doesn't this make -preconfig and -daemonize fundamentally
> incompatible?
Don't see anything that prevents both to work together fundamentally.
essentially -preconfig is extra configuration after CLI,
we potentially would be able execute commands that open files there,
so we should leave chroot & co where it is now.

> 
> 
> >      main_loop();
> >  
> >      /* from here on runstate is RUN_STATE_PRELAUNCH */
> > -- 
> > 2.7.4
> > 
> >   
> 

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 <ehabkost@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é <berrange@redhat.com>
> > > Signed-off-by: Igor Mammedov <imammedo@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: berrange@redhat.com
> > > CC: mreitz@redhat.com
> > > CC: pbonzini@redhat.com
> > > CC: ehabkost@redhat.com
> > > CC: ldoktor@redhat.com
> > > CC: eblake@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

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

[Igor Mammedov]

On Mon, 11 Jun 2018 16:06:07 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Mon, Jun 11, 2018 at 03:16:25PM +0200, Igor Mammedov wrote:
> > On Fri, 8 Jun 2018 10:21:05 -0300
> > Eduardo Habkost <ehabkost@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é <berrange@redhat.com>
> > > > Signed-off-by: Igor Mammedov <imammedo@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: berrange@redhat.com
> > > > CC: mreitz@redhat.com
> > > > CC: pbonzini@redhat.com
> > > > CC: ehabkost@redhat.com
> > > > CC: ldoktor@redhat.com
> > > > CC: eblake@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.
Is there any reason why exit() can't be called?

> > > * 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?
I'd think that in future we would want to enable object_add in preconfig
to create backends at runtime, so yes we can't do chroot at this point
 

> >  * 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.
it's already separated and that's what creates one side of problem.
What I suggest is to leave it as is and move out only
  len = write(daemon_pipe, &status, 1);
part of os_setup_post() to sync with parent process. That shouldn't
affect daemonizing flow on QEMU side and would let libvirt reuse parent's
exit as sync point to detect moment when monitor is available.
(patch is in testing, I'll post it tomorrow if it doesn't break tests)

In worst case if we can't do the later in QEMU, mgmt would have to cope with
monitor in preconfig mode without relying on parent exit(0) sync point.
(a typical daemon would fork/chroot and co in one place and clients would use
other means to detect socket availability other than watching parent process
exiting)

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

[Eduardo Habkost]

CCing libvir-list.

On Mon, Jun 11, 2018 at 11:29:24PM +0200, Igor Mammedov wrote:
> On Mon, 11 Jun 2018 16:06:07 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Mon, Jun 11, 2018 at 03:16:25PM +0200, Igor Mammedov wrote:
> > > On Fri, 8 Jun 2018 10:21:05 -0300
> > > Eduardo Habkost <ehabkost@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é <berrange@redhat.com>
> > > > > Signed-off-by: Igor Mammedov <imammedo@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: berrange@redhat.com
> > > > > CC: mreitz@redhat.com
> > > > > CC: pbonzini@redhat.com
> > > > > CC: ehabkost@redhat.com
> > > > > CC: ldoktor@redhat.com
> > > > > CC: eblake@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.
> Is there any reason why exit() can't be called?

QMP clients don't expect the QMP socket to be closed except when
using the 'quit' command.

> 
> > > > * 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?
> I'd think that in future we would want to enable object_add in preconfig
> to create backends at runtime, so yes we can't do chroot at this point
>  
> 
> > >  * 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.
> it's already separated and that's what creates one side of problem.

Is it?  Right now '$QEMU -daemonize' will never call exit(0)
before the child process it spawned did the
chdir()/stdout/stderr/etc trick.


> What I suggest is to leave it as is and move out only
>   len = write(daemon_pipe, &status, 1);
> part of os_setup_post() to sync with parent process. That shouldn't
> affect daemonizing flow on QEMU side and would let libvirt reuse parent's
> exit as sync point to detect moment when monitor is available.
> (patch is in testing, I'll post it tomorrow if it doesn't break tests)

This will affect the daemonizing flow, won't it?  It will make
QEMU exit(0) before the child does the chdir()/stderr/stdout/etc
cleanup.  A well-behaved daemon shouldn't do this.

This is probably not a problem for libvirt (which only uses
-daemonize as a sync point for QMP), but possibly a problem for
other users of -daemonize.

> 
> In worst case if we can't do the later in QEMU, mgmt would have to cope with
> monitor in preconfig mode without relying on parent exit(0) sync point.
> (a typical daemon would fork/chroot and co in one place and clients would use
> other means to detect socket availability other than watching parent process
> exiting)

Do we really need to make -daemonize and -preconfig work
together?  libvirt uses -daemonize only for its initial
capability probing, which shouldn't require -preconfig at all.

Even on that case, I wonder why libvirt doesn't simply create a
server socket and waits for QEMU to connect instead of using
-daemonize as a sync point.

-- 
Eduardo

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

[Michal Privoznik]

On 06/12/2018 12:36 AM, Eduardo Habkost wrote:
> CCing libvir-list.
> 
> On Mon, Jun 11, 2018 at 11:29:24PM +0200, Igor Mammedov wrote:
>> On Mon, 11 Jun 2018 16:06:07 -0300
>> Eduardo Habkost <ehabkost@redhat.com> wrote:
>>
>>> On Mon, Jun 11, 2018 at 03:16:25PM +0200, Igor Mammedov wrote:
>>>> On Fri, 8 Jun 2018 10:21:05 -0300
>>>> Eduardo Habkost <ehabkost@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é <berrange@redhat.com>
>>>>>> Signed-off-by: Igor Mammedov <imammedo@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: berrange@redhat.com
>>>>>> CC: mreitz@redhat.com
>>>>>> CC: pbonzini@redhat.com
>>>>>> CC: ehabkost@redhat.com
>>>>>> CC: ldoktor@redhat.com
>>>>>> CC: eblake@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.
>> Is there any reason why exit() can't be called?
> 
> QMP clients don't expect the QMP socket to be closed except when
> using the 'quit' command.

Libvirt views HANGUP on monitor socket as qemu process dying
unexpectedly so it starts cleanup (which involves 'kill -9' of qemu).

> 
>>
>>>>> * 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?
>> I'd think that in future we would want to enable object_add in preconfig
>> to create backends at runtime, so yes we can't do chroot at this point
>>  
>>
>>>>  * 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.
>> it's already separated and that's what creates one side of problem.
> 
> Is it?  Right now '$QEMU -daemonize' will never call exit(0)
> before the child process it spawned did the
> chdir()/stdout/stderr/etc trick.
> 
> 
>> What I suggest is to leave it as is and move out only
>>   len = write(daemon_pipe, &status, 1);
>> part of os_setup_post() to sync with parent process. That shouldn't
>> affect daemonizing flow on QEMU side and would let libvirt reuse parent's
>> exit as sync point to detect moment when monitor is available.
>> (patch is in testing, I'll post it tomorrow if it doesn't break tests)
> 
> This will affect the daemonizing flow, won't it?  It will make
> QEMU exit(0) before the child does the chdir()/stderr/stdout/etc
> cleanup.  A well-behaved daemon shouldn't do this.
> 
> This is probably not a problem for libvirt (which only uses
> -daemonize as a sync point for QMP), but possibly a problem for
> other users of -daemonize.

I think the proper behaviour is to exit(0) after chdir()/stderr/... AND
monitor is set up. Then, if any caller started qemu with -preconfig they
know they can start talking on monitor, set up whatever it is they want
and issue 'cont' finally (or what is the right command to exit preconfig
state). This way nothing changes for callers not using -preconfig.

> 
>>
>> In worst case if we can't do the later in QEMU, mgmt would have to cope with
>> monitor in preconfig mode without relying on parent exit(0) sync point.
>> (a typical daemon would fork/chroot and co in one place and clients would use
>> other means to detect socket availability other than watching parent process
>> exiting)
> 
> Do we really need to make -daemonize and -preconfig work
> together?  libvirt uses -daemonize only for its initial
> capability probing, which shouldn't require -preconfig at all.
> 
> Even on that case, I wonder why libvirt doesn't simply create a
> server socket and waits for QEMU to connect instead of using
> -daemonize as a sync point.
> 

because libvirt views qemu as well behaved daemon. Should anything go
wrong libvirt reads qemu's stderr and reports error to upper layers.

Michal

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

[Igor Mammedov]

On Tue, 12 Jun 2018 11:17:15 +0200
Michal Privoznik <mprivozn@redhat.com> wrote:

> On 06/12/2018 12:36 AM, Eduardo Habkost wrote:
> > CCing libvir-list.
> > 
> > On Mon, Jun 11, 2018 at 11:29:24PM +0200, Igor Mammedov wrote:  
> >> On Mon, 11 Jun 2018 16:06:07 -0300
> >> Eduardo Habkost <ehabkost@redhat.com> wrote:
> >>  
> >>> On Mon, Jun 11, 2018 at 03:16:25PM +0200, Igor Mammedov wrote:  
> >>>> On Fri, 8 Jun 2018 10:21:05 -0300
> >>>> Eduardo Habkost <ehabkost@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é <berrange@redhat.com>
> >>>>>> Signed-off-by: Igor Mammedov <imammedo@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: berrange@redhat.com
> >>>>>> CC: mreitz@redhat.com
> >>>>>> CC: pbonzini@redhat.com
> >>>>>> CC: ehabkost@redhat.com
> >>>>>> CC: ldoktor@redhat.com
> >>>>>> CC: eblake@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.  
> >> Is there any reason why exit() can't be called?  
> > 
> > QMP clients don't expect the QMP socket to be closed except when
> > using the 'quit' command.  
> 
> Libvirt views HANGUP on monitor socket as qemu process dying
> unexpectedly so it starts cleanup (which involves 'kill -9' of qemu).
So if we exit(1) there is a chance to get SIGKILL before exit(1) completes.
Do we care about it at this point?
(there are places when QEMU calls exit(1) at runtime on unrecoverable error)

> >>>>> * 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?  
> >> I'd think that in future we would want to enable object_add in preconfig
> >> to create backends at runtime, so yes we can't do chroot at this point
> >>  
> >>  
> >>>>  * 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.  
> >> it's already separated and that's what creates one side of problem.  
> > 
> > Is it?  Right now '$QEMU -daemonize' will never call exit(0)
> > before the child process it spawned did the
> > chdir()/stdout/stderr/etc trick.
> > 
> >   
> >> What I suggest is to leave it as is and move out only
> >>   len = write(daemon_pipe, &status, 1);
> >> part of os_setup_post() to sync with parent process. That shouldn't
> >> affect daemonizing flow on QEMU side and would let libvirt reuse parent's
> >> exit as sync point to detect moment when monitor is available.
> >> (patch is in testing, I'll post it tomorrow if it doesn't break tests)  
> > 
> > This will affect the daemonizing flow, won't it?  It will make
> > QEMU exit(0) before the child does the chdir()/stderr/stdout/etc
> > cleanup.  A well-behaved daemon shouldn't do this.
> > 
> > This is probably not a problem for libvirt (which only uses
> > -daemonize as a sync point for QMP), but possibly a problem for
> > other users of -daemonize.  
> 
> I think the proper behaviour is to exit(0) after chdir()/stderr/... AND
> monitor is set up. Then, if any caller started qemu with -preconfig they
> know they can start talking on monitor, set up whatever it is they want
> and issue 'cont' finally (or what is the right command to exit preconfig
> state). This way nothing changes for callers not using -preconfig.
As pointed out earlier we need -preconfig stay before chdir/chroot/chuid/stderr/stdout
are called, so it would have the same access rights/permissions for configuration
as CLI options.

> >> In worst case if we can't do the later in QEMU, mgmt would have to cope with
> >> monitor in preconfig mode without relying on parent exit(0) sync point.
> >> (a typical daemon would fork/chroot and co in one place and clients would use
> >> other means to detect socket availability other than watching parent process
> >> exiting)  
> > 
> > Do we really need to make -daemonize and -preconfig work
> > together?  libvirt uses -daemonize only for its initial
> > capability probing, which shouldn't require -preconfig at all.
> > 
> > Even on that case, I wonder why libvirt doesn't simply create a
> > server socket and waits for QEMU to connect instead of using
> > -daemonize as a sync point.
> >   
> 
> because libvirt views qemu as well behaved daemon. Should anything go
> wrong libvirt reads qemu's stderr and reports error to upper layers.
We can keep daemonizing flow in QEMU as it's now.
But Eduardo's idea about libvirt created socked + letting QEMU connect to it
has a merit. It should fix current deadlock issue with as monitor
won't be depending on lead exit event.
Can we do this way on libvirt side when --preconfig is in use
(it might even be fine for normal flow without -preconfig)?

> Michal

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

[Daniel P. Berrangé]

On Tue, Jun 12, 2018 at 02:42:05PM +0200, Igor Mammedov wrote:
> We can keep daemonizing flow in QEMU as it's now.
> But Eduardo's idea about libvirt created socked + letting QEMU connect to it
> has a merit. It should fix current deadlock issue with as monitor
> won't be depending on lead exit event.

NB, libvirt only ever uses --daemonize when probing capabilities, never
when launching QEMU for a real VM. In the latter case, we now use FD
passing, so libvirt opens the UNIX domain socket listener, and passes
this into QEMU. So libvirt knows it can connect to the listener
immediately and will only ever get a failure if QEMU has exited.

We can't use FD passing for the capabilities probing because of the
chicken & egg problem - we need to probe capabilities to find out
if FD passing it available or not. Fortunately with capabilities
probing, we don't care about using --preconfig, as were not running
a real VM

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

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

[Eduardo Habkost]

On Tue, Jun 12, 2018 at 01:50:33PM +0100, Daniel P. Berrangé wrote:
> On Tue, Jun 12, 2018 at 02:42:05PM +0200, Igor Mammedov wrote:
> > We can keep daemonizing flow in QEMU as it's now.
> > But Eduardo's idea about libvirt created socked + letting QEMU connect to it
> > has a merit. It should fix current deadlock issue with as monitor
> > won't be depending on lead exit event.
> 
> NB, libvirt only ever uses --daemonize when probing capabilities, never
> when launching QEMU for a real VM. In the latter case, we now use FD
> passing, so libvirt opens the UNIX domain socket listener, and passes
> this into QEMU. So libvirt knows it can connect to the listener
> immediately and will only ever get a failure if QEMU has exited.

So, what I'm really missing here is: do we have a good reason to
support --daemonize + --preconfig today?

The options I see are:

1) complete daemonization before preconfig main loop
----------------------------------------------------

By "complete daemonization" I mean doing chdir("/"),
stderr/stdout cleanup, chroot, and UID magic before calling
exit(0) on the main QEMU process.

Pros:
* More intuitive

Cons:
* Can break existing initialization code that don't expect
  this to happen.
  (can this be fixed?)
* Can break any preconfig-time QMP commands that rely on opening
  files
  (is it a real problem?)
* Any initialization error conditions that currently rely on
  error_report()+exit() will be very inconvenient to handle
  properly
  (this can be fixed eventually, but it's not trivial)


2) incomplete daemonization before preconfig main loop
------------------------------------------------------

This means calling exit(0) on the main process before doing the
chdir/stderr/etc magic.

Pros:
* Less likely to break initialization code and other QMP commands

Cons:
* Not what's expected from a well-behaved daemon.
  (If we're not daemonizing properly, what's the point of using
  -daemonize at all?)
* More difficult to change behavior later.


3) daemonize only after leaving preconfig state
-----------------------------------------------

AFAICS, this is the current behavior.

Pros:
* Less likely to break init code
* Keeps existing code as is

Cons:
* Less intuitive
* -daemonize becomes useless as synchronization point for monitor
  availability
* Would this be useful for anybody, at all?
* We won't be able to change this behavior later


4) Not supporting -preconfig + -daemonize
-----------------------------------------

Pros:
* Simple to implement.
* Avoids unexpected bugs.
* Saves our time.
* We can change this behavior later.

Cons:
* People might want us to support it eventually.



I believe the only reasonable options are (1) and (4).

-- 
Eduardo

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

[Daniel P. Berrangé]

On Wed, Jun 13, 2018 at 11:17:30AM -0300, Eduardo Habkost wrote:
> On Tue, Jun 12, 2018 at 01:50:33PM +0100, Daniel P. Berrangé wrote:
> > On Tue, Jun 12, 2018 at 02:42:05PM +0200, Igor Mammedov wrote:
> > > We can keep daemonizing flow in QEMU as it's now.
> > > But Eduardo's idea about libvirt created socked + letting QEMU connect to it
> > > has a merit. It should fix current deadlock issue with as monitor
> > > won't be depending on lead exit event.
> > 
> > NB, libvirt only ever uses --daemonize when probing capabilities, never
> > when launching QEMU for a real VM. In the latter case, we now use FD
> > passing, so libvirt opens the UNIX domain socket listener, and passes
> > this into QEMU. So libvirt knows it can connect to the listener
> > immediately and will only ever get a failure if QEMU has exited.
> 
> So, what I'm really missing here is: do we have a good reason to
> support --daemonize + --preconfig today?

On the libvirt zero, I don't see a compelling need for it.

> The options I see are:
> 
> 1) complete daemonization before preconfig main loop
> ----------------------------------------------------
> 
> By "complete daemonization" I mean doing chdir("/"),
> stderr/stdout cleanup, chroot, and UID magic before calling
> exit(0) on the main QEMU process.
> 
> Pros:
> * More intuitive
> 
> Cons:
> * Can break existing initialization code that don't expect
>   this to happen.
>   (can this be fixed?)
> * Can break any preconfig-time QMP commands that rely on opening
>   files
>   (is it a real problem?)

NB Use of -chroot is separate from -daemonize, so it is not
an issue with -preconfig + -daemonize alone.

There's soo many caveats around -chroot, I struggle to
care about adding another caveats.

> * Any initialization error conditions that currently rely on
>   error_report()+exit() will be very inconvenient to handle
>   properly
>   (this can be fixed eventually, but it's not trivial)


> 3) daemonize only after leaving preconfig state
> -----------------------------------------------
> 
> AFAICS, this is the current behavior.
> 
> Pros:
> * Less likely to break init code
> * Keeps existing code as is
> 
> Cons:
> * Less intuitive
> * -daemonize becomes useless as synchronization point for monitor
>   availability

Yeah that honestly kills the key benefit of having -daemonize
imho.

> * Would this be useful for anybody, at all?
> * We won't be able to change this behavior later
> 
> 

> I believe the only reasonable options are (1) and (4).

Agreed.

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

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

[Eduardo Habkost]

On Wed, Jun 13, 2018 at 03:23:09PM +0100, Daniel P. Berrangé wrote:
> On Wed, Jun 13, 2018 at 11:17:30AM -0300, Eduardo Habkost wrote:
> > On Tue, Jun 12, 2018 at 01:50:33PM +0100, Daniel P. Berrangé wrote:
> > > On Tue, Jun 12, 2018 at 02:42:05PM +0200, Igor Mammedov wrote:
> > > > We can keep daemonizing flow in QEMU as it's now.
> > > > But Eduardo's idea about libvirt created socked + letting QEMU connect to it
> > > > has a merit. It should fix current deadlock issue with as monitor
> > > > won't be depending on lead exit event.
> > > 
> > > NB, libvirt only ever uses --daemonize when probing capabilities, never
> > > when launching QEMU for a real VM. In the latter case, we now use FD
> > > passing, so libvirt opens the UNIX domain socket listener, and passes
> > > this into QEMU. So libvirt knows it can connect to the listener
> > > immediately and will only ever get a failure if QEMU has exited.
> > 
> > So, what I'm really missing here is: do we have a good reason to
> > support --daemonize + --preconfig today?
> 
> On the libvirt zero, I don't see a compelling need for it.

Good. :)

> > The options I see are:
> > 1) complete daemonization before preconfig main loop
[...]
> > 4) Not supporting -preconfig + -daemonize
[...]
> > I believe the only reasonable options are (1) and (4).
> 
> Agreed.

If it was up to me, I would just go with (4) because it's
simpler.

But if somebody wants to implement (1), the caveats should be
clearly documented.  I would prefer to simply document
"--daemonize --preconfig" as experimental, with something like:

  "Note: usage of --daemonize with the --preconfig option is
  experimental, because it can prevent QEMU from reporting
  machine initialization errors and prevent some features from
  working after QEMU is daemonized."

-- 
Eduardo

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

[Igor Mammedov]

On Wed, 13 Jun 2018 14:09:32 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Wed, Jun 13, 2018 at 03:23:09PM +0100, Daniel P. Berrangé wrote:
> > On Wed, Jun 13, 2018 at 11:17:30AM -0300, Eduardo Habkost wrote:  
> > > On Tue, Jun 12, 2018 at 01:50:33PM +0100, Daniel P. Berrangé wrote:  
> > > > On Tue, Jun 12, 2018 at 02:42:05PM +0200, Igor Mammedov wrote:  
> > > > > We can keep daemonizing flow in QEMU as it's now.
> > > > > But Eduardo's idea about libvirt created socked + letting QEMU connect to it
> > > > > has a merit. It should fix current deadlock issue with as monitor
> > > > > won't be depending on lead exit event.  
> > > > 
> > > > NB, libvirt only ever uses --daemonize when probing capabilities, never
> > > > when launching QEMU for a real VM. In the latter case, we now use FD
> > > > passing, so libvirt opens the UNIX domain socket listener, and passes
> > > > this into QEMU. So libvirt knows it can connect to the listener
> > > > immediately and will only ever get a failure if QEMU has exited.  
> > > 
> > > So, what I'm really missing here is: do we have a good reason to
> > > support --daemonize + --preconfig today?  
> > 
> > On the libvirt zero, I don't see a compelling need for it.  
> 
> Good. :)
> 
> > > The options I see are:
> > > 1) complete daemonization before preconfig main loop  
> [...]
> > > 4) Not supporting -preconfig + -daemonize  
> [...]
> > > I believe the only reasonable options are (1) and (4).  
> > 
> > Agreed.  
> 
> If it was up to me, I would just go with (4) because it's
> simpler.
Let's just disable it for now. it will be easier to allow it
than take it back later.

> 
> But if somebody wants to implement (1), the caveats should be
> clearly documented.  I would prefer to simply document
> "--daemonize --preconfig" as experimental, with something like:
> 
>   "Note: usage of --daemonize with the --preconfig option is
>   experimental, because it can prevent QEMU from reporting
>   machine initialization errors and prevent some features from
>   working after QEMU is daemonized."

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

[Michal Privoznik]

On 06/12/2018 02:42 PM, Igor Mammedov wrote:

>>>
>>> Do we really need to make -daemonize and -preconfig work
>>> together?  libvirt uses -daemonize only for its initial
>>> capability probing, which shouldn't require -preconfig at all.
>>>
>>> Even on that case, I wonder why libvirt doesn't simply create a
>>> server socket and waits for QEMU to connect instead of using
>>> -daemonize as a sync point.
>>>   
>>
>> because libvirt views qemu as well behaved daemon. Should anything go
>> wrong libvirt reads qemu's stderr and reports error to upper layers.
> We can keep daemonizing flow in QEMU as it's now.
> But Eduardo's idea about libvirt created socked + letting QEMU connect to it
> has a merit. It should fix current deadlock issue with as monitor
> won't be depending on lead exit event.

Not sure about the benefits. Currently, libvirt spawns qemu, waits for
monitor to show up (currently, the timeout dynamic depending on some
black magic involving guest RAM size) and if it does not show up in time
it kills qemu. The same algorithm must be kept in place even for case
when libvirt would pass pre-opened socket to qemu in case qemu deadlocks
before being able to communicate over qmp. The only advantage I see is
libvirt would not need to label the socket (set uid:gid, selinux, ...).
On the other hand, since it would be libvirt creating the socket what
would happen on libvirtd restart?

> Can we do this way on libvirt side when --preconfig is in use
> (it might even be fine for normal flow without -preconfig)?

I think passing pre-opened socket and --preconfig are orthogonal. What
if somebody wants to use --preconfig does not pass any FD?

Michal

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

[Peter Krempa]

[-- Attachment #1: Type: text/plain, Size: 1834 bytes --]

On Tue, Jun 12, 2018 at 15:04:42 +0200, Michal Privoznik wrote:
> On 06/12/2018 02:42 PM, Igor Mammedov wrote:
> 
> >>>
> >>> Do we really need to make -daemonize and -preconfig work
> >>> together?  libvirt uses -daemonize only for its initial
> >>> capability probing, which shouldn't require -preconfig at all.
> >>>
> >>> Even on that case, I wonder why libvirt doesn't simply create a
> >>> server socket and waits for QEMU to connect instead of using
> >>> -daemonize as a sync point.
> >>>   
> >>
> >> because libvirt views qemu as well behaved daemon. Should anything go
> >> wrong libvirt reads qemu's stderr and reports error to upper layers.
> > We can keep daemonizing flow in QEMU as it's now.
> > But Eduardo's idea about libvirt created socked + letting QEMU connect to it
> > has a merit. It should fix current deadlock issue with as monitor
> > won't be depending on lead exit event.
> 
> Not sure about the benefits. Currently, libvirt spawns qemu, waits for
> monitor to show up (currently, the timeout dynamic depending on some
> black magic involving guest RAM size) and if it does not show up in time
> it kills qemu. The same algorithm must be kept in place even for case
> when libvirt would pass pre-opened socket to qemu in case qemu deadlocks
> before being able to communicate over qmp. The only advantage I see is
> libvirt would not need to label the socket (set uid:gid, selinux, ...).
> On the other hand, since it would be libvirt creating the socket what
> would happen on libvirtd restart?

Well, if qemu deadlocks just after spewing out the monitor greeting you
end up in the same situation as the timeout code is not applied later
for regular monitor communication.

Depending on how early the preconfig state happens, keeping in the
timeout may be pointless.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Daniel P. Berrangé]

On Tue, Jun 12, 2018 at 03:04:42PM +0200, Michal Privoznik wrote:
> On 06/12/2018 02:42 PM, Igor Mammedov wrote:
> 
> >>>
> >>> Do we really need to make -daemonize and -preconfig work
> >>> together?  libvirt uses -daemonize only for its initial
> >>> capability probing, which shouldn't require -preconfig at all.
> >>>
> >>> Even on that case, I wonder why libvirt doesn't simply create a
> >>> server socket and waits for QEMU to connect instead of using
> >>> -daemonize as a sync point.
> >>>   
> >>
> >> because libvirt views qemu as well behaved daemon. Should anything go
> >> wrong libvirt reads qemu's stderr and reports error to upper layers.
> > We can keep daemonizing flow in QEMU as it's now.
> > But Eduardo's idea about libvirt created socked + letting QEMU connect to it
> > has a merit. It should fix current deadlock issue with as monitor
> > won't be depending on lead exit event.
> 
> Not sure about the benefits. Currently, libvirt spawns qemu, waits for
> monitor to show up (currently, the timeout dynamic depending on some
> black magic involving guest RAM size) and if it does not show up in time
> it kills qemu. The same algorithm must be kept in place even for case
> when libvirt would pass pre-opened socket to qemu in case qemu deadlocks
> before being able to communicate over qmp. The only advantage I see is
> libvirt would not need to label the socket (set uid:gid, selinux, ...).

As mentioned in my other reply, we already do FD passing, and that code
has intentionally got rid of the timeout, because timeouts cause false
failures to launch QEMU. This is a particular problem when using many
disks that are encrypted, since LUKS encryption has a minimum 1 second
delay on opening each disk, so with many disks we're at risk of hitting
the timeout even when QEMU is still starting normally.

I don't see a reason to special case startup with timeouts to deal
with hangs, while ignoring the possibility of hangs after initial
startup.

> On the other hand, since it would be libvirt creating the socket what
> would happen on libvirtd restart?

We're creating a *listener* socket, not a client connection, so on
restart we simply connect again as normal.


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

Re: [Qemu-devel] [PATCH v3 0/2] fix -nodefaults and -daemonize regressions caused by --preconfig introduction

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 1528207243-268226-1-git-send-email-imammedo@redhat.com
Subject: [Qemu-devel] [PATCH v3 0/2] fix -nodefaults and -daemonize regressions caused by --preconfig introduction

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
01373291b2 vl: fix use of --daemonize with --preconfig
b6f325357d cli: Don't run early event loop if no --preconfig was specified

=== OUTPUT BEGIN ===
Checking PATCH 1/2: cli: Don't run early event loop if no --preconfig was specified...
Checking PATCH 2/2: vl: fix use of --daemonize with --preconfig...
ERROR: do not initialise statics to 0 or NULL
#44: FILE: os-posix.c:312:
+    static bool os_setup_post_done = false;

total: 1 errors, 0 warnings, 28 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com