When using --daemonize, the initial lead process will fork a child and
then wait to be notified that setup is complete via a pipe, before it
exits. When using --preconfig there is an extra call to main_loop()
before the notification is done from os_setup_post(). Thus the parent
process won't exit until the mgmt application connects to the monitor
and tells QEMU to leave the RUN_STATE_PRECONFIG. The mgmt application
won't connect to the monitor until daemonizing has completed though.
This is a chicken and egg problem, leading to deadlock at startup.
The only viable way to fix this is to call os_setup_post() before
the early main_loop() call when in RUN_STATE_PRECONFIG. This has the
downside that any errors from this point onwards won't be handled
well by the mgmt application, because it will think QEMU has started
successfully, so not be expecting an abrupt exit. The only way to
deal with that is to move as much user input validation as possible
to before the main_loop() call. This is left as an exercise for
future interested developers.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
vl.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/vl.c b/vl.c
index 30d0e985f0..f4bba36d19 100644
--- a/vl.c
+++ b/vl.c
@@ -2921,6 +2921,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;
@@ -4476,6 +4477,8 @@ int main(int argc, char **argv, char **envp)
/* do monitor/qmp handling at preconfig state if requested */
if (runstate_check(RUN_STATE_PRECONFIG)) {
+ os_setup_post();
+ os_setup_post_done = true;
main_loop();
}
@@ -4606,7 +4609,9 @@ 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();
--
2.17.0
On 06/04/2018 02:03 PM, Daniel P. Berrangé wrote: > When using --daemonize, the initial lead process will fork a child and > then wait to be notified that setup is complete via a pipe, before it > exits. When using --preconfig there is an extra call to main_loop() > before the notification is done from os_setup_post(). Thus the parent > process won't exit until the mgmt application connects to the monitor > and tells QEMU to leave the RUN_STATE_PRECONFIG. The mgmt application > won't connect to the monitor until daemonizing has completed though. > > This is a chicken and egg problem, leading to deadlock at startup. > > The only viable way to fix this is to call os_setup_post() before > the early main_loop() call when in RUN_STATE_PRECONFIG. This has the > downside that any errors from this point onwards won't be handled > well by the mgmt application, because it will think QEMU has started > successfully, so not be expecting an abrupt exit. The only way to > deal with that is to move as much user input validation as possible > to before the main_loop() call. This is left as an exercise for > future interested developers. > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> > --- > vl.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) Yup, this fixes the problem I've raised in my patch. Thanks! Reviewed-by: Michal Privoznik <mprivozn@redhat.com> (if my R-b line means anything here :-)) Michal
On Mon, 4 Jun 2018 16:21:48 +0200
Michal Privoznik <mprivozn@redhat.com> wrote:
> On 06/04/2018 02:03 PM, Daniel P. Berrangé wrote:
> > When using --daemonize, the initial lead process will fork a child and
> > then wait to be notified that setup is complete via a pipe, before it
> > exits. When using --preconfig there is an extra call to main_loop()
> > before the notification is done from os_setup_post(). Thus the parent
> > process won't exit until the mgmt application connects to the monitor
> > and tells QEMU to leave the RUN_STATE_PRECONFIG. The mgmt application
> > won't connect to the monitor until daemonizing has completed though.
> >
> > This is a chicken and egg problem, leading to deadlock at startup.
not calling 1st main_loop(), solves the issue if no --preconfig
is specified like Michal has suggested. So moving os_setup_post()
earlier isn't the only option.
With Michal's patch it should work fine with old libvirt versions,
however it would mean more changes to libvirt when adding
--preconfig option handling as it would need to connect to qmp
socket earlier if the option is used.
> > The only viable way to fix this is to call os_setup_post() before
> > the early main_loop() call when in RUN_STATE_PRECONFIG. This has the
> > downside that any errors from this point onwards won't be handled
> > well by the mgmt application, because it will think QEMU has started
> > successfully, so not be expecting an abrupt exit. The only way to
> > deal with that is to move as much user input validation as possible
> > to before the main_loop() call. This is left as an exercise for
> > future interested developers.
Moving post board input validation might be problematic as
it might require existing board to create a device so it could verify
user provided parameters.
Does mgmt application starts QEMU with log file where QEMU would
write errors if any after os_setup_post() and would mgmt app look
into it/report from it to user if QEMU disappears?
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> > vl.c | 7 ++++++-
> > 1 file changed, 6 insertions(+), 1 deletion(-)
>
> Yup, this fixes the problem I've raised in my patch. Thanks!
I'd prefer your patch, as it doesn't break error handling,
or maybe even shorter version which keeps state transitions in one place:
diff --git a/vl.c b/vl.c
index c4fe255..fa44138 100644
--- a/vl.c
+++ b/vl.c
@@ -1956,7 +1956,7 @@ static void main_loop(void)
#ifdef CONFIG_PROFILER
int64_t ti;
#endif
- do {
+ while (!main_loop_should_exit()) {
#ifdef CONFIG_PROFILER
ti = profile_getclock();
#endif
@@ -1964,7 +1964,7 @@ static void main_loop(void)
#ifdef CONFIG_PROFILER
dev_time += profile_getclock() - ti;
#endif
- } while (!main_loop_should_exit());
+ }
}
> Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
>
> (if my R-b line means anything here :-))
>
> Michal
On Mon, Jun 04, 2018 at 05:01:11PM +0200, Igor Mammedov wrote: > On Mon, 4 Jun 2018 16:21:48 +0200 > Michal Privoznik <mprivozn@redhat.com> wrote: > > > On 06/04/2018 02:03 PM, Daniel P. Berrangé wrote: > > > When using --daemonize, the initial lead process will fork a child and > > > then wait to be notified that setup is complete via a pipe, before it > > > exits. When using --preconfig there is an extra call to main_loop() > > > before the notification is done from os_setup_post(). Thus the parent > > > process won't exit until the mgmt application connects to the monitor > > > and tells QEMU to leave the RUN_STATE_PRECONFIG. The mgmt application > > > won't connect to the monitor until daemonizing has completed though. > > > > > > This is a chicken and egg problem, leading to deadlock at startup. > not calling 1st main_loop(), solves the issue if no --preconfig > is specified like Michal has suggested. So moving os_setup_post() > earlier isn't the only option. > > With Michal's patch it should work fine with old libvirt versions, > however it would mean more changes to libvirt when adding > --preconfig option handling as it would need to connect to qmp > socket earlier if the option is used. This patch doesn't cause problems with old libvirt either - the opposite in fact, it fixes the problems that broke with old libvirt. > > > > The only viable way to fix this is to call os_setup_post() before > > > the early main_loop() call when in RUN_STATE_PRECONFIG. This has the > > > downside that any errors from this point onwards won't be handled > > > well by the mgmt application, because it will think QEMU has started > > > successfully, so not be expecting an abrupt exit. The only way to > > > deal with that is to move as much user input validation as possible > > > to before the main_loop() call. This is left as an exercise for > > > future interested developers. > Moving post board input validation might be problematic as > it might require existing board to create a device so it could verify > user provided parameters. > > Does mgmt application starts QEMU with log file where QEMU would > write errors if any after os_setup_post() and would mgmt app look > into it/report from it to user if QEMU disappears? Sure any app can redirect stdout/err to a file if it wishes. --daemonize was actually also a synchronization point - once the parent returns, the mgmt app knows it can successfully connect to the QEMU monitor as the listener socket is guaranteed to be created by then. So moving the os_setup_post earlier as I did still gives that sync point, which is good. It just means when the mgmt app has to be aware that more errors might appear on stderr in the window until it exits preconfig phase. Ideally there would be a way to feed them back via the monitor, but that's non-trivial, so doubt it'll happen any time in the forseeable future. > > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> > > > --- > > > vl.c | 7 ++++++- > > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > Yup, this fixes the problem I've raised in my patch. Thanks! > I'd prefer your patch, as it doesn't break error handling, Michal's patch didn't actually fix the real problem. It simply avoided triggering the bug when --preconfig was not present - QEMU still hangs with --preconfig --daemonize are used together. 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 :|
On Mon, 4 Jun 2018 16:15:23 +0100 Daniel P. Berrangé <berrange@redhat.com> wrote: > On Mon, Jun 04, 2018 at 05:01:11PM +0200, Igor Mammedov wrote: > > On Mon, 4 Jun 2018 16:21:48 +0200 > > Michal Privoznik <mprivozn@redhat.com> wrote: > > > > > On 06/04/2018 02:03 PM, Daniel P. Berrangé wrote: > > > > When using --daemonize, the initial lead process will fork a child and > > > > then wait to be notified that setup is complete via a pipe, before it > > > > exits. When using --preconfig there is an extra call to main_loop() > > > > before the notification is done from os_setup_post(). Thus the parent > > > > process won't exit until the mgmt application connects to the monitor > > > > and tells QEMU to leave the RUN_STATE_PRECONFIG. The mgmt application > > > > won't connect to the monitor until daemonizing has completed though. > > > > > > > > This is a chicken and egg problem, leading to deadlock at startup. > > not calling 1st main_loop(), solves the issue if no --preconfig > > is specified like Michal has suggested. So moving os_setup_post() > > earlier isn't the only option. > > > > With Michal's patch it should work fine with old libvirt versions, > > however it would mean more changes to libvirt when adding > > --preconfig option handling as it would need to connect to qmp > > socket earlier if the option is used. > > This patch doesn't cause problems with old libvirt either - the opposite > in fact, it fixes the problems that broke with old libvirt. > > > > > > > The only viable way to fix this is to call os_setup_post() before > > > > the early main_loop() call when in RUN_STATE_PRECONFIG. This has the > > > > downside that any errors from this point onwards won't be handled > > > > well by the mgmt application, because it will think QEMU has started > > > > successfully, so not be expecting an abrupt exit. The only way to > > > > deal with that is to move as much user input validation as possible > > > > to before the main_loop() call. This is left as an exercise for > > > > future interested developers. > > Moving post board input validation might be problematic as > > it might require existing board to create a device so it could verify > > user provided parameters. > > > > Does mgmt application starts QEMU with log file where QEMU would > > write errors if any after os_setup_post() and would mgmt app look > > into it/report from it to user if QEMU disappears? > > Sure any app can redirect stdout/err to a file if it wishes. > > --daemonize was actually also a synchronization point - once the parent > returns, the mgmt app knows it can successfully connect to the QEMU > monitor as the listener socket is guaranteed to be created by then. > So moving the os_setup_post earlier as I did still gives that sync > point, which is good. It just means when the mgmt app has to be aware > that more errors might appear on stderr in the window until it exits > preconfig phase. > > Ideally there would be a way to feed them back via the monitor, but > that's non-trivial, so doubt it'll happen any time in the forseeable > future. Question is if libvirt would notify user (in its logs) about error after sync point? > > > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> > > > > --- > > > > vl.c | 7 ++++++- > > > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > > > Yup, this fixes the problem I've raised in my patch. Thanks! > > I'd prefer your patch, as it doesn't break error handling, > > Michal's patch didn't actually fix the real problem. It simply avoided > triggering the bug when --preconfig was not present - QEMU still hangs > with --preconfig --daemonize are used together. it's not exactly a bug, it's new behavior so mgmt could adapt to it, i.e. lack of sync point (which certainly complicates mgmt part). So this patch is also fine if libvirt would be able to provide meaning-full error to users in case of problem post os_setup_post() stage. In both cases it would require some work on mgmt part when adding support for --precongig.
On Mon, Jun 04, 2018 at 05:01:11PM +0200, Igor Mammedov wrote:
[...]
> I'd prefer your patch, as it doesn't break error handling,
> or maybe even shorter version which keeps state transitions in one place:
>
> diff --git a/vl.c b/vl.c
> index c4fe255..fa44138 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1956,7 +1956,7 @@ static void main_loop(void)
> #ifdef CONFIG_PROFILER
> int64_t ti;
> #endif
> - do {
> + while (!main_loop_should_exit()) {
> #ifdef CONFIG_PROFILER
> ti = profile_getclock();
> #endif
> @@ -1964,7 +1964,7 @@ static void main_loop(void)
> #ifdef CONFIG_PROFILER
> dev_time += profile_getclock() - ti;
> #endif
> - } while (!main_loop_should_exit());
> + }
> }
Would this also fix the bug mentioned at:
"vl.c: make default main_loop_wait() timeout independed of slirp"?
--
Eduardo
On Mon, 4 Jun 2018 22:06:35 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Mon, Jun 04, 2018 at 05:01:11PM +0200, Igor Mammedov wrote:
> [...]
> > I'd prefer your patch, as it doesn't break error handling,
> > or maybe even shorter version which keeps state transitions in one place:
> >
> > diff --git a/vl.c b/vl.c
> > index c4fe255..fa44138 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -1956,7 +1956,7 @@ static void main_loop(void)
> > #ifdef CONFIG_PROFILER
> > int64_t ti;
> > #endif
> > - do {
> > + while (!main_loop_should_exit()) {
> > #ifdef CONFIG_PROFILER
> > ti = profile_getclock();
> > #endif
> > @@ -1964,7 +1964,7 @@ static void main_loop(void)
> > #ifdef CONFIG_PROFILER
> > dev_time += profile_getclock() - ti;
> > #endif
> > - } while (!main_loop_should_exit());
> > + }
> > }
>
> Would this also fix the bug mentioned at:
> "vl.c: make default main_loop_wait() timeout independed of slirp"?
>
it would mask it,
I'd apply slirp fix as well while we have a reason to clean it up
On Mon, 4 Jun 2018 13:03:45 +0100
Daniel P. Berrangé <berrange@redhat.com> wrote:
> When using --daemonize, the initial lead process will fork a child and
> then wait to be notified that setup is complete via a pipe, before it
> exits. When using --preconfig there is an extra call to main_loop()
> before the notification is done from os_setup_post(). Thus the parent
> process won't exit until the mgmt application connects to the monitor
> and tells QEMU to leave the RUN_STATE_PRECONFIG. The mgmt application
> won't connect to the monitor until daemonizing has completed though.
>
> This is a chicken and egg problem, leading to deadlock at startup.
>
> The only viable way to fix this is to call os_setup_post() before
> the early main_loop() call when in RUN_STATE_PRECONFIG. This has the
> downside that any errors from this point onwards won't be handled
> well by the mgmt application, because it will think QEMU has started
> successfully, so not be expecting an abrupt exit. The only way to
> deal with that is to move as much user input validation as possible
> to before the main_loop() call. This is left as an exercise for
> future interested developers.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
[...]
How about combining ideas from yours and Michal's patches?
It should fix broken iotests/libvirt sync point and
we can think about NONE runstate idea at without rushing it.
If it looks acceptable, I'll post proper patches + test case for it
after some testing (to ensure that iotests Max pointed out are working
as expected)
diff --git a/vl.c b/vl.c
index c4fe255..a2062d6 100644
--- a/vl.c
+++ b/vl.c
@@ -1953,10 +1953,15 @@ static bool main_loop_should_exit(void)
static void main_loop(void)
{
+ static bool os_setup_post_done = false;
#ifdef CONFIG_PROFILER
int64_t ti;
#endif
- do {
+ if (!os_setup_post_done) {
+ os_setup_post();
+ os_setup_post_done = true;
+ }
+ while (!main_loop_should_exit()) {
#ifdef CONFIG_PROFILER
ti = profile_getclock();
#endif
@@ -1964,7 +1969,7 @@ static void main_loop(void)
#ifdef CONFIG_PROFILER
dev_time += profile_getclock() - ti;
#endif
- } while (!main_loop_should_exit());
+ }
}
static void version(void)
On Mon, Jun 04, 2018 at 11:53:15PM +0200, Igor Mammedov wrote:
> On Mon, 4 Jun 2018 13:03:45 +0100
> Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> > When using --daemonize, the initial lead process will fork a child and
> > then wait to be notified that setup is complete via a pipe, before it
> > exits. When using --preconfig there is an extra call to main_loop()
> > before the notification is done from os_setup_post(). Thus the parent
> > process won't exit until the mgmt application connects to the monitor
> > and tells QEMU to leave the RUN_STATE_PRECONFIG. The mgmt application
> > won't connect to the monitor until daemonizing has completed though.
> >
> > This is a chicken and egg problem, leading to deadlock at startup.
> >
> > The only viable way to fix this is to call os_setup_post() before
> > the early main_loop() call when in RUN_STATE_PRECONFIG. This has the
> > downside that any errors from this point onwards won't be handled
> > well by the mgmt application, because it will think QEMU has started
> > successfully, so not be expecting an abrupt exit. The only way to
> > deal with that is to move as much user input validation as possible
> > to before the main_loop() call. This is left as an exercise for
> > future interested developers.
> >
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> [...]
>
> How about combining ideas from yours and Michal's patches?
> It should fix broken iotests/libvirt sync point and
> we can think about NONE runstate idea at without rushing it.
> If it looks acceptable, I'll post proper patches + test case for it
> after some testing (to ensure that iotests Max pointed out are working
> as expected)
>
> diff --git a/vl.c b/vl.c
> index c4fe255..a2062d6 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1953,10 +1953,15 @@ static bool main_loop_should_exit(void)
>
> static void main_loop(void)
> {
> + static bool os_setup_post_done = false;
> #ifdef CONFIG_PROFILER
> int64_t ti;
> #endif
> - do {
> + if (!os_setup_post_done) {
> + os_setup_post();
> + os_setup_post_done = true;
> + }
I don't really like hiding the os_setup_post() call in
the main_loop() method, since they're really independant
functionality.
> + while (!main_loop_should_exit()) {
> #ifdef CONFIG_PROFILER
> ti = profile_getclock();
> #endif
> @@ -1964,7 +1969,7 @@ static void main_loop(void)
> #ifdef CONFIG_PROFILER
> dev_time += profile_getclock() - ti;
> #endif
> - } while (!main_loop_should_exit());
> + }
> }
>
> static void version(void)
Regards,
Daniel
--
|: 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 :|
On Tue, 5 Jun 2018 13:00:54 +0100
Daniel P. Berrangé <berrange@redhat.com> wrote:
> On Mon, Jun 04, 2018 at 11:53:15PM +0200, Igor Mammedov wrote:
> > On Mon, 4 Jun 2018 13:03:45 +0100
> > Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> > > When using --daemonize, the initial lead process will fork a child and
> > > then wait to be notified that setup is complete via a pipe, before it
> > > exits. When using --preconfig there is an extra call to main_loop()
> > > before the notification is done from os_setup_post(). Thus the parent
> > > process won't exit until the mgmt application connects to the monitor
> > > and tells QEMU to leave the RUN_STATE_PRECONFIG. The mgmt application
> > > won't connect to the monitor until daemonizing has completed though.
> > >
> > > This is a chicken and egg problem, leading to deadlock at startup.
> > >
> > > The only viable way to fix this is to call os_setup_post() before
> > > the early main_loop() call when in RUN_STATE_PRECONFIG. This has the
> > > downside that any errors from this point onwards won't be handled
> > > well by the mgmt application, because it will think QEMU has started
> > > successfully, so not be expecting an abrupt exit. The only way to
> > > deal with that is to move as much user input validation as possible
> > > to before the main_loop() call. This is left as an exercise for
> > > future interested developers.
> > >
> > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > [...]
> >
> > How about combining ideas from yours and Michal's patches?
> > It should fix broken iotests/libvirt sync point and
> > we can think about NONE runstate idea at without rushing it.
> > If it looks acceptable, I'll post proper patches + test case for it
> > after some testing (to ensure that iotests Max pointed out are working
> > as expected)
> >
> > diff --git a/vl.c b/vl.c
> > index c4fe255..a2062d6 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -1953,10 +1953,15 @@ static bool main_loop_should_exit(void)
> >
> > static void main_loop(void)
> > {
> > + static bool os_setup_post_done = false;
> > #ifdef CONFIG_PROFILER
> > int64_t ti;
> > #endif
> > - do {
> > + if (!os_setup_post_done) {
> > + os_setup_post();
> > + os_setup_post_done = true;
> > + }
>
> I don't really like hiding the os_setup_post() call in
> the main_loop() method, since they're really independant
> functionality.
I've posted v3 series with this variant as both turned out
to be somewhat related and it avoids forgetting to call os_setup_post()
before main_loop() is called which is non obvious requirement from
libvirt side. Resulting code ended up cleaner and still keeps runstate
transitions in one place main_loop_should_exit().
But I don't really have a preference here, so if we don't hide
it inside of main_loop() we need to duplicate exit condition
from main_loop_should_exit(), but that's it.
It would look a bit more messier, like:
diff --git a/vl.c b/vl.c
index d6fa67f..f1bda99 100644
--- a/vl.c
+++ b/vl.c
@@ -3039,6 +3039,7 @@ int main(int argc, char **argv, char **envp)
Error *err = NULL;
bool list_data_dirs = false;
char *dir, **dirs;
+ bool os_setup_post_done = false;
typedef struct BlockdevOptions_queue {
BlockdevOptions *bdo;
Location loc;
@@ -4579,7 +4580,16 @@ int main(int argc, char **argv, char **envp)
parse_numa_opts(current_machine);
/* do monitor/qmp handling at preconfig state if requested */
- main_loop();
+ if (preconfig_exit_requested) {
+ if (runstate_check(RUN_STATE_PRECONFIG)) {
+ runstate_set(RUN_STATE_PRELAUNCH);
+ }
+ preconfig_exit_requested = false;
+ } else {
+ os_setup_post();
+ os_setup_post_done = true;
+ main_loop();
+ }
/* from here on runstate is RUN_STATE_PRELAUNCH */
machine_run_board_init(current_machine);
@@ -4709,6 +4719,9 @@ int main(int argc, char **argv, char **envp)
accel_setup_post(current_machine);
+ if (!os_setup_post_done) {
+ os_setup_post();
+ }
main_loop();
but if that's what is preferred I can repost v4.
> > + while (!main_loop_should_exit()) {
> > #ifdef CONFIG_PROFILER
> > ti = profile_getclock();
> > #endif
> > @@ -1964,7 +1969,7 @@ static void main_loop(void)
> > #ifdef CONFIG_PROFILER
> > dev_time += profile_getclock() - ti;
> > #endif
> > - } while (!main_loop_should_exit());
> > + }
> > }
> >
> > static void version(void)
>
> Regards,
> Daniel
© 2016 - 2025 Red Hat, Inc.