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 <address@hidden>
>   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é <address@hidden>
> ---
>  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