On Tue, 5 Jun 2018 13:00:54 +0100
Daniel P. Berrangé <berra...@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é <berra...@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é <berra...@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
