Re: [Qemu-devel] [PATCH 05/11] migration: move vm_old_running into global state

[Peter Xu]

On Wed, Jan 03, 2018 at 10:05:07AM +0100, Juan Quintela wrote:
> Peter Xu <address@hidden> 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 <address@hidden>

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 <address@hidden>
> > ---
> >  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