[Qemu-devel] [PATCH v2 0/2] Avoid using RUN_STATE_PRECONFIG unless explicitly requested with --preconfig

[Qemu-devel] [PATCH v2 0/2] Avoid using RUN_STATE_PRECONFIG unless explicitly requested with --preconfig

[Daniel P. Berrangé]

The change to use RUN_STATE_PRECONFIG as the default initial state, even
when not requested with --preconfig has caused a number of problems. This
series introduces a new RUN_STATE_NONE to act as the initial state, so
that we never use RUN_STATE_PRECONFIG unless the mgmt app has explicitly
requested todo so.

Daniel P. Berrangé (2):
  vl: don't use RUN_STATE_PRECONFIG as initial state
  vl: fix use of --daemonize with --preconfig

 qapi/run-state.json |  6 +++++-
 vl.c                | 49 +++++++++++++++++++++++++++------------------
 2 files changed, 35 insertions(+), 20 deletions(-)

-- 
2.17.0

[Qemu-devel] [PATCH v2 1/2] vl: don't use RUN_STATE_PRECONFIG as initial state

[Daniel P. Berrangé]

The RUN_STATE_PRECONFIG state is not supposed to be reachable unless the
--preconfig argument is given to QEMU, but when it was introduced in:

  commit 047f7038f586d2150f16c6d9ba9cfd0479f0f6ac
  Author: Igor Mammedov <imammedo@redhat.com>
  Date:   Fri May 11 19:24:43 2018 +0200

    cli: add --preconfig option

...the global 'current_run_state' variable was changed to have an initial
value of RUN_STATE_PRECONFIG regardless of whether --preconfig is given.

A second invokation of main_loop() was added which then toggles it back
to RUN_STATE_PRELAUNCH when --preconfig is not given. This is racy
because it leaves a window where QEMU is in RUN_STATE_PRECONFIG despite
--preconfig not being given. This can be seen with the failure:

  $ echo | x86_64-softmmu/qemu-system-x86_64 -monitor stdio
  QEMU 2.12.50 monitor - type 'help' for more information
  (qemu)
  HMP not available in preconfig state, use QMP instead

Using RUN_STATE_PRECONFIG required adding a state transition from
RUN_STATE_PRECONFIG to RUN_STATE_INMIGRATE, which is undesirable as
it prevented automatic detection of --preconfig & --incoming being
mutually exclusive.

If we use RUN_STATE_PRELAUNCH as the initial value, however, we need to
allow transitions between RUN_STATE_PRECONFIG & RUN_STATE_PRELAUNCH in
both directions which is also undesirable, as RUN_STATE_PRECONFIG should
be a one-time-only state that can never be returned to.

This change solves the confusion by introducing a further RUN_STATE_NONE
which is used as the initial state value. This can transition to any of
RUN_STATE_PRELAUNCH, RUN_STATE_PRECONFIG or RUN_STATE_INMIGRATE. This
avoids the need to allow any undesirable state transitions.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 qapi/run-state.json |  6 +++++-
 vl.c                | 42 ++++++++++++++++++++++++------------------
 2 files changed, 29 insertions(+), 19 deletions(-)

diff --git a/qapi/run-state.json b/qapi/run-state.json
index 332e44897b..c3bd7b9b7a 100644
--- a/qapi/run-state.json
+++ b/qapi/run-state.json
@@ -10,6 +10,10 @@
 #
 # An enumeration of VM run states.
 #
+# @none: QEMU is in early startup. This state should never be visible
+# when querying state from the monitor, as QEMU will have already
+# transitioned to another state. (Since 3.0)
+#
 # @debug: QEMU is running on a debugger
 #
 # @finish-migrate: guest is paused to finish the migration process
@@ -54,7 +58,7 @@
 #             (Since 3.0)
 ##
 { 'enum': 'RunState',
-  'data': [ 'debug', 'inmigrate', 'internal-error', 'io-error', 'paused',
+  'data': [ 'none', 'debug', 'inmigrate', 'internal-error', 'io-error', 'paused',
             'postmigrate', 'prelaunch', 'finish-migrate', 'restore-vm',
             'running', 'save-vm', 'shutdown', 'suspended', 'watchdog',
             'guest-panicked', 'colo', 'preconfig' ] }
diff --git a/vl.c b/vl.c
index 06031715ac..30d0e985f0 100644
--- a/vl.c
+++ b/vl.c
@@ -561,7 +561,7 @@ static int default_driver_check(void *opaque, QemuOpts *opts, Error **errp)
 /***********************************************************/
 /* QEMU state */
 
-static RunState current_run_state = RUN_STATE_PRECONFIG;
+static RunState current_run_state = RUN_STATE_NONE;
 
 /* We use RUN_STATE__MAX but any invalid value will do */
 static RunState vmstop_requested = RUN_STATE__MAX;
@@ -574,12 +574,11 @@ typedef struct {
 
 static const RunStateTransition runstate_transitions_def[] = {
     /*     from      ->     to      */
+    { RUN_STATE_NONE, RUN_STATE_PRELAUNCH },
+    { RUN_STATE_NONE, RUN_STATE_PRECONFIG },
+    { RUN_STATE_NONE, RUN_STATE_INMIGRATE },
+
     { RUN_STATE_PRECONFIG, RUN_STATE_PRELAUNCH },
-      /* Early switch to inmigrate state to allow  -incoming CLI option work
-       * as it used to. TODO: delay actual switching to inmigrate state to
-       * the point after machine is built and remove this hack.
-       */
-    { RUN_STATE_PRECONFIG, RUN_STATE_INMIGRATE },
 
     { RUN_STATE_DEBUG, RUN_STATE_RUNNING },
     { RUN_STATE_DEBUG, RUN_STATE_FINISH_MIGRATE },
@@ -618,7 +617,6 @@ static const RunStateTransition runstate_transitions_def[] = {
 
     { RUN_STATE_PRELAUNCH, RUN_STATE_RUNNING },
     { RUN_STATE_PRELAUNCH, RUN_STATE_FINISH_MIGRATE },
-    { RUN_STATE_PRELAUNCH, RUN_STATE_INMIGRATE },
 
     { RUN_STATE_FINISH_MIGRATE, RUN_STATE_RUNNING },
     { RUN_STATE_FINISH_MIGRATE, RUN_STATE_PAUSED },
@@ -1522,7 +1520,7 @@ static pid_t shutdown_pid;
 static int powerdown_requested;
 static int debug_requested;
 static int suspend_requested;
-static bool preconfig_exit_requested = true;
+static bool preconfig_exit_requested;
 static WakeupReason wakeup_reason;
 static NotifierList powerdown_notifiers =
     NOTIFIER_LIST_INITIALIZER(powerdown_notifiers);
@@ -3572,7 +3570,12 @@ int main(int argc, char **argv, char **envp)
                 }
                 break;
             case QEMU_OPTION_preconfig:
-                preconfig_exit_requested = false;
+                if (!runstate_check(RUN_STATE_NONE)) {
+                    error_report("'--preconfig' and '--incoming' options are "
+                                 "mutually exclusive");
+                    exit(EXIT_FAILURE);
+                }
+                runstate_set(RUN_STATE_PRECONFIG);
                 break;
             case QEMU_OPTION_enable_kvm:
                 olist = qemu_find_opts("machine");
@@ -3768,9 +3771,12 @@ int main(int argc, char **argv, char **envp)
                 }
                 break;
             case QEMU_OPTION_incoming:
-                if (!incoming) {
-                    runstate_set(RUN_STATE_INMIGRATE);
+                if (!runstate_check(RUN_STATE_NONE)) {
+                    error_report("'--preconfig' and '--incoming' options are "
+                                 "mutually exclusive");
+                    exit(EXIT_FAILURE);
                 }
+                runstate_set(RUN_STATE_INMIGRATE);
                 incoming = optarg;
                 break;
             case QEMU_OPTION_only_migratable:
@@ -3943,14 +3949,12 @@ int main(int argc, char **argv, char **envp)
      */
     loc_set_none();
 
-    replay_configure(icount_opts);
-
-    if (incoming && !preconfig_exit_requested) {
-        error_report("'preconfig' and 'incoming' options are "
-                     "mutually exclusive");
-        exit(EXIT_FAILURE);
+    if (runstate_check(RUN_STATE_NONE)) {
+        runstate_set(RUN_STATE_PRELAUNCH);
     }
 
+    replay_configure(icount_opts);
+
     machine_class = select_machine();
 
     set_memory_options(&ram_slots, &maxram_size, machine_class);
@@ -4471,7 +4475,9 @@ 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 (runstate_check(RUN_STATE_PRECONFIG)) {
+        main_loop();
+    }
 
     /* from here on runstate is RUN_STATE_PRELAUNCH */
     machine_run_board_init(current_machine);
-- 
2.17.0

Re: [Qemu-devel] [PATCH v2 1/2] vl: don't use RUN_STATE_PRECONFIG as initial state

[Michal Privoznik]

On 06/04/2018 02:03 PM, Daniel P. Berrangé wrote:
> The RUN_STATE_PRECONFIG state is not supposed to be reachable unless the
> --preconfig argument is given to QEMU, but when it was introduced in:
> 
>   commit 047f7038f586d2150f16c6d9ba9cfd0479f0f6ac
>   Author: Igor Mammedov <imammedo@redhat.com>
>   Date:   Fri May 11 19:24:43 2018 +0200
> 
>     cli: add --preconfig option
> 
> ...the global 'current_run_state' variable was changed to have an initial
> value of RUN_STATE_PRECONFIG regardless of whether --preconfig is given.
> 
> A second invokation of main_loop() was added which then toggles it back
> to RUN_STATE_PRELAUNCH when --preconfig is not given. This is racy
> because it leaves a window where QEMU is in RUN_STATE_PRECONFIG despite
> --preconfig not being given. This can be seen with the failure:
> 
>   $ echo | x86_64-softmmu/qemu-system-x86_64 -monitor stdio
>   QEMU 2.12.50 monitor - type 'help' for more information
>   (qemu)
>   HMP not available in preconfig state, use QMP instead
> 
> Using RUN_STATE_PRECONFIG required adding a state transition from
> RUN_STATE_PRECONFIG to RUN_STATE_INMIGRATE, which is undesirable as
> it prevented automatic detection of --preconfig & --incoming being
> mutually exclusive.
> 
> If we use RUN_STATE_PRELAUNCH as the initial value, however, we need to
> allow transitions between RUN_STATE_PRECONFIG & RUN_STATE_PRELAUNCH in
> both directions which is also undesirable, as RUN_STATE_PRECONFIG should
> be a one-time-only state that can never be returned to.
> 
> This change solves the confusion by introducing a further RUN_STATE_NONE
> which is used as the initial state value. This can transition to any of
> RUN_STATE_PRELAUNCH, RUN_STATE_PRECONFIG or RUN_STATE_INMIGRATE. This
> avoids the need to allow any undesirable state transitions.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  qapi/run-state.json |  6 +++++-
>  vl.c                | 42 ++++++++++++++++++++++++------------------
>  2 files changed, 29 insertions(+), 19 deletions(-)
> 
> diff --git a/qapi/run-state.json b/qapi/run-state.json
> index 332e44897b..c3bd7b9b7a 100644
> --- a/qapi/run-state.json
> +++ b/qapi/run-state.json
> @@ -10,6 +10,10 @@
>  #
>  # An enumeration of VM run states.
>  #
> +# @none: QEMU is in early startup. This state should never be visible
> +# when querying state from the monitor, as QEMU will have already
> +# transitioned to another state. (Since 3.0)
> +#
>  # @debug: QEMU is running on a debugger
>  #
>  # @finish-migrate: guest is paused to finish the migration process
> @@ -54,7 +58,7 @@
>  #             (Since 3.0)
>  ##
>  { 'enum': 'RunState',
> -  'data': [ 'debug', 'inmigrate', 'internal-error', 'io-error', 'paused',
> +  'data': [ 'none', 'debug', 'inmigrate', 'internal-error', 'io-error', 'paused',
>              'postmigrate', 'prelaunch', 'finish-migrate', 'restore-vm',
>              'running', 'save-vm', 'shutdown', 'suspended', 'watchdog',
>              'guest-panicked', 'colo', 'preconfig' ] }
> diff --git a/vl.c b/vl.c
> index 06031715ac..30d0e985f0 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -561,7 +561,7 @@ static int default_driver_check(void *opaque, QemuOpts *opts, Error **errp)
>  /***********************************************************/
>  /* QEMU state */
>  
> -static RunState current_run_state = RUN_STATE_PRECONFIG;
> +static RunState current_run_state = RUN_STATE_NONE;
>  
>  /* We use RUN_STATE__MAX but any invalid value will do */
>  static RunState vmstop_requested = RUN_STATE__MAX;
> @@ -574,12 +574,11 @@ typedef struct {
>  
>  static const RunStateTransition runstate_transitions_def[] = {
>      /*     from      ->     to      */
> +    { RUN_STATE_NONE, RUN_STATE_PRELAUNCH },
> +    { RUN_STATE_NONE, RUN_STATE_PRECONFIG },
> +    { RUN_STATE_NONE, RUN_STATE_INMIGRATE },
> +
>      { RUN_STATE_PRECONFIG, RUN_STATE_PRELAUNCH },
> -      /* Early switch to inmigrate state to allow  -incoming CLI option work
> -       * as it used to. TODO: delay actual switching to inmigrate state to
> -       * the point after machine is built and remove this hack.
> -       */
> -    { RUN_STATE_PRECONFIG, RUN_STATE_INMIGRATE },
>  
>      { RUN_STATE_DEBUG, RUN_STATE_RUNNING },
>      { RUN_STATE_DEBUG, RUN_STATE_FINISH_MIGRATE },
> @@ -618,7 +617,6 @@ static const RunStateTransition runstate_transitions_def[] = {
>  
>      { RUN_STATE_PRELAUNCH, RUN_STATE_RUNNING },
>      { RUN_STATE_PRELAUNCH, RUN_STATE_FINISH_MIGRATE },
> -    { RUN_STATE_PRELAUNCH, RUN_STATE_INMIGRATE },
>  
>      { RUN_STATE_FINISH_MIGRATE, RUN_STATE_RUNNING },
>      { RUN_STATE_FINISH_MIGRATE, RUN_STATE_PAUSED },
> @@ -1522,7 +1520,7 @@ static pid_t shutdown_pid;
>  static int powerdown_requested;
>  static int debug_requested;
>  static int suspend_requested;
> -static bool preconfig_exit_requested = true;
> +static bool preconfig_exit_requested;
>  static WakeupReason wakeup_reason;
>  static NotifierList powerdown_notifiers =
>      NOTIFIER_LIST_INITIALIZER(powerdown_notifiers);
> @@ -3572,7 +3570,12 @@ int main(int argc, char **argv, char **envp)
>                  }
>                  break;
>              case QEMU_OPTION_preconfig:
> -                preconfig_exit_requested = false;
> +                if (!runstate_check(RUN_STATE_NONE)) {
> +                    error_report("'--preconfig' and '--incoming' options are "
> +                                 "mutually exclusive");
> +                    exit(EXIT_FAILURE);
> +                }
> +                runstate_set(RUN_STATE_PRECONFIG);

Specifying --preconfig twice on the command line now fails with a very
cryptic message (there's no --incoming).

>                  break;
>              case QEMU_OPTION_enable_kvm:
>                  olist = qemu_find_opts("machine");
> @@ -3768,9 +3771,12 @@ int main(int argc, char **argv, char **envp)
>                  }
>                  break;
>              case QEMU_OPTION_incoming:
> -                if (!incoming) {
> -                    runstate_set(RUN_STATE_INMIGRATE);
> +                if (!runstate_check(RUN_STATE_NONE)) {
> +                    error_report("'--preconfig' and '--incoming' options are "
> +                                 "mutually exclusive");
> +                    exit(EXIT_FAILURE);
>                  }
> +                runstate_set(RUN_STATE_INMIGRATE);

Same here. Specifying --incoming twice fails with cryptic message. But
one can argue that specifying --incoming twice is wrong anyway.

>                  incoming = optarg;
>                  break;
>              case QEMU_OPTION_only_migratable:

Michal

Re: [Qemu-devel] [PATCH v2 1/2] vl: don't use RUN_STATE_PRECONFIG as initial state

[Eduardo Habkost]

On Mon, Jun 04, 2018 at 04:21:47PM +0200, Michal Privoznik wrote:
[...]
> > @@ -3572,7 +3570,12 @@ int main(int argc, char **argv, char **envp)
> >                  }
> >                  break;
> >              case QEMU_OPTION_preconfig:
> > -                preconfig_exit_requested = false;
> > +                if (!runstate_check(RUN_STATE_NONE)) {
> > +                    error_report("'--preconfig' and '--incoming' options are "
> > +                                 "mutually exclusive");
> > +                    exit(EXIT_FAILURE);
> > +                }
> > +                runstate_set(RUN_STATE_PRECONFIG);
> 
> Specifying --preconfig twice on the command line now fails with a very
> cryptic message (there's no --incoming).
> 
> >                  break;
> >              case QEMU_OPTION_enable_kvm:
> >                  olist = qemu_find_opts("machine");
> > @@ -3768,9 +3771,12 @@ int main(int argc, char **argv, char **envp)
> >                  }
> >                  break;
> >              case QEMU_OPTION_incoming:
> > -                if (!incoming) {
> > -                    runstate_set(RUN_STATE_INMIGRATE);
> > +                if (!runstate_check(RUN_STATE_NONE)) {
> > +                    error_report("'--preconfig' and '--incoming' options are "
> > +                                 "mutually exclusive");
> > +                    exit(EXIT_FAILURE);
> >                  }
> > +                runstate_set(RUN_STATE_INMIGRATE);
> 
> Same here. Specifying --incoming twice fails with cryptic message. But
> one can argue that specifying --incoming twice is wrong anyway.
> 

Initially I was going to suggest simply not changing runstate
during option parsing to avoid this kind of problem, but maybe
this would be a nice way to implement the command-line parsing
rules:


    case QEMU_OPTION_preconfig:
        /*
         * A INCOMING -> PRECONFIG transition would call:
         *   error_setg("--preconfig and --incoming options are mutually exclusive");
         */
        try_runstate_set(RUN_STATE_PRECONFIG, &error_fatal);
        break;
    case QEMU_OPTION_incoming:
        /*
         * A PRECONFIG -> INCOMING transition would also call:
         *   error_setg("--preconfig and --incoming options are mutually exclusive");
         *
         * Maybe a INCOMING -> INCOMING transition could
         * result in:
         *   error_setg("--incoming can't be specified twice");
         */
        try_runstate_set(RUN_STATE_INMIGRATE, &error_fatal);
        break;


> >                  incoming = optarg;
> >                  break;
> >              case QEMU_OPTION_only_migratable:
> 
> Michal
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2 1/2] vl: don't use RUN_STATE_PRECONFIG as initial state

[Igor Mammedov]

On Mon, 4 Jun 2018 21:56:47 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Mon, Jun 04, 2018 at 04:21:47PM +0200, Michal Privoznik wrote:
> [...]
> > > @@ -3572,7 +3570,12 @@ int main(int argc, char **argv, char **envp)
> > >                  }
> > >                  break;
> > >              case QEMU_OPTION_preconfig:
> > > -                preconfig_exit_requested = false;
> > > +                if (!runstate_check(RUN_STATE_NONE)) {
> > > +                    error_report("'--preconfig' and '--incoming' options are "
> > > +                                 "mutually exclusive");
> > > +                    exit(EXIT_FAILURE);
> > > +                }
> > > +                runstate_set(RUN_STATE_PRECONFIG);  
> > 
> > Specifying --preconfig twice on the command line now fails with a very
> > cryptic message (there's no --incoming).
> >   
> > >                  break;
> > >              case QEMU_OPTION_enable_kvm:
> > >                  olist = qemu_find_opts("machine");
> > > @@ -3768,9 +3771,12 @@ int main(int argc, char **argv, char **envp)
> > >                  }
> > >                  break;
> > >              case QEMU_OPTION_incoming:
> > > -                if (!incoming) {
> > > -                    runstate_set(RUN_STATE_INMIGRATE);
> > > +                if (!runstate_check(RUN_STATE_NONE)) {
> > > +                    error_report("'--preconfig' and '--incoming' options are "
> > > +                                 "mutually exclusive");
> > > +                    exit(EXIT_FAILURE);
> > >                  }
> > > +                runstate_set(RUN_STATE_INMIGRATE);  
> > 
> > Same here. Specifying --incoming twice fails with cryptic message. But
> > one can argue that specifying --incoming twice is wrong anyway.
> >   
> 
> Initially I was going to suggest simply not changing runstate
> during option parsing to avoid this kind of problem, but maybe
> this would be a nice way to implement the command-line parsing
> rules:
Is there a big reason to try making early incoming transition hack nicer?
I'd rather leave it as it is for now and fix it properly later
(i.e. postponing the transition after machine_done point, which is on my
todo list).
> 
>     case QEMU_OPTION_preconfig:
>         /*
>          * A INCOMING -> PRECONFIG transition would call:
>          *   error_setg("--preconfig and --incoming options are mutually exclusive");
>          */
>         try_runstate_set(RUN_STATE_PRECONFIG, &error_fatal);
>         break;
>     case QEMU_OPTION_incoming:
>         /*
>          * A PRECONFIG -> INCOMING transition would also call:
>          *   error_setg("--preconfig and --incoming options are mutually exclusive");
>          *
>          * Maybe a INCOMING -> INCOMING transition could
>          * result in:
>          *   error_setg("--incoming can't be specified twice");
>          */
>         try_runstate_set(RUN_STATE_INMIGRATE, &error_fatal);
>         break;
> 
> 
> > >                  incoming = optarg;
> > >                  break;
> > >              case QEMU_OPTION_only_migratable:  
> > 
> > Michal
> >   
> 

Re: [Qemu-devel] [PATCH v2 1/2] vl: don't use RUN_STATE_PRECONFIG as initial state

[Eduardo Habkost]

On Tue, Jun 05, 2018 at 10:37:55AM +0200, Igor Mammedov wrote:
> On Mon, 4 Jun 2018 21:56:47 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> > On Mon, Jun 04, 2018 at 04:21:47PM +0200, Michal Privoznik wrote:
> > [...]
> > > > @@ -3572,7 +3570,12 @@ int main(int argc, char **argv, char **envp)
> > > >                  }
> > > >                  break;
> > > >              case QEMU_OPTION_preconfig:
> > > > -                preconfig_exit_requested = false;
> > > > +                if (!runstate_check(RUN_STATE_NONE)) {
> > > > +                    error_report("'--preconfig' and '--incoming' options are "
> > > > +                                 "mutually exclusive");
> > > > +                    exit(EXIT_FAILURE);
> > > > +                }
> > > > +                runstate_set(RUN_STATE_PRECONFIG);  
> > > 
> > > Specifying --preconfig twice on the command line now fails with a very
> > > cryptic message (there's no --incoming).
> > >   
> > > >                  break;
> > > >              case QEMU_OPTION_enable_kvm:
> > > >                  olist = qemu_find_opts("machine");
> > > > @@ -3768,9 +3771,12 @@ int main(int argc, char **argv, char **envp)
> > > >                  }
> > > >                  break;
> > > >              case QEMU_OPTION_incoming:
> > > > -                if (!incoming) {
> > > > -                    runstate_set(RUN_STATE_INMIGRATE);
> > > > +                if (!runstate_check(RUN_STATE_NONE)) {
> > > > +                    error_report("'--preconfig' and '--incoming' options are "
> > > > +                                 "mutually exclusive");
> > > > +                    exit(EXIT_FAILURE);
> > > >                  }
> > > > +                runstate_set(RUN_STATE_INMIGRATE);  
> > > 
> > > Same here. Specifying --incoming twice fails with cryptic message. But
> > > one can argue that specifying --incoming twice is wrong anyway.
> > >   
> > 
> > Initially I was going to suggest simply not changing runstate
> > during option parsing to avoid this kind of problem, but maybe
> > this would be a nice way to implement the command-line parsing
> > rules:
> Is there a big reason to try making early incoming transition hack nicer?
> I'd rather leave it as it is for now and fix it properly later
> (i.e. postponing the transition after machine_done point, which is on my
> todo list).

Yeah, I'm not sure we want go towards encoding more knowledge in
the state machine, or keeping the system simple and encoding the
rules in more straightforward code+variables inside main().

> > 
> >     case QEMU_OPTION_preconfig:
> >         /*
> >          * A INCOMING -> PRECONFIG transition would call:
> >          *   error_setg("--preconfig and --incoming options are mutually exclusive");
> >          */
> >         try_runstate_set(RUN_STATE_PRECONFIG, &error_fatal);
> >         break;
> >     case QEMU_OPTION_incoming:
> >         /*
> >          * A PRECONFIG -> INCOMING transition would also call:
> >          *   error_setg("--preconfig and --incoming options are mutually exclusive");
> >          *
> >          * Maybe a INCOMING -> INCOMING transition could
> >          * result in:
> >          *   error_setg("--incoming can't be specified twice");
> >          */
> >         try_runstate_set(RUN_STATE_INMIGRATE, &error_fatal);
> >         break;
> > 
> > 
> > > >                  incoming = optarg;
> > > >                  break;
> > > >              case QEMU_OPTION_only_migratable:  
> > > 
> > > Michal
> > >   
> > 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2 1/2] vl: don't use RUN_STATE_PRECONFIG as initial state

[Igor Mammedov]

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

> On Tue, Jun 05, 2018 at 10:37:55AM +0200, Igor Mammedov wrote:
> > On Mon, 4 Jun 2018 21:56:47 -0300
> > Eduardo Habkost <ehabkost@redhat.com> wrote:  
> > > On Mon, Jun 04, 2018 at 04:21:47PM +0200, Michal Privoznik wrote:
> > > [...]  
> > > > > @@ -3572,7 +3570,12 @@ int main(int argc, char **argv, char **envp)
> > > > >                  }
> > > > >                  break;
> > > > >              case QEMU_OPTION_preconfig:
> > > > > -                preconfig_exit_requested = false;
> > > > > +                if (!runstate_check(RUN_STATE_NONE)) {
> > > > > +                    error_report("'--preconfig' and '--incoming' options are "
> > > > > +                                 "mutually exclusive");
> > > > > +                    exit(EXIT_FAILURE);
> > > > > +                }
> > > > > +                runstate_set(RUN_STATE_PRECONFIG);    
> > > > 
> > > > Specifying --preconfig twice on the command line now fails with a very
> > > > cryptic message (there's no --incoming).
> > > >     
> > > > >                  break;
> > > > >              case QEMU_OPTION_enable_kvm:
> > > > >                  olist = qemu_find_opts("machine");
> > > > > @@ -3768,9 +3771,12 @@ int main(int argc, char **argv, char **envp)
> > > > >                  }
> > > > >                  break;
> > > > >              case QEMU_OPTION_incoming:
> > > > > -                if (!incoming) {
> > > > > -                    runstate_set(RUN_STATE_INMIGRATE);
> > > > > +                if (!runstate_check(RUN_STATE_NONE)) {
> > > > > +                    error_report("'--preconfig' and '--incoming' options are "
> > > > > +                                 "mutually exclusive");
> > > > > +                    exit(EXIT_FAILURE);
> > > > >                  }
> > > > > +                runstate_set(RUN_STATE_INMIGRATE);    
> > > > 
> > > > Same here. Specifying --incoming twice fails with cryptic message. But
> > > > one can argue that specifying --incoming twice is wrong anyway.
> > > >     
> > > 
> > > Initially I was going to suggest simply not changing runstate
> > > during option parsing to avoid this kind of problem, but maybe
> > > this would be a nice way to implement the command-line parsing
> > > rules:  
> > Is there a big reason to try making early incoming transition hack nicer?
> > I'd rather leave it as it is for now and fix it properly later
> > (i.e. postponing the transition after machine_done point, which is on my
> > todo list).  
> 
> Yeah, I'm not sure we want go towards encoding more knowledge in
> the state machine, or keeping the system simple and encoding the
> rules in more straightforward code+variables inside main().
I've tried before to use code+variables for preconfig checks without
introducing a new runstate and it turned out difficult to manage
and the later changes often broke it. With new runstate end result
was much cleaner.

> 
> > > 
> > >     case QEMU_OPTION_preconfig:
> > >         /*
> > >          * A INCOMING -> PRECONFIG transition would call:
> > >          *   error_setg("--preconfig and --incoming options are mutually exclusive");
> > >          */
> > >         try_runstate_set(RUN_STATE_PRECONFIG, &error_fatal);
> > >         break;
> > >     case QEMU_OPTION_incoming:
> > >         /*
> > >          * A PRECONFIG -> INCOMING transition would also call:
> > >          *   error_setg("--preconfig and --incoming options are mutually exclusive");
> > >          *
> > >          * Maybe a INCOMING -> INCOMING transition could
> > >          * result in:
> > >          *   error_setg("--incoming can't be specified twice");
> > >          */
> > >         try_runstate_set(RUN_STATE_INMIGRATE, &error_fatal);
> > >         break;
> > > 
> > >   
> > > > >                  incoming = optarg;
> > > > >                  break;
> > > > >              case QEMU_OPTION_only_migratable:    
> > > > 
> > > > Michal
> > > >     
> > >   
> >   
> 

Re: [Qemu-devel] [PATCH v2 1/2] vl: don't use RUN_STATE_PRECONFIG as initial state

[Eduardo Habkost]

On Tue, Jun 05, 2018 at 04:25:17PM +0200, Igor Mammedov wrote:
> On Tue, 5 Jun 2018 09:01:09 -0300
> Eduardo Habkost <ehabkost@redhat.com> wrote:
> 
> > On Tue, Jun 05, 2018 at 10:37:55AM +0200, Igor Mammedov wrote:
> > > On Mon, 4 Jun 2018 21:56:47 -0300
> > > Eduardo Habkost <ehabkost@redhat.com> wrote:  
> > > > On Mon, Jun 04, 2018 at 04:21:47PM +0200, Michal Privoznik wrote:
> > > > [...]  
> > > > > > @@ -3572,7 +3570,12 @@ int main(int argc, char **argv, char **envp)
> > > > > >                  }
> > > > > >                  break;
> > > > > >              case QEMU_OPTION_preconfig:
> > > > > > -                preconfig_exit_requested = false;
> > > > > > +                if (!runstate_check(RUN_STATE_NONE)) {
> > > > > > +                    error_report("'--preconfig' and '--incoming' options are "
> > > > > > +                                 "mutually exclusive");
> > > > > > +                    exit(EXIT_FAILURE);
> > > > > > +                }
> > > > > > +                runstate_set(RUN_STATE_PRECONFIG);    
> > > > > 
> > > > > Specifying --preconfig twice on the command line now fails with a very
> > > > > cryptic message (there's no --incoming).
> > > > >     
> > > > > >                  break;
> > > > > >              case QEMU_OPTION_enable_kvm:
> > > > > >                  olist = qemu_find_opts("machine");
> > > > > > @@ -3768,9 +3771,12 @@ int main(int argc, char **argv, char **envp)
> > > > > >                  }
> > > > > >                  break;
> > > > > >              case QEMU_OPTION_incoming:
> > > > > > -                if (!incoming) {
> > > > > > -                    runstate_set(RUN_STATE_INMIGRATE);
> > > > > > +                if (!runstate_check(RUN_STATE_NONE)) {
> > > > > > +                    error_report("'--preconfig' and '--incoming' options are "
> > > > > > +                                 "mutually exclusive");
> > > > > > +                    exit(EXIT_FAILURE);
> > > > > >                  }
> > > > > > +                runstate_set(RUN_STATE_INMIGRATE);    
> > > > > 
> > > > > Same here. Specifying --incoming twice fails with cryptic message. But
> > > > > one can argue that specifying --incoming twice is wrong anyway.
> > > > >     
> > > > 
> > > > Initially I was going to suggest simply not changing runstate
> > > > during option parsing to avoid this kind of problem, but maybe
> > > > this would be a nice way to implement the command-line parsing
> > > > rules:  
> > > Is there a big reason to try making early incoming transition hack nicer?
> > > I'd rather leave it as it is for now and fix it properly later
> > > (i.e. postponing the transition after machine_done point, which is on my
> > > todo list).  
> > 
> > Yeah, I'm not sure we want go towards encoding more knowledge in
> > the state machine, or keeping the system simple and encoding the
> > rules in more straightforward code+variables inside main().
> I've tried before to use code+variables for preconfig checks without
> introducing a new runstate and it turned out difficult to manage
> and the later changes often broke it. With new runstate end result
> was much cleaner.

Right.  I wasn't talking specifically about the new runstates
like PRECONFIG and NONE (which both make sense, IMO), but about
my suggestion below to also use runstate transition rules to
encode command-line validation and error messages.


> 
> > 
> > > > 
> > > >     case QEMU_OPTION_preconfig:
> > > >         /*
> > > >          * A INCOMING -> PRECONFIG transition would call:
> > > >          *   error_setg("--preconfig and --incoming options are mutually exclusive");
> > > >          */
> > > >         try_runstate_set(RUN_STATE_PRECONFIG, &error_fatal);
> > > >         break;
> > > >     case QEMU_OPTION_incoming:
> > > >         /*
> > > >          * A PRECONFIG -> INCOMING transition would also call:
> > > >          *   error_setg("--preconfig and --incoming options are mutually exclusive");
> > > >          *
> > > >          * Maybe a INCOMING -> INCOMING transition could
> > > >          * result in:
> > > >          *   error_setg("--incoming can't be specified twice");
> > > >          */
> > > >         try_runstate_set(RUN_STATE_INMIGRATE, &error_fatal);
> > > >         break;
> > > > 
> > > >   
> > > > > >                  incoming = optarg;
> > > > > >                  break;
> > > > > >              case QEMU_OPTION_only_migratable:    
> > > > > 
> > > > > Michal
> > > > >     
> > > >   
> > >   
> > 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2 1/2] vl: don't use RUN_STATE_PRECONFIG as initial state

[Daniel P. Berrangé]

On Mon, Jun 04, 2018 at 04:21:47PM +0200, Michal Privoznik wrote:
> On 06/04/2018 02:03 PM, Daniel P. Berrangé wrote:
> > @@ -3572,7 +3570,12 @@ int main(int argc, char **argv, char **envp)
> >                  }
> >                  break;
> >              case QEMU_OPTION_preconfig:
> > -                preconfig_exit_requested = false;
> > +                if (!runstate_check(RUN_STATE_NONE)) {
> > +                    error_report("'--preconfig' and '--incoming' options are "
> > +                                 "mutually exclusive");
> > +                    exit(EXIT_FAILURE);
> > +                }
> > +                runstate_set(RUN_STATE_PRECONFIG);
> 
> Specifying --preconfig twice on the command line now fails with a very
> cryptic message (there's no --incoming).

This can be addressed with a slight rewording

   "Only a single usage of --preconfig or --incoming is permitted"

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 v2 1/2] vl: don't use RUN_STATE_PRECONFIG as initial state

[Igor Mammedov]

On Mon,  4 Jun 2018 13:03:44 +0100
Daniel P. Berrangé <berrange@redhat.com> wrote:

> The RUN_STATE_PRECONFIG state is not supposed to be reachable unless the
> --preconfig argument is given to QEMU, but when it was introduced in:
> 
>   commit 047f7038f586d2150f16c6d9ba9cfd0479f0f6ac
>   Author: Igor Mammedov <imammedo@redhat.com>
>   Date:   Fri May 11 19:24:43 2018 +0200
> 
>     cli: add --preconfig option
> 
> ...the global 'current_run_state' variable was changed to have an initial
> value of RUN_STATE_PRECONFIG regardless of whether --preconfig is given.
> 
> A second invokation of main_loop() was added which then toggles it back
> to RUN_STATE_PRELAUNCH when --preconfig is not given. This is racy
> because it leaves a window where QEMU is in RUN_STATE_PRECONFIG despite
> --preconfig not being given.
Above statements isn't exactly correct, PRECONFIG were supposed to be
the new state of QEMU from start up till machine_run_board_init(),
that would separate stage of initial configuration out from all
encompassing PRELAUNCH state. So I'd scratch out above part.

> This can be seen with the failure:


> 
>   $ echo | x86_64-softmmu/qemu-system-x86_64 -monitor stdio
>   QEMU 2.12.50 monitor - type 'help' for more information
>   (qemu)
>   HMP not available in preconfig state, use QMP instead
Michal's patch is much simpler and fixes that cleanly.

There is another issue if you start above CLI with --preconfig
you'd get HMP prompt but won't be able to do anything there
including exiting/continuing as Markus pointed out. I'm exploring
which of 2 suggested ways [1] to address it is better.

1) http://patchwork.ozlabs.org/patch/908599/

> Using RUN_STATE_PRECONFIG required adding a state transition from
> RUN_STATE_PRECONFIG to RUN_STATE_INMIGRATE, which is undesirable as
> it prevented automatic detection of --preconfig & --incoming being
> mutually exclusive.
> 
> If we use RUN_STATE_PRELAUNCH as the initial value, however, we need to
> allow transitions between RUN_STATE_PRECONFIG & RUN_STATE_PRELAUNCH in
> both directions which is also undesirable, as RUN_STATE_PRECONFIG should
> be a one-time-only state that can never be returned to.
> 
> This change solves the confusion by introducing a further RUN_STATE_NONE
> which is used as the initial state value. This can transition to any of
> RUN_STATE_PRELAUNCH, RUN_STATE_PRECONFIG or RUN_STATE_INMIGRATE. This
> avoids the need to allow any undesirable state transitions.
Ugly mutually exclusive code including related long comments are
intentional. The plan is to streamline runstate transitions in
following order
  PRECONFIG -> PRELAUNCH [-> INMIGRATE]
i.e. postpone RUN_STATE_INMIGRATE transition to the later stage.
But that's requires some cleanup work to remove invariants 
in initialization depending if INMIGRATE state is in effect or not.

Hence I'd keep this part ugly as it is for now, and if we can do
without RUN_STATE_NONE it would be better, i.e. keep current
PRECONFIG runstate meaning where we would do initial configuration
either via CLI or via QMP and one less runstate to deal with.

I'd go with Michal's version of fix and think some more on
introducing RUN_STATE_NONE.

> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  qapi/run-state.json |  6 +++++-
>  vl.c                | 42 ++++++++++++++++++++++++------------------
>  2 files changed, 29 insertions(+), 19 deletions(-)
> 
> diff --git a/qapi/run-state.json b/qapi/run-state.json
> index 332e44897b..c3bd7b9b7a 100644
> --- a/qapi/run-state.json
> +++ b/qapi/run-state.json
> @@ -10,6 +10,10 @@
>  #
>  # An enumeration of VM run states.
>  #
> +# @none: QEMU is in early startup. This state should never be visible
> +# when querying state from the monitor, as QEMU will have already
> +# transitioned to another state. (Since 3.0)
> +#
>  # @debug: QEMU is running on a debugger
>  #
>  # @finish-migrate: guest is paused to finish the migration process
> @@ -54,7 +58,7 @@
>  #             (Since 3.0)
>  ##
>  { 'enum': 'RunState',
> -  'data': [ 'debug', 'inmigrate', 'internal-error', 'io-error', 'paused',
> +  'data': [ 'none', 'debug', 'inmigrate', 'internal-error', 'io-error', 'paused',
>              'postmigrate', 'prelaunch', 'finish-migrate', 'restore-vm',
>              'running', 'save-vm', 'shutdown', 'suspended', 'watchdog',
>              'guest-panicked', 'colo', 'preconfig' ] }
> diff --git a/vl.c b/vl.c
> index 06031715ac..30d0e985f0 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -561,7 +561,7 @@ static int default_driver_check(void *opaque, QemuOpts *opts, Error **errp)
>  /***********************************************************/
>  /* QEMU state */
>  
> -static RunState current_run_state = RUN_STATE_PRECONFIG;
> +static RunState current_run_state = RUN_STATE_NONE;
>  
>  /* We use RUN_STATE__MAX but any invalid value will do */
>  static RunState vmstop_requested = RUN_STATE__MAX;
> @@ -574,12 +574,11 @@ typedef struct {
>  
>  static const RunStateTransition runstate_transitions_def[] = {
>      /*     from      ->     to      */
> +    { RUN_STATE_NONE, RUN_STATE_PRELAUNCH },
> +    { RUN_STATE_NONE, RUN_STATE_PRECONFIG },
> +    { RUN_STATE_NONE, RUN_STATE_INMIGRATE },
> +
>      { RUN_STATE_PRECONFIG, RUN_STATE_PRELAUNCH },
> -      /* Early switch to inmigrate state to allow  -incoming CLI option work
> -       * as it used to. TODO: delay actual switching to inmigrate state to
> -       * the point after machine is built and remove this hack.
> -       */
Since patch isn't postponing INMIGRATE, it's too early to remove TODO item comment.

> -    { RUN_STATE_PRECONFIG, RUN_STATE_INMIGRATE },
>  
>      { RUN_STATE_DEBUG, RUN_STATE_RUNNING },
>      { RUN_STATE_DEBUG, RUN_STATE_FINISH_MIGRATE },
> @@ -618,7 +617,6 @@ static const RunStateTransition runstate_transitions_def[] = {
>  
>      { RUN_STATE_PRELAUNCH, RUN_STATE_RUNNING },
>      { RUN_STATE_PRELAUNCH, RUN_STATE_FINISH_MIGRATE },
> -    { RUN_STATE_PRELAUNCH, RUN_STATE_INMIGRATE },
>  
>      { RUN_STATE_FINISH_MIGRATE, RUN_STATE_RUNNING },
>      { RUN_STATE_FINISH_MIGRATE, RUN_STATE_PAUSED },
> @@ -1522,7 +1520,7 @@ static pid_t shutdown_pid;
>  static int powerdown_requested;
>  static int debug_requested;
>  static int suspend_requested;
> -static bool preconfig_exit_requested = true;
> +static bool preconfig_exit_requested;
>  static WakeupReason wakeup_reason;
>  static NotifierList powerdown_notifiers =
>      NOTIFIER_LIST_INITIALIZER(powerdown_notifiers);
> @@ -3572,7 +3570,12 @@ int main(int argc, char **argv, char **envp)
>                  }
>                  break;
>              case QEMU_OPTION_preconfig:
> -                preconfig_exit_requested = false;
> +                if (!runstate_check(RUN_STATE_NONE)) {
> +                    error_report("'--preconfig' and '--incoming' options are "
> +                                 "mutually exclusive");
> +                    exit(EXIT_FAILURE);
> +                }
> +                runstate_set(RUN_STATE_PRECONFIG);
>                  break;
>              case QEMU_OPTION_enable_kvm:
>                  olist = qemu_find_opts("machine");
> @@ -3768,9 +3771,12 @@ int main(int argc, char **argv, char **envp)
>                  }
>                  break;
>              case QEMU_OPTION_incoming:
> -                if (!incoming) {
> -                    runstate_set(RUN_STATE_INMIGRATE);
> +                if (!runstate_check(RUN_STATE_NONE)) {
> +                    error_report("'--preconfig' and '--incoming' options are "
> +                                 "mutually exclusive");
> +                    exit(EXIT_FAILURE);
>                  }
2 above hunks were a part of previous revisions of preconfig patchset,
however they were dropped in favor of one check later per Eduardo's
suggestion.

> +                runstate_set(RUN_STATE_INMIGRATE);
>                  incoming = optarg;
>                  break;
>              case QEMU_OPTION_only_migratable:
> @@ -3943,14 +3949,12 @@ int main(int argc, char **argv, char **envp)
>       */
>      loc_set_none();
>  
> -    replay_configure(icount_opts);
> -
> -    if (incoming && !preconfig_exit_requested) {
> -        error_report("'preconfig' and 'incoming' options are "
> -                     "mutually exclusive");
> -        exit(EXIT_FAILURE);
> +    if (runstate_check(RUN_STATE_NONE)) {
> +        runstate_set(RUN_STATE_PRELAUNCH);
>      }
>  
> +    replay_configure(icount_opts);
> +
>      machine_class = select_machine();
>  
>      set_memory_options(&ram_slots, &maxram_size, machine_class);
> @@ -4471,7 +4475,9 @@ 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 (runstate_check(RUN_STATE_PRECONFIG)) {
> +        main_loop();
> +    }
>  
>      /* from here on runstate is RUN_STATE_PRELAUNCH */
>      machine_run_board_init(current_machine);

Re: [Qemu-devel] [PATCH v2 1/2] vl: don't use RUN_STATE_PRECONFIG as initial state

[Daniel P. Berrangé]

On Mon, Jun 04, 2018 at 05:40:32PM +0200, Igor Mammedov wrote:
> On Mon,  4 Jun 2018 13:03:44 +0100
> Daniel P. Berrangé <berrange@redhat.com> wrote:
> 
> > The RUN_STATE_PRECONFIG state is not supposed to be reachable unless the
> > --preconfig argument is given to QEMU, but when it was introduced in:
> > 
> >   commit 047f7038f586d2150f16c6d9ba9cfd0479f0f6ac
> >   Author: Igor Mammedov <imammedo@redhat.com>
> >   Date:   Fri May 11 19:24:43 2018 +0200
> > 
> >     cli: add --preconfig option
> > 
> > ...the global 'current_run_state' variable was changed to have an initial
> > value of RUN_STATE_PRECONFIG regardless of whether --preconfig is given.
> > 
> > A second invokation of main_loop() was added which then toggles it back
> > to RUN_STATE_PRELAUNCH when --preconfig is not given. This is racy
> > because it leaves a window where QEMU is in RUN_STATE_PRECONFIG despite
> > --preconfig not being given.
> Above statements isn't exactly correct, PRECONFIG were supposed to be
> the new state of QEMU from start up till machine_run_board_init(),
> that would separate stage of initial configuration out from all
> encompassing PRELAUNCH state. So I'd scratch out above part.

That doesn't really make sense, given that --preconfig is *not* the
default and thus not supposed to be an active state unless the app
has explicitly opted in.

IMHO running PRECONFIG state for any period of time when the app
has not requested --preconfig is simply broken, and a recipe for
obscure bugs like the ones we've seen right now.


> >   $ echo | x86_64-softmmu/qemu-system-x86_64 -monitor stdio
> >   QEMU 2.12.50 monitor - type 'help' for more information
> >   (qemu)
> >   HMP not available in preconfig state, use QMP instead
> Michal's patch is much simpler and fixes that cleanly.

It is incomplete IMHO as it still leaves the hang in main loop
present - it merely avoids triggering it in one code path, and
leaves the other code path broken.

> > Using RUN_STATE_PRECONFIG required adding a state transition from
> > RUN_STATE_PRECONFIG to RUN_STATE_INMIGRATE, which is undesirable as
> > it prevented automatic detection of --preconfig & --incoming being
> > mutually exclusive.
> > 
> > If we use RUN_STATE_PRELAUNCH as the initial value, however, we need to
> > allow transitions between RUN_STATE_PRECONFIG & RUN_STATE_PRELAUNCH in
> > both directions which is also undesirable, as RUN_STATE_PRECONFIG should
> > be a one-time-only state that can never be returned to.
> > 
> > This change solves the confusion by introducing a further RUN_STATE_NONE
> > which is used as the initial state value. This can transition to any of
> > RUN_STATE_PRELAUNCH, RUN_STATE_PRECONFIG or RUN_STATE_INMIGRATE. This
> > avoids the need to allow any undesirable state transitions.
> Ugly mutually exclusive code including related long comments are
> intentional. The plan is to streamline runstate transitions in
> following order
>   PRECONFIG -> PRELAUNCH [-> INMIGRATE]

This doesn't make any conceptual sense to me, given that --preconfig
is an optional flag. We can't make --preconfig the default, because
of back compat, so we'll always have

   $START ------------->  PRELAUNCH {-> INMIGRATE]
     |              ^
     |              |
     +-- PRECONFIG -+

By marking the current state as PRECONFIG by default, we've essentially
given 2 meanings to  PRECONFIG - sometimes it means stuff  that can be
unconditionally run in early startup, and sometimes it means stuff that
can only be run if --preconfig is given. Introducing the separate NONE
state clarifies that inherant contradiction in startup phases.

> i.e. postpone RUN_STATE_INMIGRATE transition to the later stage.
> But that's requires some cleanup work to remove invariants 
> in initialization depending if INMIGRATE state is in effect or not.
> 
> Hence I'd keep this part ugly as it is for now, and if we can do
> without RUN_STATE_NONE it would be better, i.e. keep current
> PRECONFIG runstate meaning where we would do initial configuration
> either via CLI or via QMP and one less runstate to deal with.
> 
> I'd go with Michal's version of fix and think some more on
> introducing RUN_STATE_NONE.
> 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  qapi/run-state.json |  6 +++++-
> >  vl.c                | 42 ++++++++++++++++++++++++------------------
> >  2 files changed, 29 insertions(+), 19 deletions(-)
> > 
> > diff --git a/qapi/run-state.json b/qapi/run-state.json
> > index 332e44897b..c3bd7b9b7a 100644
> > --- a/qapi/run-state.json
> > +++ b/qapi/run-state.json
> > @@ -10,6 +10,10 @@
> >  #
> >  # An enumeration of VM run states.
> >  #
> > +# @none: QEMU is in early startup. This state should never be visible
> > +# when querying state from the monitor, as QEMU will have already
> > +# transitioned to another state. (Since 3.0)
> > +#
> >  # @debug: QEMU is running on a debugger
> >  #
> >  # @finish-migrate: guest is paused to finish the migration process
> > @@ -54,7 +58,7 @@
> >  #             (Since 3.0)
> >  ##
> >  { 'enum': 'RunState',
> > -  'data': [ 'debug', 'inmigrate', 'internal-error', 'io-error', 'paused',
> > +  'data': [ 'none', 'debug', 'inmigrate', 'internal-error', 'io-error', 'paused',
> >              'postmigrate', 'prelaunch', 'finish-migrate', 'restore-vm',
> >              'running', 'save-vm', 'shutdown', 'suspended', 'watchdog',
> >              'guest-panicked', 'colo', 'preconfig' ] }
> > diff --git a/vl.c b/vl.c
> > index 06031715ac..30d0e985f0 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -561,7 +561,7 @@ static int default_driver_check(void *opaque, QemuOpts *opts, Error **errp)
> >  /***********************************************************/
> >  /* QEMU state */
> >  
> > -static RunState current_run_state = RUN_STATE_PRECONFIG;
> > +static RunState current_run_state = RUN_STATE_NONE;
> >  
> >  /* We use RUN_STATE__MAX but any invalid value will do */
> >  static RunState vmstop_requested = RUN_STATE__MAX;
> > @@ -574,12 +574,11 @@ typedef struct {
> >  
> >  static const RunStateTransition runstate_transitions_def[] = {
> >      /*     from      ->     to      */
> > +    { RUN_STATE_NONE, RUN_STATE_PRELAUNCH },
> > +    { RUN_STATE_NONE, RUN_STATE_PRECONFIG },
> > +    { RUN_STATE_NONE, RUN_STATE_INMIGRATE },
> > +
> >      { RUN_STATE_PRECONFIG, RUN_STATE_PRELAUNCH },
> > -      /* Early switch to inmigrate state to allow  -incoming CLI option work
> > -       * as it used to. TODO: delay actual switching to inmigrate state to
> > -       * the point after machine is built and remove this hack.
> > -       */
> Since patch isn't postponing INMIGRATE, it's too early to remove TODO item comment.

The switch to INMIGRATE happens with -incoming is processed, at
which time the current state is NONE. -preconfig is mutually
exclusive with -incoming currently, so there's no way for there
to be a valid PRECONFIG -> INMIGRATE transition.

If we do figure out a way to make -incoming work with -preconfig,
then, this transition could become valid once more, but today it
simply shouldn't exist.

> 
> > -    { RUN_STATE_PRECONFIG, RUN_STATE_INMIGRATE },
> >  
> >      { RUN_STATE_DEBUG, RUN_STATE_RUNNING },
> >      { RUN_STATE_DEBUG, RUN_STATE_FINISH_MIGRATE },
> > @@ -618,7 +617,6 @@ static const RunStateTransition runstate_transitions_def[] = {
> >  
> >      { RUN_STATE_PRELAUNCH, RUN_STATE_RUNNING },
> >      { RUN_STATE_PRELAUNCH, RUN_STATE_FINISH_MIGRATE },
> > -    { RUN_STATE_PRELAUNCH, RUN_STATE_INMIGRATE },
> >  
> >      { RUN_STATE_FINISH_MIGRATE, RUN_STATE_RUNNING },
> >      { RUN_STATE_FINISH_MIGRATE, RUN_STATE_PAUSED },
> > @@ -1522,7 +1520,7 @@ static pid_t shutdown_pid;
> >  static int powerdown_requested;
> >  static int debug_requested;
> >  static int suspend_requested;
> > -static bool preconfig_exit_requested = true;
> > +static bool preconfig_exit_requested;
> >  static WakeupReason wakeup_reason;
> >  static NotifierList powerdown_notifiers =
> >      NOTIFIER_LIST_INITIALIZER(powerdown_notifiers);
> > @@ -3572,7 +3570,12 @@ int main(int argc, char **argv, char **envp)
> >                  }
> >                  break;
> >              case QEMU_OPTION_preconfig:
> > -                preconfig_exit_requested = false;
> > +                if (!runstate_check(RUN_STATE_NONE)) {
> > +                    error_report("'--preconfig' and '--incoming' options are "
> > +                                 "mutually exclusive");
> > +                    exit(EXIT_FAILURE);
> > +                }
> > +                runstate_set(RUN_STATE_PRECONFIG);
> >                  break;
> >              case QEMU_OPTION_enable_kvm:
> >                  olist = qemu_find_opts("machine");
> > @@ -3768,9 +3771,12 @@ int main(int argc, char **argv, char **envp)
> >                  }
> >                  break;
> >              case QEMU_OPTION_incoming:
> > -                if (!incoming) {
> > -                    runstate_set(RUN_STATE_INMIGRATE);
> > +                if (!runstate_check(RUN_STATE_NONE)) {
> > +                    error_report("'--preconfig' and '--incoming' options are "
> > +                                 "mutually exclusive");
> > +                    exit(EXIT_FAILURE);
> >                  }
> 2 above hunks were a part of previous revisions of preconfig patchset,
> however they were dropped in favor of one check later per Eduardo's
> suggestion.
> 
> > +                runstate_set(RUN_STATE_INMIGRATE);
> >                  incoming = optarg;
> >                  break;
> >              case QEMU_OPTION_only_migratable:
> > @@ -3943,14 +3949,12 @@ int main(int argc, char **argv, char **envp)
> >       */
> >      loc_set_none();
> >  
> > -    replay_configure(icount_opts);
> > -
> > -    if (incoming && !preconfig_exit_requested) {
> > -        error_report("'preconfig' and 'incoming' options are "
> > -                     "mutually exclusive");
> > -        exit(EXIT_FAILURE);
> > +    if (runstate_check(RUN_STATE_NONE)) {
> > +        runstate_set(RUN_STATE_PRELAUNCH);
> >      }
> >  
> > +    replay_configure(icount_opts);
> > +
> >      machine_class = select_machine();
> >  
> >      set_memory_options(&ram_slots, &maxram_size, machine_class);
> > @@ -4471,7 +4475,9 @@ 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 (runstate_check(RUN_STATE_PRECONFIG)) {
> > +        main_loop();
> > +    }
> >  
> >      /* from here on runstate is RUN_STATE_PRELAUNCH */
> >      machine_run_board_init(current_machine);
> 

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 v2 1/2] vl: don't use RUN_STATE_PRECONFIG as initial state

[Igor Mammedov]

On Mon, 4 Jun 2018 17:11:57 +0100
Daniel P. Berrangé <berrange@redhat.com> wrote:

> On Mon, Jun 04, 2018 at 05:40:32PM +0200, Igor Mammedov wrote:
> > On Mon,  4 Jun 2018 13:03:44 +0100
> > Daniel P. Berrangé <berrange@redhat.com> wrote:
> >   
> > > The RUN_STATE_PRECONFIG state is not supposed to be reachable unless the
> > > --preconfig argument is given to QEMU, but when it was introduced in:
> > > 
> > >   commit 047f7038f586d2150f16c6d9ba9cfd0479f0f6ac
> > >   Author: Igor Mammedov <imammedo@redhat.com>
> > >   Date:   Fri May 11 19:24:43 2018 +0200
> > > 
> > >     cli: add --preconfig option
> > > 
> > > ...the global 'current_run_state' variable was changed to have an initial
> > > value of RUN_STATE_PRECONFIG regardless of whether --preconfig is given.
> > > 
> > > A second invokation of main_loop() was added which then toggles it back
> > > to RUN_STATE_PRELAUNCH when --preconfig is not given. This is racy
> > > because it leaves a window where QEMU is in RUN_STATE_PRECONFIG despite
> > > --preconfig not being given.  
> > Above statements isn't exactly correct, PRECONFIG were supposed to be
> > the new state of QEMU from start up till machine_run_board_init(),
> > that would separate stage of initial configuration out from all
> > encompassing PRELAUNCH state. So I'd scratch out above part.  
> 
> That doesn't really make sense, given that --preconfig is *not* the
> default and thus not supposed to be an active state unless the app
> has explicitly opted in.
>
> IMHO running PRECONFIG state for any period of time when the app
> has not requested --preconfig is simply broken, and a recipe for
> obscure bugs like the ones we've seen right now.
mgmt hasn't opted in for default PRELAUNCH either, for it default
PRECONFIG runstate is not visible unless it opts in so nothing
is broken in regards to this.
Default runstate selection is QEMU's internal impl. detail.

I don't think it's the reason for obscure bugs, it's rather
exposing so far hidden issues out there. The current startup
code is a mess and were blindly assuming PRELAUNCH state,
using globals to jump from one state to another depending on
CLI options combination.
So moving main_loop() earlier exposed a number of issues,
that should be fixed.
 
> > >   $ echo | x86_64-softmmu/qemu-system-x86_64 -monitor stdio
> > >   QEMU 2.12.50 monitor - type 'help' for more information
> > >   (qemu)
> > >   HMP not available in preconfig state, use QMP instead  
> > Michal's patch is much simpler and fixes that cleanly.  
> 
> It is incomplete IMHO as it still leaves the hang in main loop
> present - it merely avoids triggering it in one code path, and
> leaves the other code path broken.
It's new behavior which looses sync point on parent QEMU process exit,
hence it's not convenient for mgmt that currently expects parent
qemu process exit when it's demonized.

Since it's new option, there are 2 ways to deal with it:
  * make parent process exit earlier before main loop as you suggest in 2/2
    and teach mgmt to deal with initialization errors cleanly after
    sync point
  * teach mgmt to connect to QMP socket earlier if --preconfig were used,
    (benefit error handling works as it used to be)

I'd happy with any of this as far as user won't be confused if something
goes wrong.

> > > Using RUN_STATE_PRECONFIG required adding a state transition from
> > > RUN_STATE_PRECONFIG to RUN_STATE_INMIGRATE, which is undesirable as
> > > it prevented automatic detection of --preconfig & --incoming being
> > > mutually exclusive.
> > > 
> > > If we use RUN_STATE_PRELAUNCH as the initial value, however, we need to
> > > allow transitions between RUN_STATE_PRECONFIG & RUN_STATE_PRELAUNCH in
> > > both directions which is also undesirable, as RUN_STATE_PRECONFIG should
> > > be a one-time-only state that can never be returned to.
> > > 
> > > This change solves the confusion by introducing a further RUN_STATE_NONE
> > > which is used as the initial state value. This can transition to any of
> > > RUN_STATE_PRELAUNCH, RUN_STATE_PRECONFIG or RUN_STATE_INMIGRATE. This
> > > avoids the need to allow any undesirable state transitions.  
> > Ugly mutually exclusive code including related long comments are
> > intentional. The plan is to streamline runstate transitions in
> > following order
> >   PRECONFIG -> PRELAUNCH [-> INMIGRATE]  
> 
> This doesn't make any conceptual sense to me, given that --preconfig
> is an optional flag. We can't make --preconfig the default, because
> of back compat, so we'll always have
where is the back compat issue with preconfig default?
(it's not visible to mgmt unless --preconfig option is used)

>    $START ------------->  PRELAUNCH {-> INMIGRATE]
>      |              ^
>      |              |
>      +-- PRECONFIG -+
> 
> By marking the current state as PRECONFIG by default, we've essentially
> given 2 meanings to  PRECONFIG - sometimes it means stuff  that can be
> unconditionally run in early startup, and sometimes it means stuff that
> can only be run if --preconfig is given. Introducing the separate NONE
> state clarifies that inherant contradiction in startup phases.
Yep, one can say it this way (as merged PRECONFIG was early
configuration stage regardless of if it's unconditional early
initialization or early CLI parsing/QMP configuration).

Maybe adding NONE state makes sense but I'm not quite sure,
that's why I'd not rush it in and discuss if we really need
fragment early configuration into more stages.
(we can do it later as both stages aren't visible to user by default).

> > i.e. postpone RUN_STATE_INMIGRATE transition to the later stage.
> > But that's requires some cleanup work to remove invariants 
> > in initialization depending if INMIGRATE state is in effect or not.
> > 
> > Hence I'd keep this part ugly as it is for now, and if we can do
> > without RUN_STATE_NONE it would be better, i.e. keep current
> > PRECONFIG runstate meaning where we would do initial configuration
> > either via CLI or via QMP and one less runstate to deal with.
> > 
> > I'd go with Michal's version of fix and think some more on
> > introducing RUN_STATE_NONE.
> >   
> > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > > ---
> > >  qapi/run-state.json |  6 +++++-
> > >  vl.c                | 42 ++++++++++++++++++++++++------------------
> > >  2 files changed, 29 insertions(+), 19 deletions(-)
> > > 
> > > diff --git a/qapi/run-state.json b/qapi/run-state.json
> > > index 332e44897b..c3bd7b9b7a 100644
> > > --- a/qapi/run-state.json
> > > +++ b/qapi/run-state.json
> > > @@ -10,6 +10,10 @@
> > >  #
> > >  # An enumeration of VM run states.
> > >  #
> > > +# @none: QEMU is in early startup. This state should never be visible
> > > +# when querying state from the monitor, as QEMU will have already
> > > +# transitioned to another state. (Since 3.0)
> > > +#
> > >  # @debug: QEMU is running on a debugger
> > >  #
> > >  # @finish-migrate: guest is paused to finish the migration process
> > > @@ -54,7 +58,7 @@
> > >  #             (Since 3.0)
> > >  ##
> > >  { 'enum': 'RunState',
> > > -  'data': [ 'debug', 'inmigrate', 'internal-error', 'io-error', 'paused',
> > > +  'data': [ 'none', 'debug', 'inmigrate', 'internal-error', 'io-error', 'paused',
> > >              'postmigrate', 'prelaunch', 'finish-migrate', 'restore-vm',
> > >              'running', 'save-vm', 'shutdown', 'suspended', 'watchdog',
> > >              'guest-panicked', 'colo', 'preconfig' ] }
> > > diff --git a/vl.c b/vl.c
> > > index 06031715ac..30d0e985f0 100644
> > > --- a/vl.c
> > > +++ b/vl.c
> > > @@ -561,7 +561,7 @@ static int default_driver_check(void *opaque, QemuOpts *opts, Error **errp)
> > >  /***********************************************************/
> > >  /* QEMU state */
> > >  
> > > -static RunState current_run_state = RUN_STATE_PRECONFIG;
> > > +static RunState current_run_state = RUN_STATE_NONE;
> > >  
> > >  /* We use RUN_STATE__MAX but any invalid value will do */
> > >  static RunState vmstop_requested = RUN_STATE__MAX;
> > > @@ -574,12 +574,11 @@ typedef struct {
> > >  
> > >  static const RunStateTransition runstate_transitions_def[] = {
> > >      /*     from      ->     to      */
> > > +    { RUN_STATE_NONE, RUN_STATE_PRELAUNCH },
> > > +    { RUN_STATE_NONE, RUN_STATE_PRECONFIG },
> > > +    { RUN_STATE_NONE, RUN_STATE_INMIGRATE },
> > > +
> > >      { RUN_STATE_PRECONFIG, RUN_STATE_PRELAUNCH },
> > > -      /* Early switch to inmigrate state to allow  -incoming CLI option work
> > > -       * as it used to. TODO: delay actual switching to inmigrate state to
> > > -       * the point after machine is built and remove this hack.
> > > -       */  
> > Since patch isn't postponing INMIGRATE, it's too early to remove TODO item comment.  
> 
> The switch to INMIGRATE happens with -incoming is processed, at
It's just evolved this way but that doesn't mean that switching
to INMIGRATE should happen when -incoming option is processed,.
More correct would be to switch to INMIGRATE state after
machine is initialized and apply incoming state on top of it,
but that needs cleanup (on my todo list) to make sure that
-incoming doesn't influence machine initialization.

> which time the current state is NONE. -preconfig is mutually
> exclusive with -incoming currently, so there's no way for there
> to be a valid PRECONFIG -> INMIGRATE transition.
> 
> If we do figure out a way to make -incoming work with -preconfig,
> then, this transition could become valid once more, but today it
> simply shouldn't exist.
It's a hack as well as PRELAUNCH -> INMIGRATE and marked as
such with TODO note. Adding NONE -> INMIGRATE transition is
the same though. It's the hack to keep -incoming working without
refactoring -incoming parsing to postpone transition to point
where machine is initialized. So if we would add NONE runstate,
the TODO item still applies but this time to NONE state.

> >   
> > > -    { RUN_STATE_PRECONFIG, RUN_STATE_INMIGRATE },
> > >  
> > >      { RUN_STATE_DEBUG, RUN_STATE_RUNNING },
> > >      { RUN_STATE_DEBUG, RUN_STATE_FINISH_MIGRATE },
> > > @@ -618,7 +617,6 @@ static const RunStateTransition runstate_transitions_def[] = {
> > >  
> > >      { RUN_STATE_PRELAUNCH, RUN_STATE_RUNNING },
> > >      { RUN_STATE_PRELAUNCH, RUN_STATE_FINISH_MIGRATE },
> > > -    { RUN_STATE_PRELAUNCH, RUN_STATE_INMIGRATE },
> > >  
> > >      { RUN_STATE_FINISH_MIGRATE, RUN_STATE_RUNNING },
> > >      { RUN_STATE_FINISH_MIGRATE, RUN_STATE_PAUSED },
[...]

Re: [Qemu-devel] [PATCH v2 1/2] vl: don't use RUN_STATE_PRECONFIG as initial state

[Eduardo Habkost]

On Mon, Jun 04, 2018 at 07:37:15PM +0200, Igor Mammedov wrote:
> On Mon, 4 Jun 2018 17:11:57 +0100
> Daniel P. Berrangé <berrange@redhat.com> wrote:
> 
> > On Mon, Jun 04, 2018 at 05:40:32PM +0200, Igor Mammedov wrote:
> > > On Mon,  4 Jun 2018 13:03:44 +0100
> > > Daniel P. Berrangé <berrange@redhat.com> wrote:
> > >   
> > > > The RUN_STATE_PRECONFIG state is not supposed to be reachable unless the
> > > > --preconfig argument is given to QEMU, but when it was introduced in:
> > > > 
> > > >   commit 047f7038f586d2150f16c6d9ba9cfd0479f0f6ac
> > > >   Author: Igor Mammedov <imammedo@redhat.com>
> > > >   Date:   Fri May 11 19:24:43 2018 +0200
> > > > 
> > > >     cli: add --preconfig option
> > > > 
> > > > ...the global 'current_run_state' variable was changed to have an initial
> > > > value of RUN_STATE_PRECONFIG regardless of whether --preconfig is given.
> > > > 
> > > > A second invokation of main_loop() was added which then toggles it back
> > > > to RUN_STATE_PRELAUNCH when --preconfig is not given. This is racy
> > > > because it leaves a window where QEMU is in RUN_STATE_PRECONFIG despite
> > > > --preconfig not being given.  
> > > Above statements isn't exactly correct, PRECONFIG were supposed to be
> > > the new state of QEMU from start up till machine_run_board_init(),
> > > that would separate stage of initial configuration out from all
> > > encompassing PRELAUNCH state. So I'd scratch out above part.  
> > 
> > That doesn't really make sense, given that --preconfig is *not* the
> > default and thus not supposed to be an active state unless the app
> > has explicitly opted in.
> >
> > IMHO running PRECONFIG state for any period of time when the app
> > has not requested --preconfig is simply broken, and a recipe for
> > obscure bugs like the ones we've seen right now.
> mgmt hasn't opted in for default PRELAUNCH either, for it default
> PRECONFIG runstate is not visible unless it opts in so nothing
> is broken in regards to this.
> Default runstate selection is QEMU's internal impl. detail.

Daniel's description is how he expects it to work, but your
description reflects the way the state machine actually works
today (and how it worked befor the --preconfig series).

However, I agree with Daniel that moving to PRECONFIG or
PRELAUNCH if neither --preconfig or -S were specified is
confusing, and I would prefer to change it the way he suggests.

But:

[...]
> >    $START ------------->  PRELAUNCH {-> INMIGRATE]
> >      |              ^
> >      |              |
> >      +-- PRECONFIG -+
> > 
> > By marking the current state as PRECONFIG by default, we've essentially
> > given 2 meanings to  PRECONFIG - sometimes it means stuff  that can be
> > unconditionally run in early startup, and sometimes it means stuff that
> > can only be run if --preconfig is given. Introducing the separate NONE
> > state clarifies that inherant contradiction in startup phases.
> Yep, one can say it this way (as merged PRECONFIG was early
> configuration stage regardless of if it's unconditional early
> initialization or early CLI parsing/QMP configuration).
> 
> Maybe adding NONE state makes sense but I'm not quite sure,
> that's why I'd not rush it in and discuss if we really need
> fragment early configuration into more stages.
> (we can do it later as both stages aren't visible to user by default).

Is it possible to fix the bugs first, and discuss how to refactor
the state machine later?

In the meantime, we could even document preconfig more accurately
to avoid additional confusion:

# @preconfig: Board initialization was not run yet.  The state is
#             visible to the outside only if the --preconfig CLI
#             option is used.  (Since 3.0)

-- 
Eduardo

Re: [Qemu-devel] [PATCH v2 1/2] vl: don't use RUN_STATE_PRECONFIG as initial state

[Igor Mammedov]

On Mon, 4 Jun 2018 21:35:46 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> On Mon, Jun 04, 2018 at 07:37:15PM +0200, Igor Mammedov wrote:
> > On Mon, 4 Jun 2018 17:11:57 +0100
> > Daniel P. Berrangé <berrange@redhat.com> wrote:
> >   
> > > On Mon, Jun 04, 2018 at 05:40:32PM +0200, Igor Mammedov wrote:  
> > > > On Mon,  4 Jun 2018 13:03:44 +0100
> > > > Daniel P. Berrangé <berrange@redhat.com> wrote:
> > > >     
> > > > > The RUN_STATE_PRECONFIG state is not supposed to be reachable unless the
> > > > > --preconfig argument is given to QEMU, but when it was introduced in:
> > > > > 
> > > > >   commit 047f7038f586d2150f16c6d9ba9cfd0479f0f6ac
> > > > >   Author: Igor Mammedov <imammedo@redhat.com>
> > > > >   Date:   Fri May 11 19:24:43 2018 +0200
> > > > > 
> > > > >     cli: add --preconfig option
> > > > > 
> > > > > ...the global 'current_run_state' variable was changed to have an initial
> > > > > value of RUN_STATE_PRECONFIG regardless of whether --preconfig is given.
> > > > > 
> > > > > A second invokation of main_loop() was added which then toggles it back
> > > > > to RUN_STATE_PRELAUNCH when --preconfig is not given. This is racy
> > > > > because it leaves a window where QEMU is in RUN_STATE_PRECONFIG despite
> > > > > --preconfig not being given.    
> > > > Above statements isn't exactly correct, PRECONFIG were supposed to be
> > > > the new state of QEMU from start up till machine_run_board_init(),
> > > > that would separate stage of initial configuration out from all
> > > > encompassing PRELAUNCH state. So I'd scratch out above part.    
> > > 
> > > That doesn't really make sense, given that --preconfig is *not* the
> > > default and thus not supposed to be an active state unless the app
> > > has explicitly opted in.
> > >
> > > IMHO running PRECONFIG state for any period of time when the app
> > > has not requested --preconfig is simply broken, and a recipe for
> > > obscure bugs like the ones we've seen right now.  
> > mgmt hasn't opted in for default PRELAUNCH either, for it default
> > PRECONFIG runstate is not visible unless it opts in so nothing
> > is broken in regards to this.
> > Default runstate selection is QEMU's internal impl. detail.  
> 
> Daniel's description is how he expects it to work, but your
> description reflects the way the state machine actually works
> today (and how it worked befor the --preconfig series).
> 
> However, I agree with Daniel that moving to PRECONFIG or
> PRELAUNCH if neither --preconfig or -S were specified is
> confusing, and I would prefer to change it the way he suggests.
> 
> But:
> 
> [...]
> > >    $START ------------->  PRELAUNCH {-> INMIGRATE]
> > >      |              ^
> > >      |              |
> > >      +-- PRECONFIG -+
> > > 
> > > By marking the current state as PRECONFIG by default, we've essentially
> > > given 2 meanings to  PRECONFIG - sometimes it means stuff  that can be
> > > unconditionally run in early startup, and sometimes it means stuff that
> > > can only be run if --preconfig is given. Introducing the separate NONE
> > > state clarifies that inherant contradiction in startup phases.  
> > Yep, one can say it this way (as merged PRECONFIG was early
> > configuration stage regardless of if it's unconditional early
> > initialization or early CLI parsing/QMP configuration).
> > 
> > Maybe adding NONE state makes sense but I'm not quite sure,
> > that's why I'd not rush it in and discuss if we really need
> > fragment early configuration into more stages.
> > (we can do it later as both stages aren't visible to user by default).  
> 
> Is it possible to fix the bugs first, and discuss how to refactor
> the state machine later?
Agreed, we can discuss and settle this internal to QEMU implementation
detail independently on top of actual fix.

> In the meantime, we could even document preconfig more accurately
> to avoid additional confusion:
> 
> # @preconfig: Board initialization was not run yet.  The state is
> #             visible to the outside only if the --preconfig CLI
> #             option is used.  (Since 3.0)

I'll post a proper patch with it.

Re: [Qemu-devel] [PATCH v2 1/2] vl: don't use RUN_STATE_PRECONFIG as initial state

[Daniel P. Berrangé]

On Mon, Jun 04, 2018 at 01:03:44PM +0100, Daniel P. Berrangé wrote:

> diff --git a/vl.c b/vl.c
> index 06031715ac..30d0e985f0 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -561,7 +561,7 @@ static int default_driver_check(void *opaque, QemuOpts *opts, Error **errp)
>  /***********************************************************/
>  /* QEMU state */
>  
> -static RunState current_run_state = RUN_STATE_PRECONFIG;
> +static RunState current_run_state = RUN_STATE_NONE;

BTW, an alternative to introducing a RNU_STATE_NONE is to just use
an intentionally invalid enum value here eg

   static RunState current_run_state = -1;

on the basis that while we're parsing argv[], current_run_state
is not really meaningful. Only once we're done with argv[] do
we know what valid state will be used as the initial value of
current_run_state.

This would avoid exposing the "none" concept in QAPI schema,
or the runstate transitions list.

>  
>  /* We use RUN_STATE__MAX but any invalid value will do */
>  static RunState vmstop_requested = RUN_STATE__MAX;
> @@ -574,12 +574,11 @@ typedef struct {
>  
>  static const RunStateTransition runstate_transitions_def[] = {
>      /*     from      ->     to      */
> +    { RUN_STATE_NONE, RUN_STATE_PRELAUNCH },
> +    { RUN_STATE_NONE, RUN_STATE_PRECONFIG },
> +    { RUN_STATE_NONE, RUN_STATE_INMIGRATE },
> +
>      { RUN_STATE_PRECONFIG, RUN_STATE_PRELAUNCH },
> -      /* Early switch to inmigrate state to allow  -incoming CLI option work
> -       * as it used to. TODO: delay actual switching to inmigrate state to
> -       * the point after machine is built and remove this hack.
> -       */
> -    { RUN_STATE_PRECONFIG, RUN_STATE_INMIGRATE },
>  
>      { RUN_STATE_DEBUG, RUN_STATE_RUNNING },
>      { RUN_STATE_DEBUG, RUN_STATE_FINISH_MIGRATE },
> @@ -618,7 +617,6 @@ static const RunStateTransition runstate_transitions_def[] = {
>  
>      { RUN_STATE_PRELAUNCH, RUN_STATE_RUNNING },
>      { RUN_STATE_PRELAUNCH, RUN_STATE_FINISH_MIGRATE },
> -    { RUN_STATE_PRELAUNCH, RUN_STATE_INMIGRATE },
>  
>      { RUN_STATE_FINISH_MIGRATE, RUN_STATE_RUNNING },
>      { RUN_STATE_FINISH_MIGRATE, RUN_STATE_PAUSED },
> @@ -1522,7 +1520,7 @@ static pid_t shutdown_pid;
>  static int powerdown_requested;
>  static int debug_requested;
>  static int suspend_requested;
> -static bool preconfig_exit_requested = true;
> +static bool preconfig_exit_requested;
>  static WakeupReason wakeup_reason;
>  static NotifierList powerdown_notifiers =
>      NOTIFIER_LIST_INITIALIZER(powerdown_notifiers);
> @@ -3572,7 +3570,12 @@ int main(int argc, char **argv, char **envp)
>                  }
>                  break;
>              case QEMU_OPTION_preconfig:
> -                preconfig_exit_requested = false;
> +                if (!runstate_check(RUN_STATE_NONE)) {
> +                    error_report("'--preconfig' and '--incoming' options are "
> +                                 "mutually exclusive");
> +                    exit(EXIT_FAILURE);
> +                }
> +                runstate_set(RUN_STATE_PRECONFIG);
>                  break;
>              case QEMU_OPTION_enable_kvm:
>                  olist = qemu_find_opts("machine");
> @@ -3768,9 +3771,12 @@ int main(int argc, char **argv, char **envp)
>                  }
>                  break;
>              case QEMU_OPTION_incoming:
> -                if (!incoming) {
> -                    runstate_set(RUN_STATE_INMIGRATE);
> +                if (!runstate_check(RUN_STATE_NONE)) {
> +                    error_report("'--preconfig' and '--incoming' options are "
> +                                 "mutually exclusive");
> +                    exit(EXIT_FAILURE);
>                  }
> +                runstate_set(RUN_STATE_INMIGRATE);
>                  incoming = optarg;
>                  break;
>              case QEMU_OPTION_only_migratable:
> @@ -3943,14 +3949,12 @@ int main(int argc, char **argv, char **envp)
>       */
>      loc_set_none();
>  
> -    replay_configure(icount_opts);
> -
> -    if (incoming && !preconfig_exit_requested) {
> -        error_report("'preconfig' and 'incoming' options are "
> -                     "mutually exclusive");
> -        exit(EXIT_FAILURE);
> +    if (runstate_check(RUN_STATE_NONE)) {
> +        runstate_set(RUN_STATE_PRELAUNCH);
>      }
>  
> +    replay_configure(icount_opts);
> +
>      machine_class = select_machine();
>  
>      set_memory_options(&ram_slots, &maxram_size, machine_class);
> @@ -4471,7 +4475,9 @@ 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 (runstate_check(RUN_STATE_PRECONFIG)) {
> +        main_loop();
> +    }
>  
>      /* from here on runstate is RUN_STATE_PRELAUNCH */
>      machine_run_board_init(current_machine);
> -- 
> 2.17.0
> 

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

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

[Daniel P. Berrangé]

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

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

[Michal Privoznik]

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

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

[Igor Mammedov]

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

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

[Daniel P. Berrangé]

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

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

[Igor Mammedov]

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.

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

[Eduardo Habkost]

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

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

[Igor Mammedov]

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

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

[Igor Mammedov]

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)

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

[Daniel P. Berrangé]

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

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

[Igor Mammedov]

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

Re: [Qemu-devel] [PATCH v2 0/2] Avoid using RUN_STATE_PRECONFIG unless explicitly requested with --preconfig

[Michal Prívozník]

On 06/04/2018 02:03 PM, Daniel P. Berrangé wrote:
> The change to use RUN_STATE_PRECONFIG as the default initial state, even
> when not requested with --preconfig has caused a number of problems. This
> series introduces a new RUN_STATE_NONE to act as the initial state, so
> that we never use RUN_STATE_PRECONFIG unless the mgmt app has explicitly
> requested todo so.
> 
> Daniel P. Berrangé (2):
>   vl: don't use RUN_STATE_PRECONFIG as initial state
>   vl: fix use of --daemonize with --preconfig
> 
>  qapi/run-state.json |  6 +++++-
>  vl.c                | 49 +++++++++++++++++++++++++++------------------
>  2 files changed, 35 insertions(+), 20 deletions(-)
> 

So do we have some agreement here? I'm running qemu from git and I'm
still using my patch to make libvirt work.

Michal