Re: [Qemu-devel] [PATCH 2/3] migration: Remove use of old MigrationParams

[Peter Xu <peterx@redhat.com>]

On Fri, May 12, 2017 at 12:55:22PM +0200, Juan Quintela wrote:
> Peter Xu <peterx@redhat.com> wrote:
> > On Thu, May 11, 2017 at 06:32:27PM +0200, Juan Quintela wrote:
> 
> >> @@ -1214,9 +1218,6 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
> >>      MigrationParams params;
> >>      const char *p;
> >>  
> >> -    params.blk = has_blk && blk;
> >> -    params.shared = has_inc && inc;
> >> -
> >>      if (migration_is_setup_or_active(s->state) ||
> >>          s->state == MIGRATION_STATUS_CANCELLING ||
> >>          s->state == MIGRATION_STATUS_COLO) {
> >> @@ -1239,6 +1240,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
> >>      }
> >>  
> >>      if (has_inc && inc) {
> >> +        migrate_set_block_enabled(s, true);
> >>          migrate_set_block_shared(s, true);
> >
> > [2]
> >
> > IIUC for [1] & [2] we are solving the same problem that "shared"
> > depends on "enabled" bit. Would it be good to unitfy this dependency
> > somewhere? E.g., by changing migrate_set_block_shared() into:
> >
> > void migrate_set_block_shared(MigrationState *s, bool value)
> > {
> >     s->enabled_capabilities[MIGRATION_CAPABILITY_BLOCK_SHARED] = value;
> >     if (value) {
> >         migrate_set_block_enabled(s, true);
> >     }
> > }
> 
> ok with this.
> I will add once here that when we disable block enabled, we also disable
> shared, or just let it that way?

I should mark "nitpick" in my above comment. Any way works for me. :)

> 
> > Another thing to mention: after switching to the capability interface,
> > we'll cache the "enabled" and "shared" bits now while we don't cache
> > it before, right? IIUC it'll affect behavior of such sequence:
> >
> > - 1st migrate with enabled=1, shared=1, then
> > - 2nd migrate with enabled=0, shared=0
> >
> > Before the series, the 2nd migrate will use enabled=shared=0, but
> > after the series it should be using enabled=shared=1. Not sure whether
> > this would be a problem (or I missed anything?).
> 
> We can't be consistent with both old/new way.
> 
> Old way: we always setup the capabilities on command line (that should
> have been deprecated long, long ago)
> 
> New way: Once set, they stay set.
> 
> So, alternatives are:
> - If we are going to deprecate the old way, just let things as they are
>   on this patch (easier for me O:-)
> 
> - Always disable this features at the end of migration: Compatible with
>   old migration semantics, bet we are inconsistent with all other
>   capabilities.
> 
> - Add yet more code to only disable them when we are setting them
>   through the command line.  More code to maintain.
> 
> My idea would be to deprecate the migrate command line parameter, that
> is the reason why I did the 1st option.  I hope that in due curse, we
> would be able to remove the code.  But if anyone strongly think that any
> of the other options is better, please let me know.

I assume that sending continuous "migrate" is very rare, right (after
all, normally source VM is useless after that...)? I was just trying
to post this question up, and so... If you/Dave/others won't worry
about its compatibility, I won't either. ;)

Thanks!

-- 
Peter Xu