| Message ID | 1350657924-10624-2-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Fri, 19 Oct 2012 16:45:23 +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> Could you please add a note in cont's and stop's entries in qapi-schema.json explaining their behavior on INMIGRATE? Otherwise series looks good. > --- > v1->v2: kill MigrationExpected and QERR_MIGRATION_EXPECTED, yay! > > qapi-schema.json | 7 +------ > qerror.h | 3 --- > qmp.c | 17 +++++++++++------ > 3 file modificati, 12 inserzioni(+), 15 rimozioni(-) > > diff --git a/qapi-schema.json b/qapi-schema.json > index c615ee2..7e102c5 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 > @@ -1299,7 +1295,6 @@ > # 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. > # > 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)
Il 22/10/2012 17:06, Luiz Capitulino ha scritto: >> > 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> > Could you please add a note in cont's and stop's entries in qapi-schema.json > explaining their behavior on INMIGRATE? > > Otherwise series looks good. > Can I do that as a followup? Paolo
On Mon, 22 Oct 2012 17:35:58 +0200 Paolo Bonzini <pbonzini@redhat.com> wrote: > Il 22/10/2012 17:06, Luiz Capitulino ha scritto: > >> > 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> > > Could you please add a note in cont's and stop's entries in qapi-schema.json > > explaining their behavior on INMIGRATE? > > > > Otherwise series looks good. > > > > Can I do that as a followup? You can re-work 1/2 and submit only that one. I'll order the two patches correctly in my queue.
diff --git a/qapi-schema.json b/qapi-schema.json index c615ee2..7e102c5 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 @@ -1299,7 +1295,6 @@ # 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. # 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)
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> --- v1->v2: kill MigrationExpected and QERR_MIGRATION_EXPECTED, yay! qapi-schema.json | 7 +------ qerror.h | 3 --- qmp.c | 17 +++++++++++------ 3 file modificati, 12 inserzioni(+), 15 rimozioni(-)