Re: [Qemu-devel] [PATCH v3 1/2] qmp: handle stop/cont in INMIGRATE state

[Luiz Capitulino <lcapitulino@redhat.com>]

On Tue, 23 Oct 2012 14:54:21 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Right now, stop followed by an incoming migration will let the
> virtual machine start.  cont before an incoming migration instead
> will fail.
> 
> This is bad because the actual behavior is not predictable; it is
> racy with respect to the start of the incoming migration.  That's
> because incoming migration is blocking, and thus will delay the
> processing of stop/cont until the end of the migration.
> 
> In addition, there's nothing that really prevents the user from
> typing the block device's passwords before incoming migration is
> done, so returning the DeviceEncrypted error is also helpful in
> the QMP case.
> 
> Both things can be fixed by just toggling the autostart variable when
> stop/cont are called in INMIGRATE state.
> 
> Note that libvirt is currently working around the race by looping
> if the MigrationExpected answer is returned.  After this patch, the
> command will return right away without ever raising an error.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Applied series to the qmp branch, thanks.

> ---
>  qapi-schema.json | 23 ++++++++++++++---------
>  qerror.h         |  3 ---
>  qmp.c            | 17 +++++++++++------
>  3 file modificati, 25 inserzioni(+), 18 rimozioni(-)
> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index c615ee2..6b14edc 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -22,15 +22,11 @@
>  # @KVMMissingCap: the requested operation can't be fulfilled because a
>  #                 required KVM capability is missing
>  #
> -# @MigrationExpected: the requested operation can't be fulfilled because a
> -#                     migration process is expected
> -#
>  # Since: 1.2
>  ##
>  { 'enum': 'ErrorClass',
>    'data': [ 'GenericError', 'CommandNotFound', 'DeviceEncrypted',
> -            'DeviceNotActive', 'DeviceNotFound', 'KVMMissingCap',
> -            'MigrationExpected' ] }
> +            'DeviceNotActive', 'DeviceNotFound', 'KVMMissingCap' ] }
>  
>  ##
>  # @add_client
> @@ -149,7 +145,11 @@
>  #
>  # @finish-migrate: guest is paused to finish the migration process
>  #
> -# @inmigrate: guest is paused waiting for an incoming migration
> +# @inmigrate: guest is paused waiting for an incoming migration.  Note
> +# that this state does not tell whether the machine will start at the
> +# end of the migration.  This depends on the command-line -S option and
> +# any invocation of 'stop' or 'cont' that has happened since QEMU was
> +# started.
>  #
>  # @internal-error: An internal error that prevents further guest execution
>  # has occurred
> @@ -1210,7 +1210,9 @@
>  # Since:  0.14.0
>  #
>  # Notes:  This function will succeed even if the guest is already in the stopped
> -#         state
> +#         state.  In "inmigrate" state, it will ensure that the guest
> +#         remains paused once migration finishes, as if the -S option was
> +#         passed on the command line.
>  ##
>  { 'command': 'stop' }
>  
> @@ -1299,11 +1301,14 @@
>  # Since:  0.14.0
>  #
>  # Returns:  If successful, nothing
> -#           If the QEMU is waiting for an incoming migration, MigrationExpected
>  #           If QEMU was started with an encrypted block device and a key has
>  #              not yet been set, DeviceEncrypted.
>  #
> -# Notes:  This command will succeed if the guest is currently running.
> +# Notes:  This command will succeed if the guest is currently running.  It
> +#         will also succeed if the guest is in the "inmigrate" state; in
> +#         this case, the effect of the command is to make sure the guest
> +#         starts once migration finishes, removing the effect of the -S
> +#         command line option if it was passed.
>  ##
>  { 'command': 'cont' }
>  
> diff --git a/qerror.h b/qerror.h
> index c91708c..5e98a39 100644
> --- a/qerror.h
> +++ b/qerror.h
> @@ -165,9 +165,6 @@ void assert_no_error(Error *err);
>  #define QERR_MIGRATION_NOT_SUPPORTED \
>      ERROR_CLASS_GENERIC_ERROR, "State blocked by non-migratable device '%s'"
>  
> -#define QERR_MIGRATION_EXPECTED \
> -    ERROR_CLASS_MIGRATION_EXPECTED, "An incoming migration is expected before this command can be executed"
> -
>  #define QERR_MISSING_PARAMETER \
>      ERROR_CLASS_GENERIC_ERROR, "Parameter '%s' is missing"
>  
> diff --git a/qmp.c b/qmp.c
> index 36c54c5..2c8d559 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -85,7 +85,11 @@ void qmp_quit(Error **err)
>  
>  void qmp_stop(Error **errp)
>  {
> -    vm_stop(RUN_STATE_PAUSED);
> +    if (runstate_check(RUN_STATE_INMIGRATE)) {
> +        autostart = 0;
> +    } else {
> +        vm_stop(RUN_STATE_PAUSED);
> +    }
>  }
>  
>  void qmp_system_reset(Error **errp)
> @@ -144,10 +148,7 @@ void qmp_cont(Error **errp)
>  {
>      Error *local_err = NULL;
>  
> -    if (runstate_check(RUN_STATE_INMIGRATE)) {
> -        error_set(errp, QERR_MIGRATION_EXPECTED);
> -        return;
> -    } else if (runstate_check(RUN_STATE_INTERNAL_ERROR) ||
> +    if (runstate_check(RUN_STATE_INTERNAL_ERROR) ||
>                 runstate_check(RUN_STATE_SHUTDOWN)) {
>          error_set(errp, QERR_RESET_REQUIRED);
>          return;
> @@ -162,7 +163,11 @@ void qmp_cont(Error **errp)
>          return;
>      }
>  
> -    vm_start();
> +    if (runstate_check(RUN_STATE_INMIGRATE)) {
> +        autostart = 1;
> +    } else {
> +        vm_start();
> +    }
>  }
>  
>  void qmp_system_wakeup(Error **errp)