Avoid using RUN_STATE_PRECONFIG unless explicitly requested with --preconfig

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 <[email protected]>
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é <[email protected]>

---
 qapi/run-state.json |  6 +++++-
 vl.c                | 42 ++++++++++++++++++++++++------------------
 2 files changed, 29 insertions(+), 19 deletions(-)

qapi/run-state.json +5 -1

index 332e44897b..c3bd7b9b7a 100644
@@ -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' ] }

vl.c +6 -8

index 06031715ac..30d0e985f0 100644
@@ -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);

vl.c +0 -0

                 break;
             case QEMU_OPTION_enable_kvm:
                 olist = qemu_find_opts("machine");

@@ -3768,9 +3771,12 @@ int main(int argc, char **argv, char **envp)
                 }

vl.c int main(int argc, char **argv, char **envp) · line 3578

Quoted 11 lines

vl.c +6 -1

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

Quoted 10 lines

vl.c +1 -2

                 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)) {

vl.c int main(int argc, char **argv, char **envp) · line 3779

Quoted 5 lines

vl.c +4 -0

+                    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.

vl.c int main(int argc, char **argv, char **envp) · line 3782

Quoted 3 lines

vl.c +0 -0

                 incoming = optarg;
                 break;
             case QEMU_OPTION_only_migratable:

Michal

On Mon, Jun 04, 2018 at 04:21:47PM +0200, Michal Privoznik wrote:
[...]

Quoted 25 lines

vl.c int main(int argc, char **argv, char **envp) · line 3578

Quoted 11 lines

vl.c +6 -1

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

Quoted 10 lines

vl.c +1 -2

                 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)) {

Quoted 9 lines

vl.c int main(int argc, char **argv, char **envp) · line 3779

Quoted 5 lines

vl.c +4 -0

+                    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;

Quoted 6 lines

vl.c int main(int argc, char **argv, char **envp) · line 3782

Quoted 3 lines

vl.c +0 -0

                 incoming = optarg;
                 break;
             case QEMU_OPTION_only_migratable:  

Michal

Quoted 37 lines · On Mon, 4 Jun 2018 21:56:47 -0300 Eduardo Habkost <[email protected]> wrote:

On Mon, Jun 04, 2018 at 04:21:47PM +0200, Michal Privoznik wrote:
[...]

Quoted 34 lines

vl.c int main(int argc, char **argv, char **envp) · line 3578

Quoted 11 lines

vl.c +6 -1

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

Quoted 5 lines

vl.c +0 -0

                 break;
             case QEMU_OPTION_enable_kvm:
                 olist = qemu_find_opts("machine");

@@ -3768,9 +3771,12 @@ int main(int argc, char **argv, char **envp)
                 }

vl.c int main(int argc, char **argv, char **envp) · line 3779

Quoted 10 lines

vl.c +5 -2

                 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.

Quoted 4 lines

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

Quoted 28 lines

    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;

Quoted 6 lines

vl.c int main(int argc, char **argv, char **envp) · line 3782

Quoted 3 lines

vl.c +0 -0

                 incoming = optarg;
                 break;
             case QEMU_OPTION_only_migratable:  

Michal

Quoted 43 lines · Igor Mammedov

Quoted 41 lines · On Mon, 4 Jun 2018 21:56:47 -0300 Eduardo Habkost <[email protected]> wrote:

On Mon, Jun 04, 2018 at 04:21:47PM +0200, Michal Privoznik wrote:
[...]

Quoted 25 lines

vl.c int main(int argc, char **argv, char **envp) · line 3578

Quoted 11 lines

vl.c +6 -1

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

Quoted 10 lines

vl.c +1 -2

                 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)) {

Quoted 9 lines

vl.c int main(int argc, char **argv, char **envp) · line 3779

Quoted 5 lines

vl.c +4 -0

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

Quoted 4 lines

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

Quoted 30 lines

Quoted 29 lines

    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;

Quoted 6 lines

vl.c int main(int argc, char **argv, char **envp) · line 3782

Quoted 3 lines

vl.c +0 -0

                 incoming = optarg;
                 break;
             case QEMU_OPTION_only_migratable:    

Michal

Quoted 49 lines · On Tue, 5 Jun 2018 09:01:09 -0300 Eduardo Habkost <[email protected]> wrote:

Quoted 47 lines · Igor Mammedov

Quoted 37 lines · On Mon, 4 Jun 2018 21:56:47 -0300 Eduardo Habkost <[email protected]> wrote:

On Mon, Jun 04, 2018 at 04:21:47PM +0200, Michal Privoznik wrote:
[...]

Quoted 25 lines

vl.c int main(int argc, char **argv, char **envp) · line 3578

Quoted 11 lines

vl.c +6 -1

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

Quoted 10 lines

vl.c +1 -2

                 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)) {

Quoted 9 lines

vl.c int main(int argc, char **argv, char **envp) · line 3779

Quoted 5 lines

vl.c +4 -0

+                    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.

Quoted 4 lines

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

Quoted 3 lines

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.

Quoted 32 lines

Quoted 31 lines

Quoted 30 lines

    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;

Quoted 6 lines

vl.c int main(int argc, char **argv, char **envp) · line 3782

Quoted 3 lines

vl.c +0 -0

                 incoming = optarg;
                 break;
             case QEMU_OPTION_only_migratable:    

Michal

vl.c int main(int argc, char **argv, char **envp) · line 3578

Quoted 11 lines · Daniel P. Berrangé

vl.c +6 -1

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

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 <[email protected]>
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é <[email protected]>

---
 qapi/run-state.json |  6 +++++-
 vl.c                | 42 ++++++++++++++++++++++++------------------
 2 files changed, 29 insertions(+), 19 deletions(-)

qapi/run-state.json +5 -1

index 332e44897b..c3bd7b9b7a 100644
@@ -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' ] }

vl.c +1 -1

index 06031715ac..30d0e985f0 100644
@@ -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[] = {

vl.c +7 -4

-    { 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");

vl.c +8 -7

+                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);

Quoted 12 lines · On Mon, 4 Jun 2018 13:03:44 +0100 Daniel P. Berrangé <[email protected]>…

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 <[email protected]>
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.

Quoted 4 lines

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.

Quoted 4 lines

  $ 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.

Quoted 10 lines

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.

Quoted 4 lines

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.

Quoted 45 lines

Signed-off-by Daniel P. Berrangé <[email protected]>

---
 qapi/run-state.json |  6 +++++-
 vl.c                | 42 ++++++++++++++++++++++++------------------
 2 files changed, 29 insertions(+), 19 deletions(-)

qapi/run-state.json +5 -1

index 332e44897b..c3bd7b9b7a 100644
@@ -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' ] }

vl.c +1 -1

index 06031715ac..30d0e985f0 100644
@@ -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 {
 

vl.c typedef struct { · line 581

Quoted 11 lines

vl.c +4 -4

 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.

Quoted 37 lines

vl.c +7 -4

-    { 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");

vl.c int main(int argc, char **argv, char **envp) · line 3778

Quoted 10 lines

vl.c +4 -2

@@ -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.

Quoted 26 lines

vl.c +5 -6

+                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);
 

vl.c int main(int argc, char **argv, char **envp) · line 4483

Quoted 8 lines

vl.c +3 -1

     /* 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);

Quoted 15 lines · Igor Mammedov

Quoted 12 lines · On Mon, 4 Jun 2018 13:03:44 +0100 Daniel P. Berrangé <[email protected]>…

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 <[email protected]>
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.

Quoted 8 lines

Quoted 4 lines

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.

Quoted 5 lines

Quoted 4 lines

  $ 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.

Quoted 14 lines

Quoted 14 lines

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.

Quoted 4 lines

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.

Quoted 57 lines

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.

Quoted 45 lines

Signed-off-by Daniel P. Berrangé <[email protected]>

---
 qapi/run-state.json |  6 +++++-
 vl.c                | 42 ++++++++++++++++++++++++------------------
 2 files changed, 29 insertions(+), 19 deletions(-)

qapi/run-state.json +5 -1

index 332e44897b..c3bd7b9b7a 100644
@@ -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' ] }

vl.c +1 -1

index 06031715ac..30d0e985f0 100644
@@ -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 {
 

Quoted 12 lines

vl.c typedef struct { · line 581

Quoted 11 lines

vl.c +4 -4

 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.

Quoted 13 lines

Quoted 13 lines

vl.c +0 -2

-    { 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 },

Quoted 29 lines · On Mon, 4 Jun 2018 17:11:57 +0100 Daniel P. Berrangé <[email protected]>…

Quoted 15 lines · Igor Mammedov

Quoted 12 lines · On Mon, 4 Jun 2018 13:03:44 +0100 Daniel P. Berrangé <[email protected]>…

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 <[email protected]>
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.

Quoted 8 lines

Quoted 4 lines

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.

Quoted 3 lines

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.

Quoted 10 lines

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

Quoted 32 lines · Igor Mammedov

Quoted 29 lines · On Mon, 4 Jun 2018 17:11:57 +0100 Daniel P. Berrangé <[email protected]>…

Quoted 15 lines · Igor Mammedov

Quoted 12 lines · On Mon, 4 Jun 2018 13:03:44 +0100 Daniel P. Berrangé <[email protected]>…

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 <[email protected]>
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.

Quoted 8 lines

Quoted 4 lines

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.

Quoted 7 lines

Quoted 3 lines

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:

[...]

Quoted 14 lines

Quoted 10 lines

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

Quoted 4 lines

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)

vl.c +23 -17

 /* 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);

Signature · 1 line

2.17.0

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é <[email protected]>

---
 vl.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Quoted 9 lines · 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.

Quoted 8 lines

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.

Quoted 4 lines

Signed-off-by Daniel P. Berrangé <[email protected]>

---
 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 <[email protected]>

(if my R-b line means anything here :-))

Michal

Quoted 10 lines · On Mon, 4 Jun 2018 16:21:48 +0200 Michal Privoznik <[email protected]> wrote:

Quoted 9 lines · 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.

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.

Quoted 9 lines

Quoted 9 lines

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?

Quoted 6 lines

Quoted 4 lines

Signed-off-by Daniel P. Berrangé <[email protected]>

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

Quoted 13 lines · Igor Mammedov

Quoted 10 lines · On Mon, 4 Jun 2018 16:21:48 +0200 Michal Privoznik <[email protected]> wrote:

Quoted 9 lines · 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.

Quoted 8 lines

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.

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

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.

Quoted 7 lines

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.

Quoted 7 lines

Quoted 6 lines

Quoted 4 lines

Signed-off-by Daniel P. Berrangé <[email protected]>

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

I'd prefer your patch, as it doesn't break error handling,
or maybe even shorter version which keeps state transitions in one place:

vl.c +1 -1

index c4fe255..fa44138 100644
@@ -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

On Mon, Jun 04, 2018 at 05:01:11PM +0200, Igor Mammedov wrote:
[...]

Quoted 18 lines

I'd prefer your patch, as it doesn't break error handling,
or maybe even shorter version which keeps state transitions in one place:

vl.c +1 -1

index c4fe255..fa44138 100644
@@ -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

vl.c static void main_loop(void) · line 1968

Quoted 5 lines

vl.c +1 -1

         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"?

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é <[email protected]>

Quoted 19 lines · On Mon, 4 Jun 2018 13:03:45 +0100 Daniel P. Berrangé <[email protected]>…

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.

Quoted 1 line

Signed-off-by Daniel P. Berrangé <[email protected]>

[...]

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)

vl.c +0 -0

index c4fe255..a2062d6 100644
@@ -1953,10 +1953,15 @@ static bool main_loop_should_exit(void)
 
 static void main_loop(void)

Quoted 38 lines · Igor Mammedov

Quoted 19 lines · On Mon, 4 Jun 2018 13:03:45 +0100 Daniel P. Berrangé <[email protected]>…

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.

Quoted 1 line

Signed-off-by Daniel P. Berrangé <[email protected]>

[...]

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)

vl.c +0 -0

index c4fe255..a2062d6 100644
@@ -1953,10 +1953,15 @@ static bool main_loop_should_exit(void)
 

vl.c static bool main_loop_should_exit(void) · line 1963

Quoted 11 lines

vl.c +5 -1

 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.

vl.c static void main_loop(void) · line 1975

Quoted 13 lines

vl.c +2 -1

+    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

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