On Wed, Jan 03, 2018 at 10:05:07AM +0100, Juan Quintela wrote: > Peter Xu <pet...@redhat.com> wrote: > > Firstly, it was passed around. Let's just move it into MigrationState > > just like many other variables as state of migration. > > > > One thing to mention is that for postcopy, we actually don't need this > > knowledge at all since postcopy can't resume a VM even if it fails (we > > can see that from the old code too: when we try to resume we also check > > against "entered_postcopy" variable). So further we do this: > > > > - in postcopy_start(), we don't update vm_old_running since useless > > - in migration_thread(), we don't need to check entered_postcopy when > > resume, since it's only used for precopy. > > > > Comment this out too for that variable definition. > > Reviewed-by: Juan Quintela <quint...@redhat.com>
Taken. > > But I wonder if we can came with a better name. Best one that I can > think is > vm_was_running > > Any other name that I came is bad for precopy or colo. > > i.e. restart_vm_on_cancel_error > > is meaningful for precopy, but not for colo. Yeah actually spent >10 seconds thinking about a better naming but failed, so the old one is kept. I'll use vm_was_running. > > > Signed-off-by: Peter Xu <pet...@redhat.com> > > --- > > migration/migration.c | 17 +++++++---------- > > migration/migration.h | 6 ++++++ > > 2 files changed, 13 insertions(+), 10 deletions(-) > > > > diff --git a/migration/migration.h b/migration/migration.h > > index ac74a12713..0f5df2367c 100644 > > --- a/migration/migration.h > > +++ b/migration/migration.h > > @@ -111,6 +111,12 @@ struct MigrationState > > int64_t expected_downtime; > > bool enabled_capabilities[MIGRATION_CAPABILITY__MAX]; > > int64_t setup_time; > > + /* > > + * Whether the old VM is running for the last migration. This is > > + * used to resume the VM when precopy failed or cancelled somehow. > > + * It's never used for postcopy. > > + */ > > + bool old_vm_running; > > I think this comment is right for precopy, but not for colo. BTW, I > think that I would put the postcopy comment on its use, not here. Or, how about I just don't mention postcopy at all? > > /me tries to improve the comment > > Guest was running when we enter the completion stage. If migration don't > sucess, we need to continue running guest on source. > > What do you think? I think it's generally good. Maybe a tiny fix like: s/Guest was/Whether guest was/ s/If migration don't sucess/If migration failed/ ? Thanks, -- Peter Xu
