Re: [PATCH v2 3/7] migration: Enable precopy initial data capability

[Peter Xu <peterx@redhat.com>]

On Thu, May 18, 2023 at 10:26:04AM +0300, Avihai Horon wrote:
> 
> On 17/05/2023 19:07, Peter Xu wrote:
> > External email: Use caution opening links or attachments
> > 
> > 
> > On Wed, May 17, 2023 at 06:52:15PM +0300, Avihai Horon wrote:
> > > Now that precopy initial data logic has been implemented, enable the
> > > capability.
> > > 
> > > Signed-off-by: Avihai Horon <avihaih@nvidia.com>
> > > Reviewed-by: Juan Quintela <quintela@redhat.com>
> > > ---
> > >   migration/options.c | 4 ----
> > >   1 file changed, 4 deletions(-)
> > > 
> > > diff --git a/migration/options.c b/migration/options.c
> > > index 0a31921a7a..3449ce4f14 100644
> > > --- a/migration/options.c
> > > +++ b/migration/options.c
> > > @@ -561,10 +561,6 @@ bool migrate_caps_check(bool *old_caps, bool *new_caps, Error **errp)
> > >                                "capability 'return-path'");
> > >               return false;
> > >           }
> > > -
> > > -        /* Disable this capability until it's implemented */
> > > -        error_setg(errp, "'precopy-initial-data' is not implemented yet");
> > > -        return false;
> > >       }
> > I'm always confused why we need this and not having this squashed into
> > patch 1 (or say, never have these lines).
> > 
> > The only thing it matters is when someone backports patch 1 but not
> > backport the rest of the patches.  But that's really, really weird already
> > as a backporter doing that, and I doubt its happening.
> > 
> > Neither should we merge patch 1 without merging follow up patches to
> > master, as we should just always merge the whole feature or just keep
> > reworking on the list.
> > 
> > I'd like to know if I missed something else..
> 
> There are also git bisect considerations.
> This practice is useful for git bisect for features that are enabled by
> default, so you won't mistakenly run "half a feature" if you do bisect.
> But here the capability must be manually enabled, so maybe it's not that
> useful in this case.
> 
> I like it for the sake of good order, so this capability can't be enabled
> before it's fully implemented (even if it's unlikely that someone would do
> that).

Right.  I was kind of thinking someone bisecting such feature will always
make sure to start from the last commit got merged, but I see your point as
a general concept.

One slightly better way to not add something and remove again is, we can
introduce migrate_precopy_initial_data() in patch 2, returning constantly
false, then we can put patch 1 (qapi interface) to be after current patch
2, where you allow migrate_precopy_initial_data() to start return true.  It
saves a few lines to remove, and also one specific patch explicitly
removing it.  But I think fundamentally it's similar indeed.

In case you'd like to keep this as is, feel free to add:

Reviewed-by: Peter Xu <peterx@redhat.com>

Thanks,

-- 
Peter Xu