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 <imamm...@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é <berra...@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
