| Message ID | 1498536619-14548-7-git-send-email-peterx@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 06/26/2017 11:10 PM, Peter Xu wrote: > One less global variable, and it does only matter with migration. > > We keep the old "--only-migratable" option, but also now we support: > > -global migration.only-migratable=true Does command line introspection work to see that this new spelling is available? If not, what else can we do to let libvirt learn that the new form is preferred?
On Tue, Jun 27, 2017 at 06:15:37AM -0500, Eric Blake wrote: > On 06/26/2017 11:10 PM, Peter Xu wrote: > > One less global variable, and it does only matter with migration. > > > > We keep the old "--only-migratable" option, but also now we support: > > > > -global migration.only-migratable=true > > Does command line introspection work to see that this new spelling is > available? If not, what else can we do to let libvirt learn that the > new form is preferred? "only-migratable" will appear on the output of "device-list-properties typename=migration". But although I think implementing this using global properties internally is nice, I'm unsure about making -global the recommended interface to configure this.
On Tue, Jun 27, 2017 at 10:36:44AM -0300, Eduardo Habkost wrote: > On Tue, Jun 27, 2017 at 06:15:37AM -0500, Eric Blake wrote: > > On 06/26/2017 11:10 PM, Peter Xu wrote: > > > One less global variable, and it does only matter with migration. > > > > > > We keep the old "--only-migratable" option, but also now we support: > > > > > > -global migration.only-migratable=true > > > > Does command line introspection work to see that this new spelling is > > available? If not, what else can we do to let libvirt learn that the > > new form is preferred? > > "only-migratable" will appear on the output of > "device-list-properties typename=migration". But although I think > implementing this using global properties internally is nice, I'm unsure > about making -global the recommended interface to configure this. Could I ask why it may not good to use "-global" to configure it? Actually if we can merge this series, there is planned work to expose more migration parameters to the user, like: migration.postcopy, migration.compression, migration.speed, etc. So before I post similar patches like this to export more things, I would like to know whether there is possible issue with it in general. Thanks,
On Wed, Jun 28, 2017 at 02:54:34PM +0800, Peter Xu wrote: > On Tue, Jun 27, 2017 at 10:36:44AM -0300, Eduardo Habkost wrote: > > On Tue, Jun 27, 2017 at 06:15:37AM -0500, Eric Blake wrote: > > > On 06/26/2017 11:10 PM, Peter Xu wrote: > > > > One less global variable, and it does only matter with migration. > > > > > > > > We keep the old "--only-migratable" option, but also now we support: > > > > > > > > -global migration.only-migratable=true > > > > > > Does command line introspection work to see that this new spelling is > > > available? If not, what else can we do to let libvirt learn that the > > > new form is preferred? > > > > "only-migratable" will appear on the output of > > "device-list-properties typename=migration". But although I think > > implementing this using global properties internally is nice, I'm unsure > > about making -global the recommended interface to configure this. > > Could I ask why it may not good to use "-global" to configure it? > > Actually if we can merge this series, there is planned work to expose > more migration parameters to the user, like: migration.postcopy, > migration.compression, migration.speed, etc. So before I post similar > patches like this to export more things, I would like to know whether > there is possible issue with it in general. Thanks, I don't see any obvious issue with "-global migration.FOO=BAR", but I just want to be 100% sure before making it the recommended interface to configure migration options. If there are no objections, I'm OK with that.
On Tue, Jun 27, 2017 at 12:10:15PM +0800, Peter Xu wrote: > One less global variable, and it does only matter with migration. > > We keep the old "--only-migratable" option, but also now we support: > > -global migration.only-migratable=true > > Currently still keep the old interface. > > Hmm, now vl.c has no way to access migrate_get_current(). Export a > function for it to setup only_migratable. > > Reviewed-by: Juan Quintela <quintela@redhat.com> > Signed-off-by: Peter Xu <peterx@redhat.com> [...] > +void migration_only_migratable_set(void) > +{ > + migrate_get_current()->only_migratable = true; > +} > + [...] > @@ -3953,7 +3952,13 @@ int main(int argc, char **argv, char **envp) > incoming = optarg; > break; > case QEMU_OPTION_only_migratable: > - only_migratable = 1; > + /* > + * TODO: we can remove this option one day, and we > + * should all use: > + * > + * "-global migration.only-migratable=true" > + */ > + migration_only_migratable_set(); This triggers a premature call to migrate_get_current(): $ qemu-system-x86_64 -only-migratable qemu-system-x86_64: /home/ehabkost/rh/proj/virt/qemu/migration/migration.c:127: migrate_get_current: Assertion `current_migration' failed. Aborted (core dumped) What about just doing: qemu_global_option("migration.only-migratable=true"); > break; > case QEMU_OPTION_nodefaults: > has_defaults = 0; > -- > 2.7.4 >
On Wed, Jun 28, 2017 at 04:13:57PM -0300, Eduardo Habkost wrote: > On Tue, Jun 27, 2017 at 12:10:15PM +0800, Peter Xu wrote: > > One less global variable, and it does only matter with migration. > > > > We keep the old "--only-migratable" option, but also now we support: > > > > -global migration.only-migratable=true > > > > Currently still keep the old interface. > > > > Hmm, now vl.c has no way to access migrate_get_current(). Export a > > function for it to setup only_migratable. > > > > Reviewed-by: Juan Quintela <quintela@redhat.com> > > Signed-off-by: Peter Xu <peterx@redhat.com> > [...] > > +void migration_only_migratable_set(void) > > +{ > > + migrate_get_current()->only_migratable = true; > > +} > > + > [...] > > @@ -3953,7 +3952,13 @@ int main(int argc, char **argv, char **envp) > > incoming = optarg; > > break; > > case QEMU_OPTION_only_migratable: > > - only_migratable = 1; > > + /* > > + * TODO: we can remove this option one day, and we > > + * should all use: > > + * > > + * "-global migration.only-migratable=true" > > + */ > > + migration_only_migratable_set(); > > This triggers a premature call to migrate_get_current(): > > $ qemu-system-x86_64 -only-migratable > qemu-system-x86_64: /home/ehabkost/rh/proj/virt/qemu/migration/migration.c:127: migrate_get_current: Assertion `current_migration' failed. > Aborted (core dumped) > > What about just doing: > > qemu_global_option("migration.only-migratable=true"); Oops... Definitely. Thanks for pointing this out! I'll see whether I should respin or fix on top.
diff --git a/include/migration/misc.h b/include/migration/misc.h index 2d36cf5..6ac3307 100644 --- a/include/migration/misc.h +++ b/include/migration/misc.h @@ -55,4 +55,6 @@ bool migration_has_finished(MigrationState *); bool migration_has_failed(MigrationState *); /* ...and after the device transmission */ bool migration_in_postcopy_after_devices(MigrationState *); +void migration_only_migratable_set(void); + #endif diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h index 9841a52..b213696 100644 --- a/include/sysemu/sysemu.h +++ b/include/sysemu/sysemu.h @@ -15,7 +15,6 @@ /* vl.c */ extern const char *bios_name; -extern int only_migratable; extern const char *qemu_name; extern QemuUUID qemu_uuid; extern bool qemu_uuid_set; diff --git a/migration/migration.c b/migration/migration.c index 221b22c..67f9e68 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -115,6 +115,11 @@ MigrationState *migrate_get_current(void) return current_migration; } +void migration_only_migratable_set(void) +{ + migrate_get_current()->only_migratable = true; +} + MigrationIncomingState *migration_incoming_get_current(void) { static bool once; @@ -986,7 +991,7 @@ static GSList *migration_blockers; int migrate_add_blocker(Error *reason, Error **errp) { - if (only_migratable) { + if (migrate_get_current()->only_migratable) { error_propagate(errp, error_copy(reason)); error_prepend(errp, "disallowing migration blocker " "(--only_migratable) for: "); @@ -1979,6 +1984,7 @@ void migrate_fd_connect(MigrationState *s) static Property migration_properties[] = { DEFINE_PROP_BOOL("store-global-state", MigrationState, store_global_state, true), + DEFINE_PROP_BOOL("only-migratable", MigrationState, only_migratable, false), DEFINE_PROP_END_OF_LIST(), }; diff --git a/migration/migration.h b/migration/migration.h index 4b898e9..34377dd 100644 --- a/migration/migration.h +++ b/migration/migration.h @@ -139,6 +139,9 @@ struct MigrationState * during migration. */ bool store_global_state; + + /* Whether the VM is only allowing for migratable devices */ + bool only_migratable; }; void migrate_set_state(int *state, int old_state, int new_state); diff --git a/migration/savevm.c b/migration/savevm.c index c7a49c9..1499cd3 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -2336,7 +2336,7 @@ void vmstate_register_ram_global(MemoryRegion *mr) bool vmstate_check_only_migratable(const VMStateDescription *vmsd) { /* check needed if --only-migratable is specified */ - if (!only_migratable) { + if (!migrate_get_current()->only_migratable) { return true; } diff --git a/vl.c b/vl.c index f0a0515..36ff3f4 100644 --- a/vl.c +++ b/vl.c @@ -188,7 +188,6 @@ bool boot_strict; uint8_t *boot_splash_filedata; size_t boot_splash_filedata_size; uint8_t qemu_extra_params_fw[2]; -int only_migratable; /* turn it off unless user states otherwise */ int icount_align_option; @@ -3953,7 +3952,13 @@ int main(int argc, char **argv, char **envp) incoming = optarg; break; case QEMU_OPTION_only_migratable: - only_migratable = 1; + /* + * TODO: we can remove this option one day, and we + * should all use: + * + * "-global migration.only-migratable=true" + */ + migration_only_migratable_set(); break; case QEMU_OPTION_nodefaults: has_defaults = 0;