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

["Daniel P. Berrangé" <berrange@redhat.com>]

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